* [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module
@ 2020-11-15 19:21 Maximilian Luz
2020-11-15 19:21 ` [PATCH 9/9] platform/surface: Add Surface ACPI Notify driver Maximilian Luz
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Maximilian Luz @ 2020-11-15 19:21 UTC (permalink / raw)
To: linux-kernel
Cc: Maximilian Luz, Hans de Goede, Mark Gross, Arnd Bergmann,
Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Rafael J . Wysocki,
Len Brown, Steven Rostedt, Ingo Molnar, Masahiro Yamada,
Michal Marek, Jonathan Corbet, Blaž Hrastnik, Dorian Stoll,
platform-driver-x86, linux-serial, linux-acpi, linux-kbuild,
linux-doc
Hello,
N.B.: the following text is mostly a repeat of cover letter from the
previous RFC for the uninitiated, which can be found at
https://lore.kernel.org/linux-serial/20200923151511.3842150-1-luzmaximilian@gmail.com/
See "Changes" below for an overview of differences between the RFC and
this patchset. I hope I have addressed all comments from that in this
version, thank you again for those.
The Surface System Aggregator Module (we'll refer to it as Surface
Aggregator or SAM below) is an embedded controller (EC) found on various
Microsoft Surface devices. Specifically, all 4th and later generation
Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
exception of the Surface Go series and the Surface Duo. Notably, it
seems like this EC can also be found on the ARM-based Surface Pro X [1].
Functionality provided by this EC depends on the Surface model and can
(roughly) be broken down by their generations: Starting with 5th
generation devices (Surface Pro 2017/5, Surface Book 2, Surface Laptop
1, and later), the EC provides battery and thermal readings, as well as
access to the real-time clock. On 5th and 6th generations, these
features, specifically, are provided via the ACPI interface of the EC,
referred to as Surface ACPI Notify (SAN), i.e. they act as standard ACPI
devices of that type, but require a driver translating requests written
to an ACPI operation region to requests to the EC. On 7th generation
devices, the ACPI interface is (largely) gone, and has been replaced
with custom battery and thermal drivers, directly querying the EC.
Additionally, HID keyboard and touchpad input for Surface models with
these devices built in can be handled via the EC: On the Surface Laptops
1 and 2, this includes only the keyboard, while on the Surface Laptop 3
and Book 3, this includes both touchpad and keyboard. In this case,
actual input is provided as HID data and the EC connection acts as HID
transport, thus requiring a special transport driver for those devices
to work.
Further, all known devices (5th and later generations) also support
changing of performance/cooling modes, which can influence cooling
capabilities of the device (e.g. prefer silent operation over
performance), and may influence power limits (e.g. of the discrete GPU
found on Surface Books).
While this constitutes all major functionality, some more device
specific functionality is also handled by the EC. For example, on the
Surface Books, the EC handles detaching of the clipboard (i.e. the upper
part with screen and CPU) from the base (the lower part with keyboard
and optional discrete GPU) and can influence its behavior (i.e. it
provides an interface via which detachment can be requested, aborted, or
confirmed). It can also be used to detect if there has been a base
attached to the clipboard, and if so what type.
This patch-series adds the basis for supporting this EC and the features
provided by it, by, first, implementing a communication driver providing
a fundamental API for client drivers, handling specific aspects of the
EC. Additionally, it builds on top of that to provide a dedicated bus
and device type to better manage EC clients (and break it down pseudo-
device-wise), especially in the case when these client devices are not
described in ACPI, i.e. cannot be discovered by conventional means.
Furthermore, it provides support for debugging and prototyping via an
optional user-space interface, and, lastly, also support for the
aforementioned ACPI interface, allowing ACPI to communicate with the EC
directly.
This series only addresses 5th and later generation Surface models as
the communication interface has changed substantially from 4th to 5th
generation, and the 4th generation interface has not been reverse-
engineered yet. Specifically, the underlying transport has been changed
from HID feature and input-/output-reports to serial communication via
an UART and a custom protocol. Support for 4th generation devices may be
added in the future, but as currently not much is known about 4th
generation SAM, it yet remains to be seen if this can happen as addition
to this subsystem, or if it will be easier to implement this as separate
platform driver. Especially as the 4th generation EC does not seem to
provide much of the functionality found on 5th and later generations
(e.g. no battery status reporting, no thermal reporting, ..., we assume
it's just clipboard detachment on the Surface Book 1 and performance
mode setting).
In more detail, this series adds a driver for the Surface Serial Hub
(SSH), the 5th- and later-generation communication channel to the EC, a
pseudo-device and driver exposing a misc-device that can be used to
communicate with the EC from user-space, intended for debugging,
testing, and prototyping, as well as a driver for the Surface ACPI
Notify (SAN) device, i.e. the interface between ACPI and EC. Some more
details on those can be found on the individual commit messages.
This series, apart from the SAN and user-space drivers, does not add any
client drivers. This will be handled via future patches once the core
has been accepted (and the other client drivers have been cleaned up a
bit).
On the top level, EC communication via the SSH driver can be broken down
into requests (sent from host to EC), corresponding responses (sent from
EC to host, associated with and triggered by a request), and events
(sent from EC to host without association to a request). The SSH driver
manages all communication (i.e. matches responses to requests, enables
and disables events, and manages event handlers/notifiers installed by
client drivers). On the lower levels, SSH communication is packet-based,
and described in more detail in the documentation added in this series
(specifically ssh.rst).
This set of modules and drivers has been developed out of tree at [2]
and used/tested in the kernel we provide at [3] pretty much since its
beginnings. It has been developed by reverse-engineering the SSH
protocol, mostly through the ACPI interface, communication dumps
obtained from listening in on Windows, and deduction. So things may be
wrong. There have been some attempts at reverse-engineering existing
drivers, which also gave a bit of insight for development, however, I
haven't gotten very far on that front beyond some more higher-level
concepts and detecting a couple of new EC commands/confirming the
functionality of already known commands.
Driver and module names have been chosen to align with Windows driver
names, some field, variable, and concept names have been chosen to align
with ACPI code (or at least with what I think some of the more cryptic
names could mean and make sense in the respective context, e.g. IID ->
Instance ID, TC -> Target Category).
This patch-set can also be found at the following repository and
reference, if you prefer to look at a kernel tree instead of these
emails:
https://github.com/linux-surface/kernel tags/s/surface-aggregator/v1
Thanks,
Max
[1]: The Surface Pro X is, however, currently considered unsupported due
to a lack of test candidates and, as it seems, general lack of
Linux support on other parts. AFAIK there is an issue preventing
serial devices from being registered, on which the core driver in
this series is build on, thus there is no way to even test that at
this point. I'd be happy to work out any issues regarding SAM on
the Pro X at some point in the future, provided someone can/wants
to actually test it.
[2]: https://github.com/linux-surface/surface-aggregator-module
[3]: https://github.com/linux-surface/linux-surface
Note: This patch depends on
[PATCH v4] platform/surface: Create a platform subdirectory for
Microsoft Surface devices
which can be found at
https://lore.kernel.org/platform-driver-x86/20201009141128.683254-1-luzmaximilian@gmail.com/
and is currently in platform-drivers-x86/for-next.
Changes from the previous RFC (overview):
- move to platform/surface
- add copyright lines
- change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
- change user-space interface from debugfs to misc-device
- address issues in user-space interface
- fix typos in commit messages and documentation
- fix some bugs, address other issues
Changes regarding specific patches (and more details) can be found on
the individual patch.
Maximilian Luz (9):
platform/surface: Add Surface Aggregator subsystem
platform/surface: aggregator: Add control packet allocation caching
platform/surface: aggregator: Add event item allocation caching
platform/surface: aggregator: Add trace points
platform/surface: aggregator: Add error injection capabilities
platform/surface: aggregator: Add dedicated bus and device type
docs: driver-api: Add Surface Aggregator subsystem documentation
platform/surface: Add Surface Aggregator user-space interface
platform/surface: Add Surface ACPI Notify driver
Documentation/driver-api/index.rst | 1 +
.../surface_aggregator/client-api.rst | 38 +
.../driver-api/surface_aggregator/client.rst | 394 +++
.../surface_aggregator/clients/cdev.rst | 85 +
.../surface_aggregator/clients/index.rst | 21 +
.../surface_aggregator/clients/san.rst | 44 +
.../driver-api/surface_aggregator/index.rst | 21 +
.../surface_aggregator/internal-api.rst | 67 +
.../surface_aggregator/internal.rst | 50 +
.../surface_aggregator/overview.rst | 76 +
.../driver-api/surface_aggregator/ssh.rst | 343 +++
MAINTAINERS | 13 +
drivers/platform/surface/Kconfig | 39 +
drivers/platform/surface/Makefile | 3 +
drivers/platform/surface/aggregator/Kconfig | 65 +
drivers/platform/surface/aggregator/Makefile | 17 +
drivers/platform/surface/aggregator/bus.c | 424 +++
drivers/platform/surface/aggregator/bus.h | 27 +
.../platform/surface/aggregator/controller.c | 2557 +++++++++++++++++
.../platform/surface/aggregator/controller.h | 288 ++
drivers/platform/surface/aggregator/core.c | 831 ++++++
.../platform/surface/aggregator/ssh_msgb.h | 201 ++
.../surface/aggregator/ssh_packet_layer.c | 2009 +++++++++++++
.../surface/aggregator/ssh_packet_layer.h | 175 ++
.../platform/surface/aggregator/ssh_parser.c | 229 ++
.../platform/surface/aggregator/ssh_parser.h | 157 +
.../surface/aggregator/ssh_request_layer.c | 1254 ++++++++
.../surface/aggregator/ssh_request_layer.h | 142 +
drivers/platform/surface/aggregator/trace.h | 625 ++++
.../platform/surface/surface_acpi_notify.c | 884 ++++++
.../surface/surface_aggregator_cdev.c | 299 ++
include/linux/mod_devicetable.h | 18 +
include/linux/surface_acpi_notify.h | 39 +
include/linux/surface_aggregator/controller.h | 832 ++++++
include/linux/surface_aggregator/device.h | 430 +++
include/linux/surface_aggregator/serial_hub.h | 659 +++++
include/uapi/linux/surface_aggregator/cdev.h | 58 +
scripts/mod/devicetable-offsets.c | 8 +
scripts/mod/file2alias.c | 23 +
39 files changed, 13446 insertions(+)
create mode 100644 Documentation/driver-api/surface_aggregator/client-api.rst
create mode 100644 Documentation/driver-api/surface_aggregator/client.rst
create mode 100644 Documentation/driver-api/surface_aggregator/clients/cdev.rst
create mode 100644 Documentation/driver-api/surface_aggregator/clients/index.rst
create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst
create mode 100644 Documentation/driver-api/surface_aggregator/index.rst
create mode 100644 Documentation/driver-api/surface_aggregator/internal-api.rst
create mode 100644 Documentation/driver-api/surface_aggregator/internal.rst
create mode 100644 Documentation/driver-api/surface_aggregator/overview.rst
create mode 100644 Documentation/driver-api/surface_aggregator/ssh.rst
create mode 100644 drivers/platform/surface/aggregator/Kconfig
create mode 100644 drivers/platform/surface/aggregator/Makefile
create mode 100644 drivers/platform/surface/aggregator/bus.c
create mode 100644 drivers/platform/surface/aggregator/bus.h
create mode 100644 drivers/platform/surface/aggregator/controller.c
create mode 100644 drivers/platform/surface/aggregator/controller.h
create mode 100644 drivers/platform/surface/aggregator/core.c
create mode 100644 drivers/platform/surface/aggregator/ssh_msgb.h
create mode 100644 drivers/platform/surface/aggregator/ssh_packet_layer.c
create mode 100644 drivers/platform/surface/aggregator/ssh_packet_layer.h
create mode 100644 drivers/platform/surface/aggregator/ssh_parser.c
create mode 100644 drivers/platform/surface/aggregator/ssh_parser.h
create mode 100644 drivers/platform/surface/aggregator/ssh_request_layer.c
create mode 100644 drivers/platform/surface/aggregator/ssh_request_layer.h
create mode 100644 drivers/platform/surface/aggregator/trace.h
create mode 100644 drivers/platform/surface/surface_acpi_notify.c
create mode 100644 drivers/platform/surface/surface_aggregator_cdev.c
create mode 100644 include/linux/surface_acpi_notify.h
create mode 100644 include/linux/surface_aggregator/controller.h
create mode 100644 include/linux/surface_aggregator/device.h
create mode 100644 include/linux/surface_aggregator/serial_hub.h
create mode 100644 include/uapi/linux/surface_aggregator/cdev.h
--
2.29.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 9/9] platform/surface: Add Surface ACPI Notify driver
2020-11-15 19:21 [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module Maximilian Luz
@ 2020-11-15 19:21 ` Maximilian Luz
2020-11-17 20:09 ` Randy Dunlap
[not found] ` <20201115192143.21571-2-luzmaximilian@gmail.com>
2020-11-24 11:59 ` [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module Hans de Goede
2 siblings, 1 reply; 13+ messages in thread
From: Maximilian Luz @ 2020-11-15 19:21 UTC (permalink / raw)
To: linux-kernel
Cc: Maximilian Luz, Hans de Goede, Mark Gross, Arnd Bergmann,
Greg Kroah-Hartman, Rafael J . Wysocki, Len Brown,
Jonathan Corbet, Blaž Hrastnik, Dorian Stoll,
platform-driver-x86, linux-acpi, linux-doc
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>
---
Changes from RFC:
- add copyright lines
- change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
- fix invalid pointer calculation in san_evt_tmp
- add explicit dependency on CONFIG_ACPI
- remove default in Kconfig
---
.../surface_aggregator/clients/index.rst | 1 +
.../surface_aggregator/clients/san.rst | 44 +
MAINTAINERS | 2 +
drivers/platform/surface/Kconfig | 20 +
drivers/platform/surface/Makefile | 1 +
.../platform/surface/surface_acpi_notify.c | 884 ++++++++++++++++++
include/linux/surface_acpi_notify.h | 39 +
7 files changed, 991 insertions(+)
create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst
create mode 100644 drivers/platform/surface/surface_acpi_notify.c
create mode 100644 include/linux/surface_acpi_notify.h
diff --git a/Documentation/driver-api/surface_aggregator/clients/index.rst b/Documentation/driver-api/surface_aggregator/clients/index.rst
index ab260ec82cfb..3ccabce23271 100644
--- a/Documentation/driver-api/surface_aggregator/clients/index.rst
+++ b/Documentation/driver-api/surface_aggregator/clients/index.rst
@@ -11,6 +11,7 @@ This is the documentation for client drivers themselves. Refer to
:maxdepth: 1
cdev
+ san
.. only:: subproject and html
diff --git a/Documentation/driver-api/surface_aggregator/clients/san.rst b/Documentation/driver-api/surface_aggregator/clients/san.rst
new file mode 100644
index 000000000000..38c2580e7758
--- /dev/null
+++ b/Documentation/driver-api/surface_aggregator/clients/san.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+.. |san_client_link| replace:: :c:func:`san_client_link`
+.. |san_dgpu_notifier_register| replace:: :c:func:`san_dgpu_notifier_register`
+.. |san_dgpu_notifier_unregister| replace:: :c:func:`san_dgpu_notifier_unregister`
+
+===================
+Surface ACPI Notify
+===================
+
+The Surface ACPI Notify (SAN) device provides the bridge between ACPI and
+SAM controller. Specifically, ACPI code can execute requests and handle
+battery and thermal events via this interface. In addition to this, events
+relating to the discrete GPU (dGPU) of the Surface Book 2 can be sent from
+ACPI code (note: the Surface Book 3 uses a different method for this). The
+only currently known event sent via this interface is a dGPU power-on
+notification. While this driver handles the former part internally, it only
+relays the dGPU events to any other driver interested via its public API and
+does not handle them.
+
+The public interface of this driver is split into two parts: Client
+registration and notifier-block registration.
+
+A client to the SAN interface can be linked as consumer to the SAN device
+via |san_client_link|. This can be used to ensure that the a client
+receiving dGPU events does not miss any events due to the SAN interface not
+being set up as this forces the client driver to unbind once the SAN driver
+is unbound.
+
+Notifier-blocks can be registered by any device for as long as the module is
+loaded, regardless of being linked as client or not. Registration is done
+with |san_dgpu_notifier_register|. If the notifier is not needed any more, it
+should be unregistered via |san_dgpu_notifier_unregister|.
+
+Consult the API documentation below for more details.
+
+
+API Documentation
+=================
+
+.. kernel-doc:: include/linux/surface_acpi_notify.h
+
+.. kernel-doc:: drivers/platform/surface/surface_acpi_notify.c
+ :export:
diff --git a/MAINTAINERS b/MAINTAINERS
index c776343a49c6..f15431cbf8ff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11699,7 +11699,9 @@ W: https://github.com/linux-surface/surface-aggregator-module
C: irc://chat.freenode.net/##linux-surface
F: Documentation/driver-api/surface_aggregator/
F: drivers/platform/surface/aggregator/
+F: drivers/platform/surface/surface_acpi_notify.c
F: drivers/platform/surface/surface_aggregator_cdev.c
+F: include/linux/surface_acpi_notify.h
F: include/linux/surface_aggregator/
F: include/uapi/linux/surface_aggregator/
diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
index cecad7a0cb7b..3b0da86546fa 100644
--- a/drivers/platform/surface/Kconfig
+++ b/drivers/platform/surface/Kconfig
@@ -40,6 +40,26 @@ config SURFACE_3_POWER_OPREGION
This driver provides support for ACPI operation
region of the Surface 3 battery platform driver.
+config SURFACE_ACPI_NOTIFY
+ tristate "Surface ACPI Notify Driver"
+ depends on ACPI
+ depends on SURFACE_AGGREGATOR
+ help
+ Surface ACPI Notify (SAN) driver for Microsoft Surface devices.
+
+ This driver provides support for the ACPI interface (called SAN) of
+ the Surface System Aggregator Module (SSAM) EC. This interface is used
+ on 5th- and 6th-generation Microsoft Surface devices (including
+ Surface Pro 5 and 6, Surface Book 2, Surface Laptops 1 and 2, and in
+ reduced functionality on the Surface Laptop 3) to execute SSAM
+ requests directly from ACPI code, as well as receive SSAM events and
+ turn them into ACPI notifications. It essentially acts as a
+ translation layer between the SSAM controller and ACPI.
+
+ Specifically, this driver may be needed for battery status reporting,
+ thermal sensor access, and real-time clock information, depending on
+ the Surface device in question.
+
config SURFACE_AGGREGATOR_CDEV
tristate "Surface System Aggregator Module User-Space Interface"
depends on SURFACE_AGGREGATOR
diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
index 161f0ad05795..3eb971006877 100644
--- a/drivers/platform/surface/Makefile
+++ b/drivers/platform/surface/Makefile
@@ -7,6 +7,7 @@
obj-$(CONFIG_SURFACE3_WMI) += surface3-wmi.o
obj-$(CONFIG_SURFACE_3_BUTTON) += surface3_button.o
obj-$(CONFIG_SURFACE_3_POWER_OPREGION) += surface3_power.o
+obj-$(CONFIG_SURFACE_ACPI_NOTIFY) += surface_acpi_notify.o
obj-$(CONFIG_SURFACE_AGGREGATOR) += aggregator/
obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o
obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o
diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c
new file mode 100644
index 000000000000..7a5b8f280036
--- /dev/null
+++ b/drivers/platform/surface/surface_acpi_notify.c
@@ -0,0 +1,884 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for the Surface ACPI Notify (SAN) interface/shim.
+ *
+ * Translates communication from ACPI to Surface System Aggregator Module
+ * (SSAM/SAM) requests and back, specifically SAM-over-SSH. Translates SSAM
+ * events back to ACPI notifications. Allows handling of discrete GPU
+ * notifications sent from ACPI via the SAN interface by providing them to any
+ * registered external driver.
+ *
+ * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
+ */
+
+#include <asm/unaligned.h>
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/rwsem.h>
+
+#include <linux/surface_aggregator/controller.h>
+#include <linux/surface_acpi_notify.h>
+
+
+struct san_data {
+ struct device *dev;
+ struct ssam_controller *ctrl;
+
+ struct acpi_connection_info info;
+
+ struct ssam_event_notifier nf_bat;
+ struct ssam_event_notifier nf_tmp;
+};
+
+#define to_san_data(ptr, member) \
+ container_of(ptr, struct san_data, member)
+
+
+/* -- dGPU notifier interface. ---------------------------------------------- */
+
+struct san_rqsg_if {
+ struct rw_semaphore lock;
+ struct device *dev;
+ struct blocking_notifier_head nh;
+};
+
+static struct san_rqsg_if san_rqsg_if = {
+ .lock = __RWSEM_INITIALIZER(san_rqsg_if.lock),
+ .dev = NULL,
+ .nh = BLOCKING_NOTIFIER_INIT(san_rqsg_if.nh),
+};
+
+static int san_set_rqsg_interface_device(struct device *dev)
+{
+ int status = 0;
+
+ down_write(&san_rqsg_if.lock);
+ if (!san_rqsg_if.dev && dev)
+ san_rqsg_if.dev = dev;
+ else
+ status = -EBUSY;
+ up_write(&san_rqsg_if.lock);
+
+ return status;
+}
+
+/**
+ * san_client_link() - Link client as consumer to SAN device.
+ * @client: The client to link.
+ *
+ * Sets up a device link between the provided client device as consumer and
+ * the SAN device as provider. This function can be used to ensure that the
+ * SAN interface has been set up and will be set up for as long as the driver
+ * of the client device is bound. This guarantees that, during that time, all
+ * dGPU events will be received by any registered notifier.
+ *
+ * The link will be automatically removed once the client device's driver is
+ * unbound.
+ *
+ * Return: Returns zero on succes, %-ENXIO if the SAN interface has not been
+ * set up yet, and %-ENOMEM if device link creation failed.
+ */
+int san_client_link(struct device *client)
+{
+ const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
+ struct device_link *link;
+
+ down_read(&san_rqsg_if.lock);
+
+ if (!san_rqsg_if.dev) {
+ up_read(&san_rqsg_if.lock);
+ return -ENXIO;
+ }
+
+ link = device_link_add(client, san_rqsg_if.dev, flags);
+ if (!link) {
+ up_read(&san_rqsg_if.lock);
+ return -ENOMEM;
+ }
+
+ if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND) {
+ up_read(&san_rqsg_if.lock);
+ return -ENXIO;
+ }
+
+ up_read(&san_rqsg_if.lock);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(san_client_link);
+
+/**
+ * san_dgpu_notifier_register() - Register a SAN dGPU notifier.
+ * @nb: The notifier-block to register.
+ *
+ * Registers a SAN dGPU notifier, receiving any new SAN dGPU events sent from
+ * ACPI. The registered notifier will be called with &struct san_dgpu_event
+ * as notifier data and the command ID of that event as notifier action.
+ */
+int san_dgpu_notifier_register(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&san_rqsg_if.nh, nb);
+}
+EXPORT_SYMBOL_GPL(san_dgpu_notifier_register);
+
+/**
+ * san_dgpu_notifier_unregister() - Unregister a SAN dGPU notifier.
+ * @nb: The notifier-block to unregister.
+ */
+int san_dgpu_notifier_unregister(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&san_rqsg_if.nh, nb);
+}
+EXPORT_SYMBOL_GPL(san_dgpu_notifier_unregister);
+
+static int san_dgpu_notifier_call(struct san_dgpu_event *evt)
+{
+ int ret;
+
+ ret = blocking_notifier_call_chain(&san_rqsg_if.nh, evt->command, evt);
+ return notifier_to_errno(ret);
+}
+
+
+/* -- ACPI _DSM event relay. ------------------------------------------------ */
+
+#define SAN_DSM_REVISION 0
+
+static const guid_t SAN_DSM_UUID =
+ GUID_INIT(0x93b666c5, 0x70c6, 0x469f, 0xa2, 0x15, 0x3d,
+ 0x48, 0x7c, 0x91, 0xab, 0x3c);
+
+enum san_dsm_event_fn {
+ SAN_DSM_EVENT_FN_BAT1_STAT = 0x03,
+ SAN_DSM_EVENT_FN_BAT1_INFO = 0x04,
+ SAN_DSM_EVENT_FN_ADP1_STAT = 0x05,
+ SAN_DSM_EVENT_FN_ADP1_INFO = 0x06,
+ SAN_DSM_EVENT_FN_BAT2_STAT = 0x07,
+ SAN_DSM_EVENT_FN_BAT2_INFO = 0x08,
+ SAN_DSM_EVENT_FN_THERMAL = 0x09,
+ SAN_DSM_EVENT_FN_DPTF = 0x0a,
+};
+
+enum sam_event_cid_bat {
+ SAM_EVENT_CID_BAT_BIX = 0x15,
+ SAM_EVENT_CID_BAT_BST = 0x16,
+ SAM_EVENT_CID_BAT_ADP = 0x17,
+ SAM_EVENT_CID_BAT_PROT = 0x18,
+ SAM_EVENT_CID_BAT_DPTF = 0x4f,
+};
+
+enum sam_event_cid_tmp {
+ SAM_EVENT_CID_TMP_TRIP = 0x0b,
+};
+
+struct san_event_work {
+ struct delayed_work work;
+ struct device *dev;
+ struct ssam_event event; // must be last
+};
+
+static int san_acpi_notify_event(struct device *dev, u64 func,
+ union acpi_object *param)
+{
+ acpi_handle san = ACPI_HANDLE(dev);
+ union acpi_object *obj;
+ int status = 0;
+
+ if (!acpi_check_dsm(san, &SAN_DSM_UUID, SAN_DSM_REVISION, 1 << func))
+ return 0;
+
+ dev_dbg(dev, "notify event 0x%02llx\n", func);
+
+ obj = acpi_evaluate_dsm_typed(san, &SAN_DSM_UUID, SAN_DSM_REVISION,
+ func, param, ACPI_TYPE_BUFFER);
+ if (!obj)
+ return -EFAULT;
+
+ if (obj->buffer.length != 1 || obj->buffer.pointer[0] != 0) {
+ dev_err(dev, "got unexpected result from _DSM\n");
+ status = -EPROTO;
+ }
+
+ ACPI_FREE(obj);
+ return status;
+}
+
+static int san_evt_bat_adp(struct device *dev, const struct ssam_event *event)
+{
+ int status;
+
+ status = san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_ADP1_STAT, NULL);
+ if (status)
+ return status;
+
+ /*
+ * Enusre that the battery states get updated correctly.
+ * When the battery is fully charged and an adapter is plugged in, it
+ * sometimes is not updated correctly, instead showing it as charging.
+ * Explicitly trigger battery updates to fix this.
+ */
+
+ status = san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_BAT1_STAT, NULL);
+ if (status)
+ return status;
+
+ return san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_BAT2_STAT, NULL);
+}
+
+static int san_evt_bat_bix(struct device *dev, const struct ssam_event *event)
+{
+ enum san_dsm_event_fn fn;
+
+ if (event->instance_id == 0x02)
+ fn = SAN_DSM_EVENT_FN_BAT2_INFO;
+ else
+ fn = SAN_DSM_EVENT_FN_BAT1_INFO;
+
+ return san_acpi_notify_event(dev, fn, NULL);
+}
+
+static int san_evt_bat_bst(struct device *dev, const struct ssam_event *event)
+{
+ enum san_dsm_event_fn fn;
+
+ if (event->instance_id == 0x02)
+ fn = SAN_DSM_EVENT_FN_BAT2_STAT;
+ else
+ fn = SAN_DSM_EVENT_FN_BAT1_STAT;
+
+ return san_acpi_notify_event(dev, fn, NULL);
+}
+
+static int san_evt_bat_dptf(struct device *dev, const struct ssam_event *event)
+{
+ union acpi_object payload;
+
+ /*
+ * The Surface ACPI expects a buffer and not a package. It specifically
+ * checks for ObjectType (Arg3) == 0x03. This will cause a warning in
+ * acpica/nsarguments.c, but that warning can be safely ignored.
+ */
+ payload.type = ACPI_TYPE_BUFFER;
+ payload.buffer.length = event->length;
+ payload.buffer.pointer = (u8 *)&event->data[0];
+
+ return san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_DPTF, &payload);
+}
+
+static unsigned long san_evt_bat_delay(u8 cid)
+{
+ switch (cid) {
+ case SAM_EVENT_CID_BAT_ADP:
+ /*
+ * Wait for battery state to update before signalling adapter
+ * change.
+ */
+ return msecs_to_jiffies(5000);
+
+ case SAM_EVENT_CID_BAT_BST:
+ /* Ensure we do not miss anything important due to caching. */
+ return msecs_to_jiffies(2000);
+
+ default:
+ return 0;
+ }
+}
+
+static bool san_evt_bat(const struct ssam_event *event, struct device *dev)
+{
+ int status;
+
+ switch (event->command_id) {
+ case SAM_EVENT_CID_BAT_BIX:
+ status = san_evt_bat_bix(dev, event);
+ break;
+
+ case SAM_EVENT_CID_BAT_BST:
+ status = san_evt_bat_bst(dev, event);
+ break;
+
+ case SAM_EVENT_CID_BAT_ADP:
+ status = san_evt_bat_adp(dev, event);
+ break;
+
+ case SAM_EVENT_CID_BAT_PROT:
+ /*
+ * TODO: Implement support for battery protection status change
+ * event.
+ */
+ return true;
+
+ case SAM_EVENT_CID_BAT_DPTF:
+ status = san_evt_bat_dptf(dev, event);
+ break;
+
+ default:
+ return false;
+ }
+
+ if (status)
+ dev_err(dev, "error handling power event (cid = %x)\n",
+ event->command_id);
+
+ return true;
+}
+
+static void san_evt_bat_workfn(struct work_struct *work)
+{
+ struct san_event_work *ev;
+
+ ev = container_of(work, struct san_event_work, work.work);
+ san_evt_bat(&ev->event, ev->dev);
+ kfree(ev);
+}
+
+static u32 san_evt_bat_nf(struct ssam_event_notifier *nf,
+ const struct ssam_event *event)
+{
+ struct san_data *d = to_san_data(nf, nf_bat);
+ struct san_event_work *work;
+ unsigned long delay = san_evt_bat_delay(event->command_id);
+
+ if (delay == 0)
+ return san_evt_bat(event, d->dev) ? SSAM_NOTIF_HANDLED : 0;
+
+ work = kzalloc(sizeof(*work) + event->length, GFP_KERNEL);
+ if (!work)
+ return ssam_notifier_from_errno(-ENOMEM);
+
+ INIT_DELAYED_WORK(&work->work, san_evt_bat_workfn);
+ work->dev = d->dev;
+
+ memcpy(&work->event, event, sizeof(struct ssam_event) + event->length);
+
+ schedule_delayed_work(&work->work, delay);
+ return SSAM_NOTIF_HANDLED;
+}
+
+static int san_evt_tmp_trip(struct device *dev, const struct ssam_event *event)
+{
+ union acpi_object param;
+
+ /*
+ * The Surface ACPI expects an integer and not a package. This will
+ * cause a warning in acpica/nsarguments.c, but that warning can be
+ * safely ignored.
+ */
+ param.type = ACPI_TYPE_INTEGER;
+ param.integer.value = event->instance_id;
+
+ return san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_THERMAL, ¶m);
+}
+
+static bool san_evt_tmp(const struct ssam_event *event, struct device *dev)
+{
+ int status;
+
+ switch (event->command_id) {
+ case SAM_EVENT_CID_TMP_TRIP:
+ status = san_evt_tmp_trip(dev, event);
+ break;
+
+ default:
+ return false;
+ }
+
+ if (status) {
+ dev_err(dev, "error handling thermal event (cid = %x)\n",
+ event->command_id);
+ }
+
+ return true;
+}
+
+static u32 san_evt_tmp_nf(struct ssam_event_notifier *nf,
+ const struct ssam_event *event)
+{
+ struct san_data *d = to_san_data(nf, nf_tmp);
+
+ return san_evt_tmp(event, d->dev) ? SSAM_NOTIF_HANDLED : 0;
+}
+
+
+/* -- ACPI GSB OperationRegion handler -------------------------------------- */
+
+struct gsb_data_in {
+ u8 cv;
+} __packed;
+
+struct gsb_data_rqsx {
+ u8 cv; // command value (san_gsb_request_cv)
+ u8 tc; // target category
+ u8 tid; // target ID
+ u8 iid; // instance ID
+ u8 snc; // expect-response-flag?
+ u8 cid; // command ID
+ u16 cdl; // payload length
+ u8 pld[]; // payload
+} __packed;
+
+struct gsb_data_etwl {
+ u8 cv; // command value (should be 0x02)
+ u8 etw3; // unknown
+ u8 etw4; // unknown
+ u8 msg[]; // error message (ASCIIZ)
+} __packed;
+
+struct gsb_data_out {
+ u8 status; // _SSH communication status
+ u8 len; // _SSH payload length
+ u8 pld[]; // _SSH payload
+} __packed;
+
+union gsb_buffer_data {
+ struct gsb_data_in in; // common input
+ struct gsb_data_rqsx rqsx; // RQSX input
+ struct gsb_data_etwl etwl; // ETWL input
+ struct gsb_data_out out; // output
+};
+
+struct gsb_buffer {
+ u8 status; // GSB AttribRawProcess status
+ u8 len; // GSB AttribRawProcess length
+ union gsb_buffer_data data;
+} __packed;
+
+#define SAN_GSB_MAX_RQSX_PAYLOAD (U8_MAX - 2 - sizeof(struct gsb_data_rqsx))
+#define SAN_GSB_MAX_RESPONSE (U8_MAX - 2 - sizeof(struct gsb_data_out))
+
+#define SAN_GSB_COMMAND 0
+
+enum san_gsb_request_cv {
+ SAN_GSB_REQUEST_CV_RQST = 0x01,
+ SAN_GSB_REQUEST_CV_ETWL = 0x02,
+ SAN_GSB_REQUEST_CV_RQSG = 0x03,
+};
+
+#define SAN_REQUEST_NUM_TRIES 5
+
+static acpi_status san_etwl(struct san_data *d, struct gsb_buffer *b)
+{
+ struct gsb_data_etwl *etwl = &b->data.etwl;
+
+ if (b->len < sizeof(struct gsb_data_etwl)) {
+ dev_err(d->dev, "invalid ETWL package (len = %d)\n", b->len);
+ return AE_OK;
+ }
+
+ dev_err(d->dev, "ETWL(0x%02x, 0x%02x): %.*s\n", etwl->etw3, etwl->etw4,
+ (unsigned int)(b->len - sizeof(struct gsb_data_etwl)),
+ (char *)etwl->msg);
+
+ // indicate success
+ b->status = 0x00;
+ b->len = 0x00;
+
+ return AE_OK;
+}
+
+static struct gsb_data_rqsx *san_validate_rqsx(struct device *dev,
+ const char *type, struct gsb_buffer *b)
+{
+ struct gsb_data_rqsx *rqsx = &b->data.rqsx;
+
+ if (b->len < sizeof(struct gsb_data_rqsx)) {
+ dev_err(dev, "invalid %s package (len = %d)\n", type, b->len);
+ return NULL;
+ }
+
+ if (get_unaligned(&rqsx->cdl) != b->len - sizeof(struct gsb_data_rqsx)) {
+ dev_err(dev, "bogus %s package (len = %d, cdl = %d)\n",
+ type, b->len, get_unaligned(&rqsx->cdl));
+ return NULL;
+ }
+
+ if (get_unaligned(&rqsx->cdl) > SAN_GSB_MAX_RQSX_PAYLOAD) {
+ dev_err(dev, "payload for %s package too large (cdl = %d)\n",
+ type, get_unaligned(&rqsx->cdl));
+ return NULL;
+ }
+
+ return rqsx;
+}
+
+static void gsb_rqsx_response_error(struct gsb_buffer *gsb, int status)
+{
+ gsb->status = 0x00;
+ gsb->len = 0x02;
+ gsb->data.out.status = (u8)(-status);
+ gsb->data.out.len = 0x00;
+}
+
+static void gsb_rqsx_response_success(struct gsb_buffer *gsb, u8 *ptr, size_t len)
+{
+ gsb->status = 0x00;
+ gsb->len = len + 2;
+ gsb->data.out.status = 0x00;
+ gsb->data.out.len = len;
+
+ if (len)
+ memcpy(&gsb->data.out.pld[0], ptr, len);
+}
+
+static acpi_status san_rqst_fixup_suspended(struct san_data *d,
+ struct ssam_request *rqst,
+ struct gsb_buffer *gsb)
+{
+ if (rqst->target_category == SSAM_SSH_TC_BAS && rqst->command_id == 0x0D) {
+ u8 base_state = 1;
+
+ /* Base state quirk:
+ * The base state may be queried from ACPI when the EC is still
+ * suspended. In this case it will return '-EPERM'. This query
+ * will only be triggered from the ACPI lid GPE interrupt, thus
+ * we are either in laptop or studio mode (base status 0x01 or
+ * 0x02). Furthermore, we will only get here if the device (and
+ * EC) have been suspended.
+ *
+ * We now assume that the device is in laptop mode (0x01). This
+ * has the drawback that it will wake the device when unfolding
+ * it in studio mode, but it also allows us to avoid actively
+ * waiting for the EC to wake up, which may incur a notable
+ * delay.
+ */
+
+ dev_dbg(d->dev, "rqst: fixup: base-state quirk\n");
+
+ gsb_rqsx_response_success(gsb, &base_state, sizeof(base_state));
+ return AE_OK;
+ }
+
+ gsb_rqsx_response_error(gsb, -ENXIO);
+ return AE_OK;
+}
+
+static acpi_status san_rqst(struct san_data *d, struct gsb_buffer *buffer)
+{
+ u8 rspbuf[SAN_GSB_MAX_RESPONSE];
+ struct gsb_data_rqsx *gsb_rqst;
+ struct ssam_request rqst;
+ struct ssam_response rsp;
+ int status = 0;
+
+ gsb_rqst = san_validate_rqsx(d->dev, "RQST", buffer);
+ if (!gsb_rqst)
+ return AE_OK;
+
+ rqst.target_category = gsb_rqst->tc;
+ rqst.target_id = gsb_rqst->tid;
+ rqst.command_id = gsb_rqst->cid;
+ rqst.instance_id = gsb_rqst->iid;
+ rqst.flags = gsb_rqst->snc ? SSAM_REQUEST_HAS_RESPONSE : 0;
+ rqst.length = get_unaligned(&gsb_rqst->cdl);
+ rqst.payload = &gsb_rqst->pld[0];
+
+ rsp.capacity = ARRAY_SIZE(rspbuf);
+ rsp.length = 0;
+ rsp.pointer = &rspbuf[0];
+
+ // handle suspended device
+ if (d->dev->power.is_suspended) {
+ dev_warn(d->dev, "rqst: device is suspended, not executing\n");
+ return san_rqst_fixup_suspended(d, &rqst, buffer);
+ }
+
+ status = __ssam_retry(ssam_request_sync_onstack, SAN_REQUEST_NUM_TRIES,
+ d->ctrl, &rqst, &rsp, SAN_GSB_MAX_RQSX_PAYLOAD);
+
+ if (!status) {
+ gsb_rqsx_response_success(buffer, rsp.pointer, rsp.length);
+ } else {
+ dev_err(d->dev, "rqst: failed with error %d\n", status);
+ gsb_rqsx_response_error(buffer, status);
+ }
+
+ return AE_OK;
+}
+
+static acpi_status san_rqsg(struct san_data *d, struct gsb_buffer *buffer)
+{
+ struct gsb_data_rqsx *gsb_rqsg;
+ struct san_dgpu_event evt;
+ int status;
+
+ gsb_rqsg = san_validate_rqsx(d->dev, "RQSG", buffer);
+ if (!gsb_rqsg)
+ return AE_OK;
+
+ evt.category = gsb_rqsg->tc;
+ evt.target = gsb_rqsg->tid;
+ evt.command = gsb_rqsg->cid;
+ evt.instance = gsb_rqsg->iid;
+ evt.length = get_unaligned(&gsb_rqsg->cdl);
+ evt.payload = &gsb_rqsg->pld[0];
+
+ status = san_dgpu_notifier_call(&evt);
+ if (!status) {
+ gsb_rqsx_response_success(buffer, NULL, 0);
+ } else {
+ dev_err(d->dev, "rqsg: failed with error %d\n", status);
+ gsb_rqsx_response_error(buffer, status);
+ }
+
+ return AE_OK;
+}
+
+static acpi_status san_opreg_handler(u32 function,
+ acpi_physical_address command, u32 bits, u64 *value64,
+ void *opreg_context, void *region_context)
+{
+ struct san_data *d = to_san_data(opreg_context, info);
+ struct gsb_buffer *buffer = (struct gsb_buffer *)value64;
+ int accessor_type = (function & 0xFFFF0000) >> 16;
+
+ if (command != SAN_GSB_COMMAND) {
+ dev_warn(d->dev, "unsupported command: 0x%02llx\n", command);
+ return AE_OK;
+ }
+
+ if (accessor_type != ACPI_GSB_ACCESS_ATTRIB_RAW_PROCESS) {
+ dev_err(d->dev, "invalid access type: 0x%02x\n", accessor_type);
+ return AE_OK;
+ }
+
+ // buffer must have at least contain the command-value
+ if (buffer->len == 0) {
+ dev_err(d->dev, "request-package too small\n");
+ return AE_OK;
+ }
+
+ switch (buffer->data.in.cv) {
+ case SAN_GSB_REQUEST_CV_RQST:
+ return san_rqst(d, buffer);
+
+ case SAN_GSB_REQUEST_CV_ETWL:
+ return san_etwl(d, buffer);
+
+ case SAN_GSB_REQUEST_CV_RQSG:
+ return san_rqsg(d, buffer);
+
+ default:
+ dev_warn(d->dev, "unsupported SAN0 request (cv: 0x%02x)\n",
+ buffer->data.in.cv);
+ return AE_OK;
+ }
+}
+
+
+/* -- Driver setup. --------------------------------------------------------- */
+
+static int san_events_register(struct platform_device *pdev)
+{
+ struct san_data *d = platform_get_drvdata(pdev);
+ int status;
+
+ d->nf_bat.base.priority = 1;
+ d->nf_bat.base.fn = san_evt_bat_nf;
+ d->nf_bat.event.reg = SSAM_EVENT_REGISTRY_SAM;
+ d->nf_bat.event.id.target_category = SSAM_SSH_TC_BAT;
+ d->nf_bat.event.id.instance = 0;
+ d->nf_bat.event.mask = SSAM_EVENT_MASK_TARGET;
+ d->nf_bat.event.flags = SSAM_EVENT_SEQUENCED;
+
+ d->nf_tmp.base.priority = 1;
+ d->nf_tmp.base.fn = san_evt_tmp_nf;
+ d->nf_tmp.event.reg = SSAM_EVENT_REGISTRY_SAM;
+ d->nf_tmp.event.id.target_category = SSAM_SSH_TC_TMP;
+ d->nf_tmp.event.id.instance = 0;
+ d->nf_tmp.event.mask = SSAM_EVENT_MASK_TARGET;
+ d->nf_tmp.event.flags = SSAM_EVENT_SEQUENCED;
+
+ status = ssam_notifier_register(d->ctrl, &d->nf_bat);
+ if (status)
+ return status;
+
+ status = ssam_notifier_register(d->ctrl, &d->nf_tmp);
+ if (status)
+ ssam_notifier_unregister(d->ctrl, &d->nf_bat);
+
+ return status;
+}
+
+static void san_events_unregister(struct platform_device *pdev)
+{
+ struct san_data *d = platform_get_drvdata(pdev);
+
+ ssam_notifier_unregister(d->ctrl, &d->nf_bat);
+ ssam_notifier_unregister(d->ctrl, &d->nf_tmp);
+}
+
+#define san_consumer_printk(level, dev, handle, fmt, ...) \
+do { \
+ char *path = "<error getting consumer path>"; \
+ struct acpi_buffer buffer = { \
+ .length = ACPI_ALLOCATE_BUFFER, \
+ .pointer = NULL, \
+ }; \
+ \
+ if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer))) \
+ path = buffer.pointer; \
+ \
+ dev_##level(dev, "[%s]: " fmt, path, ##__VA_ARGS__); \
+ kfree(buffer.pointer); \
+} while (0)
+
+#define san_consumer_dbg(dev, handle, fmt, ...) \
+ san_consumer_printk(dbg, dev, handle, fmt, ##__VA_ARGS__)
+
+#define san_consumer_warn(dev, handle, fmt, ...) \
+ san_consumer_printk(warn, dev, handle, fmt, ##__VA_ARGS__)
+
+static bool is_san_consumer(struct platform_device *pdev, acpi_handle handle)
+{
+ struct acpi_handle_list dep_devices;
+ acpi_handle supplier = ACPI_HANDLE(&pdev->dev);
+ acpi_status status;
+ int i;
+
+ if (!acpi_has_method(handle, "_DEP"))
+ return false;
+
+ status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
+ if (ACPI_FAILURE(status)) {
+ san_consumer_dbg(&pdev->dev, handle, "failed to evaluate _DEP\n");
+ return false;
+ }
+
+ for (i = 0; i < dep_devices.count; i++) {
+ if (dep_devices.handles[i] == supplier)
+ return true;
+ }
+
+ return false;
+}
+
+static acpi_status san_consumer_setup(acpi_handle handle, u32 lvl,
+ void *context, void **rv)
+{
+ const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER;
+ struct platform_device *pdev = context;
+ struct acpi_device *adev;
+ struct device_link *link;
+
+ if (!is_san_consumer(pdev, handle))
+ return AE_OK;
+
+ // ignore ACPI devices that are not present
+ if (acpi_bus_get_device(handle, &adev) != 0)
+ return AE_OK;
+
+ san_consumer_dbg(&pdev->dev, handle, "creating device link\n");
+
+ // try to set up device links, ignore but log errors
+ link = device_link_add(&adev->dev, &pdev->dev, flags);
+ if (!link) {
+ san_consumer_warn(&pdev->dev, handle,
+ "failed to create device link\n");
+ return AE_OK;
+ }
+
+ return AE_OK;
+}
+
+static int san_consumer_links_setup(struct platform_device *pdev)
+{
+ acpi_status status;
+
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+ ACPI_UINT32_MAX, san_consumer_setup, NULL,
+ pdev, NULL);
+
+ return status ? -EFAULT : 0;
+}
+
+static int san_probe(struct platform_device *pdev)
+{
+ acpi_handle san = ACPI_HANDLE(&pdev->dev);
+ struct ssam_controller *ctrl;
+ struct san_data *data;
+ acpi_status astatus;
+ int status;
+
+ status = ssam_client_bind(&pdev->dev, &ctrl);
+ if (status)
+ return status == -ENXIO ? -EPROBE_DEFER : status;
+
+ status = san_consumer_links_setup(pdev);
+ if (status)
+ return status;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->dev = &pdev->dev;
+ data->ctrl = ctrl;
+
+ platform_set_drvdata(pdev, data);
+
+ astatus = acpi_install_address_space_handler(san, ACPI_ADR_SPACE_GSBUS,
+ &san_opreg_handler, NULL, &data->info);
+ if (ACPI_FAILURE(astatus))
+ return -ENXIO;
+
+ status = san_events_register(pdev);
+ if (status)
+ goto err_enable_events;
+
+ status = san_set_rqsg_interface_device(&pdev->dev);
+ if (status)
+ goto err_install_dev;
+
+ acpi_walk_dep_device_list(san);
+ return 0;
+
+err_install_dev:
+ san_events_unregister(pdev);
+err_enable_events:
+ acpi_remove_address_space_handler(san, ACPI_ADR_SPACE_GSBUS,
+ &san_opreg_handler);
+ return status;
+}
+
+static int san_remove(struct platform_device *pdev)
+{
+ acpi_handle san = ACPI_HANDLE(&pdev->dev);
+
+ san_set_rqsg_interface_device(NULL);
+ acpi_remove_address_space_handler(san, ACPI_ADR_SPACE_GSBUS,
+ &san_opreg_handler);
+ san_events_unregister(pdev);
+
+ /*
+ * We have unregistered our event sources. Now we need to ensure that
+ * all delayed works they may have spawned are run to completion.
+ */
+ flush_scheduled_work();
+
+ return 0;
+}
+
+static const struct acpi_device_id san_match[] = {
+ { "MSHW0091" },
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, san_match);
+
+static struct platform_driver surface_acpi_notify = {
+ .probe = san_probe,
+ .remove = san_remove,
+ .driver = {
+ .name = "surface_acpi_notify",
+ .acpi_match_table = san_match,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+};
+module_platform_driver(surface_acpi_notify);
+
+MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
+MODULE_DESCRIPTION("Surface ACPI Notify driver for Surface System Aggregator Module");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/surface_acpi_notify.h b/include/linux/surface_acpi_notify.h
new file mode 100644
index 000000000000..8e3e86c7d78c
--- /dev/null
+++ b/include/linux/surface_acpi_notify.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Interface for Surface ACPI Notify (SAN) driver.
+ *
+ * Provides access to discrete GPU notifications sent from ACPI via the SAN
+ * driver, which are not handled by this driver directly.
+ *
+ * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
+ */
+
+#ifndef _LINUX_SURFACE_ACPI_NOTIFY_H
+#define _LINUX_SURFACE_ACPI_NOTIFY_H
+
+#include <linux/notifier.h>
+#include <linux/types.h>
+
+/**
+ * struct san_dgpu_event - Discrete GPU ACPI event.
+ * @category: Category of the event.
+ * @target: Target ID of the event source.
+ * @command: Command ID of the event.
+ * @instance: Instance ID of the event source.
+ * @length: Length of the event's payload data (in bytes).
+ * @payload: Pointer to the event's payload data.
+ */
+struct san_dgpu_event {
+ u8 category;
+ u8 target;
+ u8 command;
+ u8 instance;
+ u16 length;
+ u8 *payload;
+};
+
+int san_client_link(struct device *client);
+int san_dgpu_notifier_register(struct notifier_block *nb);
+int san_dgpu_notifier_unregister(struct notifier_block *nb);
+
+#endif /* _LINUX_SURFACE_ACPI_NOTIFY_H */
--
2.29.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
[not found] ` <20201115192143.21571-2-luzmaximilian@gmail.com>
@ 2020-11-16 13:33 ` Andy Shevchenko
2020-11-16 17:03 ` Maximilian Luz
2020-11-18 0:28 ` Barnabás Pőcze
1 sibling, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2020-11-16 13:33 UTC (permalink / raw)
To: Maximilian Luz
Cc: Linux Kernel Mailing List, Hans de Goede, Mark Gross,
Arnd Bergmann, Greg Kroah-Hartman, Rob Herring, Jiri Slaby,
Rafael J . Wysocki, Len Brown, Blaž Hrastnik, Dorian Stoll,
Platform Driver, open list:SERIAL DRIVERS,
ACPI Devel Maling List
On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> Add Surface System Aggregator Module core and Surface Serial Hub driver,
> required for the embedded controller found on Microsoft Surface devices.
>
> The Surface System Aggregator Module (SSAM, SAM or Surface Aggregator)
> is an embedded controller (EC) found on 4th and later generation
> Microsoft Surface devices, with the exception of the Surface Go series.
> This EC provides various functionality, depending on the device in
> question. This can include battery status and thermal reporting (5th and
> later generations), but also HID keyboard (6th+) and touchpad input
> (7th+) on Surface Laptop and Surface Book 3 series devices.
>
> This patch provides the basic necessities for communication with the SAM
> EC on 5th and later generation devices. On these devices, the EC
> provides an interface that acts as serial device, called the Surface
> Serial Hub (SSH). 4th generation devices, on which the EC interface is
> provided via an HID-over-I2C device, are not supported by this patch.
>
> Specifically, this patch adds a driver for the SSH device (device HID
> MSHW0084 in ACPI), as well as a controller structure and associated API.
> This represents the functional core of the Surface Aggregator kernel
> subsystem, introduced with this patch, and will be expanded upon in
> subsequent commits.
>
> The SSH driver acts as the main attachment point for this subsystem and
> sets-up and manages the controller structure. The controller in turn
> provides a basic communication interface, allowing to send requests from
> host to EC and receiving the corresponding responses, as well as
> managing and receiving events, sent from EC to host. It is structured
> into multiple layers, with the top layer presenting the API used by
> other kernel drivers and the lower layers modeled after the serial
> protocol used for communication.
>
> Said other drivers are then responsible for providing the (Surface model
> specific) functionality accessible through the EC (e.g. battery status
> reporting, thermal information, ...) via said controller structure and
> API, and will be added in future commits.
...
> +menuconfig SURFACE_AGGREGATOR
> + tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers"
> + depends on SERIAL_DEV_BUS
> + depends on ACPI
> + select CRC_CCITT
> + help
> + The Surface System Aggregator Module (Surface SAM or SSAM) is an
> + embedded controller (EC) found on 5th- and later-generation Microsoft
> + Surface devices (i.e. Surface Pro 5, Surface Book 2, Surface Laptop,
> + and newer, with exception of Surface Go series devices).
> +
> + Depending on the device in question, this EC provides varying
> + functionality, including:
> + - EC access from ACPI via Surface ACPI Notify (5th- and 6th-generation)
> + - battery status information (all devices)
> + - thermal sensor access (all devices)
> + - performance mode / cooling mode control (all devices)
> + - clipboard detachment system control (Surface Book 2 and 3)
> + - HID / keyboard input (Surface Laptops, Surface Book 3)
> +
> + This option controls whether the Surface SAM subsystem core will be
> + built. This includes a driver for the Surface Serial Hub (SSH), which
> + is the device responsible for the communication with the EC, and a
> + basic kernel interface exposing the EC functionality to other client
> + drivers, i.e. allowing them to make requests to the EC and receive
> + events from it. Selecting this option alone will not provide any
> + client drivers and therefore no functionality beyond the in-kernel
> + interface. Said functionality is the responsibility of the respective
> + client drivers.
> +
> + Note: While 4th-generation Surface devices also make use of a SAM EC,
> + due to a difference in the communication interface of the controller,
> + only 5th and later generations are currently supported. Specifically,
> + devices using SAM-over-SSH are supported, whereas devices using
> + SAM-over-HID, which is used on the 4th generation, are currently not
> + supported.
From this help text I didn't get if it will be a module or what if I
chose the above?
...
> +/* -- Safe counters. -------------------------------------------------------- */
Why can't XArray be used here?
...
> +
> +
One blank line is enough
...
> +static bool ssam_event_matches_notifier(
> + const struct ssam_event_notifier *notif,
> + const struct ssam_event *event)
Perhaps
static bool
ssam_event_matches_notifier(const struct ssam_event_notifier *n,
const struct ssam_event *event)
(or even switch to 100 limit, also note notif ->n — no need to repeat same word)
...
> + nb = rcu_dereference_raw(nh->head);
> + while (nb) {
> + nf = container_of(nb, struct ssam_event_notifier, base);
> + next_nb = rcu_dereference_raw(nb->next);
> +
> + if (ssam_event_matches_notifier(nf, event)) {
> + ret = (ret & SSAM_NOTIF_STATE_MASK) | nb->fn(nf, event);
> + if (ret & SSAM_NOTIF_STOP)
> + break;
The returned value is a bitmask?!
What are you returning at the end?
> + }
> +
> + nb = next_nb;
> + }
...
> +static int __ssam_nfblk_insert(struct ssam_nf_head *nh, struct ssam_notifier_block *nb)
> +{
> + struct ssam_notifier_block **link = &nh->head;
> +
> + while ((*link) != NULL) {
> + if (unlikely((*link) == nb)) {
> + WARN(1, "double register detected");
> + return -EINVAL;
> + }
> +
> + if (nb->priority > (*link)->priority)
> + break;
> +
> + link = &((*link)->next);
> + }
> +
> + nb->next = *link;
> + rcu_assign_pointer(*link, nb);
> +
> + return 0;
> +}
If you need RCU (which is also the Q per se), why not use RCU list?
...
> + while ((*link) != NULL) {
Redundant parentheses, redundant ' != NULL' part.
> + if ((*link) == nb)
> + return link;
> +
> + link = &((*link)->next);
> + }
...
> + * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting event
In above you mistyped 'which' as 'whic' or so, and here reference.
Perhaps go thru spell checker?
...
> + * registered, or ``ERR_PTR(-ENOMEM)`` if the entry could not be allocated.
Better to spell out "error pointer" instead of cryptic ERR_PTR().
...
> + * Executa registered callbacks in order of their priority until either no
> + * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
returns
> + * bit set. Note that this bit is set automatically when converting non.zero
> + * error values via ssam_notifier_from_errno() to notifier values.
...
> + for (i = i - 1; i >= 0; i--)
while (i--)
> + ssam_nf_head_destroy(&nf->head[i]);
...
> + // limit number of processed events to avoid livelocking
> + for (i = 0; i < 10; i++) {
Magic number! Also, this will be better to read in a form of
unsigned int iterations = 10;
do {
...
} while (--iterations);
> + item = ssam_event_queue_pop(queue);
> + if (item == NULL)
> + return;
> +
> + ssam_nf_call(nf, dev, item->rqid, &item->event);
> + kfree(item);
> + }
...
> +static const guid_t SSAM_SSH_DSM_GUID = GUID_INIT(0xd5e383e1, 0xd892, 0x4a76,
> + 0x89, 0xfc, 0xf6, 0xaa, 0xae, 0x7e, 0xd5, 0xb5);
Can you use usual pattern for these UIDs, like
static const guid_t SSAM_SSH_DSM_GUID =
GUID_INIT(0xd5e383e1, 0xd892, 0x4a76,
0x89, 0xfc, 0xf6, 0xaa, 0xae, 0x7e, 0xd5, 0xb5);
?
Also put a comment how this UID will look like in a string representation.
...
> + if (!acpi_has_method(handle, "_DSM"))
> + return 0;
Hmm... What's the expectation?
> + obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
> + SSAM_SSH_DSM_REVISION, 0, NULL,
> + ACPI_TYPE_BUFFER);
> + if (!obj)
> + return -EFAULT;
EFAULT?! Perhaps you can simply return 0 here, no?
> + for (i = 0; i < obj->buffer.length && i < 8; i++)
> + mask |= (((u64)obj->buffer.pointer[i]) << (i * 8));
Don't we have some helpers for this? At least I remember similar code
went to one of PDx86 drivers like intel-vbtn or so.
> + if (mask & 0x01)
BIT(0) ?
> + *funcs = mask;
...
> + caps->ssh_power_profile = (u32)-1;
> + caps->screen_on_sleep_idle_timeout = (u32)-1;
> + caps->screen_off_sleep_idle_timeout = (u32)-1;
> + caps->d3_closes_handle = false;
> + caps->ssh_buffer_size = (u32)-1;
Use proper types and their limits (limits.h missed?).
...
> + // initialize request and packet transport layers
Inconsistent style of comments.
...
> + * In the course of this shutdown procedure, all currently registered
> + * notifiers will be unregistered. It is, however, strongly recommended to not
> + * rely on this behavior, and instead the party registring the notifier should
registering
> + * unregister it before the controller gets shut down, e.g. via the SSAM bus
> + * which guarantees client devices to be removed before a shutdown.
> + * Note that events may still be pending after this call, but due to the
> + * notifiers being unregistered, the will be dropped when the controller is
the?!
> + * subsequently being destroyed via ssam_controller_destroy().
...
> + * Ensures that all resources associated with the controller get freed. This
> + * function should only be called after the controller has been stopped via
> + * ssam_controller_shutdown(). In general, this function should not be called
> + * directly. The only valid place to call this function direclty is during
directly
> + * initialization, before the controller has been fully initialized and passed
> + * to other processes. This function is called automatically when the
> + * reference count of the controller reaches zero.
...
> + * ssam_request_sync_free() - Free a synchronous request.
> + * @rqst: The request to free.
to be freed?
...
> + * Allocates a synchronous request struct on the stack, fully initializes it
> + * using the provided buffer as message data buffer, submits it, and then
> + * waits for its completion before returning its staus. The
status
> + * SSH_COMMAND_MESSAGE_LENGTH() macro can be used to compute the required
> + * message buffer size.
...
> + * This is a wrapper for the raw SAM request to enable an event, thus it does
> + * not handle referecnce counting for enable/disable of events. If an event
> + * has already been enabled, the EC will ignore this request.
Grammar and English language style somehow feels not okay.
...
> + u8 buf[1] = { 0x00 };
Can't be simply buf ?
...
> + * This function will only send the display-off notification command if
> + * display noticications are supported by the EC. Currently all known devices
> + * support these notification.
Spell check!
...
> + * This function will only send the display-on notification command if display
> + * noticications are supported by the EC. Currently all known devices support
> + * these notification.
Ditto.
...
> + ssam_err(ctrl, "unexpected response from D0-exit notification:"
> + " 0x%02x\n", response);
Don't split string literals. Had you run a checkpatch?
Please do and use strict mode.
...
Overall impression of this submission:
- it's quite huge as for a single patch
- it feels like quite an overengineered solution: READ_ONCE, RCU,
list + spin_lock, RB tree of notifiers, my head!
- where is the architectural document of all these?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
2020-11-16 13:33 ` [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem Andy Shevchenko
@ 2020-11-16 17:03 ` Maximilian Luz
2020-11-17 6:05 ` Maximilian Luz
0 siblings, 1 reply; 13+ messages in thread
From: Maximilian Luz @ 2020-11-16 17:03 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, Hans de Goede, Mark Gross,
Arnd Bergmann, Greg Kroah-Hartman, Rob Herring, Jiri Slaby,
Rafael J . Wysocki, Len Brown, Blaž Hrastnik, Dorian Stoll,
Platform Driver, open list:SERIAL DRIVERS,
ACPI Devel Maling List
On 11/16/20 2:33 PM, Andy Shevchenko wrote:
> On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
>> Add Surface System Aggregator Module core and Surface Serial Hub driver,
>> required for the embedded controller found on Microsoft Surface devices.
>>
>> The Surface System Aggregator Module (SSAM, SAM or Surface Aggregator)
>> is an embedded controller (EC) found on 4th and later generation
>> Microsoft Surface devices, with the exception of the Surface Go series.
>> This EC provides various functionality, depending on the device in
>> question. This can include battery status and thermal reporting (5th and
>> later generations), but also HID keyboard (6th+) and touchpad input
>> (7th+) on Surface Laptop and Surface Book 3 series devices.
>>
>> This patch provides the basic necessities for communication with the SAM
>> EC on 5th and later generation devices. On these devices, the EC
>> provides an interface that acts as serial device, called the Surface
>> Serial Hub (SSH). 4th generation devices, on which the EC interface is
>> provided via an HID-over-I2C device, are not supported by this patch.
>>
>> Specifically, this patch adds a driver for the SSH device (device HID
>> MSHW0084 in ACPI), as well as a controller structure and associated API.
>> This represents the functional core of the Surface Aggregator kernel
>> subsystem, introduced with this patch, and will be expanded upon in
>> subsequent commits.
>>
>> The SSH driver acts as the main attachment point for this subsystem and
>> sets-up and manages the controller structure. The controller in turn
>> provides a basic communication interface, allowing to send requests from
>> host to EC and receiving the corresponding responses, as well as
>> managing and receiving events, sent from EC to host. It is structured
>> into multiple layers, with the top layer presenting the API used by
>> other kernel drivers and the lower layers modeled after the serial
>> protocol used for communication.
>>
>> Said other drivers are then responsible for providing the (Surface model
>> specific) functionality accessible through the EC (e.g. battery status
>> reporting, thermal information, ...) via said controller structure and
>> API, and will be added in future commits.
>
> ...
>
>> +menuconfig SURFACE_AGGREGATOR
>> + tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers"
>> + depends on SERIAL_DEV_BUS
>> + depends on ACPI
>> + select CRC_CCITT
>> + help
>> + The Surface System Aggregator Module (Surface SAM or SSAM) is an
>> + embedded controller (EC) found on 5th- and later-generation Microsoft
>> + Surface devices (i.e. Surface Pro 5, Surface Book 2, Surface Laptop,
>> + and newer, with exception of Surface Go series devices).
>> +
>> + Depending on the device in question, this EC provides varying
>> + functionality, including:
>> + - EC access from ACPI via Surface ACPI Notify (5th- and 6th-generation)
>> + - battery status information (all devices)
>> + - thermal sensor access (all devices)
>> + - performance mode / cooling mode control (all devices)
>> + - clipboard detachment system control (Surface Book 2 and 3)
>> + - HID / keyboard input (Surface Laptops, Surface Book 3)
>> +
>> + This option controls whether the Surface SAM subsystem core will be
>> + built. This includes a driver for the Surface Serial Hub (SSH), which
>> + is the device responsible for the communication with the EC, and a
>> + basic kernel interface exposing the EC functionality to other client
>> + drivers, i.e. allowing them to make requests to the EC and receive
>> + events from it. Selecting this option alone will not provide any
>> + client drivers and therefore no functionality beyond the in-kernel
>> + interface. Said functionality is the responsibility of the respective
>> + client drivers.
>> +
>> + Note: While 4th-generation Surface devices also make use of a SAM EC,
>> + due to a difference in the communication interface of the controller,
>> + only 5th and later generations are currently supported. Specifically,
>> + devices using SAM-over-SSH are supported, whereas devices using
>> + SAM-over-HID, which is used on the 4th generation, are currently not
>> + supported.
>
> From this help text I didn't get if it will be a module or what if I
> chose the above?
I thought this would be implied by the tristate options. So should I
simply add something like
Choose m if you want to build the SAM subsystem core and SSH driver
as module, y if you want to build it into the kernel and n if you
don't want it at all.
> ...
>
>> +/* -- Safe counters. -------------------------------------------------------- */
>
> Why can't XArray be used here?
These are basically packet and request IDs (that are sent to the EC and
received from it). XArray is a type of collection. I'm not sure how this
would work.
What happens is: For each new packet/request we get a new ID by
incrementing the counter (with wrap-around). This ID is used, for
example, to match ACK-packets to the corresponding data packet. The EC
will also use this to detect re-transmission, e.g. if we send two
packets with the same ID in sequence, the EC will detect the second
packet as re-transmission of the first one (which is the reason why
simple IDA won't work).
Furthermore, both the Windows driver and the EC itself (for packets sent
by it) use wrapping counters as well, so it seems to be the safest way
to do this without possibly triggering re-transmission detection by
accident.
> ...
>
>> +
>> +
>
> One blank line is enough
>
Can I keep two blank lines for the section separator comments?
> ...
>
>> +static bool ssam_event_matches_notifier(
>> + const struct ssam_event_notifier *notif,
>> + const struct ssam_event *event)
>
> Perhaps
>
> static bool
> ssam_event_matches_notifier(const struct ssam_event_notifier *n,
> const struct ssam_event *event)
>
> (or even switch to 100 limit, also note notif ->n — no need to repeat same word)
Ack.
> ...
>
>> + nb = rcu_dereference_raw(nh->head);
>> + while (nb) {
>> + nf = container_of(nb, struct ssam_event_notifier, base);
>> + next_nb = rcu_dereference_raw(nb->next);
>> +
>> + if (ssam_event_matches_notifier(nf, event)) {
>> + ret = (ret & SSAM_NOTIF_STATE_MASK) | nb->fn(nf, event);
>> + if (ret & SSAM_NOTIF_STOP)
>> + break;
>
> The returned value is a bitmask?!
In part, yes. I've referred to this as the "notifier status value" in
the (function) documentation. This is basically
| bit 0 | bit 1 | bit 2 to 31 |
+-------------+----------+---------------------+
| handled-bit | stop-bit | error-value or zero |
The ssam_notifier_to_errno() function can be used to convert an error
value to this notifier status value. This is the same as the return
value of the notifier function and is described a bit better in the
documentation of struct ssam_notifier_block.
> What are you returning at the end?
The notifier status value as described above. This can be tested for the
SSAM_NOTIF_HANDLED and SSAM_NOTIF_STOP bits. The error value can, as
mentioned in the documentation, be extracted via
ssam_notifier_to_errno().
All in all, this is based on notifier_call_chain() and SRCU notifiers
(linux/notifier.h, kernel/notifier.c) in general, with the key
difference that it includes a bit to indicate if the notification has
been handled (or detect if it goes unhandled).
>
>> + }
>> +
>> + nb = next_nb;
>> + }
>
> ...
>
>> +static int __ssam_nfblk_insert(struct ssam_nf_head *nh, struct ssam_notifier_block *nb)
>> +{
>> + struct ssam_notifier_block **link = &nh->head;
>> +
>> + while ((*link) != NULL) {
>> + if (unlikely((*link) == nb)) {
>> + WARN(1, "double register detected");
>> + return -EINVAL;
>> + }
>> +
>> + if (nb->priority > (*link)->priority)
>> + break;
>> +
>> + link = &((*link)->next);
>> + }
>> +
>> + nb->next = *link;
>> + rcu_assign_pointer(*link, nb);
>> +
>> + return 0;
>> +}
>
> If you need RCU (which is also the Q per se), why not use RCU list?
This part has been mostly copied and adapted from
notifier_chain_register(), which uses the exact same mechanism. I can
change it to use RCU list primitives if that's preferred.
> ...
>
>> + while ((*link) != NULL) {
>
> Redundant parentheses, redundant ' != NULL' part.
Ack.
>
>> + if ((*link) == nb)
>> + return link;
>> +
>> + link = &((*link)->next);
>> + }
>
> ...
>
>> + * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting event
>
> In above you mistyped 'which' as 'whic' or so, and here reference.
> Perhaps go thru spell checker?
Ack. I'll do that.
>
> ...
>
>> + * registered, or ``ERR_PTR(-ENOMEM)`` if the entry could not be allocated.
>
> Better to spell out "error pointer" instead of cryptic ERR_PTR().
Ack.
>
> ...
>
>> + * Executa registered callbacks in order of their priority until either no
>> + * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
>
> returns
Ack.
>
>> + * bit set. Note that this bit is set automatically when converting non.zero
>> + * error values via ssam_notifier_from_errno() to notifier values.
>
> ...
>
>> + for (i = i - 1; i >= 0; i--)
>
> while (i--)
Ack.
>
>> + ssam_nf_head_destroy(&nf->head[i]);
>
> ...
>
>> + // limit number of processed events to avoid livelocking
>> + for (i = 0; i < 10; i++) {
>
> Magic number! Also, this will be better to read in a form of
>
> unsigned int iterations = 10;
>
> do {
> ...
> } while (--iterations);
Okay.
>
>> + item = ssam_event_queue_pop(queue);
>> + if (item == NULL)
>> + return;
>> +
>> + ssam_nf_call(nf, dev, item->rqid, &item->event);
>> + kfree(item);
>> + }
>
> ...
>
>> +static const guid_t SSAM_SSH_DSM_GUID = GUID_INIT(0xd5e383e1, 0xd892, 0x4a76,
>> + 0x89, 0xfc, 0xf6, 0xaa, 0xae, 0x7e, 0xd5, 0xb5);
>
> Can you use usual pattern for these UIDs, like
> static const guid_t SSAM_SSH_DSM_GUID =
> GUID_INIT(0xd5e383e1, 0xd892, 0x4a76,
> 0x89, 0xfc, 0xf6, 0xaa, 0xae, 0x7e, 0xd5, 0xb5);
> ?
>
> Also put a comment how this UID will look like in a string representation.
Will change that.
>
> ...
>
>> + if (!acpi_has_method(handle, "_DSM"))
>> + return 0;
>
> Hmm... What's the expectation?
Depends on the device. Old devices don't have that _DSM (thus fallback
to defaults), newer devices do have it.
>
>> + obj = acpi_evaluate_dsm_typed(handle, &SSAM_SSH_DSM_GUID,
>> + SSAM_SSH_DSM_REVISION, 0, NULL,
>> + ACPI_TYPE_BUFFER);
>> + if (!obj)
>> + return -EFAULT;
>
> EFAULT?! Perhaps you can simply return 0 here, no?
I'd prefer returning an error (_DSM index 0 should always be present and
return a buffer), but I agree that EFAULT is probably the wrong one.
>
>> + for (i = 0; i < obj->buffer.length && i < 8; i++)
>> + mask |= (((u64)obj->buffer.pointer[i]) << (i * 8));
>
> Don't we have some helpers for this? At least I remember similar code
> went to one of PDx86 drivers like intel-vbtn or so.
There is acpi_check_dsm(), which checks if a single function is
supported, but as far as I can tell no helper that returns a bitmask of
all supported functions. I can change it to use acpi_check_dsm() in
ssam_dsm_load_u32() though.
>
>> + if (mask & 0x01)
>
> BIT(0) ?
BIT(0) looks better, ack.
>
>> + *funcs = mask;
>
> ...
>
>> + caps->ssh_power_profile = (u32)-1;
>> + caps->screen_on_sleep_idle_timeout = (u32)-1;
>> + caps->screen_off_sleep_idle_timeout = (u32)-1;
>> + caps->d3_closes_handle = false;
>> + caps->ssh_buffer_size = (u32)-1;
>
> Use proper types and their limits (limits.h missed?).
Okay.
>
> ...
>
>> + // initialize request and packet transport layers
>
> Inconsistent style of comments.
Will change that.
>
> ...
>
>> + * In the course of this shutdown procedure, all currently registered
>> + * notifiers will be unregistered. It is, however, strongly recommended to not
>> + * rely on this behavior, and instead the party registring the notifier should
>
> registering
Ack.
>
>> + * unregister it before the controller gets shut down, e.g. via the SSAM bus
>> + * which guarantees client devices to be removed before a shutdown.
>
>> + * Note that events may still be pending after this call, but due to the
>> + * notifiers being unregistered, the will be dropped when the controller is
>
> the?!
Should be "the events".
>
>> + * subsequently being destroyed via ssam_controller_destroy().
>
> ...
>
>> + * Ensures that all resources associated with the controller get freed. This
>> + * function should only be called after the controller has been stopped via
>> + * ssam_controller_shutdown(). In general, this function should not be called
>> + * directly. The only valid place to call this function direclty is during
>
> directly
Ack.
>
>> + * initialization, before the controller has been fully initialized and passed
>> + * to other processes. This function is called automatically when the
>> + * reference count of the controller reaches zero.
>
> ...
>
>> + * ssam_request_sync_free() - Free a synchronous request.
>> + * @rqst: The request to free.
>
> to be freed?
I think both should be grammatically correct, but I'll change it.
>
> ...
>
>> + * Allocates a synchronous request struct on the stack, fully initializes it
>> + * using the provided buffer as message data buffer, submits it, and then
>> + * waits for its completion before returning its staus. The
>
> status
Ack.
>
>> + * SSH_COMMAND_MESSAGE_LENGTH() macro can be used to compute the required
>> + * message buffer size.
>
> ...
>
>> + * This is a wrapper for the raw SAM request to enable an event, thus it does
>> + * not handle referecnce counting for enable/disable of events. If an event
>> + * has already been enabled, the EC will ignore this request.
>
> Grammar and English language style somehow feels not okay.
I'll try to reword that. I agree that this feels a bit off.
>
> ...
>
>> + u8 buf[1] = { 0x00 };
>
> Can't be simply buf ?
It can be, I'll change that
>
> ...
>
>> + * This function will only send the display-off notification command if
>> + * display noticications are supported by the EC. Currently all known devices
>> + * support these notification.
>
> Spell check!
Will do that.
>
> ...
>
>> + * This function will only send the display-on notification command if display
>> + * noticications are supported by the EC. Currently all known devices support
>> + * these notification.
>
> Ditto.
Ack.
>
> ...
>
>> + ssam_err(ctrl, "unexpected response from D0-exit notification:"
>> + " 0x%02x\n", response);
>
> Don't split string literals. Had you run a checkpatch?
> Please do and use strict mode.
I did run checkpatch, however, I decided to split strings as some of
them are a bit long. When doing that, I only split at places where I
personally wouldn't expect a grep to succeed for, e.g. before or after
format specifiers. I though that this is okay as the main reason for
"don't split string literals" is searchability.
>
>
> ...
>
> Overall impression of this submission:
> - it's quite huge as for a single patch
I tried to split it into parts, but this is the smallest I've been able
to get it while also keeping it a functional unit.
> - it feels like quite an overengineered solution: READ_ONCE, RCU,
> list + spin_lock, RB tree of notifiers, my head!
Sorry about any headaches. My main goal was stability and reliability,
while keeping things somewhat performant and generic.
The "generic" part mostly because this whole thing is based on
reverse-engineering, and I believe that makes it easier to correct for
things that we got wrong, e.g. in what we think the protocol looks like.
Packet state is tracked via atomic flags (which is also laid out in a
comment), and is the simplest solution I found to track that. I found
that a simple enum based state will complicate things as the driver has
to support and properly handle re-submission of packets.
READ_ONCE and WRITE_ONCE are used to ensure proper access to state that
can be changed outside of the queue/pending locks (or under any one of
them). In general, I have tried to document all critical access of such
state with an explanation of why it is safe to do so.
RCU is only used for notifier callbacks, which is based on the SRCU
notifiers from linux/notifier.h and kernel/notifier.c, but adapted to
detect unhandled events.
While it is somewhat interwoven, the (RCU-based) notifier chain stores
the notifiers (one chain per target category) and the RB-tree stores the
number of times an event has been enabled. Essentially, those should be
seen as two separate parts (see e.g. documentation of struct ssam_nf,
which brings those two together).
So the RB-tree is not for notifiers, but for tracking which events are
enabled. Basically reference-counting for enabled events, as the EC
itself only allows turning them on/off. It's necessary to do that as
there can be multiple clients listening to the same event. It's also
required to do this with some sort of map, as (in context of
enabling/disabling) the events are uniquely identified by both their
registry and ID. This combination is a bit too complex to do that with a
simple array, so a RB-tree looked like the best solution.
Note that we store notifiers per target category only, so for that we
can use a simple array.
> - where is the architectural document of all these?
What specifically do you want to see documented? I've documented the
basic communication and principles in another patch of this series, but
kept implementation details somewhat vague as I think those are better
covered by struct and function documentation (which won't become
outdated as easily as a separate documentation).
Thank you for your comments.
Regards,
Max
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
2020-11-16 17:03 ` Maximilian Luz
@ 2020-11-17 6:05 ` Maximilian Luz
0 siblings, 0 replies; 13+ messages in thread
From: Maximilian Luz @ 2020-11-17 6:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, Hans de Goede, Mark Gross,
Arnd Bergmann, Greg Kroah-Hartman, Rob Herring, Jiri Slaby,
Rafael J . Wysocki, Len Brown, Blaž Hrastnik, Dorian Stoll,
Platform Driver, open list:SERIAL DRIVERS,
ACPI Devel Maling List
On 11/16/20 6:03 PM, Maximilian Luz wrote:
> On 11/16/20 2:33 PM, Andy Shevchenko wrote:
>> On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
[...]
> READ_ONCE and WRITE_ONCE are used to ensure proper access to state that
> can be changed outside of the queue/pending locks (or under any one of
> them). In general, I have tried to document all critical access of such
> state with an explanation of why it is safe to do so.
I've looked at this again and noticed that I can guard the packet
timestamp by the pending lock and the packet priority by the queue lock
(after first submission). This makes reasoning about access to them
significantly easier and removes the need for WRITE_ONCE / READ_ONCE.
After that, READ_ONCE is used
- to access the controller state for smoke-testing to (hopefully) detect
invalid request function usage (note that all other access to this
state happens under the controller state lock)
- for the "safe counters", where access to the shared value is, after
initialization, restricted to the increment function
- to update the timeout reaper, where access to the shared value
(rtx_timeout.expires) is, after initialization, restricted to its
modification function (ssh_ptl_timeout_reaper_mod() /
ssh_rtl_timeout_reaper_mod()) and the timer function
- to access the request timestamp, which is, after initialization, only
set once in the lifetime of a request (all other access is read-only)
- to access the 'ptl' reference of the packet, which, after
initialization, is only set once, either at packet or request
submission (all other access is read-only). Note due to this,
READ_ONCE is only required for functions that can run concurrently
with ssh_ptl_submit() and ssh_rtl_submit(), i.e. ssh_ptl_cancel() and
ssh_rtl_cancel().
- to access request state outside of bit-ops when canceling
I'd argue that all of these cases can be checked and verified with a
reasonable amount of effort. Cancellation (last two points) is probably
the most complex one. Unfortunately, I don't see any way to simplify
that part without disallowing cancellation to run concurrently to
submission, which is something I'd like to support as this makes
implementing asynchronous requests in the future easier.
Regards,
Max
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] platform/surface: Add Surface ACPI Notify driver
2020-11-15 19:21 ` [PATCH 9/9] platform/surface: Add Surface ACPI Notify driver Maximilian Luz
@ 2020-11-17 20:09 ` Randy Dunlap
2020-11-17 20:18 ` Maximilian Luz
0 siblings, 1 reply; 13+ messages in thread
From: Randy Dunlap @ 2020-11-17 20:09 UTC (permalink / raw)
To: Maximilian Luz, linux-kernel
Cc: Hans de Goede, Mark Gross, Arnd Bergmann, Greg Kroah-Hartman,
Rafael J . Wysocki, Len Brown, Jonathan Corbet,
Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-acpi, linux-doc
On 11/15/20 11:21 AM, Maximilian Luz wrote:
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index cecad7a0cb7b..3b0da86546fa 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -40,6 +40,26 @@ config SURFACE_3_POWER_OPREGION
> This driver provides support for ACPI operation
> region of the Surface 3 battery platform driver.
>
> +config SURFACE_ACPI_NOTIFY
> + tristate "Surface ACPI Notify Driver"
> + depends on ACPI
> + depends on SURFACE_AGGREGATOR
> + help
> + Surface ACPI Notify (SAN) driver for Microsoft Surface devices.
Drop "depends on ACPI" since SURFACE_AGGREGATOR already depends on ACPI.
--
~Randy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] platform/surface: Add Surface ACPI Notify driver
2020-11-17 20:09 ` Randy Dunlap
@ 2020-11-17 20:18 ` Maximilian Luz
0 siblings, 0 replies; 13+ messages in thread
From: Maximilian Luz @ 2020-11-17 20:18 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel
Cc: Hans de Goede, Mark Gross, Arnd Bergmann, Greg Kroah-Hartman,
Rafael J . Wysocki, Len Brown, Jonathan Corbet,
Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-acpi, linux-doc
On 11/17/20 9:09 PM, Randy Dunlap wrote:
> On 11/15/20 11:21 AM, Maximilian Luz wrote:
>> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
>> index cecad7a0cb7b..3b0da86546fa 100644
>> --- a/drivers/platform/surface/Kconfig
>> +++ b/drivers/platform/surface/Kconfig
>> @@ -40,6 +40,26 @@ config SURFACE_3_POWER_OPREGION
>> This driver provides support for ACPI operation
>> region of the Surface 3 battery platform driver.
>>
>> +config SURFACE_ACPI_NOTIFY
>> + tristate "Surface ACPI Notify Driver"
>> + depends on ACPI
>> + depends on SURFACE_AGGREGATOR
>> + help
>> + Surface ACPI Notify (SAN) driver for Microsoft Surface devices.
>
> Drop "depends on ACPI" since SURFACE_AGGREGATOR already depends on ACPI.
Okay, will do that.
Thanks,
Max
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
[not found] ` <20201115192143.21571-2-luzmaximilian@gmail.com>
2020-11-16 13:33 ` [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem Andy Shevchenko
@ 2020-11-18 0:28 ` Barnabás Pőcze
2020-11-18 23:06 ` Maximilian Luz
1 sibling, 1 reply; 13+ messages in thread
From: Barnabás Pőcze @ 2020-11-18 0:28 UTC (permalink / raw)
To: Maximilian Luz
Cc: linux-kernel, Hans de Goede, Mark Gross, Arnd Bergmann,
Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Rafael J . Wysocki,
Len Brown, Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-serial, linux-acpi
Hi
I have attached some thoughts and comments inline.
2020. november 15., vasárnap 20:21 keltezéssel, Maximilian Luz írta:
> [...]
> +/* -- Event notifier/callbacks. --------------------------------------------- */
> +/*
> + * The notifier system is based on linux/notifier.h, specifically the SRCU
> + * implementation. The difference to that is, that some bits of the notifier
> + * call return value can be tracked across multiple calls. This is done so that
> + * handling of events can be tracked and a warning can be issued in case an
> + * event goes unhandled. The idea of that waring is that it should help discover
^
"warning"
> + * and identify new/currently unimplemented features.
> + */
> +
> +
> +/**
> + * ssam_event_matches_notifier() - Test if an event matches a notifier;
^
Shouldn't it be a period?
> + * @notif: The event notifier to test against.
> + * @event: The event to test.
> + *
> + * Return: Returns %true iff the given event matches the given notifier
> + * according to the rules set in the notifier's event mask, %false otherwise.
> + */
> [...]
> +static int __ssam_nfblk_remove(struct ssam_nf_head *nh,
> + struct ssam_notifier_block *nb)
> +{
> + struct ssam_notifier_block **link;
> +
> + link = __ssam_nfblk_find_link(nh, nb);
> + if (!link)
> + return -ENOENT;
I find it odd that here you return ENOENT, but in `__ssam_nfblk_insert()`
EINVAL is returned instead of EEXIST. I believe either both should be EINVAL,
or EEXIST+ENOENT.
> +
> + __ssam_nfblk_erase(link);
I'm wondering if it's necessary to create a new function which contains just
a single line.
> + return 0;
> +}
> [...]
> +/**
> + * ssam_nf_head_destroy() - Deinitialize the given notifier head.
> + * @nh: The notifier head to deinitialize.
> + */
> +static void ssam_nf_head_destroy(struct ssam_nf_head *nh)
> +{
> + cleanup_srcu_struct(&nh->srcu);
> +}
I'm also wondering if there's any reason why these static one-liner functions are
not explicitly marked inline.
> [...]
> +/**
> + * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting event
> + * activations.
> + * @node: The node of this entry in the rb-tree.
> + * @key: The key of the event.
> + * @refcount: The reference-count of the event.
> + * @flags: The flags used when enabling the event.
> + */
> +struct ssam_nf_refcount_entry {
> + struct rb_node node;
> + struct ssam_nf_refcount_key key;
> + int refcount;
Is there any reason why a signed type is used for reference counting?
> + u8 flags;
> +};
> +
> +
> +/**
> + * ssam_nf_refcount_inc() - Increment reference-/activation-count of the given
> + * event.
> + * @nf: The notifier system reference.
> + * @reg: The registry used to enable/disable the event.
> + * @id: The event ID.
> + *
> + * Increments the reference-/activation-count associated with the specified
> + * event type/ID, allocating a new entry for this event ID if necessary. A
> + * newly allocated entry will have a refcount of one.
Shouldn't it be noted that nf->lock(?) must(?) be held when calling?
> [...]
> +/**
> + * ssam_nf_call() - Call notification callbacks for the provided event.
> + * @nf: The notifier system
> + * @dev: The associated device, only used for logging.
> + * @rqid: The request ID of the event.
> + * @event: The event provided to the callbacks.
> + *
> + * Executa registered callbacks in order of their priority until either no
^
"execute"
> + * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
> + * bit set. Note that this bit is set automatically when converting non.zero
^
maybe "non-zero"?
> + * error values via ssam_notifier_from_errno() to notifier values.
> + *
> + * Also note that any callback that could handle an event should return a value
> + * with bit %SSAM_NOTIF_HANDLED set, indicating that the event does not go
> + * unhandled/ignored. In case no registered callback could handle an event,
> + * this function will emit a warning.
> + *
> + * In case a callback failed, this function will emit an error message.
> + */
> +static void ssam_nf_call(struct ssam_nf *nf, struct device *dev, u16 rqid,
> + struct ssam_event *event)
> +{
> + struct ssam_nf_head *nf_head;
> + int status, nf_ret;
> +
> + if (!ssh_rqid_is_event(rqid)) {
> + dev_warn(dev, "event: unsupported rqid: 0x%04x\n", rqid);
A small note, "%#04x" would insert the "0x" prefix.
> + return;
> + }
> [...]
> +/**
> + * ssam_event_item_alloc() - Allocate an event item with the given payload size.
> + * @len: The event payload length.
> + * @flags: The flags used for allocation.
> + *
> + * Allocate an event item with the given payload size. Sets the item
> + * operations and payload length values. The item free callback (``ops.free``)
> + * should not be overwritten after this call.
> + *
> + * Return: Returns the newly allocated event item.
> + */
> +static struct ssam_event_item *ssam_event_item_alloc(size_t len, gfp_t flags)
The `flags` argument is seemingly ignored.
> +{
> + struct ssam_event_item *item;
> +
> + item = kzalloc(sizeof(*item) + len, GFP_KERNEL);
I believe `struct_size(item, event.data, len)` could be utilized here.
> + if (!item)
> + return NULL;
> +
> + item->event.length = len;
> + return item;
> +}
> [...]
> +static void ssam_event_queue_work_fn(struct work_struct *work)
> +{
> + struct ssam_event_queue *queue;
> + struct ssam_event_item *item;
> + struct ssam_nf *nf;
> + struct device *dev;
> + int i;
> +
> + queue = container_of(work, struct ssam_event_queue, work);
> + nf = &queue->cplt->event.notif;
> + dev = queue->cplt->dev;
> +
> + // limit number of processed events to avoid livelocking
> + for (i = 0; i < 10; i++) {
> + item = ssam_event_queue_pop(queue);
> + if (item == NULL)
I believe `!item` is preferred.
> + return;
> +
> + ssam_nf_call(nf, dev, item->rqid, &item->event);
> + kfree(item);
> + }
> +
> + if (!ssam_event_queue_is_empty(queue))
> + ssam_cplt_submit(queue->cplt, &queue->work);
> +}
> [...]
> +static void ssam_handle_event(struct ssh_rtl *rtl,
> + const struct ssh_command *cmd,
> + const struct ssam_span *data)
> +{
> + struct ssam_controller *ctrl = to_ssam_controller(rtl, rtl);
> + struct ssam_event_item *item;
> +
> + item = ssam_event_item_alloc(data->len, GFP_KERNEL);
> + if (!item)
> + return;
> +
> + item->rqid = get_unaligned_le16(&cmd->rqid);
> + item->event.target_category = cmd->tc;
> + item->event.target_id = cmd->tid_in;
> + item->event.command_id = cmd->cid;
> + item->event.instance_id = cmd->iid;
> + memcpy(&item->event.data[0], data->ptr, data->len);
> +
> + WARN_ON(ssam_cplt_submit_event(&ctrl->cplt, item));
I believe that if submission fails, `item` is leaked.
> +}
> +
> +static const struct ssh_rtl_ops ssam_rtl_ops = {
> + .handle_event = ssam_handle_event,
> +};
> +
> +
> +static bool ssam_notifier_empty(struct ssam_controller *ctrl);
I think it'd be better to name it `ssam_notifier_is_empty()` to be consistent
with the rest of the patch.
> [...]
> +static int ssam_controller_caps_load_from_acpi(
> + acpi_handle handle, struct ssam_controller_caps *caps)
> +{
> + u32 d3_closes_handle = false;
Assinging a boolean like this to a `u32` looks very odd to me.
> + u64 funcs;
> + int status;
> +
> + // set defaults
> + caps->ssh_power_profile = (u32)-1;
> + caps->screen_on_sleep_idle_timeout = (u32)-1;
> + caps->screen_off_sleep_idle_timeout = (u32)-1;
> + caps->d3_closes_handle = false;
> + caps->ssh_buffer_size = (u32)-1;
> [...]
> +
> +/**
> + * ssam_controller_start() - Start the receiver and transmitter threads of the
> + * controller.
> + * @ctrl: The controller.
> + *
> + * Note: When this function is called, the controller shouldbe properly hooked
^
space
> + * up to the serdev core via &struct serdev_device_ops. Please refert to
^
"refer"
> [...]
> +void ssam_controller_shutdown(struct ssam_controller *ctrl)
> +{
> + enum ssam_controller_state s = ctrl->state;
> + int status;
> +
> + if (s == SSAM_CONTROLLER_UNINITIALIZED || s == SSAM_CONTROLLER_STOPPED)
> + return;
> +
> + // try to flush pending events and requests while everything still works
> + status = ssh_rtl_flush(&ctrl->rtl, msecs_to_jiffies(5000));
Wouldn't it be better to name that 5000?
> + if (status) {
> + ssam_err(ctrl, "failed to flush request transport layer: %d\n",
> + status);
> + }
> [...]
> +
> +/**
> + * ssam_controller_destroy() - Destroy the controller and free its resources.
> + * @ctrl: The controller.
> + *
> + * Ensures that all resources associated with the controller get freed. This
> + * function should only be called after the controller has been stopped via
> + * ssam_controller_shutdown(). In general, this function should not be called
> + * directly. The only valid place to call this function direclty is during
^
"directly"
> + * initialization, before the controller has been fully initialized and passed
> + * to other processes. This function is called automatically when the
> + * reference count of the controller reaches zero.
> + *
> + * Must be called from an exclusive context with regards to the controller
> + * state.
> + */
> [...]
> +int ssam_request_sync_alloc(size_t payload_len, gfp_t flags,
> + struct ssam_request_sync **rqst,
> + struct ssam_span *buffer)
> +{
> + size_t msglen = SSH_COMMAND_MESSAGE_LENGTH(payload_len);
> +
> + *rqst = kzalloc(sizeof(**rqst) + msglen, flags);
> + if (!*rqst)
> + return -ENOMEM;
> +
> + buffer->ptr = (u8 *)(*rqst + 1);
> + buffer->len = msglen;
> +
> + return 0;
> +}
I think there is a bit of incosistency: sometimes you use ** pointer + return int,
sometimes you return a pointer with potentially embedded errno. I think it would
be better if you stuck with one or the other.
> [...]
> +static int ssam_ssh_event_disable(struct ssam_controller *ctrl,
> + struct ssam_event_registry reg,
> + struct ssam_event_id id, u8 flags)
> +{
> [...]
> + rqst.command_id = reg.cid_disable;
If I see it correctly, this line is the only significant difference between this one
and the previous function. Is there any reason they're not combined?
> [...]
> +/**
> + * ssam_notifier_disable_registered() - Disable events for all registered
> + * notifiers.
> + * @ctrl: The controller for which to disable the notifiers/events.
> + *
> + * Disables events for all currently registered notifiers. In case of an error
> + * (EC command failing), all previously disabled events will be restored and
> + * the error code returned.
> + *
> + * This function is intended to disable all events prior to hibenration entry.
^
"hibernation"
> [...]
> +void ssam_irq_disarm_wakeup(struct ssam_controller *ctrl)
> +{
> + int status;
> +
> + if (ctrl->irq.wakeup_enabled) {
> + status = disable_irq_wake(ctrl->irq.num);
> + if (status)
> + ssam_err(ctrl, "failed to disable wake IRQ: %d\n", status);
> +
> + ctrl->irq.wakeup_enabled = false;
It's set to false even if `disable_irq_wake()` fails?
> + }
> + disable_irq(ctrl->irq.num);
> +}
> [...]
> +static int ssam_try_set_controller(struct ssam_controller *ctrl)
> +{
> + int status = 0;
> +
> + spin_lock(&__ssam_controller_lock);
> + if (!__ssam_controller)
> + __ssam_controller = ctrl;
> + else
> + status = -EBUSY;
I feel like EBUSY might not be the best errno here.
> + spin_unlock(&__ssam_controller_lock);
> +
> + return status;
> +}
> [...]
> +int ssam_client_link(struct ssam_controller *c, struct device *client)
> +{
> + const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> + struct device_link *link;
> + struct device *ctrldev;
> +
> + ssam_controller_statelock(c);
> +
> + if (c->state != SSAM_CONTROLLER_STARTED) {
> + ssam_controller_stateunlock(c);
> + return -ENXIO;
> + }
> +
> + ctrldev = ssam_controller_device(c);
> + if (!ctrldev) {
> + ssam_controller_stateunlock(c);
> + return -ENXIO;
> + }
> +
I'm not sure if ENXIO is the best errno in the last two returns;
> + link = device_link_add(client, ctrldev, flags);
> + if (!link) {
> + ssam_controller_stateunlock(c);
> + return -ENOMEM;
> + }
> +
> + /*
> + * Return -ENXIO if supplier driver is on its way to be removed. In this
> + * case, the controller won't be around for much longer and the device
> + * link is not going to save us any more, as unbinding is already in
> + * progress.
> + */
> + if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND) {
> + ssam_controller_stateunlock(c);
> + return -ENXIO;
> + }
> +
> + ssam_controller_stateunlock(c);
> + return 0;
> +}
> [...]
> +int ssam_client_bind(struct device *client, struct ssam_controller **ctrl)
> +{
> + struct ssam_controller *c;
> + int status;
> +
> + c = ssam_get_controller();
> + if (!c)
> + return -ENXIO;
To me, ENODEV seems like a better fit here.
> +
> + status = ssam_client_link(c, client);
> [...]
> +static ssize_t firmware_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ssam_controller *ctrl = dev_get_drvdata(dev);
> + u32 version, a, b, c;
> + int status;
> +
> + status = ssam_get_firmware_version(ctrl, &version);
> + if (status < 0)
> + return status;
> +
> + a = (version >> 24) & 0xff;
> + b = ((version >> 8) & 0xffff);
> + c = version & 0xff;
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%u.%u.%u\n", a, b, c);
`snprintf()` takes care of the null byte, so simply `PAGE_SIZE` would've been
sufficient. But that doesn't matter much since you should use `sysfs_emit()`.
> +}
> +static DEVICE_ATTR_RO(firmware_version);
> +
> +static struct attribute *ssam_sam_attrs[] = {
> + &dev_attr_firmware_version.attr,
> + NULL,
^
I believe it is preferred to omit the comma after the terminating entry.
> [...]
> +/**
> + * msgb_push_cmd() - Push a SSH command frame with payload to the buffer.
> + * @msgb: The message buffer.
> + * @seq: The sequence ID (SEQ) of the frame/packet.
> + * @rqid: The request ID (RQID) of the request contained in the frame.
> + * @rqst: The request to wrap in the frame.
> + */
> +static inline void msgb_push_cmd(struct msgbuf *msgb, u8 seq, u16 rqid,
> + const struct ssam_request *rqst)
> +{
> + struct ssh_command *cmd;
> + const u8 *cmd_begin;
> + const u8 type = SSH_FRAME_TYPE_DATA_SEQ;
> +
> + // SYN
> + msgb_push_syn(msgb);
> +
> + // command frame + crc
> + msgb_push_frame(msgb, type, sizeof(*cmd) + rqst->length, seq);
> +
> + // frame payload: command struct + payload
> + if (WARN_ON(msgb->ptr + sizeof(*cmd) > msgb->end))
> + return;
> +
> + cmd_begin = msgb->ptr;
> + cmd = (struct ssh_command *)msgb->ptr;
I believe this violates strict aliasing.
> [...]
> + * Note that the packet completion callback is, in case of success and for a
> + * sequenced packet, guaranteed to run on the receiver thread, thus providing
> + * a way to reliably identify responses to the packet. The packet completion
> + * callback is only run once and it does not indicate that the packet has
> + * fully left the system (for this, one should rely on the release method,
> + * triggered when the reference count of the packet reaches zero). In case of
> + * re-submission (and with somewhat unlikely timing), it may be possible that
> + * the packet is being re-transmitted while the completion callback runs.
> + * Completion will occur both on success and internal error, as well as when
> + * the packet is canceled.
If I understand it correctly, it is possible that submission of a packet fails
for the first time, but it's scheduled for resubmission, and this retransmission
happens at the same time when the complete() callback is called. If that's the
case, then the callback is called with an error condition, no? Thus it is possible
that a packet is successfully submitted (for the second, third, etc. time), but the
complete() callback receives notification about failure? Or am I missing something?
> [...]
> +int ssh_ptl_rx_rcvbuf(struct ssh_ptl *ptl, const u8 *buf, size_t n)
> +{
> + int used;
> +
> + if (test_bit(SSH_PTL_SF_SHUTDOWN_BIT, &ptl->state))
> + return -ESHUTDOWN;
> +
> + used = kfifo_in(&ptl->rx.fifo, buf, n);
Isn't it possible that `n` is greater than the free space in the fifo? What
happens then?
> + if (used)
> + ssh_ptl_rx_wakeup(ptl);
> +
> + return used;
> +}
> [...]
> +void ssh_ptl_shutdown(struct ssh_ptl *ptl)
> +{
> [...]
> + * Note 2: We can re-use queue_node (or pending_node) if we mark the
> + * packet as locked an then remove it from the queue (or pending set
> + * respecitvely). Marking the packet as locked avoids re-queueing
> + * (which should already be prevented by having stopped the treads...)
> + * and not setting QUEUED_BIT (or PENDING_BIT) prevents removal from a
> + * new list via other threads (e.g. canellation).
^
"cancellation"
> + *
> + * Note 3: There may be overlap between complete_p and complete_q.
> + * This is handled via test_and_set_bit() on the "completed" flag
> + * (also handles cancellation).
> + */
> [...]
> +#define __ssam_prcond(func, p, fmt, ...) \
> + do { \
> + if ((p)) \
I believe `if (p)` is sufficient.
> + func((p), fmt, ##__VA_ARGS__); \
> + } while (0)
> [...]
> +int sshp_parse_frame(const struct device *dev, const struct ssam_span *source,
> + struct ssh_frame **frame, struct ssam_span *payload,
> + size_t maxlen)
> +{
> [...]
> + // ensure packet does not exceed maximum length
> + sp.len = get_unaligned_le16(&((struct ssh_frame *)sf.ptr)->len);
> + if (unlikely(sp.len + SSH_MESSAGE_LENGTH(0) > maxlen)) {
> + dev_warn(dev, "rx: parser: frame too large: %u bytes\n",
I believe `%hu` would be more appropriate.
> + ((struct ssh_frame *)sf.ptr)->len);
Why isn't `get_unaligned_le16()` used here? (Or simply even `sp.len`.)
> + return -EMSGSIZE;
> + }
> [...]
> + *frame = (struct ssh_frame *)sf.ptr;
This also violates strict aliasing.
> + *payload = sp;
> +
> + dev_dbg(dev, "rx: parser: valid frame found (type: 0x%02x, len: %u)\n",
> + (*frame)->type, (*frame)->len);
> +
> + return 0;
> +}
> [...]
> +int sshp_parse_command(const struct device *dev, const struct ssam_span *source,
> + struct ssh_command **command,
> + struct ssam_span *command_data)
> +{
> + // check for minimum length
> + if (unlikely(source->len < sizeof(struct ssh_command))) {
> + *command = NULL;
> + command_data->ptr = NULL;
> + command_data->len = 0;
> +
> + dev_err(dev, "rx: parser: command payload is too short\n");
> + return -ENOMSG;
> + }
> +
> + *command = (struct ssh_command *)source->ptr;
I'm quite sure this also violates strict aliasing.
> + command_data->ptr = source->ptr + sizeof(struct ssh_command);
> + command_data->len = source->len - sizeof(struct ssh_command);
> +
> + dev_dbg(dev, "rx: parser: valid command found (tc: 0x%02x, cid: 0x%02x)\n",
> + (*command)->tc, (*command)->cid);
> +
> + return 0;
> +}
> [...]
> +#define SSH_MSGOFFSET_FRAME(field) \
> + (sizeof(u16) + offsetof(struct ssh_frame, field))
> +
> +/**
> + * SSH_MSGOFFSET_COMMAND() - Compute offset in SSH message to specified field
> + * in command.
> + * @field: The field for which the offset should be computed.
> + *
> + * Return: Returns the offset of the specified &struct ssh_command field in
> + * the raw SSH message data.
> + */
> +#define SSH_MSGOFFSET_COMMAND(field) \
> + (2ull * sizeof(u16) + sizeof(struct ssh_frame) \
> + + offsetof(struct ssh_command, field))
I believe it should be noted (here and for `SSH_MSGOFFSET_FRAME()`) why the
`sizeof(u16)`s are necessary.
> +
> +/*
> + * SSH_MSG_SYN - SSH message synchronization (SYN) bytes as u16.
> + */
> +#define SSH_MSG_SYN ((u16)0x55aa)
> [...]
> +enum ssam_ssh_tc {
> + /* Known SSH/EC target categories. */
> + // category 0x00 is invalid for EC use
> + SSAM_SSH_TC_SAM = 0x01, // generic system functionality, real-time clock
> + SSAM_SSH_TC_BAT = 0x02, // battery/power subsystem
> + SSAM_SSH_TC_TMP = 0x03, // thermal subsystem
> + SSAM_SSH_TC_PMC = 0x04,
> + SSAM_SSH_TC_FAN = 0x05,
> + SSAM_SSH_TC_PoM = 0x06,
> + SSAM_SSH_TC_DBG = 0x07,
> + SSAM_SSH_TC_KBD = 0x08, // legacy keyboard (Laptop 1/2)
> + SSAM_SSH_TC_FWU = 0x09,
> + SSAM_SSH_TC_UNI = 0x0a,
> + SSAM_SSH_TC_LPC = 0x0b,
> + SSAM_SSH_TC_TCL = 0x0c,
> + SSAM_SSH_TC_SFL = 0x0d,
> + SSAM_SSH_TC_KIP = 0x0e,
> + SSAM_SSH_TC_EXT = 0x0f,
> + SSAM_SSH_TC_BLD = 0x10,
> + SSAM_SSH_TC_BAS = 0x11, // detachment system (Surface Book 2/3)
> + SSAM_SSH_TC_SEN = 0x12,
> + SSAM_SSH_TC_SRQ = 0x13,
> + SSAM_SSH_TC_MCU = 0x14,
> + SSAM_SSH_TC_HID = 0x15, // generic HID input subsystem
> + SSAM_SSH_TC_TCH = 0x16,
> + SSAM_SSH_TC_BKL = 0x17,
> + SSAM_SSH_TC_TAM = 0x18,
> + SSAM_SSH_TC_ACC = 0x19,
> + SSAM_SSH_TC_UFI = 0x1a,
> + SSAM_SSH_TC_USC = 0x1b,
> + SSAM_SSH_TC_PEN = 0x1c,
> + SSAM_SSH_TC_VID = 0x1d,
> + SSAM_SSH_TC_AUD = 0x1e,
> + SSAM_SSH_TC_SMC = 0x1f,
> + SSAM_SSH_TC_KPD = 0x20,
> + SSAM_SSH_TC_REG = 0x21,
> +};
Is it known what these abbreviations stand for? Maybe I missed them?
> [...]
> +/**
> + * struct ssh_request_ops - Callback operations for a SSH request.
> + * @release: Function called when the request's reference count reaches zero.
> + * This callback must be relied upon to ensure that the request has
> + * left the transport systems (both, packet an request systems).
> + * @complete: Function called when the request is completed, either with
> + * success or failure. The command data for the request response
> + * is provided via the &struct ssh_command parameter (``cmd``),
> + * the command payload of the request response via the &struct
> + * ssh_span parameter (``data``).
> + *
> + * If the request does not have any response or has not been
> + * completed with success, both ``cmd`` and ``data`` parameters will
> + * be NULL. If the request response does not have any command
> + * payload, the ``data`` span will be an empty (zero-length) span.
> + *
> + * In case of failure, the reason for the failure is indicated by
> + * the value of the provided status code argument (``status``). This
> + * value will be zero in case of success.
I believe it should be noted if the `status` argument is a regular errno,
or something different.
> + *
> + * Note that a call to this callback does not guarantee that the
> + * request is not in use by the transport systems any more.
> + */
> +struct ssh_request_ops {
> + void (*release)(struct ssh_request *rqst);
> + void (*complete)(struct ssh_request *rqst,
> + const struct ssh_command *cmd,
> + const struct ssam_span *data, int status);
> +};
> [...]
I have to agree, this is quite a sizable patch, although I think it's well-commented,
which helps reading the code by a lot, however, in some places I feel like it's a bit
over-engineered (or maybe I just cannot fully appreciate the subject at hand at the moment),
nonetheless, I applaud your efforts, I can only imagine the hours that must have gone into it.
Regards,
Barnabás Pőcze
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
2020-11-18 0:28 ` Barnabás Pőcze
@ 2020-11-18 23:06 ` Maximilian Luz
2020-11-19 15:54 ` Barnabás Pőcze
0 siblings, 1 reply; 13+ messages in thread
From: Maximilian Luz @ 2020-11-18 23:06 UTC (permalink / raw)
To: Barnabás Pőcze
Cc: linux-kernel, Hans de Goede, Mark Gross, Arnd Bergmann,
Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Rafael J . Wysocki,
Len Brown, Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-serial, linux-acpi
On 11/18/20 1:28 AM, Barnabás Pőcze wrote:
> Hi
>
> I have attached some thoughts and comments inline.
Thanks!
> 2020. november 15., vasárnap 20:21 keltezéssel, Maximilian Luz írta:
>
>> [...]
>> +/* -- Event notifier/callbacks. --------------------------------------------- */
>> +/*
>> + * The notifier system is based on linux/notifier.h, specifically the SRCU
>> + * implementation. The difference to that is, that some bits of the notifier
>> + * call return value can be tracked across multiple calls. This is done so that
>> + * handling of events can be tracked and a warning can be issued in case an
>> + * event goes unhandled. The idea of that waring is that it should help discover
> ^
> "warning"
Thanks, I'll run a spell-checker over the comments and (hopefully) fix all typos.
>> + * and identify new/currently unimplemented features.
>> + */
>> +
>> +
>> +/**
>> + * ssam_event_matches_notifier() - Test if an event matches a notifier;
> ^
> Shouldn't it be a period?
Yes, it should.
>> + * @notif: The event notifier to test against.
>> + * @event: The event to test.
>> + *
>> + * Return: Returns %true iff the given event matches the given notifier
>> + * according to the rules set in the notifier's event mask, %false otherwise.
>> + */
>
>> [...]
>
>> +static int __ssam_nfblk_remove(struct ssam_nf_head *nh,
>> + struct ssam_notifier_block *nb)
>> +{
>> + struct ssam_notifier_block **link;
>> +
>> + link = __ssam_nfblk_find_link(nh, nb);
>> + if (!link)
>> + return -ENOENT;
>
> I find it odd that here you return ENOENT, but in `__ssam_nfblk_insert()`
> EINVAL is returned instead of EEXIST. I believe either both should be EINVAL,
> or EEXIST+ENOENT.
Yes, EEXIST is better for insert.
>> +
>> + __ssam_nfblk_erase(link);
>
> I'm wondering if it's necessary to create a new function which contains just
> a single line.
I prefer to create these sorts of abstractions as this, in my opinion,
keeps things contained and makes things easier to reason about. In this
case, it also allows for documentation.
>> + return 0;
>> +}
>
>> [...]
>
>> +/**
>> + * ssam_nf_head_destroy() - Deinitialize the given notifier head.
>> + * @nh: The notifier head to deinitialize.
>> + */
>> +static void ssam_nf_head_destroy(struct ssam_nf_head *nh)
>> +{
>> + cleanup_srcu_struct(&nh->srcu);
>> +}
>
> I'm also wondering if there's any reason why these static one-liner functions are
> not explicitly marked inline.
Isn't inline more of a linkage keyword at this point? I fully expect any
compiler to inline things like this automatically at the first hint of
an optimization flag.
>> [...]
>
>> +/**
>> + * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting event
>> + * activations.
>> + * @node: The node of this entry in the rb-tree.
>> + * @key: The key of the event.
>> + * @refcount: The reference-count of the event.
>> + * @flags: The flags used when enabling the event.
>> + */
>> +struct ssam_nf_refcount_entry {
>> + struct rb_node node;
>> + struct ssam_nf_refcount_key key;
>> + int refcount;
>
> Is there any reason why a signed type is used for reference counting?
Can help to spot overflows when debugging, not that that should happen.
We also don't need values larger than INT_MAX. We shouldn't even get
remotely close to it.
>> + u8 flags;
>> +};
>> +
>> +
>> +/**
>> + * ssam_nf_refcount_inc() - Increment reference-/activation-count of the given
>> + * event.
>> + * @nf: The notifier system reference.
>> + * @reg: The registry used to enable/disable the event.
>> + * @id: The event ID.
>> + *
>> + * Increments the reference-/activation-count associated with the specified
>> + * event type/ID, allocating a new entry for this event ID if necessary. A
>> + * newly allocated entry will have a refcount of one.
>
> Shouldn't it be noted that nf->lock(?) must(?) be held when calling?
All functions taking 'struct ssam_nf *' as first parameter must run
synchronized with regards to each other, except for ssam_nf_call().
Same for all access to struct ssam_nf directly. Whether that is
guaranteed by nf->lock or other means (de-/initialization) doesn't
really matter. Only higher-level functions may run concurrently.
On the higher level, the lock must also be held to ensure that the EC
requests are sent at the right time (which is its main purpose).
Arguably that's not well documented here, I'll try to fix that.
[...]
>> + * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
>> + * bit set. Note that this bit is set automatically when converting non.zero
> ^
> maybe "non-zero"?
Right.
>> + * error values via ssam_notifier_from_errno() to notifier values.
>> + *
>> + * Also note that any callback that could handle an event should return a value
>> + * with bit %SSAM_NOTIF_HANDLED set, indicating that the event does not go
>> + * unhandled/ignored. In case no registered callback could handle an event,
>> + * this function will emit a warning.
>> + *
>> + * In case a callback failed, this function will emit an error message.
>> + */
>> +static void ssam_nf_call(struct ssam_nf *nf, struct device *dev, u16 rqid,
>> + struct ssam_event *event)
>> +{
>> + struct ssam_nf_head *nf_head;
>> + int status, nf_ret;
>> +
>> + if (!ssh_rqid_is_event(rqid)) {
>> + dev_warn(dev, "event: unsupported rqid: 0x%04x\n", rqid);
>
> A small note, "%#04x" would insert the "0x" prefix.
Oh neat, didn't know that. Should be "%#06x" though, right?
>> + return;
>> + }
>
>> [...]
>
>> +/**
>> + * ssam_event_item_alloc() - Allocate an event item with the given payload size.
>> + * @len: The event payload length.
>> + * @flags: The flags used for allocation.
>> + *
>> + * Allocate an event item with the given payload size. Sets the item
>> + * operations and payload length values. The item free callback (``ops.free``)
>> + * should not be overwritten after this call.
>> + *
>> + * Return: Returns the newly allocated event item.
>> + */
>> +static struct ssam_event_item *ssam_event_item_alloc(size_t len, gfp_t flags)
>
> The `flags` argument is seemingly ignored.
Oh, right.
>> +{
>> + struct ssam_event_item *item;
>> +
>> + item = kzalloc(sizeof(*item) + len, GFP_KERNEL);
>
> I believe `struct_size(item, event.data, len)` could be utilized here.
Right, I'll use that.
>> + if (!item)
>> + return NULL;
>> +
>> + item->event.length = len;
>> + return item;
>> +}
>
>> [...]
>
>> +static void ssam_event_queue_work_fn(struct work_struct *work)
>> +{
>> + struct ssam_event_queue *queue;
>> + struct ssam_event_item *item;
>> + struct ssam_nf *nf;
>> + struct device *dev;
>> + int i;
>> +
>> + queue = container_of(work, struct ssam_event_queue, work);
>> + nf = &queue->cplt->event.notif;
>> + dev = queue->cplt->dev;
>> +
>> + // limit number of processed events to avoid livelocking
>> + for (i = 0; i < 10; i++) {
>> + item = ssam_event_queue_pop(queue);
>> + if (item == NULL)
>
> I believe `!item` is preferred.
Right.
>> + return;
>> +
>> + ssam_nf_call(nf, dev, item->rqid, &item->event);
>> + kfree(item);
>> + }
>> +
>> + if (!ssam_event_queue_is_empty(queue))
>> + ssam_cplt_submit(queue->cplt, &queue->work);
>> +}
>
>> [...]
>
>> +static void ssam_handle_event(struct ssh_rtl *rtl,
>> + const struct ssh_command *cmd,
>> + const struct ssam_span *data)
>> +{
>> + struct ssam_controller *ctrl = to_ssam_controller(rtl, rtl);
>> + struct ssam_event_item *item;
>> +
>> + item = ssam_event_item_alloc(data->len, GFP_KERNEL);
>> + if (!item)
>> + return;
>> +
>> + item->rqid = get_unaligned_le16(&cmd->rqid);
>> + item->event.target_category = cmd->tc;
>> + item->event.target_id = cmd->tid_in;
>> + item->event.command_id = cmd->cid;
>> + item->event.instance_id = cmd->iid;
>> + memcpy(&item->event.data[0], data->ptr, data->len);
>> +
>> + WARN_ON(ssam_cplt_submit_event(&ctrl->cplt, item));
>
> I believe that if submission fails, `item` is leaked.
Oh, that is correct. Thanks for spotting that!
>> +}
>> +
>> +static const struct ssh_rtl_ops ssam_rtl_ops = {
>> + .handle_event = ssam_handle_event,
>> +};
>> +
>> +
>> +static bool ssam_notifier_empty(struct ssam_controller *ctrl);
>
> I think it'd be better to name it `ssam_notifier_is_empty()` to be consistent
> with the rest of the patch.
Okay, I'll change that.
>> [...]
>
>> +static int ssam_controller_caps_load_from_acpi(
>> + acpi_handle handle, struct ssam_controller_caps *caps)
>> +{
>> + u32 d3_closes_handle = false;
>
> Assinging a boolean like this to a `u32` looks very odd to me.
The value returned by the corresponding DSM call is an integer, but
essentially only contains a bool value (zero vs. one). I thought using
false here explicitly for initialization helps clarify that. That also
makes it consistent with the "caps->d3_closes_handle = false" line
below.
>> + u64 funcs;
>> + int status;
>> +
>> + // set defaults
>> + caps->ssh_power_profile = (u32)-1;
>> + caps->screen_on_sleep_idle_timeout = (u32)-1;
>> + caps->screen_off_sleep_idle_timeout = (u32)-1;
>> + caps->d3_closes_handle = false;
>> + caps->ssh_buffer_size = (u32)-1;
[...]
>> +void ssam_controller_shutdown(struct ssam_controller *ctrl)
>> +{
>> + enum ssam_controller_state s = ctrl->state;
>> + int status;
>> +
>> + if (s == SSAM_CONTROLLER_UNINITIALIZED || s == SSAM_CONTROLLER_STOPPED)
>> + return;
>> +
>> + // try to flush pending events and requests while everything still works
>> + status = ssh_rtl_flush(&ctrl->rtl, msecs_to_jiffies(5000));
>
> Wouldn't it be better to name that 5000?
I'll do that.
>> + if (status) {
>> + ssam_err(ctrl, "failed to flush request transport layer: %d\n",
>> + status);
>> + }
[...]
>> +int ssam_request_sync_alloc(size_t payload_len, gfp_t flags,
>> + struct ssam_request_sync **rqst,
>> + struct ssam_span *buffer)
>> +{
>> + size_t msglen = SSH_COMMAND_MESSAGE_LENGTH(payload_len);
>> +
>> + *rqst = kzalloc(sizeof(**rqst) + msglen, flags);
>> + if (!*rqst)
>> + return -ENOMEM;
>> +
>> + buffer->ptr = (u8 *)(*rqst + 1);
>> + buffer->len = msglen;
>> +
>> + return 0;
>> +}
>
> I think there is a bit of incosistency: sometimes you use ** pointer + return int,
> sometimes you return a pointer with potentially embedded errno. I think it would
> be better if you stuck with one or the other.
My rule of thumb is: If it's one output parameter, use a pointer as
return value. If it's two or more output parameters (as in this case,
"buffer" is written to), use two pointer arguments and an int as return.
I noticed that ssam_client_bind doesn't follow that scheme so I'll
change that.
>> [...]
>
>> +static int ssam_ssh_event_disable(struct ssam_controller *ctrl,
>> + struct ssam_event_registry reg,
>> + struct ssam_event_id id, u8 flags)
>> +{
>> [...]
>> + rqst.command_id = reg.cid_disable;
>
> If I see it correctly, this line is the only significant difference between this one
> and the previous function. Is there any reason they're not combined?
Debug and error messages are also different. I'll try to put the common
code into one function though.
[...]
>> +void ssam_irq_disarm_wakeup(struct ssam_controller *ctrl)
>> +{
>> + int status;
>> +
>> + if (ctrl->irq.wakeup_enabled) {
>> + status = disable_irq_wake(ctrl->irq.num);
>> + if (status)
>> + ssam_err(ctrl, "failed to disable wake IRQ: %d\n", status);
>> +
>> + ctrl->irq.wakeup_enabled = false;
>
> It's set to false even if `disable_irq_wake()` fails?
Yes, on purpose. The irq.wakeup_enabled flag is there to ensure that
disable_irq_wake() gets called only if there has been a successful
enable_irq_wake() call.
This seems to be a common pattern, see e.g. /drivers/mmc/host/sdhci.c
>> + }
>> + disable_irq(ctrl->irq.num);
>> +}
>
>> [...]
>
>> +static int ssam_try_set_controller(struct ssam_controller *ctrl)
>> +{
>> + int status = 0;
>> +
>> + spin_lock(&__ssam_controller_lock);
>> + if (!__ssam_controller)
>> + __ssam_controller = ctrl;
>> + else
>> + status = -EBUSY;
>
> I feel like EBUSY might not be the best errno here.
Okay, I'll replace it with EEXIST.
>> + spin_unlock(&__ssam_controller_lock);
>> +
>> + return status;
>> +}
>
>> [...]
>
>> +int ssam_client_link(struct ssam_controller *c, struct device *client)
>> +{
>> + const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
>> + struct device_link *link;
>> + struct device *ctrldev;
>> +
>> + ssam_controller_statelock(c);
>> +
>> + if (c->state != SSAM_CONTROLLER_STARTED) {
>> + ssam_controller_stateunlock(c);
>> + return -ENXIO;
>> + }
>> +
>> + ctrldev = ssam_controller_device(c);
>> + if (!ctrldev) {
>> + ssam_controller_stateunlock(c);
>> + return -ENXIO;
>> + }
>> +
>
> I'm not sure if ENXIO is the best errno in the last two returns;
I guess I can return ENODEV.
>> + link = device_link_add(client, ctrldev, flags);
>> + if (!link) {
>> + ssam_controller_stateunlock(c);
>> + return -ENOMEM;
>> + }
>> +
>> + /*
>> + * Return -ENXIO if supplier driver is on its way to be removed. In this
>> + * case, the controller won't be around for much longer and the device
>> + * link is not going to save us any more, as unbinding is already in
>> + * progress.
>> + */
>> + if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND) {
>> + ssam_controller_stateunlock(c);
>> + return -ENXIO;
>> + }
>> +
>> + ssam_controller_stateunlock(c);
>> + return 0;
>> +}
>
>> [...]
>
>> +int ssam_client_bind(struct device *client, struct ssam_controller **ctrl)
>> +{
>> + struct ssam_controller *c;
>> + int status;
>> +
>> + c = ssam_get_controller();
>> + if (!c)
>> + return -ENXIO;
>
> To me, ENODEV seems like a better fit here.
Okay, I'll change it as well as above.
>> +
>> + status = ssam_client_link(c, client);
>
>> [...]
>
>
>> +static ssize_t firmware_version_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct ssam_controller *ctrl = dev_get_drvdata(dev);
>> + u32 version, a, b, c;
>> + int status;
>> +
>> + status = ssam_get_firmware_version(ctrl, &version);
>> + if (status < 0)
>> + return status;
>> +
>> + a = (version >> 24) & 0xff;
>> + b = ((version >> 8) & 0xffff);
>> + c = version & 0xff;
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%u.%u.%u\n", a, b, c);
>
> `snprintf()` takes care of the null byte, so simply `PAGE_SIZE` would've been
> sufficient. But that doesn't matter much since you should use `sysfs_emit()`.
I didn't know about sysfs_emit(), I'll switch to that then. Also you're
right about that "- 1", not sure where I got that from.
>> +}
>> +static DEVICE_ATTR_RO(firmware_version);
>> +
>> +static struct attribute *ssam_sam_attrs[] = {
>> + &dev_attr_firmware_version.attr,
>> + NULL,
> ^
> I believe it is preferred to omit the comma after the terminating entry.
Right.
>> [...]
>
>> +/**
>> + * msgb_push_cmd() - Push a SSH command frame with payload to the buffer.
>> + * @msgb: The message buffer.
>> + * @seq: The sequence ID (SEQ) of the frame/packet.
>> + * @rqid: The request ID (RQID) of the request contained in the frame.
>> + * @rqst: The request to wrap in the frame.
>> + */
>> +static inline void msgb_push_cmd(struct msgbuf *msgb, u8 seq, u16 rqid,
>> + const struct ssam_request *rqst)
>> +{
>> + struct ssh_command *cmd;
>> + const u8 *cmd_begin;
>> + const u8 type = SSH_FRAME_TYPE_DATA_SEQ;
>> +
>> + // SYN
>> + msgb_push_syn(msgb);
>> +
>> + // command frame + crc
>> + msgb_push_frame(msgb, type, sizeof(*cmd) + rqst->length, seq);
>> +
>> + // frame payload: command struct + payload
>> + if (WARN_ON(msgb->ptr + sizeof(*cmd) > msgb->end))
>> + return;
>> +
>> + cmd_begin = msgb->ptr;
>> + cmd = (struct ssh_command *)msgb->ptr;
>
> I believe this violates strict aliasing.
It does. The same happens in msgb_push_frame(), too. The kernel is
built with -fno-strict-aliasing though if I remember correctly.
I guess I can try to avoid that in those two functions with
put_unaligned and offsetof to compute the pointers.
>> [...]
>
>> + * Note that the packet completion callback is, in case of success and for a
>> + * sequenced packet, guaranteed to run on the receiver thread, thus providing
>> + * a way to reliably identify responses to the packet. The packet completion
>> + * callback is only run once and it does not indicate that the packet has
>> + * fully left the system (for this, one should rely on the release method,
>> + * triggered when the reference count of the packet reaches zero). In case of
>> + * re-submission (and with somewhat unlikely timing), it may be possible that
>> + * the packet is being re-transmitted while the completion callback runs.
>> + * Completion will occur both on success and internal error, as well as when
>> + * the packet is canceled.
>
> If I understand it correctly, it is possible that submission of a packet fails
> for the first time, but it's scheduled for resubmission, and this retransmission
> happens at the same time when the complete() callback is called. If that's the
> case, then the callback is called with an error condition, no? Thus it is possible
> that a packet is successfully submitted (for the second, third, etc. time), but the
> complete() callback receives notification about failure? Or am I missing something?
Not sure I can follow you here. What do you mean by "fails for the first time"?
A couple of notes before I try to explain the error cases:
- Resubmission only happens on timeout or NAK.
- Only sequenced packets can be resubmitted (unsequenced are not added
to the pending set, thus do not have a packet timeout and do not
react to NAKs).
- Completion means the packet gets locked, which prevents it from being
resubmitted and retransmitted, and is removed from all collections.
- Completion cannot be triggered by NAK. In case a NAK is received and
all tries have been exceeded, the packet still waits for the timeout
(or ACK, whatever happens first) one last time.
The error cases:
- serdev: Failure to send via serdev results in immediate completion
with error on the transmitter thread. Due to a NAK (or a _very_
unlikely timeout), the packet can be queued for a second time at that
point, but, as completion runs on the transmitter thread in this
case, the packet is removed from the queue before the thread goes on
to get the next packet.
- Timeout: On resubmission, the timeout of a packet is disabled. It is
(re-)armed directly before starting transmission on the transmitter
thread.
What _could_ now happen is that, for some reason, the transmitter
thread waits the full timeout period between arming the timeout and
actually sending it (which I'd consider highly unlikely). Note that
"actually sending it" includes whatever asynchronous stuff the serdev
core does after serdev_device_write_buf() returns.
In that case, the packet can be transmitted _after_ it has received a
timeout. So assuming this is the last try, then the packet can be
sent after completion with -ETIMEDOUT.
I believe this is what you meant?
Again, I consider this highly unlikely as this would require either
the scheduler to pause our thread for a full timeout period or
require the serdev core to basically do nothing for a full timeout
period. I'd argue this is a lot less likely than a dropped ACK packet
from the EC (which results in the same thing: EC receives data but we
think it didn't, causing a timeout error).
Note that putting the timeout (re-)arming step after the last call to
serdev_device_write_buf() doesn't really help either, as that only
writes to the transport buffer. If we would want to guarantee that
this doesn't happen, we'd have to explicitly wait for the hardware to
finish transmitting, which essentially breaks the whole point of
buffering.
Also I prefer to have the timeout cover the transmission part as
well. In the very least, this simplifies reasoning about it once I've
restricted it to be (re-)set under the pending lock only (as
mentioned in the message to Andy).
- Finally, there is completion by cancellation or shutdown, for which
the packet can have been (or currently be) transmitted successfully
but completion returns with -ECANCELLED or -ESHUTDOWN, respectively.
This is the expected behavior in those cases. Again, the completion
procedure takes care of locking the packet and removing all
references from the collections.
Also there's one success case, when we receive an ACK after a timeout
and during the re-transmission. Then the packet is completed with
success, but might still be transmitted once more. The EC should detect
that packet as re-submission via its packet ID and ignore it.
>> [...]
>
>> +int ssh_ptl_rx_rcvbuf(struct ssh_ptl *ptl, const u8 *buf, size_t n)
>> +{
>> + int used;
>> +
>> + if (test_bit(SSH_PTL_SF_SHUTDOWN_BIT, &ptl->state))
>> + return -ESHUTDOWN;
>> +
>> + used = kfifo_in(&ptl->rx.fifo, buf, n);
>
> Isn't it possible that `n` is greater than the free space in the fifo? What
> happens then?
If I've read the documentation of kfifo_in() right, it takes at most n
elements. If n is greater than the free space, it will only take as much
elements as it has space. The return value of kfifo_in() is the number
of elements actually consumed. We simply return that here (as elements =
bytes) and ultimately let the serdev/tty core take care of handling
that.
>> + if (used)
>> + ssh_ptl_rx_wakeup(ptl);
>> +
>> + return used;
>> +}
[...]
>> +#define __ssam_prcond(func, p, fmt, ...) \
>> + do { \
>> + if ((p)) \
>
> I believe `if (p)` is sufficient.
Right.
>> + func((p), fmt, ##__VA_ARGS__); \
>> + } while (0)
>
>> [...]
>
>> +int sshp_parse_frame(const struct device *dev, const struct ssam_span *source,
>> + struct ssh_frame **frame, struct ssam_span *payload,
>> + size_t maxlen)
>> +{
>> [...]
>> + // ensure packet does not exceed maximum length
>> + sp.len = get_unaligned_le16(&((struct ssh_frame *)sf.ptr)->len);
>> + if (unlikely(sp.len + SSH_MESSAGE_LENGTH(0) > maxlen)) {
>> + dev_warn(dev, "rx: parser: frame too large: %u bytes\n",
>
> I believe `%hu` would be more appropriate.
Right.
>> + ((struct ssh_frame *)sf.ptr)->len);
>
> Why isn't `get_unaligned_le16()` used here? (Or simply even `sp.len`.)
This should actually be "sp.len + SSH_MESSAGE_LENGTH(0)", and also
format spezifier %zu with that as SSH_MESSAGE_LENGTH returns size_t.
>> + return -EMSGSIZE;
>> + }
>> [...]
>> + *frame = (struct ssh_frame *)sf.ptr;
>
> This also violates strict aliasing.
Sure, the whole point of this function (and ssh_parse_command below) is
to get pointers to the data structures in the given buffer, i.e. create
aliases of the right types to the right places in the buffer. Not sure
how I'd do that differently, apart from copying.
I think that this would even work properly (in practice) without setting
-fno-strict-aliasing, as all data at this point is effectively
read-only. Furthermore, after the call access is restricted to the
non-aliasing parts respectively (frame and payload), whereas the
underlying buffer they do alias isn't accessed for the rest of that
scope (i.e. until the other references don't exist any more).
Anyway, as far as I know the kernel is built with -fno-strict-aliasing.
Please correct me if I'm wrong or relying on aliasing is frowned upon.
>> + *payload = sp;
>> +
>> + dev_dbg(dev, "rx: parser: valid frame found (type: 0x%02x, len: %u)\n",
>> + (*frame)->type, (*frame)->len);
>> +
>> + return 0;
>> +}
>
>> [...]
>
>> +int sshp_parse_command(const struct device *dev, const struct ssam_span *source,
>> + struct ssh_command **command,
>> + struct ssam_span *command_data)
>> +{
>> + // check for minimum length
>> + if (unlikely(source->len < sizeof(struct ssh_command))) {
>> + *command = NULL;
>> + command_data->ptr = NULL;
>> + command_data->len = 0;
>> +
>> + dev_err(dev, "rx: parser: command payload is too short\n");
>> + return -ENOMSG;
>> + }
>> +
>> + *command = (struct ssh_command *)source->ptr;
>
> I'm quite sure this also violates strict aliasing.
It does, see above.
>> + command_data->ptr = source->ptr + sizeof(struct ssh_command);
>> + command_data->len = source->len - sizeof(struct ssh_command);
>> +
>> + dev_dbg(dev, "rx: parser: valid command found (tc: 0x%02x, cid: 0x%02x)\n",
>> + (*command)->tc, (*command)->cid);
>> +
>> + return 0;
>> +}
>
>> [...]
>
>> +#define SSH_MSGOFFSET_FRAME(field) \
>> + (sizeof(u16) + offsetof(struct ssh_frame, field))
>> +
>> +/**
>> + * SSH_MSGOFFSET_COMMAND() - Compute offset in SSH message to specified field
>> + * in command.
>> + * @field: The field for which the offset should be computed.
>> + *
>> + * Return: Returns the offset of the specified &struct ssh_command field in
>> + * the raw SSH message data.
>> + */
>> +#define SSH_MSGOFFSET_COMMAND(field) \
>> + (2ull * sizeof(u16) + sizeof(struct ssh_frame) \
>> + + offsetof(struct ssh_command, field))
>
> I believe it should be noted (here and for `SSH_MSGOFFSET_FRAME()`) why the
> `sizeof(u16)`s are necessary.
Right, I can add an explanation for this. One is for the SYN bytes, the
other one for the frame CRC (between frame and command).
>> +
>> +/*
>> + * SSH_MSG_SYN - SSH message synchronization (SYN) bytes as u16.
>> + */
>> +#define SSH_MSG_SYN ((u16)0x55aa)
>
>> [...]
>
>> +enum ssam_ssh_tc {
>> + /* Known SSH/EC target categories. */
>> + // category 0x00 is invalid for EC use
>> + SSAM_SSH_TC_SAM = 0x01, // generic system functionality, real-time clock
>> + SSAM_SSH_TC_BAT = 0x02, // battery/power subsystem
>> + SSAM_SSH_TC_TMP = 0x03, // thermal subsystem
>> + SSAM_SSH_TC_PMC = 0x04,
>> + SSAM_SSH_TC_FAN = 0x05,
>> + SSAM_SSH_TC_PoM = 0x06,
>> + SSAM_SSH_TC_DBG = 0x07,
>> + SSAM_SSH_TC_KBD = 0x08, // legacy keyboard (Laptop 1/2)
>> + SSAM_SSH_TC_FWU = 0x09,
>> + SSAM_SSH_TC_UNI = 0x0a,
>> + SSAM_SSH_TC_LPC = 0x0b,
>> + SSAM_SSH_TC_TCL = 0x0c,
>> + SSAM_SSH_TC_SFL = 0x0d,
>> + SSAM_SSH_TC_KIP = 0x0e,
>> + SSAM_SSH_TC_EXT = 0x0f,
>> + SSAM_SSH_TC_BLD = 0x10,
>> + SSAM_SSH_TC_BAS = 0x11, // detachment system (Surface Book 2/3)
>> + SSAM_SSH_TC_SEN = 0x12,
>> + SSAM_SSH_TC_SRQ = 0x13,
>> + SSAM_SSH_TC_MCU = 0x14,
>> + SSAM_SSH_TC_HID = 0x15, // generic HID input subsystem
>> + SSAM_SSH_TC_TCH = 0x16,
>> + SSAM_SSH_TC_BKL = 0x17,
>> + SSAM_SSH_TC_TAM = 0x18,
>> + SSAM_SSH_TC_ACC = 0x19,
>> + SSAM_SSH_TC_UFI = 0x1a,
>> + SSAM_SSH_TC_USC = 0x1b,
>> + SSAM_SSH_TC_PEN = 0x1c,
>> + SSAM_SSH_TC_VID = 0x1d,
>> + SSAM_SSH_TC_AUD = 0x1e,
>> + SSAM_SSH_TC_SMC = 0x1f,
>> + SSAM_SSH_TC_KPD = 0x20,
>> + SSAM_SSH_TC_REG = 0x21,
>> +};
>
> Is it known what these abbreviations stand for? Maybe I missed them?
The comments state all we really know about these (through observation
and experimentation). The table itself has been extracted from the
Windows driver, but the abbreviations and values are all we're getting
from it.
I could go ahead and guess for a couple of them but for most of them we
haven't really seen any request at all, let alone know what the request
does.
For those that we now, the abbreviations should be fairly easy to guess
based on the comments, e.g. BAT -> battery, TMP -> temperature. I'm not
sure about BAS, which could mean base (as that's the detachment system
allowing to separate the top part of the Surface Books from the base).
REG is currently missing a comment because I'm not entirely sure if it's
just for registring (enabling/disabling) events or if it also has any
other purpose. I'll add a comment for it though, then we have comments
on all that are somewhat known.
>> [...]
>
>> +/**
>> + * struct ssh_request_ops - Callback operations for a SSH request.
>> + * @release: Function called when the request's reference count reaches zero.
>> + * This callback must be relied upon to ensure that the request has
>> + * left the transport systems (both, packet an request systems).
>> + * @complete: Function called when the request is completed, either with
>> + * success or failure. The command data for the request response
>> + * is provided via the &struct ssh_command parameter (``cmd``),
>> + * the command payload of the request response via the &struct
>> + * ssh_span parameter (``data``).
>> + *
>> + * If the request does not have any response or has not been
>> + * completed with success, both ``cmd`` and ``data`` parameters will
>> + * be NULL. If the request response does not have any command
>> + * payload, the ``data`` span will be an empty (zero-length) span.
>> + *
>> + * In case of failure, the reason for the failure is indicated by
>> + * the value of the provided status code argument (``status``). This
>> + * value will be zero in case of success.
>
> I believe it should be noted if the `status` argument is a regular errno,
> or something different.
Right, I'll add that.
>> + *
>> + * Note that a call to this callback does not guarantee that the
>> + * request is not in use by the transport systems any more.
>> + */
>> +struct ssh_request_ops {
>> + void (*release)(struct ssh_request *rqst);
>> + void (*complete)(struct ssh_request *rqst,
>> + const struct ssh_command *cmd,
>> + const struct ssam_span *data, int status);
>> +};
>> [...]
>
>
> I have to agree, this is quite a sizable patch, although I think it's well-commented,
> which helps reading the code by a lot, however, in some places I feel like it's a bit
> over-engineered (or maybe I just cannot fully appreciate the subject at hand at the moment),
> nonetheless, I applaud your efforts, I can only imagine the hours that must have gone into it.
I think much of what's probably considered over-engineered is the result
of reworking the core after that had some reliability issues (dropped
packets and timeouts). Specifically, the introduction of dedicated
transmitter and receiver threads combined with the need to properly
handle NAKs has lead to the current asynchronous system.
I have probably gone a bit too far towards generifying that, but I
belive that this should make it easier to adapt for protocol additions
or errors in the future.
Other parts, like the RB-tree for event reference counting seem,
unfortunately, to be necessary. Events are enabled by registry
(specifying which command to use to enable/disable the event) and ID
(which event to enable/disable on the registry). So without knowing
which registry and ID pairs are valid, we'll have to use some sort of
map. Could have been a simple list, but I think an RB-tree makes a bit
more sense here.
Thank you for your review!
Regards,
Max
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
2020-11-18 23:06 ` Maximilian Luz
@ 2020-11-19 15:54 ` Barnabás Pőcze
2020-11-19 17:35 ` Maximilian Luz
0 siblings, 1 reply; 13+ messages in thread
From: Barnabás Pőcze @ 2020-11-19 15:54 UTC (permalink / raw)
To: Maximilian Luz
Cc: linux-kernel, Hans de Goede, Mark Gross, Arnd Bergmann,
Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Rafael J . Wysocki,
Len Brown, Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-serial, linux-acpi
Hi
2020. november 19., csütörtök 0:06 keltezéssel, Maximilian Luz <luzmaximilian@gmail.com> írta:
> [...]
> >> +/**
> >> + * ssam_nf_head_destroy() - Deinitialize the given notifier head.
> >> + * @nh: The notifier head to deinitialize.
> >> + */
> >> +static void ssam_nf_head_destroy(struct ssam_nf_head *nh)
> >> +{
> >> + cleanup_srcu_struct(&nh->srcu);
> >> +}
> >
> > I'm also wondering if there's any reason why these static one-liner functions are
> > not explicitly marked inline.
>
> Isn't inline more of a linkage keyword at this point? I fully expect any
> compiler to inline things like this automatically at the first hint of
> an optimization flag.
>
I also expect the compiler to inline such functions even without the `inline`
specifier. In retrospect I'm not sure why I actually made this comment, possibly
because of my preference, but I believe it's fine either way, so my comment is
moot, sorry.
> [...]
> >> + * error values via ssam_notifier_from_errno() to notifier values.
> >> + *
> >> + * Also note that any callback that could handle an event should return a value
> >> + * with bit %SSAM_NOTIF_HANDLED set, indicating that the event does not go
> >> + * unhandled/ignored. In case no registered callback could handle an event,
> >> + * this function will emit a warning.
> >> + *
> >> + * In case a callback failed, this function will emit an error message.
> >> + */
> >> +static void ssam_nf_call(struct ssam_nf *nf, struct device *dev, u16 rqid,
> >> + struct ssam_event *event)
> >> +{
> >> + struct ssam_nf_head *nf_head;
> >> + int status, nf_ret;
> >> +
> >> + if (!ssh_rqid_is_event(rqid)) {
> >> + dev_warn(dev, "event: unsupported rqid: 0x%04x\n", rqid);
> >
> > A small note, "%#04x" would insert the "0x" prefix.
>
> Oh neat, didn't know that. Should be "%#06x" though, right?
>
Yes, indeed, sorry, it should be "%#06x".
> [...]
> >> +static int ssam_controller_caps_load_from_acpi(
> >> + acpi_handle handle, struct ssam_controller_caps *caps)
> >> +{
> >> + u32 d3_closes_handle = false;
> >
> > Assinging a boolean like this to a `u32` looks very odd to me.
>
> The value returned by the corresponding DSM call is an integer, but
> essentially only contains a bool value (zero vs. one). I thought using
> false here explicitly for initialization helps clarify that. That also
> makes it consistent with the "caps->d3_closes_handle = false" line
> below.
>
Although I still find it pretty odd, your explanation makes sense to me.
> >> +int ssam_request_sync_alloc(size_t payload_len, gfp_t flags,
> >> + struct ssam_request_sync **rqst,
> >> + struct ssam_span *buffer)
> >> +{
> >> + size_t msglen = SSH_COMMAND_MESSAGE_LENGTH(payload_len);
> >> +
> >> + *rqst = kzalloc(sizeof(**rqst) + msglen, flags);
> >> + if (!*rqst)
> >> + return -ENOMEM;
> >> +
> >> + buffer->ptr = (u8 *)(*rqst + 1);
> >> + buffer->len = msglen;
> >> +
> >> + return 0;
> >> +}
> >
> > I think there is a bit of incosistency: sometimes you use ** pointer + return int,
> > sometimes you return a pointer with potentially embedded errno. I think it would
> > be better if you stuck with one or the other.
>
> My rule of thumb is: If it's one output parameter, use a pointer as
> return value. If it's two or more output parameters (as in this case,
> "buffer" is written to), use two pointer arguments and an int as return.
>
> I noticed that ssam_client_bind doesn't follow that scheme so I'll
> change that.
>
I see, thanks for the explanation, what prompted me to make this comment
was that, as you noted, there were functions with one output parameter that
were not consistent.
> [...]
> >> + * Note that the packet completion callback is, in case of success and for a
> >> + * sequenced packet, guaranteed to run on the receiver thread, thus providing
> >> + * a way to reliably identify responses to the packet. The packet completion
> >> + * callback is only run once and it does not indicate that the packet has
> >> + * fully left the system (for this, one should rely on the release method,
> >> + * triggered when the reference count of the packet reaches zero). In case of
> >> + * re-submission (and with somewhat unlikely timing), it may be possible that
> >> + * the packet is being re-transmitted while the completion callback runs.
> >> + * Completion will occur both on success and internal error, as well as when
> >> + * the packet is canceled.
> >
> > If I understand it correctly, it is possible that submission of a packet fails
> > for the first time, but it's scheduled for resubmission, and this retransmission
> > happens at the same time when the complete() callback is called. If that's the
> > case, then the callback is called with an error condition, no? Thus it is possible
> > that a packet is successfully submitted (for the second, third, etc. time), but the
> > complete() callback receives notification about failure? Or am I missing something?
>
> Not sure I can follow you here. What do you mean by "fails for the first time"?
>
> A couple of notes before I try to explain the error cases:
>
> - Resubmission only happens on timeout or NAK.
>
> - Only sequenced packets can be resubmitted (unsequenced are not added
> to the pending set, thus do not have a packet timeout and do not
> react to NAKs).
>
> - Completion means the packet gets locked, which prevents it from being
> resubmitted and retransmitted, and is removed from all collections.
>
> - Completion cannot be triggered by NAK. In case a NAK is received and
> all tries have been exceeded, the packet still waits for the timeout
> (or ACK, whatever happens first) one last time.
>
> The error cases:
>
> - serdev: Failure to send via serdev results in immediate completion
> with error on the transmitter thread. Due to a NAK (or a _very_
> unlikely timeout), the packet can be queued for a second time at that
> point, but, as completion runs on the transmitter thread in this
> case, the packet is removed from the queue before the thread goes on
> to get the next packet.
>
> - Timeout: On resubmission, the timeout of a packet is disabled. It is
> (re-)armed directly before starting transmission on the transmitter
> thread.
>
> What _could_ now happen is that, for some reason, the transmitter
> thread waits the full timeout period between arming the timeout and
> actually sending it (which I'd consider highly unlikely). Note that
> "actually sending it" includes whatever asynchronous stuff the serdev
> core does after serdev_device_write_buf() returns.
>
> In that case, the packet can be transmitted _after_ it has received a
> timeout. So assuming this is the last try, then the packet can be
> sent after completion with -ETIMEDOUT.
>
> I believe this is what you meant?
>
Yes, that's what I meant, thanks for the clarification and broader explanation
about the things at play here.
> Again, I consider this highly unlikely as this would require either
> the scheduler to pause our thread for a full timeout period or
> require the serdev core to basically do nothing for a full timeout
> period. I'd argue this is a lot less likely than a dropped ACK packet
> from the EC (which results in the same thing: EC receives data but we
> think it didn't, causing a timeout error).
>
> Note that putting the timeout (re-)arming step after the last call to
> serdev_device_write_buf() doesn't really help either, as that only
> writes to the transport buffer. If we would want to guarantee that
> this doesn't happen, we'd have to explicitly wait for the hardware to
> finish transmitting, which essentially breaks the whole point of
> buffering.
>
> Also I prefer to have the timeout cover the transmission part as
> well. In the very least, this simplifies reasoning about it once I've
> restricted it to be (re-)set under the pending lock only (as
> mentioned in the message to Andy).
>
> - Finally, there is completion by cancellation or shutdown, for which
> the packet can have been (or currently be) transmitted successfully
> but completion returns with -ECANCELLED or -ESHUTDOWN, respectively.
> This is the expected behavior in those cases. Again, the completion
> procedure takes care of locking the packet and removing all
> references from the collections.
>
> Also there's one success case, when we receive an ACK after a timeout
> and during the re-transmission. Then the packet is completed with
> success, but might still be transmitted once more. The EC should detect
> that packet as re-submission via its packet ID and ignore it.
>
> >> [...]
> >
> >> +int ssh_ptl_rx_rcvbuf(struct ssh_ptl *ptl, const u8 *buf, size_t n)
> >> +{
> >> + int used;
> >> +
> >> + if (test_bit(SSH_PTL_SF_SHUTDOWN_BIT, &ptl->state))
> >> + return -ESHUTDOWN;
> >> +
> >> + used = kfifo_in(&ptl->rx.fifo, buf, n);
> >
> > Isn't it possible that `n` is greater than the free space in the fifo? What
> > happens then?
>
> If I've read the documentation of kfifo_in() right, it takes at most n
> elements. If n is greater than the free space, it will only take as much
> elements as it has space. The return value of kfifo_in() is the number
> of elements actually consumed. We simply return that here (as elements =
> bytes) and ultimately let the serdev/tty core take care of handling
> that.
>
Ahh, sorry, you're right.
> >> + if (used)
> >> + ssh_ptl_rx_wakeup(ptl);
> >> +
> >> + return used;
> >> +}
> [...]
> >> + ((struct ssh_frame *)sf.ptr)->len);
> >
> > Why isn't `get_unaligned_le16()` used here? (Or simply even `sp.len`.)
>
> This should actually be "sp.len + SSH_MESSAGE_LENGTH(0)", and also
> format spezifier %zu with that as SSH_MESSAGE_LENGTH returns size_t.
>
Yes, indeed, sorry, I missed the `+ SSH_MESSAGE_LENGTH(0)` part.
> >> + return -EMSGSIZE;
> >> + }
> >> [...]
> >> + *frame = (struct ssh_frame *)sf.ptr;
> >
> > This also violates strict aliasing.
>
> Sure, the whole point of this function (and ssh_parse_command below) is
> to get pointers to the data structures in the given buffer, i.e. create
> aliases of the right types to the right places in the buffer. Not sure
> how I'd do that differently, apart from copying.
>
> I think that this would even work properly (in practice) without setting
> -fno-strict-aliasing, as all data at this point is effectively
> read-only. Furthermore, after the call access is restricted to the
> non-aliasing parts respectively (frame and payload), whereas the
> underlying buffer they do alias isn't accessed for the rest of that
> scope (i.e. until the other references don't exist any more).
>
> Anyway, as far as I know the kernel is built with -fno-strict-aliasing.
> Please correct me if I'm wrong or relying on aliasing is frowned upon.
>
I don't believe it could cause issue here, I just wanted to note it.
> [...]
> >> +enum ssam_ssh_tc {
> >> + /* Known SSH/EC target categories. */
> >> + // category 0x00 is invalid for EC use
> >> + SSAM_SSH_TC_SAM = 0x01, // generic system functionality, real-time clock
> >> + SSAM_SSH_TC_BAT = 0x02, // battery/power subsystem
> >> + SSAM_SSH_TC_TMP = 0x03, // thermal subsystem
> >> + SSAM_SSH_TC_PMC = 0x04,
> >> + SSAM_SSH_TC_FAN = 0x05,
> >> + SSAM_SSH_TC_PoM = 0x06,
> >> + SSAM_SSH_TC_DBG = 0x07,
> >> + SSAM_SSH_TC_KBD = 0x08, // legacy keyboard (Laptop 1/2)
> >> + SSAM_SSH_TC_FWU = 0x09,
> >> + SSAM_SSH_TC_UNI = 0x0a,
> >> + SSAM_SSH_TC_LPC = 0x0b,
> >> + SSAM_SSH_TC_TCL = 0x0c,
> >> + SSAM_SSH_TC_SFL = 0x0d,
> >> + SSAM_SSH_TC_KIP = 0x0e,
> >> + SSAM_SSH_TC_EXT = 0x0f,
> >> + SSAM_SSH_TC_BLD = 0x10,
> >> + SSAM_SSH_TC_BAS = 0x11, // detachment system (Surface Book 2/3)
> >> + SSAM_SSH_TC_SEN = 0x12,
> >> + SSAM_SSH_TC_SRQ = 0x13,
> >> + SSAM_SSH_TC_MCU = 0x14,
> >> + SSAM_SSH_TC_HID = 0x15, // generic HID input subsystem
> >> + SSAM_SSH_TC_TCH = 0x16,
> >> + SSAM_SSH_TC_BKL = 0x17,
> >> + SSAM_SSH_TC_TAM = 0x18,
> >> + SSAM_SSH_TC_ACC = 0x19,
> >> + SSAM_SSH_TC_UFI = 0x1a,
> >> + SSAM_SSH_TC_USC = 0x1b,
> >> + SSAM_SSH_TC_PEN = 0x1c,
> >> + SSAM_SSH_TC_VID = 0x1d,
> >> + SSAM_SSH_TC_AUD = 0x1e,
> >> + SSAM_SSH_TC_SMC = 0x1f,
> >> + SSAM_SSH_TC_KPD = 0x20,
> >> + SSAM_SSH_TC_REG = 0x21,
> >> +};
> >
> > Is it known what these abbreviations stand for? Maybe I missed them?
>
> The comments state all we really know about these (through observation
> and experimentation). The table itself has been extracted from the
> Windows driver, but the abbreviations and values are all we're getting
> from it.
I see, thanks for the clarification. For some reason, I believed the
"Known SSH/EC target categories" comment means that those are known, as in,
it is known what they are for, etc., not just the abbreviation.
> [...]
Regards,
Barnabás Pőcze
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
2020-11-19 15:54 ` Barnabás Pőcze
@ 2020-11-19 17:35 ` Maximilian Luz
0 siblings, 0 replies; 13+ messages in thread
From: Maximilian Luz @ 2020-11-19 17:35 UTC (permalink / raw)
To: Barnabás Pőcze
Cc: linux-kernel, Hans de Goede, Mark Gross, Arnd Bergmann,
Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Rafael J . Wysocki,
Len Brown, Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-serial, linux-acpi
On 11/19/20 4:54 PM, Barnabás Pőcze wrote:
> Hi
[...]
>>>> +enum ssam_ssh_tc {
>>>> + /* Known SSH/EC target categories. */
>>>> + // category 0x00 is invalid for EC use
>>>> + SSAM_SSH_TC_SAM = 0x01, // generic system functionality, real-time clock
>>>> + SSAM_SSH_TC_BAT = 0x02, // battery/power subsystem
>>>> + SSAM_SSH_TC_TMP = 0x03, // thermal subsystem
>>>> + SSAM_SSH_TC_PMC = 0x04,
>>>> + SSAM_SSH_TC_FAN = 0x05,
>>>> + SSAM_SSH_TC_PoM = 0x06,
>>>> + SSAM_SSH_TC_DBG = 0x07,
>>>> + SSAM_SSH_TC_KBD = 0x08, // legacy keyboard (Laptop 1/2)
>>>> + SSAM_SSH_TC_FWU = 0x09,
>>>> + SSAM_SSH_TC_UNI = 0x0a,
>>>> + SSAM_SSH_TC_LPC = 0x0b,
>>>> + SSAM_SSH_TC_TCL = 0x0c,
>>>> + SSAM_SSH_TC_SFL = 0x0d,
>>>> + SSAM_SSH_TC_KIP = 0x0e,
>>>> + SSAM_SSH_TC_EXT = 0x0f,
>>>> + SSAM_SSH_TC_BLD = 0x10,
>>>> + SSAM_SSH_TC_BAS = 0x11, // detachment system (Surface Book 2/3)
>>>> + SSAM_SSH_TC_SEN = 0x12,
>>>> + SSAM_SSH_TC_SRQ = 0x13,
>>>> + SSAM_SSH_TC_MCU = 0x14,
>>>> + SSAM_SSH_TC_HID = 0x15, // generic HID input subsystem
>>>> + SSAM_SSH_TC_TCH = 0x16,
>>>> + SSAM_SSH_TC_BKL = 0x17,
>>>> + SSAM_SSH_TC_TAM = 0x18,
>>>> + SSAM_SSH_TC_ACC = 0x19,
>>>> + SSAM_SSH_TC_UFI = 0x1a,
>>>> + SSAM_SSH_TC_USC = 0x1b,
>>>> + SSAM_SSH_TC_PEN = 0x1c,
>>>> + SSAM_SSH_TC_VID = 0x1d,
>>>> + SSAM_SSH_TC_AUD = 0x1e,
>>>> + SSAM_SSH_TC_SMC = 0x1f,
>>>> + SSAM_SSH_TC_KPD = 0x20,
>>>> + SSAM_SSH_TC_REG = 0x21,
>>>> +};
>>>
>>> Is it known what these abbreviations stand for? Maybe I missed them?
>>
>> The comments state all we really know about these (through observation
>> and experimentation). The table itself has been extracted from the
>> Windows driver, but the abbreviations and values are all we're getting
>> from it.
>
> I see, thanks for the clarification. For some reason, I believed the
> "Known SSH/EC target categories" comment means that those are known, as in,
> it is known what they are for, etc., not just the abbreviation.
Right, that can be misunderstood, sorry. It's probably best if I add a
comment explaining that in a bit more detail and noting where those
values and abbreviations come from.
Thanks,
Max
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module
2020-11-15 19:21 [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module Maximilian Luz
2020-11-15 19:21 ` [PATCH 9/9] platform/surface: Add Surface ACPI Notify driver Maximilian Luz
[not found] ` <20201115192143.21571-2-luzmaximilian@gmail.com>
@ 2020-11-24 11:59 ` Hans de Goede
2020-11-24 13:43 ` Maximilian Luz
2 siblings, 1 reply; 13+ messages in thread
From: Hans de Goede @ 2020-11-24 11:59 UTC (permalink / raw)
To: Maximilian Luz, linux-kernel
Cc: Mark Gross, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Jiri Slaby, Rafael J . Wysocki, Len Brown, Steven Rostedt,
Ingo Molnar, Masahiro Yamada, Michal Marek, Jonathan Corbet,
Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-serial, linux-acpi, linux-kbuild, linux-doc
Hi,
On 11/15/20 8:21 PM, Maximilian Luz wrote:
> Hello,
>
> N.B.: the following text is mostly a repeat of cover letter from the
> previous RFC for the uninitiated, which can be found at
>
> https://lore.kernel.org/linux-serial/20200923151511.3842150-1-luzmaximilian@gmail.com/
>
> See "Changes" below for an overview of differences between the RFC and
> this patchset. I hope I have addressed all comments from that in this
> version, thank you again for those.
>
> The Surface System Aggregator Module (we'll refer to it as Surface
> Aggregator or SAM below) is an embedded controller (EC) found on various
> Microsoft Surface devices. Specifically, all 4th and later generation
> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
> exception of the Surface Go series and the Surface Duo. Notably, it
> seems like this EC can also be found on the ARM-based Surface Pro X [1].
<snip>
> This patch-set can also be found at the following repository and
> reference, if you prefer to look at a kernel tree instead of these
> emails:
>
> https://github.com/linux-surface/kernel tags/s/surface-aggregator/v1
>
> Thanks,
> Max
Thank you for your work on this. It would be great if we can get better
support for the Surface line in the mainline kernel.
Since a lot of people have already commented on this series I think that
you have enough feedback to do a v2 addressing that feedback right?
For now I'm going to assume that you will do a v2 addressing the
initial round of comments and not review this myself (IOW I'll review
this when v2 is posted).
Let me know if you see things differently.
Regards,
Hans
> [1]: The Surface Pro X is, however, currently considered unsupported due
> to a lack of test candidates and, as it seems, general lack of
> Linux support on other parts. AFAIK there is an issue preventing
> serial devices from being registered, on which the core driver in
> this series is build on, thus there is no way to even test that at
> this point. I'd be happy to work out any issues regarding SAM on
> the Pro X at some point in the future, provided someone can/wants
> to actually test it.
>
> [2]: https://github.com/linux-surface/surface-aggregator-module
> [3]: https://github.com/linux-surface/linux-surface
>
>
> Note: This patch depends on
>
> [PATCH v4] platform/surface: Create a platform subdirectory for
> Microsoft Surface devices
>
> which can be found at
>
> https://lore.kernel.org/platform-driver-x86/20201009141128.683254-1-luzmaximilian@gmail.com/
>
> and is currently in platform-drivers-x86/for-next.
>
>
> Changes from the previous RFC (overview):
> - move to platform/surface
> - add copyright lines
> - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
> - change user-space interface from debugfs to misc-device
> - address issues in user-space interface
> - fix typos in commit messages and documentation
> - fix some bugs, address other issues
>
> Changes regarding specific patches (and more details) can be found on
> the individual patch.
>
>
> Maximilian Luz (9):
> platform/surface: Add Surface Aggregator subsystem
> platform/surface: aggregator: Add control packet allocation caching
> platform/surface: aggregator: Add event item allocation caching
> platform/surface: aggregator: Add trace points
> platform/surface: aggregator: Add error injection capabilities
> platform/surface: aggregator: Add dedicated bus and device type
> docs: driver-api: Add Surface Aggregator subsystem documentation
> platform/surface: Add Surface Aggregator user-space interface
> platform/surface: Add Surface ACPI Notify driver
>
> Documentation/driver-api/index.rst | 1 +
> .../surface_aggregator/client-api.rst | 38 +
> .../driver-api/surface_aggregator/client.rst | 394 +++
> .../surface_aggregator/clients/cdev.rst | 85 +
> .../surface_aggregator/clients/index.rst | 21 +
> .../surface_aggregator/clients/san.rst | 44 +
> .../driver-api/surface_aggregator/index.rst | 21 +
> .../surface_aggregator/internal-api.rst | 67 +
> .../surface_aggregator/internal.rst | 50 +
> .../surface_aggregator/overview.rst | 76 +
> .../driver-api/surface_aggregator/ssh.rst | 343 +++
> MAINTAINERS | 13 +
> drivers/platform/surface/Kconfig | 39 +
> drivers/platform/surface/Makefile | 3 +
> drivers/platform/surface/aggregator/Kconfig | 65 +
> drivers/platform/surface/aggregator/Makefile | 17 +
> drivers/platform/surface/aggregator/bus.c | 424 +++
> drivers/platform/surface/aggregator/bus.h | 27 +
> .../platform/surface/aggregator/controller.c | 2557 +++++++++++++++++
> .../platform/surface/aggregator/controller.h | 288 ++
> drivers/platform/surface/aggregator/core.c | 831 ++++++
> .../platform/surface/aggregator/ssh_msgb.h | 201 ++
> .../surface/aggregator/ssh_packet_layer.c | 2009 +++++++++++++
> .../surface/aggregator/ssh_packet_layer.h | 175 ++
> .../platform/surface/aggregator/ssh_parser.c | 229 ++
> .../platform/surface/aggregator/ssh_parser.h | 157 +
> .../surface/aggregator/ssh_request_layer.c | 1254 ++++++++
> .../surface/aggregator/ssh_request_layer.h | 142 +
> drivers/platform/surface/aggregator/trace.h | 625 ++++
> .../platform/surface/surface_acpi_notify.c | 884 ++++++
> .../surface/surface_aggregator_cdev.c | 299 ++
> include/linux/mod_devicetable.h | 18 +
> include/linux/surface_acpi_notify.h | 39 +
> include/linux/surface_aggregator/controller.h | 832 ++++++
> include/linux/surface_aggregator/device.h | 430 +++
> include/linux/surface_aggregator/serial_hub.h | 659 +++++
> include/uapi/linux/surface_aggregator/cdev.h | 58 +
> scripts/mod/devicetable-offsets.c | 8 +
> scripts/mod/file2alias.c | 23 +
> 39 files changed, 13446 insertions(+)
> create mode 100644 Documentation/driver-api/surface_aggregator/client-api.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/client.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/clients/cdev.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/clients/index.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/index.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/internal-api.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/internal.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/overview.rst
> create mode 100644 Documentation/driver-api/surface_aggregator/ssh.rst
> create mode 100644 drivers/platform/surface/aggregator/Kconfig
> create mode 100644 drivers/platform/surface/aggregator/Makefile
> create mode 100644 drivers/platform/surface/aggregator/bus.c
> create mode 100644 drivers/platform/surface/aggregator/bus.h
> create mode 100644 drivers/platform/surface/aggregator/controller.c
> create mode 100644 drivers/platform/surface/aggregator/controller.h
> create mode 100644 drivers/platform/surface/aggregator/core.c
> create mode 100644 drivers/platform/surface/aggregator/ssh_msgb.h
> create mode 100644 drivers/platform/surface/aggregator/ssh_packet_layer.c
> create mode 100644 drivers/platform/surface/aggregator/ssh_packet_layer.h
> create mode 100644 drivers/platform/surface/aggregator/ssh_parser.c
> create mode 100644 drivers/platform/surface/aggregator/ssh_parser.h
> create mode 100644 drivers/platform/surface/aggregator/ssh_request_layer.c
> create mode 100644 drivers/platform/surface/aggregator/ssh_request_layer.h
> create mode 100644 drivers/platform/surface/aggregator/trace.h
> create mode 100644 drivers/platform/surface/surface_acpi_notify.c
> create mode 100644 drivers/platform/surface/surface_aggregator_cdev.c
> create mode 100644 include/linux/surface_acpi_notify.h
> create mode 100644 include/linux/surface_aggregator/controller.h
> create mode 100644 include/linux/surface_aggregator/device.h
> create mode 100644 include/linux/surface_aggregator/serial_hub.h
> create mode 100644 include/uapi/linux/surface_aggregator/cdev.h
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module
2020-11-24 11:59 ` [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module Hans de Goede
@ 2020-11-24 13:43 ` Maximilian Luz
0 siblings, 0 replies; 13+ messages in thread
From: Maximilian Luz @ 2020-11-24 13:43 UTC (permalink / raw)
To: Hans de Goede, linux-kernel
Cc: Mark Gross, Arnd Bergmann, Greg Kroah-Hartman, Rob Herring,
Jiri Slaby, Rafael J . Wysocki, Len Brown, Steven Rostedt,
Ingo Molnar, Masahiro Yamada, Michal Marek, Jonathan Corbet,
Blaž Hrastnik, Dorian Stoll, platform-driver-x86,
linux-serial, linux-acpi, linux-kbuild, linux-doc
On 11/24/20 12:59 PM, Hans de Goede wrote:
> Hi,
>
> On 11/15/20 8:21 PM, Maximilian Luz wrote:
>> Hello,
>>
>> N.B.: the following text is mostly a repeat of cover letter from the
>> previous RFC for the uninitiated, which can be found at
>>
>> https://lore.kernel.org/linux-serial/20200923151511.3842150-1-luzmaximilian@gmail.com/
>>
>> See "Changes" below for an overview of differences between the RFC and
>> this patchset. I hope I have addressed all comments from that in this
>> version, thank you again for those.
>>
>> The Surface System Aggregator Module (we'll refer to it as Surface
>> Aggregator or SAM below) is an embedded controller (EC) found on various
>> Microsoft Surface devices. Specifically, all 4th and later generation
>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
>> exception of the Surface Go series and the Surface Duo. Notably, it
>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
>
> <snip>
>
>> This patch-set can also be found at the following repository and
>> reference, if you prefer to look at a kernel tree instead of these
>> emails:
>>
>> https://github.com/linux-surface/kernel tags/s/surface-aggregator/v1
>>
>> Thanks,
>> Max
>
> Thank you for your work on this. It would be great if we can get better
> support for the Surface line in the mainline kernel.
>
> Since a lot of people have already commented on this series I think that
> you have enough feedback to do a v2 addressing that feedback right?
Yes, I'm already working on it.
> For now I'm going to assume that you will do a v2 addressing the
> initial round of comments and not review this myself (IOW I'll review
> this when v2 is posted).
Sure, no need for you to review v1 at this point.
> Let me know if you see things differently.
Thanks,
Max
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-11-24 13:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-15 19:21 [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module Maximilian Luz
2020-11-15 19:21 ` [PATCH 9/9] platform/surface: Add Surface ACPI Notify driver Maximilian Luz
2020-11-17 20:09 ` Randy Dunlap
2020-11-17 20:18 ` Maximilian Luz
[not found] ` <20201115192143.21571-2-luzmaximilian@gmail.com>
2020-11-16 13:33 ` [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem Andy Shevchenko
2020-11-16 17:03 ` Maximilian Luz
2020-11-17 6:05 ` Maximilian Luz
2020-11-18 0:28 ` Barnabás Pőcze
2020-11-18 23:06 ` Maximilian Luz
2020-11-19 15:54 ` Barnabás Pőcze
2020-11-19 17:35 ` Maximilian Luz
2020-11-24 11:59 ` [PATCH 0/9] Add support for Microsoft Surface System Aggregator Module Hans de Goede
2020-11-24 13:43 ` Maximilian Luz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).