Using list_move_tail() instead of list_del() + list_add_tail() in ssh_request_layer.c.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210609072638.1358174-1-libaokun1@huawei.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The status variable in ssam_nf_refcount_disable_free() is only set when
the reference count equals zero. Otherwise, it is returned
uninitialized. Fix this by always initializing status to zero.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: 640ee17199e4 ("platform/surface: aggregator: Allow enabling of events without notifiers")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210604210907.25738-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Mark functions with locking requirements via the corresponding lockdep
calls for debugging and documentary purposes.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20210604134755.535590-7-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
While events can already be enabled and disabled via the generic request
IOCTL, this bypasses the internal reference counting mechanism of the
controller. Due to that, disabling an event will turn it off regardless
of any other client having requested said event, which may break
functionality of that client.
To solve this, add IOCTLs wrapping the ssam_controller_event_enable()
and ssam_controller_event_disable() functions, which have been
previously introduced for this specific purpose.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20210604134755.535590-6-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Currently, debugging unknown events requires writing a custom driver.
This is somewhat difficult, slow to adapt, and not entirely
user-friendly for quickly trying to figure out things on devices of some
third-party user. We can do better. We already have a user-space
interface intended for debugging SAM EC requests, so let's add support
for receiving events to that.
This commit provides support for receiving events by reading from the
controller file. It additionally introduces two new IOCTLs to control
which event categories will be forwarded. Specifically, a user-space
client can specify which target categories it wants to receive events
from by registering the corresponding notifier(s) via the IOCTLs and
after that, read the received events by reading from the controller
device.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20210604134755.535590-5-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
We can already enable and disable SAM events via one of two ways: either
via a (non-observer) notifier tied to a specific event group, or a
generic event enable/disable request. In some instances, however,
neither method may be desirable.
The first method will tie the event enable request to a specific
notifier, however, when we want to receive notifications for multiple
event groups of the same target category and forward this to the same
notifier callback, we may receive duplicate events, i.e. one event per
registered notifier. The second method will bypass the internal
reference counting mechanism, meaning that a disable request will
disable the event regardless of any other client driver using it, which
may break the functionality of that driver.
To address this problem, add new functions that allow enabling and
disabling of events via the event reference counting mechanism built
into the controller, without needing to register a notifier.
This can then be used in combination with observer notifiers to process
multiple events of the same target category without duplication in the
same callback function.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210604134755.535590-3-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Currently, each SSAM event notifier is directly tied to one group of
events. This makes sense as registering a notifier will automatically
take care of enabling the corresponding event group and normally drivers
only need notifications for a very limited number of events, associated
with different callbacks for each group.
However, there are rare cases, especially for debugging, when we want to
get notifications for a whole event target category instead of just a
single group of events in that category. Registering multiple notifiers,
i.e. one per group, may be infeasible due to two issues: a) we might not
know every event enable/disable specification as some events are
auto-enabled by the EC and b) forwarding this to the same callback will
lead to duplicate events as we might not know the full event
specification to perform the appropriate filtering.
This commit introduces observer-notifiers, which are notifiers that are
not tied to a specific event group and do not attempt to manage any
events. In other words, they can be registered without enabling any
event group or incrementing the corresponding reference count and just
act as silent observers, listening to all currently/previously enabled
events based on their match-specification.
Essentially, this allows us to register one single notifier for a full
event target category, meaning that we can process all events of that
target category in a single callback without duplication. Specifically,
this will be used in the cdev debug interface to forward events to
user-space via a device file from which the events can be read.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20210604134755.535590-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
When we fail to open the device file due to DTX being shut down, the
mutex is initialized but never destroyed. We are destroying it when
releasing the file, so add the missing call in the failure path as well.
Fixes: 1d60999283 ("platform/surface: Add DTX driver")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210604132540.533036-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Disabling events silently fails due to the wrong command ID being used.
Instead of the command ID for the disable call, the command ID for the
enable call was being used. This causes the disable call to enable the
event instead. As the event is already enabled when we call this
function, the EC silently drops this command and does nothing.
Use the correct command ID for disabling the event to fix this.
Fixes: c167b9c7e3 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210603000636.568846-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
5th- and 6th-generation Surface devices have all SAM clients defined in
ACPI, except for the platform profile/performance mode which his handled
via the WSID (Windows Surface Integration Device). Thus, the node groups
for those devices are the same and we can just use a single one instead
of re-defining the same one over and over again.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210523134528.798887-4-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add support for the 13" Intel version of the Surface Laptop 4.
Use the existing node group for the Surface Laptop 3 since the 15" AMD
version already shares its WSID HID with its predecessor and there don't
seem to be any significant differences with regards to SAM.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210523134528.798887-3-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The 15" AMD version of the Surface Laptop 4 shares its WSID HID with the
15" AMD version of the Surface Laptop 3. Update the comments
accordingly.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210523134528.798887-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The poll function should not return -ERESTARTSYS.
Furthermore, locking in this function is completely unnecessary. The
ddev->lock protects access to the main device and controller (ddev->dev
and ddev->ctrl), ensuring that both are and remain valid while being
accessed by clients. Both are, however, never accessed in the poll
function. The shutdown test (via atomic bit flags) be safely done
without locking, so drop locking here entirely.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: 1d60999283 ("platform/surface: Add DTX driver)
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210513134437.2431022-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Having both IRQF_NO_AUTOEN and IRQF_SHARED set causes
request_threaded_irq() to return with -EINVAL (see comment in flag
validation in that function). As the interrupt is currently not shared
between multiple devices, drop the IRQF_SHARED flag.
Fixes: 507cf5a2f1 ("platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210505133635.1499703-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The "funcs" variable is a u64. If "func" is more than 31 then the
BIT() shift will wrap instead of testing the high bits.
Fixes: c167b9c7e3 ("platform/surface: Add Surface Aggregator subsystem")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/YH6UUhJhGk3mk13b@mwanda
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
disable_irq() after request_irq() still has a time gap in which
interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
disable IRQ auto-enable because of requesting.
this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
is being merged: https://lore.kernel.org/patchwork/patch/1388765/
Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/1617778852-26492-1-git-send-email-tiantao6@hisilicon.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Sometimes, the "base connected" event that we rely on to (re-)attach the
device connected to the base is sent a bit too early. When this happens,
some devices may not be completely ready yet.
Specifically, the battery has been observed to report zero-values for
things like full charge capacity, which, however, is only loaded once
when the driver for that device probes. This can thus result in battery
readings being unavailable.
As we cannot easily and reliably discern between devices that are not
ready yet and devices that are not connected (i.e. will never be ready),
delay adding these devices. This should give them enough time to set up.
The delay is set to 2.5 seconds, which should give us a good safety
margin based on testing and still be fairly responsive for users.
To achieve that delay switch to updating via a delayed work struct,
which means that we can also get rid of some locking.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210405231222.358113-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The "&client->ddev->lock" and "&ddev->lock" are the same thing. Let's
use "&ddev->lock" consistently.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/YF3TgCcpcCYl3a//@mwanda
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The Surface Pro 7+ is essentially a refresh of the Surface Pro 7 with
updated hardware and a new WSID identifier.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210309162550.302161-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The sparse tool complains as follows:
drivers/platform/surface/surface_aggregator_registry.c:355:30: warning:
symbol 'ssam_base_hub_group' was not declared. Should it be static?
This symbol is not used outside of surface_aggregator_registry.c, so this
commit marks it static.
Fixes: 797e785646 ("platform/surface: aggregator_registry: Add base device hub")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210309131500.1885772-1-weiyongjun1@huawei.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add support for native SSAM devices to the DTX driver. This allows
support for the Surface Book 3, on which the DTX device is not present
in ACPI.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210308184819.437438-3-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The Microsoft Surface Book series devices consist of a so-called
clipboard part (containing the CPU, touchscreen, and primary battery)
and a base part (containing keyboard, secondary battery, and optional
discrete GPU). These parts can be separated, i.e. the clipboard can be
detached and used as tablet.
This detachment process is initiated by pressing a button. On the
Surface Book 2 and 3 (targeted with this commit), the Surface Aggregator
Module (i.e. the embedded controller on those devices) attempts to send
a notification to any listening client driver and waits for further
instructions (i.e. whether the detachment process should continue or be
aborted). If it does not receive a response in a certain time-frame, the
detachment process (by default) continues and the clipboard can be
physically separated. In other words, (by default and) without a driver,
the detachment process takes about 10 seconds to complete.
This commit introduces a driver for this detachment system (called DTX).
This driver allows a user-space daemon to control and influence the
detachment behavior. Specifically, it forwards any detachment requests
to user-space, allows user-space to make such requests itself, and
allows handling of those requests. Requests can be handled by either
aborting, continuing/allowing, or delaying (i.e. resetting the timeout
via a heartbeat commend). The user-space API is implemented via the
/dev/surface/dtx miscdevice.
In addition, user-space can change the default behavior on timeout from
allowing detachment to disallowing it, which is useful if the (optional)
discrete GPU is in use.
Furthermore, this driver allows user-space to receive notifications
about the state of the base, specifically when it is physically removed
(as opposed to detachment requested), in what manner it is connected
(i.e. in reverse-/tent-/studio- or laptop-mode), and what type of base
is connected. Based on this information, the driver also provides a
simple tablet-mode switch (aliasing all modes without keyboard access,
i.e. tablet-mode and studio-mode to its reported tablet-mode).
An implementation of such a user-space daemon, allowing configuration of
detachment behavior via scripts (e.g. safely unmounting USB devices
connected to the base before continuing) can be found at [1].
[1]: https://github.com/linux-surface/surface-dtx-daemon
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210308184819.437438-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
A few x86 platform drivers use ACPI_DEBUG_PRINT() or ACPI_EXCEPTION()
for printing messages, but that is questionable, because those macros
belong to ACPICA and they should not be used elsewhere. In addition,
ACPI_DEBUG_PRINT() requires special enabling to allow it to actually
print the message, which is a nuisance, and the _COMPONENT symbol
generally needed for that is not defined in any of the files in
question.
For this reason, replace the ACPI_DEBUG_PRINT() in lg-laptop.c with
pr_debug() and the one in xo15-ebook.c with acpi_handle_debug()
(with the additional benefit that the source object can be identified
more easily after this change).
Also drop the ACPI_MODULE_NAME() definitions that are only used by
the ACPICA message printing macros from those files and from wmi.c
and surfacepro3_button.c (while at it).
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Link: https://lore.kernel.org/r/2074665.VPHYfYaQb6@kreacher
[hdegoede@redhat.com: Drop acer-wmi.c chunk, a similar patch was already merged]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The SSAM_DEFINE_SYNC_REQUEST_x() macros are intended to reduce
boiler-plate code for SSAM request definitions by defining a wrapper
function for the specified request. The client device variants of those
macros, i.e. SSAM_DEFINE_SYNC_REQUEST_CL_x() in particular rely on the
multi-device (MD) variants, e.g.:
#define SSAM_DEFINE_SYNC_REQUEST_CL_R(name, rtype, spec...) \
SSAM_DEFINE_SYNC_REQUEST_MD_R(__raw_##name, rtype, spec) \
int name(struct ssam_device *sdev, rtype *ret) \
{ \
return __raw_##name(sdev->ctrl, sdev->uid.target, \
sdev->uid.instance, ret); \
}
This now creates the problem that it is not possible to declare the
generated functions static via
static SSAM_DEFINE_SYNC_REQUEST_CL_R(...)
as this will only apply to the function defined by the multi-device
macro, i.e. SSAM_DEFINE_SYNC_REQUEST_MD_R(). Thus compiling with
`-Wmissing-prototypes' rightfully complains that there is a 'static'
keyword missing.
To solve this, make all SSAM_DEFINE_SYNC_REQUEST_x() macros define
static functions. Non-client-device macros are also changed for
consistency. In general, we expect those functions to be only used
locally in the respective drivers for the corresponding interfaces, so
having to define a wrapper function to be able to export this should be
the odd case out.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: b78b4982d7 ("platform/surface: Add platform profile driver")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210304190524.1172197-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add a driver to provide platform profile support on 5th- and later
generation Microsoft Surface devices with a Surface System Aggregator
Module. On those devices, the platform profile can be used to influence
cooling behavior and power consumption.
For example, the default 'quiet' profile limits fan noise and in turn
sacrifices performance of the discrete GPU found on Surface Books. Its
full performance can only be unlocked on the 'performance' profile.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20210211201703.658240-5-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add HID subsystem (TC=0x15) devices. These devices need to be registered
for 7th-generation Surface models. On previous generations, these
devices are either provided as platform devices via ACPI (Surface Laptop
1 and 2) or implemented as standard USB device.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210212115439.1525216-7-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add the detachment system (DTX) SSAM device for the Surface Book 3. This
device is accessible under the base (TC=0x11) subsystem.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210212115439.1525216-6-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add the SSAM platform profile device to the SSAM device registry. This
device is accessible under the thermal subsystem (TC=0x03) and needs to
be registered for all Surface models.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210212115439.1525216-5-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add battery subsystem (TC=0x02) devices (battery and AC) to the SSAM
device registry. These devices need to be registered for 7th-generation
Surface models. On 5th- and 6th-generation models, these devices are
handled via the standard ACPI battery/AC interface, which in turn
accesses the same SSAM interface via the Surface ACPI Notify (SAN)
driver.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210212115439.1525216-4-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The Surface Book 3 has a detachable base part. While the top part
(so-called clipboard) contains the CPU, touchscreen, and primary
battery, the base contains, among other things, a keyboard, touchpad,
and secondary battery.
Those devices do not react well to being accessed when the base part is
detached and should thus be removed and added in sync with the base. To
facilitate this, we introduce a virtual base device hub, which
automatically removes or adds the devices registered under it.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210212115439.1525216-3-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The Surface System Aggregator Module (SSAM) subsystem provides various
functionalities, which are separated by spreading them across multiple
devices and corresponding drivers. Parts of that functionality / some of
those devices, however, can (as far as we currently know) not be
auto-detected by conventional means. While older (specifically 5th- and
6th-)generation models do advertise most of their functionality via
standard platform devices in ACPI, newer generations do not.
As we are currently also not aware of any feasible way to query said
functionalities dynamically, this poses a problem. There is, however, a
device in ACPI that seems to be used by Windows for identifying
different Surface models: The Windows Surface Integration Device (WSID).
This device seems to have a HID corresponding to the overall set of
functionalities SSAM provides for the associated model.
This commit introduces a registry providing non-detectable device
information via software nodes. In addition, a SSAM platform hub driver
is introduced, which takes care of creating and managing the SSAM
devices specified in this registry. This approach allows for a
hierarchical setup akin to ACPI and is easily extendable, e.g. via
firmware node properties.
Note that this commit only provides the basis for the platform hub and
registry, and does not add any content to it. The registry will be
expanded in subsequent commits.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210212115439.1525216-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The raw message frame length is unaligned and explicitly marked as
little endian. It should not be accessed without the appropriate
accessor functions. Fix this.
Note that payload.len already contains the correct length after parsing
via sshp_parse_frame(), so we can simply use that instead.
Reported-by: kernel-test-robot <lkp@intel.com>
Fixes: c167b9c7e3 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Acked-by: Mark Gross <mgross@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Link: https://lore.kernel.org/r/20210211124149.2439007-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Some Surface Book 2 and 3 models have a discrete GPU (dGPU) that is
hot-pluggable. On those devices, the dGPU is contained in the base,
which can be separated from the tablet part (containing CPU and
touchscreen) while the device is running.
It (in general) is presented as/behaves like a standard PCIe hot-plug
capable device, however, this device can also be put into D3cold. In
D3cold, the device itself is turned off and can thus not submit any
standard PCIe hot-plug events. To properly detect hot-(un)plugging while
the dGPU is in D3cold, out-of-band signaling is required. Without this,
the device state will only get updated during the next bus-check, eg.
via a manually issued lspci call.
This commit adds a driver to handle out-of-band PCIe hot-(un)plug events
on Microsoft Surface devices. On those devices, said events can be
detected via GPIO interrupts, which are then forwarded to the
corresponding ACPI DSM calls by this driver. The DSM then takes care of
issuing the appropriate bus-/device-check, causing the PCI core to
properly pick up the device change.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210205012657.1951753-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Explicitly check the status rather then relying on output.pointer staying
NULL on an error. This silences the following compiler warning:
drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable]
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20210204113848.105994-1-hdegoede@redhat.com
The braces of the unlikely() macro inside the if condition only cover
the subtraction part, not the whole statement. This causes the result of
the subtraction to be converted to zero or one. While that still works
in this context, it causes static analysis tools to complain (and is
just plain wrong).
Fix the bracket placement and, while at it, simplify the if-condition.
Also add a comment to the if-condition explaining what we expect the
result to be and what happens on the failure path, as it seems to have
caused a bit of confusion.
This commit should not cause any difference in behavior or generated
code.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: c167b9c7e3 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210126172202.1428367-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Both, ssh_rtl_rx_start() and ssh_rtl_tx_start() functions, do not exist
and have been consolidated into ssh_rtl_start(). Nevertheless,
kernel-doc references the former functions. Replace those references
with references to ssh_rtl_start().
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210114150826.19109-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
A function has a different name between their prototype
and its kernel-doc markup:
../drivers/platform/surface/aggregator/ssh_request_layer.c:1065: warning: expecting prototype for ssh_rtl_tx_start(). Prototype was for ssh_rtl_start() instead
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/4a6bf33cfbd06654d78294127f2b6d354d073089.1610610937.git.mchehab+huawei@kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
CI static analysis complains about the allocation size in payload and
response buffers being unchecked. In general, these allocations should
be safe as the user-input is u16 and thus limited to U16_MAX, which is
only slightly larger than the theoretical maximum imposed by the
underlying SSH protocol.
All bounds on these values required by the underlying protocol are
enforced in ssam_request_sync() (or rather the functions called by it),
thus bounds here are only relevant for allocation.
Add comments explaining that this should be safe.
Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: 178f6ab77e ("platform/surface: Add Surface Aggregator user-space interface")
Addresses-Coverity: ("Untrusted allocation size")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210111154851.325404-3-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
When copy_struct_from_user() in ssam_cdev_request() fails, we directly
jump to the 'out' label. In this case, however 'spec' and 'rsp' are not
initialized, but we still access fields of those variables. Fix this by
initializing them at the time of their declaration.
Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: 178f6ab77e ("platform/surface: Add Surface Aggregator user-space interface")
Addresses-Coverity: ("Uninitialized pointer read")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210111154851.325404-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The left shift of int 32 bit integer constant 1 is evaluated using 32 bit
arithmetic and then passed as a 64 bit function argument. In the case where
func is 32 or more this can lead to an oveflow. Avoid this by shifting
using the BIT_ULL macro instead.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: fc00bc8ac1 ("platform/surface: Add Surface ACPI Notify driver")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210111144648.20498-1-colin.king@canonical.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The Surface ACPI Notify (SAN) device provides an ACPI interface to the
Surface Aggregator EC, specifically the Surface Serial Hub interface.
This interface allows EC requests to be made from ACPI code and can
convert a subset of EC events back to ACPI notifications.
Specifically, this interface provides a GenericSerialBus operation
region ACPI code can execute a request by writing the request command
data and payload to this operation region and reading back the
corresponding response via a write-then-read operation. Furthermore,
this interface provides a _DSM method to be called when certain events
from the EC have been received, essentially turning them into ACPI
notifications.
The driver provided in this commit essentially takes care of translating
the request data written to the operation region, executing the request,
waiting for it to finish, and finally writing and translating back the
response (if the request has one). Furthermore, this driver takes care
of enabling the events handled via ACPI _DSM calls. Lastly, this driver
also exposes an interface providing discrete GPU (dGPU) power-on
notifications on the Surface Book 2, which are also received via the
operation region interface (but not handled by the SAN driver directly),
making them accessible to other drivers (such as a dGPU hot-plug driver
that may be added later on).
On 5th and 6th generation Surface devices (Surface Pro 5/2017, Pro 6,
Book 2, Laptop 1 and 2), the SAN interface provides full battery and
thermal subsystem access, as well as other EC based functionality. On
those models, battery and thermal sensor devices are implemented as
standard ACPI devices of that type, however, forward ACPI calls to the
corresponding Surface Aggregator EC request via the SAN interface and
receive corresponding notifications (e.g. battery information change)
from it. This interface is therefore required to provide said
functionality on those devices.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20201221183959.1186143-10-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add a misc-device providing user-space access to the Surface Aggregator
EC, mainly intended for debugging, testing, and reverse-engineering.
This interface gives user-space applications the ability to send
requests to the EC and receive the corresponding responses.
The device-file is managed by a pseudo platform-device and corresponding
driver to avoid dependence on the dedicated bus, allowing it to be
loaded in a minimal configuration.
A python library and scripts to access this device can be found at [1].
[1]: https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20201221183959.1186143-9-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The Surface Aggregator EC provides varying functionality, depending on
the Surface device. To manage this functionality, we use dedicated
client devices for each subsystem or virtual device of the EC. While
some of these clients are described as standard devices in ACPI and the
corresponding client drivers can be implemented as platform drivers in
the kernel (making use of the controller API already present), many
devices, especially on newer Surface models, cannot be found there.
To simplify management of these devices, we introduce a new bus and
client device type for the Surface Aggregator subsystem. The new device
type takes care of managing the controller reference, essentially
guaranteeing its validity for as long as the client device exists, thus
alleviating the need to manually establish device links for that purpose
in the client driver (as has to be done with the platform devices).
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20201221183959.1186143-7-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
This commit adds error injection hooks to the Surface Serial Hub
communication protocol implementation, to:
- simulate simple serial transmission errors,
- drop packets, requests, and responses, simulating communication
failures and potentially trigger retransmission timeouts, as well as
- inject invalid data into submitted and received packets.
Together with the trace points introduced in the previous commit, these
facilities are intended to aid in testing, validation, and debugging of
the Surface Aggregator communication layer.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Link: https://lore.kernel.org/r/20201221183959.1186143-6-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add trace points to the Surface Aggregator subsystem core. These trace
points can be used to track packets, requests, and allocations. They are
further intended for debugging and testing/validation, specifically in
combination with the error injection capabilities introduced in the
subsequent commit.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Link: https://lore.kernel.org/r/20201221183959.1186143-5-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Event items are used for completing Surface Aggregator EC events, i.e.
placing event command data and payload on a workqueue for later
processing to avoid doing said processing directly on the receiver
thread. This means that event items are allocated for each incoming
event, regardless of that event being transmitted via sequenced or
unsequenced packets.
On the Surface Book 3 and Surface Laptop 3, touchpad HID input events
(unsequenced), can constitute a larger amount of traffic, and therefore
allocation of event items. This warrants caching event items to reduce
memory fragmentation. The size of the cached objects is specifically
tuned to accommodate keyboard and touchpad input events and their
payloads on those devices. As a result, this effectively also covers
most other event types. In case of a larger event payload, event item
allocation will fall back to kzalloc().
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20201221183959.1186143-4-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Surface Serial Hub communication is, in its core, packet based. Each
sequenced packet requires to be acknowledged, via an ACK-type control
packet. In case invalid data has been received by the driver, a NAK-type
(not-acknowledge/negative acknowledge) control packet is sent,
triggering retransmission.
Control packets are therefore a core communication primitive and used
frequently enough (with every sequenced packet transmission sent by the
embedded controller, including events and request responses) that it may
warrant caching their allocations to reduce possible memory
fragmentation.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20201221183959.1186143-3-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Baokun Li