[PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

Hello,

Here is version two of the Surface System Aggregator Module (SAM/SSAM)
driver series, adding initial support for the embedded controller on 5th
and later generation Microsoft Surface devices. Initial support includes
the ACPI interface to the controller, via which battery and thermal
information is provided on some of these devices.

The previous version and cover letter detailing what this series is
about can be found at

  https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/

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/v2

Thank you all for the feedback to v1, I hope I have addressed all
comments.

Regards,
Max


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 in v1 (from 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 in v2 (overview):
 - simplify some code, mostly with regards to concurrency
 - add architectural overview to documentation
 - improve comments for documentation
 - use printk specifier for hex prefix instead of hard-coding it
 - spell check comments and strings, fix typos
 - unify comment style
 - run checkpatch --strict, fix these and other style 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  |  393 +++
 .../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           |  577 ++++
 .../surface_aggregator/overview.rst           |   77 +
 .../driver-api/surface_aggregator/ssh.rst     |  344 +++
 .../userspace-api/ioctl/ioctl-number.rst      |    2 +
 MAINTAINERS                                   |   13 +
 drivers/platform/surface/Kconfig              |   38 +
 drivers/platform/surface/Makefile             |    3 +
 drivers/platform/surface/aggregator/Kconfig   |   69 +
 drivers/platform/surface/aggregator/Makefile  |   17 +
 drivers/platform/surface/aggregator/bus.c     |  415 +++
 drivers/platform/surface/aggregator/bus.h     |   27 +
 .../platform/surface/aggregator/controller.c  | 2543 +++++++++++++++++
 .../platform/surface/aggregator/controller.h  |  285 ++
 drivers/platform/surface/aggregator/core.c    |  833 ++++++
 .../platform/surface/aggregator/ssh_msgb.h    |  205 ++
 .../surface/aggregator/ssh_packet_layer.c     | 2046 +++++++++++++
 .../surface/aggregator/ssh_packet_layer.h     |  190 ++
 .../platform/surface/aggregator/ssh_parser.c  |  228 ++
 .../platform/surface/aggregator/ssh_parser.h  |  154 +
 .../surface/aggregator/ssh_request_layer.c    | 1256 ++++++++
 .../surface/aggregator/ssh_request_layer.h    |  143 +
 drivers/platform/surface/aggregator/trace.h   |  632 ++++
 .../platform/surface/surface_acpi_notify.c    |  886 ++++++
 .../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 |  824 ++++++
 include/linux/surface_aggregator/device.h     |  423 +++
 include/linux/surface_aggregator/serial_hub.h |  672 +++++
 include/uapi/linux/surface_aggregator/cdev.h  |   58 +
 scripts/mod/devicetable-offsets.c             |    8 +
 scripts/mod/file2alias.c                      |   23 +
 40 files changed, 14017 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

[PATCH v2 6/9] platform/surface: aggregator: Add dedicated bus and device type

[Maximilian Luz]

The Surface Aggregator EC provides varying functionality, depending on
the Surface device. To manage this functionality, we use dedicated
client devices for each subsystem or virtual device of the EC. While
some of these clients are described as standard devices in ACPI and the
corresponding client drivers can be implemented as platform drivers in
the kernel (making use of the controller API already present), many
devices, especially on newer Surface models, cannot be found there.

To simplify management of these devices, we introduce a new bus and
client device type for the Surface Aggregator subsystem. The new device
type takes care of managing the controller reference, essentially
guaranteeing its validity for as long as the client device exists, thus
alleviating the need to manually establish device links for that purpose
in the client driver (as has to be done with the platform devices).

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---

Changes in v1 (from RFC):
 - add copyright lines
 - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
 - remove unnecessary READ_ONCE on multiple occasions
 - improve documentation of special values for SSAM_DEVICE()
 - add NULL checks for ssam_device_get, ssam_device_put

Changes in v2:
 - return ENODEV instead of ENXIO if controller is not present
 - use sysfs_emit for sysfs attributes
 - spell check comments and strings, fix typos
 - unify comment style
 - run checkpatch --strict, fix warnings and style issues

---
 drivers/platform/surface/aggregator/Kconfig  |  12 +
 drivers/platform/surface/aggregator/Makefile |   4 +
 drivers/platform/surface/aggregator/bus.c    | 415 ++++++++++++++++++
 drivers/platform/surface/aggregator/bus.h    |  27 ++
 drivers/platform/surface/aggregator/core.c   |  12 +
 include/linux/mod_devicetable.h              |  18 +
 include/linux/surface_aggregator/device.h    | 423 +++++++++++++++++++
 scripts/mod/devicetable-offsets.c            |   8 +
 scripts/mod/file2alias.c                     |  23 +
 9 files changed, 942 insertions(+)
 create mode 100644 drivers/platform/surface/aggregator/bus.c
 create mode 100644 drivers/platform/surface/aggregator/bus.h
 create mode 100644 include/linux/surface_aggregator/device.h

diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
index 48f40c345e29..44c2493706bc 100644
--- a/drivers/platform/surface/aggregator/Kconfig
+++ b/drivers/platform/surface/aggregator/Kconfig
@@ -42,6 +42,18 @@ menuconfig SURFACE_AGGREGATOR
 	  module, y if you want to build it into the kernel and n if you don't
 	  want it at all.

+config SURFACE_AGGREGATOR_BUS
+	bool "Surface System Aggregator Module Bus"
+	depends on SURFACE_AGGREGATOR
+	default y
+	help
+	  Expands the Surface System Aggregator Module (SSAM) core driver by
+	  providing a dedicated bus and client-device type.
+
+	  This bus and device type are intended to provide and simplify support
+	  for non-platform and non-ACPI SSAM devices, i.e. SSAM devices that are
+	  not auto-detectable via the conventional means (e.g. ACPI).
+
 config SURFACE_AGGREGATOR_ERROR_INJECTION
 	bool "Surface System Aggregator Module Error Injection Capabilities"
 	depends on SURFACE_AGGREGATOR
diff --git a/drivers/platform/surface/aggregator/Makefile b/drivers/platform/surface/aggregator/Makefile
index b8b24c8ec310..c112e2c7112b 100644
--- a/drivers/platform/surface/aggregator/Makefile
+++ b/drivers/platform/surface/aggregator/Makefile
@@ -11,3 +11,7 @@ surface_aggregator-objs += ssh_parser.o
 surface_aggregator-objs += ssh_packet_layer.o
 surface_aggregator-objs += ssh_request_layer.o
 surface_aggregator-objs += controller.o
+
+ifeq ($(CONFIG_SURFACE_AGGREGATOR_BUS),y)
+surface_aggregator-objs += bus.o
+endif
diff --git a/drivers/platform/surface/aggregator/bus.c b/drivers/platform/surface/aggregator/bus.c
new file mode 100644
index 000000000000..9f7b4f91a87e
--- /dev/null
+++ b/drivers/platform/surface/aggregator/bus.c
@@ -0,0 +1,415 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Surface System Aggregator Module bus and device integration.
+ *
+ * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
+ */
+
+#include <linux/device.h>
+#include <linux/slab.h>
+
+#include <linux/surface_aggregator/controller.h>
+#include <linux/surface_aggregator/device.h>
+
+#include "bus.h"
+#include "controller.h"
+
+static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct ssam_device *sdev = to_ssam_device(dev);
+
+	return sysfs_emit(buf, "ssam:d%02Xc%02Xt%02Xi%02Xf%02X\n",
+			sdev->uid.domain, sdev->uid.category, sdev->uid.target,
+			sdev->uid.instance, sdev->uid.function);
+}
+static DEVICE_ATTR_RO(modalias);
+
+static struct attribute *ssam_device_attrs[] = {
+	&dev_attr_modalias.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(ssam_device);
+
+static int ssam_device_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct ssam_device *sdev = to_ssam_device(dev);
+
+	return add_uevent_var(env, "MODALIAS=ssam:d%02Xc%02Xt%02Xi%02Xf%02X",
+			      sdev->uid.domain, sdev->uid.category,
+			      sdev->uid.target, sdev->uid.instance,
+			      sdev->uid.function);
+}
+
+static void ssam_device_release(struct device *dev)
+{
+	struct ssam_device *sdev = to_ssam_device(dev);
+
+	ssam_controller_put(sdev->ctrl);
+	kfree(sdev);
+}
+
+const struct device_type ssam_device_type = {
+	.name    = "surface_aggregator_device",
+	.groups  = ssam_device_groups,
+	.uevent  = ssam_device_uevent,
+	.release = ssam_device_release,
+};
+EXPORT_SYMBOL_GPL(ssam_device_type);
+
+/**
+ * ssam_device_alloc() - Allocate and initialize a SSAM client device.
+ * @ctrl: The controller under which the device should be added.
+ * @uid:  The UID of the device to be added.
+ *
+ * Allocates and initializes a new client device. The parent of the device
+ * will be set to the controller device and the name will be set based on the
+ * UID. Note that the device still has to be added via ssam_device_add().
+ * Refer to that function for more details.
+ *
+ * Return: Returns the newly allocated and initialized SSAM client device, or
+ * %NULL if it could not be allocated.
+ */
+struct ssam_device *ssam_device_alloc(struct ssam_controller *ctrl,
+				      struct ssam_device_uid uid)
+{
+	struct ssam_device *sdev;
+
+	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
+	if (!sdev)
+		return NULL;
+
+	device_initialize(&sdev->dev);
+	sdev->dev.bus = &ssam_bus_type;
+	sdev->dev.type = &ssam_device_type;
+	sdev->dev.parent = ssam_controller_device(ctrl);
+	sdev->ctrl = ssam_controller_get(ctrl);
+	sdev->uid = uid;
+
+	dev_set_name(&sdev->dev, "%02x:%02x:%02x:%02x:%02x",
+		     sdev->uid.domain, sdev->uid.category, sdev->uid.target,
+		     sdev->uid.instance, sdev->uid.function);
+
+	return sdev;
+}
+EXPORT_SYMBOL_GPL(ssam_device_alloc);
+
+/**
+ * ssam_device_add() - Add a SSAM client device.
+ * @sdev: The SSAM client device to be added.
+ *
+ * Added client devices must be guaranteed to always have a valid and active
+ * controller. Thus, this function will fail with %-ENODEV if the controller
+ * of the device has not been initialized yet, has been suspended, or has been
+ * shut down.
+ *
+ * The caller of this function should ensure that the corresponding call to
+ * ssam_device_remove() is issued before the controller is shut down. If the
+ * added device is a direct child of the controller device (default), it will
+ * be automatically removed when the controller is shut down.
+ *
+ * By default, the controller device will become the parent of the newly
+ * created client device. The parent may be changed before ssam_device_add is
+ * called, but care must be taken that a) the correct suspend/resume ordering
+ * is guaranteed and b) the client device does not outlive the controller,
+ * i.e. that the device is removed before the controller is being shut down.
+ * In case these guarantees have to be manually enforced, please refer to the
+ * ssam_client_link() and ssam_client_bind() functions, which are intended to
+ * set up device-links for this purpose.
+ *
+ * Return: Returns zero on success, a negative error code on failure.
+ */
+int ssam_device_add(struct ssam_device *sdev)
+{
+	int status;
+
+	/*
+	 * Ensure that we can only add new devices to a controller if it has
+	 * been started and is not going away soon. This works in combination
+	 * with ssam_controller_remove_clients to ensure driver presence for the
+	 * controller device, i.e. it ensures that the controller (sdev->ctrl)
+	 * is always valid and can be used for requests as long as the client
+	 * device we add here is registered as child under it. This essentially
+	 * guarantees that the client driver can always expect the preconditions
+	 * for functions like ssam_request_sync (controller has to be started
+	 * and is not suspended) to hold and thus does not have to check for
+	 * them.
+	 *
+	 * Note that for this to work, the controller has to be a parent device.
+	 * If it is not a direct parent, care has to be taken that the device is
+	 * removed via ssam_device_remove(), as device_unregister does not
+	 * remove child devices recursively.
+	 */
+	ssam_controller_statelock(sdev->ctrl);
+
+	if (sdev->ctrl->state != SSAM_CONTROLLER_STARTED) {
+		ssam_controller_stateunlock(sdev->ctrl);
+		return -ENODEV;
+	}
+
+	status = device_add(&sdev->dev);
+
+	ssam_controller_stateunlock(sdev->ctrl);
+	return status;
+}
+EXPORT_SYMBOL_GPL(ssam_device_add);
+
+/**
+ * ssam_device_remove() - Remove a SSAM client device.
+ * @sdev: The device to remove.
+ *
+ * Removes and unregisters the provided SSAM client device.
+ */
+void ssam_device_remove(struct ssam_device *sdev)
+{
+	device_unregister(&sdev->dev);
+}
+EXPORT_SYMBOL_GPL(ssam_device_remove);
+
+/**
+ * ssam_device_id_compatible() - Check if a device ID matches a UID.
+ * @id:  The device ID as potential match.
+ * @uid: The device UID matching against.
+ *
+ * Check if the given ID is a match for the given UID, i.e. if a device with
+ * the provided UID is compatible to the given ID following the match rules
+ * described in its &ssam_device_id.match_flags member.
+ *
+ * Return: Returns %true iff the given UID is compatible to the match rule
+ * described by the given ID, %false otherwise.
+ */
+static bool ssam_device_id_compatible(const struct ssam_device_id *id,
+				      struct ssam_device_uid uid)
+{
+	if (id->domain != uid.domain || id->category != uid.category)
+		return false;
+
+	if ((id->match_flags & SSAM_MATCH_TARGET) && id->target != uid.target)
+		return false;
+
+	if ((id->match_flags & SSAM_MATCH_INSTANCE) && id->instance != uid.instance)
+		return false;
+
+	if ((id->match_flags & SSAM_MATCH_FUNCTION) && id->function != uid.function)
+		return false;
+
+	return true;
+}
+
+/**
+ * ssam_device_id_is_null() - Check if a device ID is null.
+ * @id: The device ID to check.
+ *
+ * Check if a given device ID is null, i.e. all zeros. Used to check for the
+ * end of ``MODULE_DEVICE_TABLE(ssam, ...)`` or similar lists.
+ *
+ * Return: Returns %true if the given ID represents a null ID, %false
+ * otherwise.
+ */
+static bool ssam_device_id_is_null(const struct ssam_device_id *id)
+{
+	return id->match_flags == 0 &&
+		id->domain == 0 &&
+		id->category == 0 &&
+		id->target == 0 &&
+		id->instance == 0 &&
+		id->function == 0 &&
+		id->driver_data == 0;
+}
+
+/**
+ * ssam_device_id_match() - Find the matching ID table entry for the given UID.
+ * @table: The table to search in.
+ * @uid:   The UID to matched against the individual table entries.
+ *
+ * Find the first match for the provided device UID in the provided ID table
+ * and return it. Returns %NULL if no match could be found.
+ */
+const struct ssam_device_id *ssam_device_id_match(const struct ssam_device_id *table,
+						  const struct ssam_device_uid uid)
+{
+	const struct ssam_device_id *id;
+
+	for (id = table; !ssam_device_id_is_null(id); ++id)
+		if (ssam_device_id_compatible(id, uid))
+			return id;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(ssam_device_id_match);
+
+/**
+ * ssam_device_get_match() - Find and return the ID matching the device in the
+ * ID table of the bound driver.
+ * @dev: The device for which to get the matching ID table entry.
+ *
+ * Find the fist match for the UID of the device in the ID table of the
+ * currently bound driver and return it. Returns %NULL if the device does not
+ * have a driver bound to it, the driver does not have match_table (i.e. it is
+ * %NULL), or there is no match in the driver's match_table.
+ *
+ * This function essentially calls ssam_device_id_match() with the ID table of
+ * the bound device driver and the UID of the device.
+ *
+ * Return: Returns the first match for the UID of the device in the device
+ * driver's match table, or %NULL if no such match could be found.
+ */
+const struct ssam_device_id *ssam_device_get_match(const struct ssam_device *dev)
+{
+	const struct ssam_device_driver *sdrv;
+
+	sdrv = to_ssam_device_driver(dev->dev.driver);
+	if (!sdrv)
+		return NULL;
+
+	if (!sdrv->match_table)
+		return NULL;
+
+	return ssam_device_id_match(sdrv->match_table, dev->uid);
+}
+EXPORT_SYMBOL_GPL(ssam_device_get_match);
+
+/**
+ * ssam_device_get_match_data() - Find the ID matching the device in the
+ * ID table of the bound driver and return its ``driver_data`` member.
+ * @dev: The device for which to get the match data.
+ *
+ * Find the fist match for the UID of the device in the ID table of the
+ * corresponding driver and return its driver_data. Returns %NULL if the
+ * device does not have a driver bound to it, the driver does not have
+ * match_table (i.e. it is %NULL), there is no match in the driver's
+ * match_table, or the match does not have any driver_data.
+ *
+ * This function essentially calls ssam_device_get_match() and, if any match
+ * could be found, returns its ``struct ssam_device_id.driver_data`` member.
+ *
+ * Return: Returns the driver data associated with the first match for the UID
+ * of the device in the device driver's match table, or %NULL if no such match
+ * could be found.
+ */
+const void *ssam_device_get_match_data(const struct ssam_device *dev)
+{
+	const struct ssam_device_id *id;
+
+	id = ssam_device_get_match(dev);
+	if (!id)
+		return NULL;
+
+	return (const void *)id->driver_data;
+}
+EXPORT_SYMBOL_GPL(ssam_device_get_match_data);
+
+static int ssam_bus_match(struct device *dev, struct device_driver *drv)
+{
+	struct ssam_device_driver *sdrv = to_ssam_device_driver(drv);
+	struct ssam_device *sdev = to_ssam_device(dev);
+
+	if (!is_ssam_device(dev))
+		return 0;
+
+	return !!ssam_device_id_match(sdrv->match_table, sdev->uid);
+}
+
+static int ssam_bus_probe(struct device *dev)
+{
+	return to_ssam_device_driver(dev->driver)
+		->probe(to_ssam_device(dev));
+}
+
+static int ssam_bus_remove(struct device *dev)
+{
+	struct ssam_device_driver *sdrv = to_ssam_device_driver(dev->driver);
+
+	if (sdrv->remove)
+		sdrv->remove(to_ssam_device(dev));
+
+	return 0;
+}
+
+struct bus_type ssam_bus_type = {
+	.name   = "surface_aggregator",
+	.match  = ssam_bus_match,
+	.probe  = ssam_bus_probe,
+	.remove = ssam_bus_remove,
+};
+EXPORT_SYMBOL_GPL(ssam_bus_type);
+
+/**
+ * __ssam_device_driver_register() - Register a SSAM client device driver.
+ * @sdrv:  The driver to register.
+ * @owner: The module owning the provided driver.
+ *
+ * Please refer to the ssam_device_driver_register() macro for the normal way
+ * to register a driver from inside its owning module.
+ */
+int __ssam_device_driver_register(struct ssam_device_driver *sdrv,
+				  struct module *owner)
+{
+	sdrv->driver.owner = owner;
+	sdrv->driver.bus = &ssam_bus_type;
+
+	/* force drivers to async probe so I/O is possible in probe */
+	sdrv->driver.probe_type = PROBE_PREFER_ASYNCHRONOUS;
+
+	return driver_register(&sdrv->driver);
+}
+EXPORT_SYMBOL_GPL(__ssam_device_driver_register);
+
+/**
+ * ssam_device_driver_unregister - Unregister a SSAM device driver.
+ * @sdrv: The driver to unregister.
+ */
+void ssam_device_driver_unregister(struct ssam_device_driver *sdrv)
+{
+	driver_unregister(&sdrv->driver);
+}
+EXPORT_SYMBOL_GPL(ssam_device_driver_unregister);
+
+static int ssam_remove_device(struct device *dev, void *_data)
+{
+	struct ssam_device *sdev = to_ssam_device(dev);
+
+	if (is_ssam_device(dev))
+		ssam_device_remove(sdev);
+
+	return 0;
+}
+
+/**
+ * ssam_controller_remove_clients() - Remove SSAM client devices registered as
+ * direct children under the given controller.
+ * @ctrl: The controller to remove all direct clients for.
+ *
+ * Remove all SSAM client devices registered as direct children under the
+ * given controller. Note that this only accounts for direct children of the
+ * controller device. This does not take care of any client devices where the
+ * parent device has been manually set before calling ssam_device_add. Refer
+ * to ssam_device_add()/ssam_device_remove() for more details on those cases.
+ *
+ * To avoid new devices being added in parallel to this call, the main
+ * controller lock (not statelock) must be held during this (and if
+ * necessary, any subsequent deinitialization) call.
+ */
+void ssam_controller_remove_clients(struct ssam_controller *ctrl)
+{
+	struct device *dev;
+
+	dev = ssam_controller_device(ctrl);
+	device_for_each_child_reverse(dev, NULL, ssam_remove_device);
+}
+
+/**
+ * ssam_bus_register() - Register and set-up the SSAM client device bus.
+ */
+int ssam_bus_register(void)
+{
+	return bus_register(&ssam_bus_type);
+}
+
+/**
+ * ssam_bus_unregister() - Unregister the SSAM client device bus.
+ */
+void ssam_bus_unregister(void)
+{
+	return bus_unregister(&ssam_bus_type);
+}
diff --git a/drivers/platform/surface/aggregator/bus.h b/drivers/platform/surface/aggregator/bus.h
new file mode 100644
index 000000000000..7712baaed6a5
--- /dev/null
+++ b/drivers/platform/surface/aggregator/bus.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Surface System Aggregator Module bus and device integration.
+ *
+ * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
+ */
+
+#ifndef _SURFACE_AGGREGATOR_BUS_H
+#define _SURFACE_AGGREGATOR_BUS_H
+
+#include <linux/surface_aggregator/controller.h>
+
+#ifdef CONFIG_SURFACE_AGGREGATOR_BUS
+
+void ssam_controller_remove_clients(struct ssam_controller *ctrl);
+
+int ssam_bus_register(void);
+void ssam_bus_unregister(void);
+
+#else /* CONFIG_SURFACE_AGGREGATOR_BUS */
+
+static inline void ssam_controller_remove_clients(struct ssam_controller *ctrl) {}
+static inline int ssam_bus_register(void) { return 0; }
+static inline void ssam_bus_unregister(void) {}
+
+#endif /* CONFIG_SURFACE_AGGREGATOR_BUS */
+#endif /* _SURFACE_AGGREGATOR_BUS_H */
diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
index b5d44ab61f06..cde66c497d3e 100644
--- a/drivers/platform/surface/aggregator/core.c
+++ b/drivers/platform/surface/aggregator/core.c
@@ -22,6 +22,8 @@
 #include <linux/sysfs.h>

 #include <linux/surface_aggregator/controller.h>
+
+#include "bus.h"
 #include "controller.h"

 #define CREATE_TRACE_POINTS
@@ -733,6 +735,9 @@ static void ssam_serial_hub_remove(struct serdev_device *serdev)
 	sysfs_remove_group(&serdev->dev.kobj, &ssam_sam_group);
 	ssam_controller_lock(ctrl);

+	/* Remove all client devices. */
+	ssam_controller_remove_clients(ctrl);
+
 	/* Act as if suspending to silence events. */
 	status = ssam_ctrl_notif_display_off(ctrl);
 	if (status) {
@@ -785,6 +790,10 @@ static int __init ssam_core_init(void)
 {
 	int status;

+	status = ssam_bus_register();
+	if (status)
+		goto err_bus;
+
 	status = ssh_ctrl_packet_cache_init();
 	if (status)
 		goto err_cpkg;
@@ -804,6 +813,8 @@ static int __init ssam_core_init(void)
 err_evitem:
 	ssh_ctrl_packet_cache_destroy();
 err_cpkg:
+	ssam_bus_unregister();
+err_bus:
 	return status;
 }
 module_init(ssam_core_init);
@@ -813,6 +824,7 @@ static void __exit ssam_core_exit(void)
 	serdev_device_driver_unregister(&ssam_serial_hub);
 	ssam_event_item_cache_destroy();
 	ssh_ctrl_packet_cache_destroy();
+	ssam_bus_unregister();
 }
 module_exit(ssam_core_exit);

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..0b8f1feefe0e 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -838,4 +838,22 @@ struct mhi_device_id {
 	kernel_ulong_t driver_data;
 };

+/* Surface System Aggregator Module */
+
+#define SSAM_MATCH_TARGET	0x1
+#define SSAM_MATCH_INSTANCE	0x2
+#define SSAM_MATCH_FUNCTION	0x4
+
+struct ssam_device_id {
+	__u8 match_flags;
+
+	__u8 domain;
+	__u8 category;
+	__u8 target;
+	__u8 instance;
+	__u8 function;
+
+	kernel_ulong_t driver_data;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
new file mode 100644
index 000000000000..7221d4a9c1c1
--- /dev/null
+++ b/include/linux/surface_aggregator/device.h
@@ -0,0 +1,423 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Surface System Aggregator Module (SSAM) bus and client-device subsystem.
+ *
+ * Main interface for the surface-aggregator bus, surface-aggregator client
+ * devices, and respective drivers building on top of the SSAM controller.
+ * Provides support for non-platform/non-ACPI SSAM clients via dedicated
+ * subsystem.
+ *
+ * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
+ */
+
+#ifndef _LINUX_SURFACE_AGGREGATOR_DEVICE_H
+#define _LINUX_SURFACE_AGGREGATOR_DEVICE_H
+
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+
+#include <linux/surface_aggregator/controller.h>
+
+
+/* -- Surface System Aggregator Module bus. --------------------------------- */
+
+/**
+ * enum ssam_device_domain - SAM device domain.
+ * @SSAM_DOMAIN_VIRTUAL:   Virtual device.
+ * @SSAM_DOMAIN_SERIALHUB: Physical device connected via Surface Serial Hub.
+ */
+enum ssam_device_domain {
+	SSAM_DOMAIN_VIRTUAL   = 0x00,
+	SSAM_DOMAIN_SERIALHUB = 0x01,
+};
+
+/**
+ * enum ssam_virtual_tc - Target categories for the virtual SAM domain.
+ * @SSAM_VIRTUAL_TC_HUB: Device hub category.
+ */
+enum ssam_virtual_tc {
+	SSAM_VIRTUAL_TC_HUB = 0x00,
+};
+
+/**
+ * struct ssam_device_uid - Unique identifier for SSAM device.
+ * @domain:   Domain of the device.
+ * @category: Target category of the device.
+ * @target:   Target ID of the device.
+ * @instance: Instance ID of the device.
+ * @function: Sub-function of the device. This field can be used to split a
+ *            single SAM device into multiple virtual subdevices to separate
+ *            different functionality of that device and allow one driver per
+ *            such functionality.
+ */
+struct ssam_device_uid {
+	u8 domain;
+	u8 category;
+	u8 target;
+	u8 instance;
+	u8 function;
+};
+
+/*
+ * Special values for device matching.
+ *
+ * These values are intended to be used with SSAM_DEVICE(), SSAM_VDEV(), and
+ * SSAM_SDEV() exclusively. Specifically, they are used to initialize the
+ * match_flags member of the device ID structure. Do not use them directly
+ * with struct ssam_device_id or struct ssam_device_uid.
+ */
+#define SSAM_ANY_TID		0xffff
+#define SSAM_ANY_IID		0xffff
+#define SSAM_ANY_FUN		0xffff
+
+/**
+ * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
+ * parameters.
+ * @d:   Domain of the device.
+ * @cat: Target category of the device.
+ * @tid: Target ID of the device.
+ * @iid: Instance ID of the device.
+ * @fun: Sub-function of the device.
+ *
+ * Initializes a &struct ssam_device_id with the given parameters. See &struct
+ * ssam_device_uid for details regarding the parameters. The special values
+ * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify that
+ * matching should ignore target ID, instance ID, and/or sub-function,
+ * respectively. This macro initializes the ``match_flags`` field based on the
+ * given parameters.
+ *
+ * Note: The parameters @d and @cat must be valid &u8 values, the parameters
+ * @tid, @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
+ * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
+ * allowed.
+ */
+#define SSAM_DEVICE(d, cat, tid, iid, fun)					\
+	.match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0)	\
+		     | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0)	\
+		     | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),	\
+	.domain   = d,								\
+	.category = cat,							\
+	.target   = ((tid) != SSAM_ANY_TID) ? (tid) : 0,			\
+	.instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0,			\
+	.function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0				\
+
+/**
+ * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with
+ * the given parameters.
+ * @cat: Target category of the device.
+ * @tid: Target ID of the device.
+ * @iid: Instance ID of the device.
+ * @fun: Sub-function of the device.
+ *
+ * Initializes a &struct ssam_device_id with the given parameters in the
+ * virtual domain. See &struct ssam_device_uid for details regarding the
+ * parameters. The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and
+ * %SSAM_ANY_FUN can be used to specify that matching should ignore target ID,
+ * instance ID, and/or sub-function, respectively. This macro initializes the
+ * ``match_flags`` field based on the given parameters.
+ *
+ * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
+ * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
+ * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
+ * allowed.
+ */
+#define SSAM_VDEV(cat, tid, iid, fun) \
+	SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun)
+
+/**
+ * SSAM_SDEV() - Initialize a &struct ssam_device_id as physical SSH device
+ * with the given parameters.
+ * @cat: Target category of the device.
+ * @tid: Target ID of the device.
+ * @iid: Instance ID of the device.
+ * @fun: Sub-function of the device.
+ *
+ * Initializes a &struct ssam_device_id with the given parameters in the SSH
+ * domain. See &struct ssam_device_uid for details regarding the parameters.
+ * The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be
+ * used to specify that matching should ignore target ID, instance ID, and/or
+ * sub-function, respectively. This macro initializes the ``match_flags``
+ * field based on the given parameters.
+ *
+ * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
+ * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
+ * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
+ * allowed.
+ */
+#define SSAM_SDEV(cat, tid, iid, fun) \
+	SSAM_DEVICE(SSAM_DOMAIN_SERIALHUB, SSAM_SSH_TC_##cat, tid, iid, fun)
+
+/**
+ * struct ssam_device - SSAM client device.
+ * @dev:  Driver model representation of the device.
+ * @ctrl: SSAM controller managing this device.
+ * @uid:  UID identifying the device.
+ */
+struct ssam_device {
+	struct device dev;
+	struct ssam_controller *ctrl;
+
+	struct ssam_device_uid uid;
+};
+
+/**
+ * struct ssam_device_driver - SSAM client device driver.
+ * @driver:      Base driver model structure.
+ * @match_table: Match table specifying which devices the driver should bind to.
+ * @probe:       Called when the driver is being bound to a device.
+ * @remove:      Called when the driver is being unbound from the device.
+ */
+struct ssam_device_driver {
+	struct device_driver driver;
+
+	const struct ssam_device_id *match_table;
+
+	int  (*probe)(struct ssam_device *sdev);
+	void (*remove)(struct ssam_device *sdev);
+};
+
+extern struct bus_type ssam_bus_type;
+extern const struct device_type ssam_device_type;
+
+/**
+ * is_ssam_device() - Check if the given device is a SSAM client device.
+ * @d: The device to test the type of.
+ *
+ * Return: Returns %true iff the specified device is of type &struct
+ * ssam_device, i.e. the device type points to %ssam_device_type, and %false
+ * otherwise.
+ */
+static inline bool is_ssam_device(struct device *d)
+{
+	return d->type == &ssam_device_type;
+}
+
+/**
+ * to_ssam_device() - Casts the given device to a SSAM client device.
+ * @d: The device to cast.
+ *
+ * Casts the given &struct device to a &struct ssam_device. The caller has to
+ * ensure that the given device is actually enclosed in a &struct ssam_device,
+ * e.g. by calling is_ssam_device().
+ *
+ * Return: Returns a pointer to the &struct ssam_device wrapping the given
+ * device @d.
+ */
+static inline struct ssam_device *to_ssam_device(struct device *d)
+{
+	return container_of(d, struct ssam_device, dev);
+}
+
+/**
+ * to_ssam_device_driver() - Casts the given device driver to a SSAM client
+ * device driver.
+ * @d: The driver to cast.
+ *
+ * Casts the given &struct device_driver to a &struct ssam_device_driver. The
+ * caller has to ensure that the given driver is actually enclosed in a
+ * &struct ssam_device_driver.
+ *
+ * Return: Returns the pointer to the &struct ssam_device_driver wrapping the
+ * given device driver @d.
+ */
+static inline
+struct ssam_device_driver *to_ssam_device_driver(struct device_driver *d)
+{
+	return container_of(d, struct ssam_device_driver, driver);
+}
+
+const struct ssam_device_id *ssam_device_id_match(const struct ssam_device_id *table,
+						  const struct ssam_device_uid uid);
+
+const struct ssam_device_id *ssam_device_get_match(const struct ssam_device *dev);
+
+const void *ssam_device_get_match_data(const struct ssam_device *dev);
+
+struct ssam_device *ssam_device_alloc(struct ssam_controller *ctrl,
+				      struct ssam_device_uid uid);
+
+int ssam_device_add(struct ssam_device *sdev);
+void ssam_device_remove(struct ssam_device *sdev);
+
+/**
+ * ssam_device_get() - Increment reference count of SSAM client device.
+ * @sdev: The device to increment the reference count of.
+ *
+ * Increments the reference count of the given SSAM client device by
+ * incrementing the reference count of the enclosed &struct device via
+ * get_device().
+ *
+ * See ssam_device_put() for the counter-part of this function.
+ *
+ * Return: Returns the device provided as input.
+ */
+static inline struct ssam_device *ssam_device_get(struct ssam_device *sdev)
+{
+	return sdev ? to_ssam_device(get_device(&sdev->dev)) : NULL;
+}
+
+/**
+ * ssam_device_put() - Decrement reference count of SSAM client device.
+ * @sdev: The device to decrement the reference count of.
+ *
+ * Decrements the reference count of the given SSAM client device by
+ * decrementing the reference count of the enclosed &struct device via
+ * put_device().
+ *
+ * See ssam_device_get() for the counter-part of this function.
+ */
+static inline void ssam_device_put(struct ssam_device *sdev)
+{
+	if (sdev)
+		put_device(&sdev->dev);
+}
+
+/**
+ * ssam_device_get_drvdata() - Get driver-data of SSAM client device.
+ * @sdev: The device to get the driver-data from.
+ *
+ * Return: Returns the driver-data of the given device, previously set via
+ * ssam_device_set_drvdata().
+ */
+static inline void *ssam_device_get_drvdata(struct ssam_device *sdev)
+{
+	return dev_get_drvdata(&sdev->dev);
+}
+
+/**
+ * ssam_device_set_drvdata() - Set driver-data of SSAM client device.
+ * @sdev: The device to set the driver-data of.
+ * @data: The data to set the device's driver-data pointer to.
+ */
+static inline void ssam_device_set_drvdata(struct ssam_device *sdev, void *data)
+{
+	dev_set_drvdata(&sdev->dev, data);
+}
+
+int __ssam_device_driver_register(struct ssam_device_driver *d, struct module *o);
+void ssam_device_driver_unregister(struct ssam_device_driver *d);
+
+/**
+ * ssam_device_driver_register() - Register a SSAM client device driver.
+ * @drv: The driver to register.
+ */
+#define ssam_device_driver_register(drv) \
+	__ssam_device_driver_register(drv, THIS_MODULE)
+
+/**
+ * module_ssam_device_driver() - Helper macro for SSAM device driver
+ * registration.
+ * @drv: The driver managed by this module.
+ *
+ * Helper macro to register a SSAM device driver via module_init() and
+ * module_exit(). This macro may only be used once per module and replaces the
+ * aforementioned definitions.
+ */
+#define module_ssam_device_driver(drv)			\
+	module_driver(drv, ssam_device_driver_register,	\
+		      ssam_device_driver_unregister)
+
+
+/* -- Helpers for client-device requests. ----------------------------------- */
+
+/**
+ * SSAM_DEFINE_SYNC_REQUEST_CL_N() - Define synchronous client-device SAM
+ * request function with neither argument nor return value.
+ * @name: Name of the generated function.
+ * @spec: Specification (&struct ssam_request_spec_md) defining the request.
+ *
+ * Defines a function executing the synchronous SAM request specified by
+ * @spec, with the request having neither argument nor return value. Device
+ * specifying parameters are not hard-coded, but instead are provided via the
+ * client device, specifically its UID, supplied when calling this function.
+ * The generated function takes care of setting up the request struct, buffer
+ * allocation, as well as execution of the request itself, returning once the
+ * request has been fully completed. The required transport buffer will be
+ * allocated on the stack.
+ *
+ * The generated function is defined as ``int name(struct ssam_device *sdev)``,
+ * returning the status of the request, which is zero on success and negative
+ * on failure. The ``sdev`` parameter specifies both the target device of the
+ * request and by association the controller via which the request is sent.
+ *
+ * Refer to ssam_request_sync_onstack() for more details on the behavior of
+ * the generated function.
+ */
+#define SSAM_DEFINE_SYNC_REQUEST_CL_N(name, spec...)			\
+	SSAM_DEFINE_SYNC_REQUEST_MD_N(__raw_##name, spec)		\
+	int name(struct ssam_device *sdev)				\
+	{								\
+		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
+				    sdev->uid.instance);		\
+	}
+
+/**
+ * SSAM_DEFINE_SYNC_REQUEST_CL_W() - Define synchronous client-device SAM
+ * request function with argument.
+ * @name:  Name of the generated function.
+ * @atype: Type of the request's argument.
+ * @spec:  Specification (&struct ssam_request_spec_md) defining the request.
+ *
+ * Defines a function executing the synchronous SAM request specified by
+ * @spec, with the request taking an argument of type @atype and having no
+ * return value. Device specifying parameters are not hard-coded, but instead
+ * are provided via the client device, specifically its UID, supplied when
+ * calling this function. The generated function takes care of setting up the
+ * request struct, buffer allocation, as well as execution of the request
+ * itself, returning once the request has been fully completed. The required
+ * transport buffer will be allocated on the stack.
+ *
+ * The generated function is defined as ``int name(struct ssam_device *sdev,
+ * const atype *arg)``, returning the status of the request, which is zero on
+ * success and negative on failure. The ``sdev`` parameter specifies both the
+ * target device of the request and by association the controller via which
+ * the request is sent. The request's argument is specified via the ``arg``
+ * pointer.
+ *
+ * Refer to ssam_request_sync_onstack() for more details on the behavior of
+ * the generated function.
+ */
+#define SSAM_DEFINE_SYNC_REQUEST_CL_W(name, atype, spec...)		\
+	SSAM_DEFINE_SYNC_REQUEST_MD_W(__raw_##name, atype, spec)	\
+	int name(struct ssam_device *sdev, const atype *arg)		\
+	{								\
+		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
+				    sdev->uid.instance, arg);		\
+	}
+
+/**
+ * SSAM_DEFINE_SYNC_REQUEST_CL_R() - Define synchronous client-device SAM
+ * request function with return value.
+ * @name:  Name of the generated function.
+ * @rtype: Type of the request's return value.
+ * @spec:  Specification (&struct ssam_request_spec_md) defining the request.
+ *
+ * Defines a function executing the synchronous SAM request specified by
+ * @spec, with the request taking no argument but having a return value of
+ * type @rtype. Device specifying parameters are not hard-coded, but instead
+ * are provided via the client device, specifically its UID, supplied when
+ * calling this function. The generated function takes care of setting up the
+ * request struct, buffer allocation, as well as execution of the request
+ * itself, returning once the request has been fully completed. The required
+ * transport buffer will be allocated on the stack.
+ *
+ * The generated function is defined as ``int name(struct ssam_device *sdev,
+ * rtype *ret)``, returning the status of the request, which is zero on
+ * success and negative on failure. The ``sdev`` parameter specifies both the
+ * target device of the request and by association the controller via which
+ * the request is sent. The request's return value is written to the memory
+ * pointed to by the ``ret`` parameter.
+ *
+ * Refer to ssam_request_sync_onstack() for more details on the behavior of
+ * the generated function.
+ */
+#define SSAM_DEFINE_SYNC_REQUEST_CL_R(name, rtype, spec...)		\
+	SSAM_DEFINE_SYNC_REQUEST_MD_R(__raw_##name, rtype, spec)	\
+	int name(struct ssam_device *sdev, rtype *ret)			\
+	{								\
+		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
+				    sdev->uid.instance, ret);		\
+	}
+
+#endif /* _LINUX_SURFACE_AGGREGATOR_DEVICE_H */
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 27007c18e754..4339377ad929 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -243,5 +243,13 @@ int main(void)
 	DEVID(mhi_device_id);
 	DEVID_FIELD(mhi_device_id, chan);

+	DEVID(ssam_device_id);
+	DEVID_FIELD(ssam_device_id, match_flags);
+	DEVID_FIELD(ssam_device_id, domain);
+	DEVID_FIELD(ssam_device_id, category);
+	DEVID_FIELD(ssam_device_id, target);
+	DEVID_FIELD(ssam_device_id, instance);
+	DEVID_FIELD(ssam_device_id, function);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1dee33..5b79fdc42641 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1368,6 +1368,28 @@ static int do_mhi_entry(const char *filename, void *symval, char *alias)
 	return 1;
 }

+/*
+ * Looks like: ssam:dNcNtNiNfN
+ *
+ * N is exactly 2 digits, where each is an upper-case hex digit.
+ */
+static int do_ssam_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD(symval, ssam_device_id, match_flags);
+	DEF_FIELD(symval, ssam_device_id, domain);
+	DEF_FIELD(symval, ssam_device_id, category);
+	DEF_FIELD(symval, ssam_device_id, target);
+	DEF_FIELD(symval, ssam_device_id, instance);
+	DEF_FIELD(symval, ssam_device_id, function);
+
+	sprintf(alias, "ssam:d%02Xc%02X", domain, category);
+	ADD(alias, "t", match_flags & SSAM_MATCH_TARGET, target);
+	ADD(alias, "i", match_flags & SSAM_MATCH_INSTANCE, instance);
+	ADD(alias, "f", match_flags & SSAM_MATCH_FUNCTION, function);
+
+	return 1;
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1442,6 +1464,7 @@ static const struct devtable devtable[] = {
 	{"tee", SIZE_tee_client_device_id, do_tee_entry},
 	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
 	{"mhi", SIZE_mhi_device_id, do_mhi_entry},
+	{"ssam", SIZE_ssam_device_id, do_ssam_entry},
 };

 /* Create MODULE_ALIAS() statements.
--
2.29.2

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
> Hello,
>
> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
> driver series, adding initial support for the embedded controller on 5th
> and later generation Microsoft Surface devices. Initial support includes
> the ACPI interface to the controller, via which battery and thermal
> information is provided on some of these devices.
>
> The previous version and cover letter detailing what this series is
> about can be found at
>
>   https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>
> 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/v2
>
> Thank you all for the feedback to v1, I hope I have addressed all
> comments.


I think that it is too far fetched to attempt and expose UAPI headers
for some obscure char device that we are all know won't be around in
a couple of years from now due to the nature of how this embedded world
works.

More on that, the whole purpose of proposed interface is to debug and
not intended to be used by any user space code.

Also the idea that you are creating new bus just for this device doesn't
really sound right. I recommend you to take a look on auxiliary bus and
use it or come with very strong justifications why it is not fit yet.

I'm sorry to say, but this series is not ready to be merged yet.

NAK: Leon Romanovsky <leon@kernel.org>

Thanks

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Greg Kroah-Hartman]

On Sun, Dec 06, 2020 at 09:07:05AM +0200, Leon Romanovsky wrote:
> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
> > Hello,
> >
> > Here is version two of the Surface System Aggregator Module (SAM/SSAM)
> > driver series, adding initial support for the embedded controller on 5th
> > and later generation Microsoft Surface devices. Initial support includes
> > the ACPI interface to the controller, via which battery and thermal
> > information is provided on some of these devices.
> >
> > The previous version and cover letter detailing what this series is
> > about can be found at
> >
> >   https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
> >
> > 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/v2
> >
> > Thank you all for the feedback to v1, I hope I have addressed all
> > comments.
> 
> 
> I think that it is too far fetched to attempt and expose UAPI headers
> for some obscure char device that we are all know won't be around in
> a couple of years from now due to the nature of how this embedded world
> works.

No, that's not ok, we do this for loads of devices out there.  If there
is a device that wants to be supported for Linux, and a developer that
wants to support it, we will take it.

> More on that, the whole purpose of proposed interface is to debug and
> not intended to be used by any user space code.

I thought that debugfs was going to be used for most of the debugging
code, or has that changed in newer versions of this patchset?

thanks,

greg k-h

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Sun, Dec 06, 2020 at 09:32:30AM +0100, Greg Kroah-Hartman wrote:
> On Sun, Dec 06, 2020 at 09:07:05AM +0200, Leon Romanovsky wrote:
> > On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
> > > Hello,
> > >
> > > Here is version two of the Surface System Aggregator Module (SAM/SSAM)
> > > driver series, adding initial support for the embedded controller on 5th
> > > and later generation Microsoft Surface devices. Initial support includes
> > > the ACPI interface to the controller, via which battery and thermal
> > > information is provided on some of these devices.
> > >
> > > The previous version and cover letter detailing what this series is
> > > about can be found at
> > >
> > >   https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
> > >
> > > 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/v2
> > >
> > > Thank you all for the feedback to v1, I hope I have addressed all
> > > comments.
> >
> >
> > I think that it is too far fetched to attempt and expose UAPI headers
> > for some obscure char device that we are all know won't be around in
> > a couple of years from now due to the nature of how this embedded world
> > works.
>
> No, that's not ok, we do this for loads of devices out there.  If there
> is a device that wants to be supported for Linux, and a developer that
> wants to support it, we will take it.
>
> > More on that, the whole purpose of proposed interface is to debug and
> > not intended to be used by any user space code.
>
> I thought that debugfs was going to be used for most of the debugging
> code, or has that changed in newer versions of this patchset?

https://lore.kernel.org/lkml/20201203212640.663931-9-luzmaximilian@gmail.com/#Z30include:uapi:linux:surface_aggregator:cdev.h

+ * Definitions, structs, and IOCTLs for the /dev/surface/aggregator misc
+ * device. This device provides direct user-space access to the SSAM EC.
+ * Intended for debugging and development.

>
> thanks,
>
> greg k-h

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Hans de Goede]

Hi Leon,

On 12/6/20 8:07 AM, Leon Romanovsky wrote:
> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
>> Hello,
>>
>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
>> driver series, adding initial support for the embedded controller on 5th
>> and later generation Microsoft Surface devices. Initial support includes
>> the ACPI interface to the controller, via which battery and thermal
>> information is provided on some of these devices.
>>
>> The previous version and cover letter detailing what this series is
>> about can be found at
>>
>>   https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>>
>> 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/v2
>>
>> Thank you all for the feedback to v1, I hope I have addressed all
>> comments.
> 
> 
> I think that it is too far fetched to attempt and expose UAPI headers
> for some obscure char device that we are all know won't be around in
> a couple of years from now due to the nature of how this embedded world
> works.

This is not for an embedded device, but for the popular line of
Microsoft Surface laptops / 2-in-1s...

> More on that, the whole purpose of proposed interface is to debug and
> not intended to be used by any user space code.

The purpose is to provide raw access to the Surface Serial Hub protocol,
just like we provide raw access to USB devices and have hidraw devices.

So this goes a litle beyond just debugging; and eventually the choice
may be made to implement some functionality with userspace drivers,
just like we do for some HID and USB devices.

Still I agree with you that adding new userspace API is something which
needs to be considered carefully. So I will look at this closely when
reviewing this set.

> Also the idea that you are creating new bus just for this device doesn't
> really sound right. I recommend you to take a look on auxiliary bus and
> use it or come with very strong justifications why it is not fit yet.

AFAIK the auxiliary bus is for sharing a single device between multiple
drivers, while the main device-driver also still offers functionality
(beyond the providing of access) itself.

This is more akin to how the WMI driver also models different WMI
functions as a bus + devices on the bus.

Or how the SDIO driver multiplex a single SDIO device into its
functions by again using a bus + devices on the bus model.

Also this has been in the works for quite a while now, the Linux on
Microsoft Surface devices community has been working on this out of
tree for a long time, see:
https://github.com/linux-surface/

And an RFC and a v1 have been posted a while ago, while auxiliary
bus support is not even in the mainline kernel yet. I would agree
with you that this should switch to auxiliary bus, despite the timing,
if that would lead to much better code. But ATM I don't really see
switching to auxiliary bus offering much benefits here.

> I'm sorry to say, but this series is not ready to be merged yet.
> 
> NAK: Leon Romanovsky <leon@kernel.org>

See above, I believe that this all is a bit harsh and I have not
really heard convincing arguments for not merging this.

Moreover such a quick nack does not really promote working upstream,
where as we actually want people to work upstream as much as possible.
I know this is not a reason for taking bad code, but I'm not
convinced that this is bad code.

I have not reviewed this myself yet, but once I have reviewed
this and any review remarks have been addressed I do expect to
merge this series through the platform-drivers-x86 tree.

Regards,

Hans de Goede
(drivers/platform/x86 and drivers/platform/surface subsys maintainer)

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Sun, Dec 06, 2020 at 09:41:21AM +0100, Hans de Goede wrote:
> Hi Leon,
>
> On 12/6/20 8:07 AM, Leon Romanovsky wrote:
> > On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
> >> Hello,
> >>
> >> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
> >> driver series, adding initial support for the embedded controller on 5th
> >> and later generation Microsoft Surface devices. Initial support includes
> >> the ACPI interface to the controller, via which battery and thermal
> >> information is provided on some of these devices.
> >>
> >> The previous version and cover letter detailing what this series is
> >> about can be found at
> >>
> >>   https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
> >>
> >> 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/v2
> >>
> >> Thank you all for the feedback to v1, I hope I have addressed all
> >> comments.
> >
> >
> > I think that it is too far fetched to attempt and expose UAPI headers
> > for some obscure char device that we are all know won't be around in
> > a couple of years from now due to the nature of how this embedded world
> > works.
>
> This is not for an embedded device, but for the popular line of
> Microsoft Surface laptops / 2-in-1s...

It is the naming, we don't have char device for every "laptop" vendor.
Why is Microsoft different here?

>
> > More on that, the whole purpose of proposed interface is to debug and
> > not intended to be used by any user space code.
>
> The purpose is to provide raw access to the Surface Serial Hub protocol,
> just like we provide raw access to USB devices and have hidraw devices.

USB devices implement standard protocol, this surface hub is nothing
even close to that.

>
> So this goes a litle beyond just debugging; and eventually the choice
> may be made to implement some functionality with userspace drivers,
> just like we do for some HID and USB devices.

I don't know how it goes in device/platform area, but in other large
subsystems, UAPI should be presented with working user-space part.

>
> Still I agree with you that adding new userspace API is something which
> needs to be considered carefully. So I will look at this closely when
> reviewing this set.
>
> > Also the idea that you are creating new bus just for this device doesn't
> > really sound right. I recommend you to take a look on auxiliary bus and
> > use it or come with very strong justifications why it is not fit yet.
>
> AFAIK the auxiliary bus is for sharing a single device between multiple
> drivers, while the main device-driver also still offers functionality
> (beyond the providing of access) itself.

The idea behind auxiliary bus is to slice various functionalities into
different sub-drivers, see it as a way to create subsystem inside one
driver.

>
> This is more akin to how the WMI driver also models different WMI
> functions as a bus + devices on the bus.
>
> Or how the SDIO driver multiplex a single SDIO device into its
> functions by again using a bus + devices on the bus model.
>
> Also this has been in the works for quite a while now, the Linux on
> Microsoft Surface devices community has been working on this out of
> tree for a long time, see:
> https://github.com/linux-surface/

It is not relevant, the code is merged than it is ready.

>
> And an RFC and a v1 have been posted a while ago, while auxiliary
> bus support is not even in the mainline kernel yet. I would agree
> with you that this should switch to auxiliary bus, despite the timing,
> if that would lead to much better code. But ATM I don't really see
> switching to auxiliary bus offering much benefits here.

The auxiliary bus is merged and part of linux-next.
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/tag/?h=auxbus-5.11-rc1

>
> > I'm sorry to say, but this series is not ready to be merged yet.
> >
> > NAK: Leon Romanovsky <leon@kernel.org>
>
> See above, I believe that this all is a bit harsh and I have not
> really heard convincing arguments for not merging this.
>
> Moreover such a quick nack does not really promote working upstream,
> where as we actually want people to work upstream as much as possible.
> I know this is not a reason for taking bad code, but I'm not
> convinced that this is bad code.

I naked explicitly for two reasons: UAPI(chardev) and lack of rationale
for the custom bus, never said "bad code".

>
> I have not reviewed this myself yet, but once I have reviewed
> this and any review remarks have been addressed I do expect to
> merge this series through the platform-drivers-x86 tree.
>
> Regards,
>
> Hans de Goede
> (drivers/platform/x86 and drivers/platform/surface subsys maintainer)
>

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Blaž Hrastnik]

> 
> > More on that, the whole purpose of proposed interface is to debug and
> > not intended to be used by any user space code.
> 
> The purpose is to provide raw access to the Surface Serial Hub protocol,
> just like we provide raw access to USB devices and have hidraw devices.
> 
> So this goes a litle beyond just debugging; and eventually the choice
> may be made to implement some functionality with userspace drivers,
> just like we do for some HID and USB devices.
> 
> Still I agree with you that adding new userspace API is something which
> needs to be considered carefully. So I will look at this closely when
> reviewing this set.

To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
https://lkml.org/lkml/2020/9/24/96

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote:
> >
> > > More on that, the whole purpose of proposed interface is to debug and
> > > not intended to be used by any user space code.
> >
> > The purpose is to provide raw access to the Surface Serial Hub protocol,
> > just like we provide raw access to USB devices and have hidraw devices.
> >
> > So this goes a litle beyond just debugging; and eventually the choice
> > may be made to implement some functionality with userspace drivers,
> > just like we do for some HID and USB devices.
> >
> > Still I agree with you that adding new userspace API is something which
> > needs to be considered carefully. So I will look at this closely when
> > reviewing this set.
>
> To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
> https://lkml.org/lkml/2020/9/24/96

There is a huge difference between the suggestion and final implementation.

Greg suggested to add new debug module to the drivers/misc that will
open char device explicitly after user loaded that module to debug this
hub. However, the author added full blown char device as a first citizen
that has all not-break-user constrains.

Thanks

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Hans de Goede]

Hi,

On 12/6/20 9:56 AM, Leon Romanovsky wrote:
> On Sun, Dec 06, 2020 at 09:41:21AM +0100, Hans de Goede wrote:
>> Hi Leon,
>>
>> On 12/6/20 8:07 AM, Leon Romanovsky wrote:
>>> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
>>>> Hello,
>>>>
>>>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
>>>> driver series, adding initial support for the embedded controller on 5th
>>>> and later generation Microsoft Surface devices. Initial support includes
>>>> the ACPI interface to the controller, via which battery and thermal
>>>> information is provided on some of these devices.
>>>>
>>>> The previous version and cover letter detailing what this series is
>>>> about can be found at
>>>>
>>>>   https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>>>>
>>>> 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/v2
>>>>
>>>> Thank you all for the feedback to v1, I hope I have addressed all
>>>> comments.
>>>
>>>
>>> I think that it is too far fetched to attempt and expose UAPI headers
>>> for some obscure char device that we are all know won't be around in
>>> a couple of years from now due to the nature of how this embedded world
>>> works.
>>
>> This is not for an embedded device, but for the popular line of
>> Microsoft Surface laptops / 2-in-1s...
> 
> It is the naming, we don't have char device for every "laptop" vendor.
> Why is Microsoft different here?

Because their hardware department has invented a whole new way of dealing
with a bunch of things at the hardware level (for some reason).

Also almost all laptop vendors have a whole bunch of laptop vendor
specific userspace API in the form of sysfs files exported by
drivers/platform/x86/laptop-vendor.c drivers. E.g. do:

ls /sys/bus/platform/devices/thinkpad_acpi/

An any IBM/Lenovo Thinkpad (and only on a Thinkpad) to see a bunch
of laptop vendor specific UAPI.

Since I've become the pdx86 subsys maintainer I've actually been
pushing back against adding more of this, instead trying to
either use existing UAPIs, or defining new common UAPIs which can
be shared between vendors.

So I agree very much with you that we need to be careful about
needlessly introducing new UAPI.

But there is a difference between being careful and just nacking
it because no new UAPI may be added at all (also see GKH's response).

>>> More on that, the whole purpose of proposed interface is to debug and
>>> not intended to be used by any user space code.
>>
>> The purpose is to provide raw access to the Surface Serial Hub protocol,
>> just like we provide raw access to USB devices and have hidraw devices.
> 
> USB devices implement standard protocol, this surface hub is nothing
> even close to that.

The USB protocol just defines a transport layer, outside of the USB classes
there are plenty of proprietary protocols on top of that transport.

And this chardev just offers access to the Surface Serial Hub transport
protocol. And if you want something even closer the i2cdev module offers
raw I2C transfer access and I2C defines no protocol other then
how to read or write a number of bytes.

I do a lot of hw enablement work and being able to poke HID / USB / I2C
devices directly from userspace is very useful for this.

>> So this goes a litle beyond just debugging; and eventually the choice
>> may be made to implement some functionality with userspace drivers,
>> just like we do for some HID and USB devices.
> 
> I don't know how it goes in device/platform area, but in other large
> subsystems, UAPI should be presented with working user-space part.
> 
>>
>> Still I agree with you that adding new userspace API is something which
>> needs to be considered carefully. So I will look at this closely when
>> reviewing this set.

So this ^^^ still stands, I agree with you that adding new UAPI needs
to be considered carefully and when I get around to reviewing this
that is exactly what I will do.

Maximilian, can you perhaps explain a bit more of what you want / expect
to use the chardev for, and maybe provide pointers to the matching
userspace utilities (which I presume you have) ?

>>> Also the idea that you are creating new bus just for this device doesn't
>>> really sound right. I recommend you to take a look on auxiliary bus and
>>> use it or come with very strong justifications why it is not fit yet.
>>
>> AFAIK the auxiliary bus is for sharing a single device between multiple
>> drivers, while the main device-driver also still offers functionality
>> (beyond the providing of access) itself.
> 
> The idea behind auxiliary bus is to slice various functionalities into
> different sub-drivers, see it as a way to create subsystem inside one
> driver.

AFAIK the idea is to be able to combine multiple physical devices, e.g.
a PCI device + an ACPI enumerated platform device and then slice the
combination of those 2 up in new devices which may use parts of both
parent devices, quoting from:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/tree/Documentation/driver-api/auxiliary_bus.rst?h=driver-core-next&id=7de3697e9cbd4bd3d62bafa249d57990e1b8f294

"multiple devices might implement a common intersection of functionality"

IOW this is for cases where the simpler bus + devices model does not
work well. AFAICT in this case the simpler bus + devices does work
well, so there is no need to use the auxiliary bus.

>> This is more akin to how the WMI driver also models different WMI
>> functions as a bus + devices on the bus.
>>
>> Or how the SDIO driver multiplex a single SDIO device into its
>> functions by again using a bus + devices on the bus model.
>>
>> Also this has been in the works for quite a while now, the Linux on
>> Microsoft Surface devices community has been working on this out of
>> tree for a long time, see:
>> https://github.com/linux-surface/
> 
> It is not relevant, the code is merged than it is ready.

It is relevant, you cannot expect drivers which were written during
the last 6 months to use functionality which is not even in the
mainline kernel yet (yes it is in -next, but not in mainline).

Now if that new functionality where to provide major benefits to
the code making it much cleaner / better then yes asking to rewrite
it to use that new functionality would make sense.

I need to take a closer look at the code but ATM I'm not convinced
that rewriting it to use the new auxiliary bus stuff will make it
better at all, let alone enough to warrant the effort of the rewrite.

<snip>

Regards,

Hans


p.s.

For the record I do not own any Microsoft Surface devices myself, so
I have no interests here other then to make sure that the Linux
kernel is welcoming to new new contributors and does not needlessly
scare them away.

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Sun, Dec 06, 2020 at 11:04:06AM +0100, Hans de Goede wrote:
> Hi,
>
> On 12/6/20 9:56 AM, Leon Romanovsky wrote:
> > On Sun, Dec 06, 2020 at 09:41:21AM +0100, Hans de Goede wrote:
> >> Hi Leon,
> >>
> >> On 12/6/20 8:07 AM, Leon Romanovsky wrote:
> >>> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
> >>>> Hello,
> >>>>
> >>>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
> >>>> driver series, adding initial support for the embedded controller on 5th
> >>>> and later generation Microsoft Surface devices. Initial support includes
> >>>> the ACPI interface to the controller, via which battery and thermal
> >>>> information is provided on some of these devices.
> >>>>
> >>>> The previous version and cover letter detailing what this series is
> >>>> about can be found at
> >>>>
> >>>>   https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
> >>>>
> >>>> 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/v2
> >>>>
> >>>> Thank you all for the feedback to v1, I hope I have addressed all
> >>>> comments.
> >>>
> >>>
> >>> I think that it is too far fetched to attempt and expose UAPI headers
> >>> for some obscure char device that we are all know won't be around in
> >>> a couple of years from now due to the nature of how this embedded world
> >>> works.
> >>
> >> This is not for an embedded device, but for the popular line of
> >> Microsoft Surface laptops / 2-in-1s...
> >
> > It is the naming, we don't have char device for every "laptop" vendor.
> > Why is Microsoft different here?
>
> Because their hardware department has invented a whole new way of dealing
> with a bunch of things at the hardware level (for some reason).

They are not different from any other vendor, it is much cheaper and easier
to do not follow standard implementations.

>
> Also almost all laptop vendors have a whole bunch of laptop vendor
> specific userspace API in the form of sysfs files exported by
> drivers/platform/x86/laptop-vendor.c drivers. E.g. do:
>
> ls /sys/bus/platform/devices/thinkpad_acpi/

It is different from the proposed /dev/surface... char device.

>
> An any IBM/Lenovo Thinkpad (and only on a Thinkpad) to see a bunch
> of laptop vendor specific UAPI.

Yes, it is gross, IBM did it in early days of Linux. Other vendors don't
have anything like that.

>
> Since I've become the pdx86 subsys maintainer I've actually been
> pushing back against adding more of this, instead trying to
> either use existing UAPIs, or defining new common UAPIs which can
> be shared between vendors.
>
> So I agree very much with you that we need to be careful about
> needlessly introducing new UAPI.
>
> But there is a difference between being careful and just nacking
> it because no new UAPI may be added at all (also see GKH's response).

I saw, the author misunderstood the Greg's comments.

>
> >>> More on that, the whole purpose of proposed interface is to debug and
> >>> not intended to be used by any user space code.
> >>
> >> The purpose is to provide raw access to the Surface Serial Hub protocol,
> >> just like we provide raw access to USB devices and have hidraw devices.
> >
> > USB devices implement standard protocol, this surface hub is nothing
> > even close to that.
>
> The USB protocol just defines a transport layer, outside of the USB classes
> there are plenty of proprietary protocols on top of that transport.
>
> And this chardev just offers access to the Surface Serial Hub transport
> protocol. And if you want something even closer the i2cdev module offers
> raw I2C transfer access and I2C defines no protocol other then
> how to read or write a number of bytes.
>
> I do a lot of hw enablement work and being able to poke HID / USB / I2C
> devices directly from userspace is very useful for this.

Greg wrote how to do it.

>
> >> So this goes a litle beyond just debugging; and eventually the choice
> >> may be made to implement some functionality with userspace drivers,
> >> just like we do for some HID and USB devices.
> >
> > I don't know how it goes in device/platform area, but in other large
> > subsystems, UAPI should be presented with working user-space part.
> >
> >>
> >> Still I agree with you that adding new userspace API is something which
> >> needs to be considered carefully. So I will look at this closely when
> >> reviewing this set.
>
> So this ^^^ still stands, I agree with you that adding new UAPI needs
> to be considered carefully and when I get around to reviewing this
> that is exactly what I will do.
>
> Maximilian, can you perhaps explain a bit more of what you want / expect
> to use the chardev for, and maybe provide pointers to the matching
> userspace utilities (which I presume you have) ?
>
> >>> Also the idea that you are creating new bus just for this device doesn't
> >>> really sound right. I recommend you to take a look on auxiliary bus and
> >>> use it or come with very strong justifications why it is not fit yet.
> >>
> >> AFAIK the auxiliary bus is for sharing a single device between multiple
> >> drivers, while the main device-driver also still offers functionality
> >> (beyond the providing of access) itself.
> >
> > The idea behind auxiliary bus is to slice various functionalities into
> > different sub-drivers, see it as a way to create subsystem inside one
> > driver.
>
> AFAIK the idea is to be able to combine multiple physical devices, e.g.
> a PCI device + an ACPI enumerated platform device and then slice the
> combination of those 2 up in new devices which may use parts of both
> parent devices, quoting from:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/tree/Documentation/driver-api/auxiliary_bus.rst?h=driver-core-next&id=7de3697e9cbd4bd3d62bafa249d57990e1b8f294
>
> "multiple devices might implement a common intersection of functionality"

It is one way, another is to take one device and create many small
devices out of it.
https://lore.kernel.org/alsa-devel/20201026111849.1035786-1-leon@kernel.org/

>
> IOW this is for cases where the simpler bus + devices model does not
> work well. AFAICT in this case the simpler bus + devices does work
> well, so there is no need to use the auxiliary bus.

It is designed to replace invention of custom buses.

>
> >> This is more akin to how the WMI driver also models different WMI
> >> functions as a bus + devices on the bus.
> >>
> >> Or how the SDIO driver multiplex a single SDIO device into its
> >> functions by again using a bus + devices on the bus model.
> >>
> >> Also this has been in the works for quite a while now, the Linux on
> >> Microsoft Surface devices community has been working on this out of
> >> tree for a long time, see:
> >> https://github.com/linux-surface/
> >
> > It is not relevant, the code is merged than it is ready.
>
> It is relevant, you cannot expect drivers which were written during
> the last 6 months to use functionality which is not even in the
> mainline kernel yet (yes it is in -next, but not in mainline).
>
> Now if that new functionality where to provide major benefits to
> the code making it much cleaner / better then yes asking to rewrite
> it to use that new functionality would make sense.

And who will rewrite it? My experience shows that right code should be
written from the beginning otherwise the code has too many chances to be
abandoned.

Thanks

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 12/6/20 10:06 AM, Leon Romanovsky wrote:> On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote:
>>>
>>>> More on that, the whole purpose of proposed interface is to debug and
>>>> not intended to be used by any user space code.
>>>
>>> The purpose is to provide raw access to the Surface Serial Hub protocol,
>>> just like we provide raw access to USB devices and have hidraw devices.
>>>
>>> So this goes a litle beyond just debugging; and eventually the choice
>>> may be made to implement some functionality with userspace drivers,
>>> just like we do for some HID and USB devices.
>>>
>>> Still I agree with you that adding new userspace API is something which
>>> needs to be considered carefully. So I will look at this closely when
>>> reviewing this set.
>>
>> To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
>> https://lkml.org/lkml/2020/9/24/96
> 
> There is a huge difference between the suggestion and final implementation.
> 
> Greg suggested to add new debug module to the drivers/misc that will
> open char device explicitly after user loaded that module to debug this
> hub. However, the author added full blown char device as a first citizen
> that has all not-break-user constrains.

This module still needs to be loaded explicitly. And (I might be wrong
about this) the "not-break-user constraints" hold as soon as I register
a misc device at all, no? So I don't see how this is a) any different
than previously discussed with Greg and b) how the uapi header now
introduces any not-break-user constraints that would not be there
without it.

This interface is intended as a stable interface. That's something that
I committed to as soon as I decided to implement this via a misc-device.

Sure, I can move the definitions in the uapi header to the module
itself, but I don't see any benefit in that. If someone really wants to
use this interface, they can just as well copy the definitions from the
module source itself. So why not be upfront about it and make life
easier for everyone?

Regards,
Max

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Hans de Goede]

Hi,

On 12/6/20 11:33 AM, Leon Romanovsky wrote:
> On Sun, Dec 06, 2020 at 11:04:06AM +0100, Hans de Goede wrote:

<snip>

>> But there is a difference between being careful and just nacking
>> it because no new UAPI may be added at all (also see GKH's response).
> 
> I saw, the author misunderstood the Greg's comments.

Quoting from patch 8/9:

"
+==============================
+User-Space EC Interface (cdev)
+==============================
+
+The ``surface_aggregator_cdev`` module provides a misc-device for the SSAM
+controller to allow for a (more or less) direct connection from user-space to
+the SAM EC. It is intended to be used for development and debugging, and
+therefore should not be used or relied upon in any other way. Note that this
+module is not loaded automatically, but instead must be loaded manually.
"

If I'm not mistaken that seems to be pretty much what Greg asked for.

Regards,

Hans

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Hans de Goede]

Hi,

On 12/6/20 11:33 AM, Maximilian Luz wrote:
> On 12/6/20 10:06 AM, Leon Romanovsky wrote:> On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote:
>>>>
>>>>> More on that, the whole purpose of proposed interface is to debug and
>>>>> not intended to be used by any user space code.
>>>>
>>>> The purpose is to provide raw access to the Surface Serial Hub protocol,
>>>> just like we provide raw access to USB devices and have hidraw devices.
>>>>
>>>> So this goes a litle beyond just debugging; and eventually the choice
>>>> may be made to implement some functionality with userspace drivers,
>>>> just like we do for some HID and USB devices.
>>>>
>>>> Still I agree with you that adding new userspace API is something which
>>>> needs to be considered carefully. So I will look at this closely when
>>>> reviewing this set.
>>>
>>> To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
>>> https://lkml.org/lkml/2020/9/24/96
>>
>> There is a huge difference between the suggestion and final implementation.
>>
>> Greg suggested to add new debug module to the drivers/misc that will
>> open char device explicitly after user loaded that module to debug this
>> hub. However, the author added full blown char device as a first citizen
>> that has all not-break-user constrains.
> 
> This module still needs to be loaded explicitly.

Good then I really do not see a problem with this.

> And (I might be wrong
> about this) the "not-break-user constraints" hold as soon as I register
> a misc device at all, no?

Correct.

> So I don't see how this is a) any different
> than previously discussed with Greg and b) how the uapi header now
> introduces any not-break-user constraints that would not be there
> without it.
> 
> This interface is intended as a stable interface. That's something that
> I committed to as soon as I decided to implement this via a misc-device.
> 
> Sure, I can move the definitions in the uapi header to the module
> itself, but I don't see any benefit in that.

Right, if we are going to use a misc chardev for this, then the
correct thing to do is to put the API bits for that chardev under
include/uapi.

It would still be good if you can provide a pointer to some userspace
tools using this new API; and for the next version maybe add that
pointer to the commit message

Regards,

Hans

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 12/6/20 11:04 AM, Hans de Goede wrote:
> Hi,
> 
> On 12/6/20 9:56 AM, Leon Romanovsky wrote:
>> On Sun, Dec 06, 2020 at 09:41:21AM +0100, Hans de Goede wrote:
>>> Hi Leon,
>>>
>>> On 12/6/20 8:07 AM, Leon Romanovsky wrote:
>>>> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
>>>>> Hello,
>>>>>
>>>>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
>>>>> driver series, adding initial support for the embedded controller on 5th
>>>>> and later generation Microsoft Surface devices. Initial support includes
>>>>> the ACPI interface to the controller, via which battery and thermal
>>>>> information is provided on some of these devices.
>>>>>
>>>>> The previous version and cover letter detailing what this series is
>>>>> about can be found at
>>>>>
>>>>>    https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>>>>>
>>>>> 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/v2
>>>>>
>>>>> Thank you all for the feedback to v1, I hope I have addressed all
>>>>> comments.
>>>>
>>>>
>>>> I think that it is too far fetched to attempt and expose UAPI headers
>>>> for some obscure char device that we are all know won't be around in
>>>> a couple of years from now due to the nature of how this embedded world
>>>> works.
>>>
>>> This is not for an embedded device, but for the popular line of
>>> Microsoft Surface laptops / 2-in-1s...
>>
>> It is the naming, we don't have char device for every "laptop" vendor.
>> Why is Microsoft different here?
> 
> Because their hardware department has invented a whole new way of dealing
> with a bunch of things at the hardware level (for some reason).
> 
> Also almost all laptop vendors have a whole bunch of laptop vendor
> specific userspace API in the form of sysfs files exported by
> drivers/platform/x86/laptop-vendor.c drivers. E.g. do:
> 
> ls /sys/bus/platform/devices/thinkpad_acpi/
> 
> An any IBM/Lenovo Thinkpad (and only on a Thinkpad) to see a bunch
> of laptop vendor specific UAPI.
> 
> Since I've become the pdx86 subsys maintainer I've actually been
> pushing back against adding more of this, instead trying to
> either use existing UAPIs, or defining new common UAPIs which can
> be shared between vendors.
> 
> So I agree very much with you that we need to be careful about
> needlessly introducing new UAPI.
> 
> But there is a difference between being careful and just nacking
> it because no new UAPI may be added at all (also see GKH's response).
> 
>>>> More on that, the whole purpose of proposed interface is to debug and
>>>> not intended to be used by any user space code.
>>>
>>> The purpose is to provide raw access to the Surface Serial Hub protocol,
>>> just like we provide raw access to USB devices and have hidraw devices.
>>
>> USB devices implement standard protocol, this surface hub is nothing
>> even close to that.
> 
> The USB protocol just defines a transport layer, outside of the USB classes
> there are plenty of proprietary protocols on top of that transport.
> 
> And this chardev just offers access to the Surface Serial Hub transport
> protocol. And if you want something even closer the i2cdev module offers
> raw I2C transfer access and I2C defines no protocol other then
> how to read or write a number of bytes.
> 
> I do a lot of hw enablement work and being able to poke HID / USB / I2C
> devices directly from userspace is very useful for this.

This is pretty much the whole reason for this module. Surface devices
vary from generation to generation, and I can't expect some random user
to write some kernel module (or repeatedly pull 10 different versions
of it)to test if some EC command does x or y. I can tell them to run a
script with some arguments though. The main reason for this is
development, debugging is secondary and IMHO part of development.

So, IOW this interface provides direct access to the EC that would,
without it, require you to write a kernel driver.

The whole "this is intended for development and debugging" shtick is to
deter people from using it to implement user-space based drivers. While
this may be possible at some point, at the moment there is no good way
to (reliably) detect which client devices are present from user-space.
So any attempts at that will likely be unstable and should be
implemented in the kernel anyway. It is a way of prototyping drivers
though.

>>> So this goes a litle beyond just debugging; and eventually the choice
>>> may be made to implement some functionality with userspace drivers,
>>> just like we do for some HID and USB devices.
>>
>> I don't know how it goes in device/platform area, but in other large
>> subsystems, UAPI should be presented with working user-space part.
>>
>>>
>>> Still I agree with you that adding new userspace API is something which
>>> needs to be considered carefully. So I will look at this closely when
>>> reviewing this set.
> 
> So this ^^^ still stands, I agree with you that adding new UAPI needs
> to be considered carefully and when I get around to reviewing this
> that is exactly what I will do.
> 
> Maximilian, can you perhaps explain a bit more of what you want / expect
> to use the chardev for, and maybe provide pointers to the matching
> userspace utilities (which I presume you have) ?

Sure. There's a bunch of scripts at

   https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam

As described above, the main idea behind this simplifying development
for devices that I can't test myself. See e.g. the "ctrl.py" script
which can be used to send a basic command to the EC. The "hid.py" script
is one that was successfully used to test commands for the Keyboard
driver on the Surface Laptop 1 and 2.

At some point my plan is to maybe split that out into its own repo and
improve usability for all that, but I haven't gotten to that yet.

[...]

Regards,
Max

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 12/6/20 11:43 AM, Hans de Goede wrote:
> Hi,
> 
> On 12/6/20 11:33 AM, Maximilian Luz wrote:
>> On 12/6/20 10:06 AM, Leon Romanovsky wrote:> On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote:
>>>>>
>>>>>> More on that, the whole purpose of proposed interface is to debug and
>>>>>> not intended to be used by any user space code.
>>>>>
>>>>> The purpose is to provide raw access to the Surface Serial Hub protocol,
>>>>> just like we provide raw access to USB devices and have hidraw devices.
>>>>>
>>>>> So this goes a litle beyond just debugging; and eventually the choice
>>>>> may be made to implement some functionality with userspace drivers,
>>>>> just like we do for some HID and USB devices.
>>>>>
>>>>> Still I agree with you that adding new userspace API is something which
>>>>> needs to be considered carefully. So I will look at this closely when
>>>>> reviewing this set.
>>>>
>>>> To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
>>>> https://lkml.org/lkml/2020/9/24/96
>>>
>>> There is a huge difference between the suggestion and final implementation.
>>>
>>> Greg suggested to add new debug module to the drivers/misc that will
>>> open char device explicitly after user loaded that module to debug this
>>> hub. However, the author added full blown char device as a first citizen
>>> that has all not-break-user constrains.
>>
>> This module still needs to be loaded explicitly.
> 
> Good then I really do not see a problem with this.
> 
>> And (I might be wrong
>> about this) the "not-break-user constraints" hold as soon as I register
>> a misc device at all, no?
> 
> Correct.
> 
>> So I don't see how this is a) any different
>> than previously discussed with Greg and b) how the uapi header now
>> introduces any not-break-user constraints that would not be there
>> without it.
>>
>> This interface is intended as a stable interface. That's something that
>> I committed to as soon as I decided to implement this via a misc-device.
>>
>> Sure, I can move the definitions in the uapi header to the module
>> itself, but I don't see any benefit in that.
> 
> Right, if we are going to use a misc chardev for this, then the
> correct thing to do is to put the API bits for that chardev under
> include/uapi.
> 
> It would still be good if you can provide a pointer to some userspace
> tools using this new API; and for the next version maybe add that
> pointer to the commit message

Right, I will add that to the commit message. I just linked you the
scripts in my other response, but here again for completeness:

   https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam

While I'm not using the header directly (the scripts are written in
python) I still think uapi is the right place to put this (please
correct me if I'm wrong). Not putting them there seems to be needless
obfuscating to me.

Regards,
Max

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 12/6/20 9:32 AM, Greg Kroah-Hartman wrote:
> On Sun, Dec 06, 2020 at 09:07:05AM +0200, Leon Romanovsky wrote:
>> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
>>> Hello,
>>>
>>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
>>> driver series, adding initial support for the embedded controller on 5th
>>> and later generation Microsoft Surface devices. Initial support includes
>>> the ACPI interface to the controller, via which battery and thermal
>>> information is provided on some of these devices.
>>>
>>> The previous version and cover letter detailing what this series is
>>> about can be found at
>>>
>>>    https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>>>
>>> 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/v2
>>>
>>> Thank you all for the feedback to v1, I hope I have addressed all
>>> comments.
>>
>>
>> I think that it is too far fetched to attempt and expose UAPI headers
>> for some obscure char device that we are all know won't be around in
>> a couple of years from now due to the nature of how this embedded world
>> works.
> 
> No, that's not ok, we do this for loads of devices out there.  If there
> is a device that wants to be supported for Linux, and a developer that
> wants to support it, we will take it.
> 
>> More on that, the whole purpose of proposed interface is to debug and
>> not intended to be used by any user space code.
> 
> I thought that debugfs was going to be used for most of the debugging
> code, or has that changed in newer versions of this patchset?
As per previous discussion (https://lkml.org/lkml/2020/9/24/96) I have
replaced the debugfs device by a misc-device with stable interface.

I also believe that this is probably the better option long-term. The
general idea is to have a device that has direct access to the
EC/transport protocol and can be used for development and prototyping.
Debugging is a part of that. So it's more akin to something raw access
via i2cdev, hidraw, or raw access to USB devices as Hans de Goede
mentioned in one of his mails. Note that the module must still be loaded
manually

Regards,
Max

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Sun, Dec 06, 2020 at 11:33:40AM +0100, Maximilian Luz wrote:
> On 12/6/20 10:06 AM, Leon Romanovsky wrote:> On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote:
> > > >
> > > > > More on that, the whole purpose of proposed interface is to debug and
> > > > > not intended to be used by any user space code.
> > > >
> > > > The purpose is to provide raw access to the Surface Serial Hub protocol,
> > > > just like we provide raw access to USB devices and have hidraw devices.
> > > >
> > > > So this goes a litle beyond just debugging; and eventually the choice
> > > > may be made to implement some functionality with userspace drivers,
> > > > just like we do for some HID and USB devices.
> > > >
> > > > Still I agree with you that adding new userspace API is something which
> > > > needs to be considered carefully. So I will look at this closely when
> > > > reviewing this set.
> > >
> > > To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
> > > https://lkml.org/lkml/2020/9/24/96
> >
> > There is a huge difference between the suggestion and final implementation.
> >
> > Greg suggested to add new debug module to the drivers/misc that will
> > open char device explicitly after user loaded that module to debug this
> > hub. However, the author added full blown char device as a first citizen
> > that has all not-break-user constrains.
>
> This module still needs to be loaded explicitly. And (I might be wrong
> about this) the "not-break-user constraints" hold as soon as I register
> a misc device at all, no?

I don't think so, files in drivers/misc/* don't have such strict policy.

> than previously discussed with Greg and b) how the uapi header now
> introduces any not-break-user constraints that would not be there
> without it.

There is a huge difference between char device for the debug and
exposed UAPI header. The first requires from the user to build and
explicitly run it, while header allows to reliably build on top of
it various applications that we don't control. The not-break-rule
talks about the second.

>
> This interface is intended as a stable interface. That's something that
> I committed to as soon as I decided to implement this via a misc-device.
>
> Sure, I can move the definitions in the uapi header to the module
> itself, but I don't see any benefit in that. If someone really wants to
> use this interface, they can just as well copy the definitions from the
> module source itself. So why not be upfront about it and make life
> easier for everyone?

Because you are actually making life harder for everyone who cares about
UAPIs exposed by the Linux and they definitely different in numbers from
those who needs debug interface for the Microsoft Surface board.

Thanks

>
> Regards,
> Max
>

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Sun, Dec 06, 2020 at 11:41:46AM +0100, Hans de Goede wrote:
> Hi,
>
> On 12/6/20 11:33 AM, Leon Romanovsky wrote:
> > On Sun, Dec 06, 2020 at 11:04:06AM +0100, Hans de Goede wrote:
>
> <snip>
>
> >> But there is a difference between being careful and just nacking
> >> it because no new UAPI may be added at all (also see GKH's response).
> >
> > I saw, the author misunderstood the Greg's comments.
>
> Quoting from patch 8/9:
>
> "
> +==============================
> +User-Space EC Interface (cdev)
> +==============================
> +
> +The ``surface_aggregator_cdev`` module provides a misc-device for the SSAM
> +controller to allow for a (more or less) direct connection from user-space to
> +the SAM EC. It is intended to be used for development and debugging, and
> +therefore should not be used or relied upon in any other way. Note that this
> +module is not loaded automatically, but instead must be loaded manually.
> "
>
> If I'm not mistaken that seems to be pretty much what Greg asked for.

Right, unless you forget the end of his request.
 "
  The "joy" of creating a user api is that no matter how much you tell
  people "do not depend on this", they will, so no matter the file being
  in debugfs, or a misc device, you might be stuck with it for forever,
  sorry.
 "

So I still think that exposing user api for a development and debug of device
that has no future is wrong thing to do.

Thanks

>
> Regards,
>
> Hans
>

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 12/6/20 12:30 PM, Leon Romanovsky wrote:
> On Sun, Dec 06, 2020 at 11:33:40AM +0100, Maximilian Luz wrote:
>> On 12/6/20 10:06 AM, Leon Romanovsky wrote:> On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote:
>>>>>
>>>>>> More on that, the whole purpose of proposed interface is to debug and
>>>>>> not intended to be used by any user space code.
>>>>>
>>>>> The purpose is to provide raw access to the Surface Serial Hub protocol,
>>>>> just like we provide raw access to USB devices and have hidraw devices.
>>>>>
>>>>> So this goes a litle beyond just debugging; and eventually the choice
>>>>> may be made to implement some functionality with userspace drivers,
>>>>> just like we do for some HID and USB devices.
>>>>>
>>>>> Still I agree with you that adding new userspace API is something which
>>>>> needs to be considered carefully. So I will look at this closely when
>>>>> reviewing this set.
>>>>
>>>> To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
>>>> https://lkml.org/lkml/2020/9/24/96
>>>
>>> There is a huge difference between the suggestion and final implementation.
>>>
>>> Greg suggested to add new debug module to the drivers/misc that will
>>> open char device explicitly after user loaded that module to debug this
>>> hub. However, the author added full blown char device as a first citizen
>>> that has all not-break-user constrains.
>>
>> This module still needs to be loaded explicitly. And (I might be wrong
>> about this) the "not-break-user constraints" hold as soon as I register
>> a misc device at all, no?
> 
> I don't think so, files in drivers/misc/* don't have such strict policy.

Can I get a link to the documentation stating that or someone else
confirming that?

Also I don't think it makes sense to have a platform/surface device in
drivers/misc, after we've explicitly decided to move this code out of
there. IIRC drivers/misc is not a place for misc-devices, but the
directory for devices that don't have any good place elsewhere.

>> than previously discussed with Greg and b) how the uapi header now
>> introduces any not-break-user constraints that would not be there
>> without it.
> 
> There is a huge difference between char device for the debug and
> exposed UAPI header. The first requires from the user to build and
> explicitly run it, while header allows to reliably build on top of
> it various applications that we don't control. The not-break-rule
> talks about the second.

So it's okay to break stuff that's not explicitly in include/uapi/?
Again, can I get someone to confirm that for me?

As already said, I'm okay with moving the definitions from the header to
the module itself (if there is a consensus on that, CC Greg, Hans),
however both allow you to build user-space tools against the API. Case
in point my python scripts, which don't use the header. Or any other
non-C-based tool. So unless there's a rule that anything without a
header in uapi is fair game, I fail to see your point.

>> This interface is intended as a stable interface. That's something that
>> I committed to as soon as I decided to implement this via a misc-device.
>>
>> Sure, I can move the definitions in the uapi header to the module
>> itself, but I don't see any benefit in that. If someone really wants to
>> use this interface, they can just as well copy the definitions from the
>> module source itself. So why not be upfront about it and make life
>> easier for everyone?
> 
> Because you are actually making life harder for everyone who cares about
> UAPIs exposed by the Linux and they definitely different in numbers from
> those who needs debug interface for the Microsoft Surface board.

This point again depends on the interface not being stable. Unless, of
course, you want me to remove the interface completely and just maintain
it out of tree...

I'm happy to be told otherwise by the authorities here, but from past
conversations it seems that basically everything providing some sort of
user-space access falls under the "don't break user-space" rule as soon
as somebody uses it.

Regards,
Max

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 12/6/20 12:41 PM, Leon Romanovsky wrote:
> On Sun, Dec 06, 2020 at 11:41:46AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 12/6/20 11:33 AM, Leon Romanovsky wrote:
>>> On Sun, Dec 06, 2020 at 11:04:06AM +0100, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> But there is a difference between being careful and just nacking
>>>> it because no new UAPI may be added at all (also see GKH's response).
>>>
>>> I saw, the author misunderstood the Greg's comments.
>>
>> Quoting from patch 8/9:
>>
>> "
>> +==============================
>> +User-Space EC Interface (cdev)
>> +==============================
>> +
>> +The ``surface_aggregator_cdev`` module provides a misc-device for the SSAM
>> +controller to allow for a (more or less) direct connection from user-space to
>> +the SAM EC. It is intended to be used for development and debugging, and
>> +therefore should not be used or relied upon in any other way. Note that this
>> +module is not loaded automatically, but instead must be loaded manually.
>> "
>>
>> If I'm not mistaken that seems to be pretty much what Greg asked for.
> 
> Right, unless you forget the end of his request.
>   "
>    The "joy" of creating a user api is that no matter how much you tell
>    people "do not depend on this", they will, so no matter the file being
>    in debugfs, or a misc device, you might be stuck with it for forever,
>    sorry.
>   "

Which to me reads as "if you want a user-space interface for development and
debugging, you'll have to make it a stable interface, regardless where it is
implemented". Rather making a point for a well though-out stable interface.
Specifically with regards to

    "
     >  - So you suggest I go with a misc device instead of putting this into
     >    debugfs?

     Yes.
    "

Unless of course I'm misunderstanding things entirely. Greg, please feel free
to yell at me if I've got this wrong.

> So I still think that exposing user api for a development and debug of device
> that has no future is wrong thing to do.

Unless you know something that I don't, MS is rumored to come out with a new
Surface Pro 8 and Surface Laptop 4 early next year, which I fully expect to
also have this EC built in. And if they once again decided to move some
functionality normally provided via ACPI or other means to the EC for some
reason, we'll likely need that interface again.

Yes, it has no future outside of Surface devices, but so has every other
platform driver with respect to their specific platform. What are your
alternatives to exposing a user API? If we want to be able to easily test
and attempt to provide support for Surface devices, there has to be some
interaction with user-space.

With respect to stability of the interface and future changes, I believe
that IOCTLs are the way to go. If that's in debugfs or, as was the
result from the previous discussion about this, via a misc-device...
I'll be happy to implement whatever a consensus yields, as long as it
can be used for its intended purpose: aid development with regards to
the EC found on Surface devices.

Regards,
Max

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 12/6/20 8:07 AM, Leon Romanovsky wrote:
> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
>> Hello,
>>
>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
>> driver series, adding initial support for the embedded controller on 5th
>> and later generation Microsoft Surface devices. Initial support includes
>> the ACPI interface to the controller, via which battery and thermal
>> information is provided on some of these devices.
>>
>> The previous version and cover letter detailing what this series is
>> about can be found at
>>
>>    https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>>
>> 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/v2
>>
>> Thank you all for the feedback to v1, I hope I have addressed all
>> comments.
> 
> 
> I think that it is too far fetched to attempt and expose UAPI headers
> for some obscure char device that we are all know won't be around in
> a couple of years from now due to the nature of how this embedded world
> works.
> 
> More on that, the whole purpose of proposed interface is to debug and
> not intended to be used by any user space code.

I believe this has already been extensively discussed. I want to focus
more on the part below in this response:

> Also the idea that you are creating new bus just for this device doesn't
> really sound right. I recommend you to take a look on auxiliary bus and
> use it or come with very strong justifications why it is not fit yet.

I tend to agree that this is a valid concern to bring up, and adding a
new bus is not something that should be done lightly.

Let's ignore that this has been merged into -next after I've submitted
this (and that I only recently became aware of this) for the time being.
If I would see a clear benefit, I would not hesitate to switch the
driver and subsystem over to this.

What does concern me most, is the device/driver matching by string.
Right now, this subsystem matches those via a device UID. This UID is
directly tied to the EC functionality provided by the device. A bit of
background to this:

Requests sent to the EC contain an address, so to say. This consists of

  - Target category (TC): Broad group of functionality, e.g. battery/AC,
    thermal, HID input, ..., i.e. a subsystem of sorts.

  - Target ID (TID): Some major device, e.g. the dual batteries on the
    Surface Book 3 are addressed by target ID 1 and 2, some functionality
    is only available at 2 and some only at 1. May be related to physical
    parts of/locations on the device.

  - Instance ID (IID): A device instance, e.g. for thermal sensors each
    sensor is at TC=0x03 (thermal) and has a different instance ID.

Those can be used to pretty much uniquely identify a sub-device on the
EC.

Note the "pretty much". To truly make them unique we can add a function
ID (FN). With that, we can for example match for TC=0x03, TID=*, IID=*,
FN=0x00 to load a driver against all thermal sensors. And this is
basically the device UID that the subsystem uses for matching (modulo
domain for virtual devices, i.e. device hubs). Sure, we can use some
string, but that then leads to having to come up with creative names
once we need some driver specific data, e.g. in the battery driver [1]:

     const struct auxiliary_device_id my_auxiliary_id_table[] = {
         { .name = "surface_aggregator_registry.battery", .driver_data = x },
         { .name = "surface_aggregator_registry.battery_sb3", .driver_data = y },
         { },
     }

Arguably, not _that_ big of a deal.

What worries me more is that this will block any path of auto-detecting
devices on a more general/global level. Right now, we hard-code devices
because we haven't found any way to detect them via some EC query yet
[2] (FYI the node groups contain all devices that will eventually be
added to the bus, which are already 11 devices on the Surface Book 3
without taking missing thermal sensors into account; also they are
spread across a bunch of subsystems, so not just platform). That's of
course not an ideal solution and one that I hope we can eventually fix.
If we can auto-detect devices, it's very likely that we know or can
easily get to the device UID. A meaningful string is somewhat more
difficult.

This registry, which is loaded against a platform device that, from what
we can tell differentiates the models for some driver bindings by
Windows (that's speculation), is also the reason why we don't register
client devices directly under the main module, so instead of a nice
"surface_aggregator.<devicename>", you'll get
"surface_aggregator_registry.<devicename>". And it may not end there.

Something that's currently not implemented is support for thermal
sensors on 7th generation devices. With thermal sensors, we can already
detect which sensors, i.e. which IIDs, are present. Naturally, that's
part of the EC-API for thermal devices (TC=0x03), so would warrant a
master driver that registers the individual sensor drivers (that's a
place where I'd argue that in a normal situation, the auxiliary bus
makes sense). So with the auxiliary bus we'd now end up with devices
with "surface_thermal.sensor" for the sensors as well as
"surface_aggregator_registry.<devicename>", both of type ssam_device
(which then would be a wrapper around auxiliary_device with UID stored
in that wrapper). Note that they need to be of type ssam_device (or
another wrapper around that) as they again need the reference to the
controller device, their UID for access, etc. With a proper bus, device,
and the UID for matching, we can just add the sensor devices to the bus
again, as they will have a meaningful and guaranteed unique UID.

 From some reports I've seen it looks like thermal sensors may also be
available separately on TID=0x01 as well as TID=0x02 on some devices,
at which point I believe you'd need to introduce some IDA for ID
allocation to not cause a clash with IDs. At least if you separate the
base drivers for each TC, which I guess should be preferred due to
code-reuse. Then again they might use different event registries so you
may end up needing "surface_thermal.sensor_tc1" and
"surface_thermal.sensor_tc2" as device names to differentiate those
for driver loading. Or store the registry in software node properties
when registering the device.

I'm repeating myself here, but to me it looks cleaner to have a single
bus type as opposed to spreading the same base auxiliary device type
over several namespaces.

Which then leads me to the question of how a function like
"is_ssam_device()", i.e. a function testing if the device is of a given
type, would be implemented without enforcing and testing against some
part of the device name. Something that, again, doesn't look clean to
me. Although the use of such a function could probably avoided, but that
then feels like working around the auxiliary bus.

Unfortunately, there are a couple more hypotheticals at play than I'd
like to have (making this not an easy decision), but it's a reverse
engineered driver so I guess that comes with the territory. All in all,
I believe it's possible to do this (i.e. use the auxiliary bus), but, to
me at least, the implementation using a discrete bus feels tidier and
more true to the hardware (or virtual hardware anyway) behind this. I'm
happy to hear any arguments against this though.

Regards,
Max

[1]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_battery.c#L1075-L1079
[2]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_aggregator_registry.c

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky]

On Sun, Dec 06, 2020 at 04:58:52PM +0100, Maximilian Luz wrote:
> On 12/6/20 8:07 AM, Leon Romanovsky wrote:
> > On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
> > > Hello,
> > >
> > > Here is version two of the Surface System Aggregator Module (SAM/SSAM)
> > > driver series, adding initial support for the embedded controller on 5th
> > > and later generation Microsoft Surface devices. Initial support includes
> > > the ACPI interface to the controller, via which battery and thermal
> > > information is provided on some of these devices.
> > >
> > > The previous version and cover letter detailing what this series is
> > > about can be found at
> > >
> > >    https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
> > >
> > > 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/v2
> > >
> > > Thank you all for the feedback to v1, I hope I have addressed all
> > > comments.
> >
> >
> > I think that it is too far fetched to attempt and expose UAPI headers
> > for some obscure char device that we are all know won't be around in
> > a couple of years from now due to the nature of how this embedded world
> > works.
> >
> > More on that, the whole purpose of proposed interface is to debug and
> > not intended to be used by any user space code.
>
> I believe this has already been extensively discussed. I want to focus
> more on the part below in this response:
>
> > Also the idea that you are creating new bus just for this device doesn't
> > really sound right. I recommend you to take a look on auxiliary bus and
> > use it or come with very strong justifications why it is not fit yet.
>
> I tend to agree that this is a valid concern to bring up, and adding a
> new bus is not something that should be done lightly.
>
> Let's ignore that this has been merged into -next after I've submitted
> this (and that I only recently became aware of this) for the time being.
> If I would see a clear benefit, I would not hesitate to switch the
> driver and subsystem over to this.
>
> What does concern me most, is the device/driver matching by string.
> Right now, this subsystem matches those via a device UID. This UID is
> directly tied to the EC functionality provided by the device. A bit of
> background to this:
>
> Requests sent to the EC contain an address, so to say. This consists of
>
>  - Target category (TC): Broad group of functionality, e.g. battery/AC,
>    thermal, HID input, ..., i.e. a subsystem of sorts.
>
>  - Target ID (TID): Some major device, e.g. the dual batteries on the
>    Surface Book 3 are addressed by target ID 1 and 2, some functionality
>    is only available at 2 and some only at 1. May be related to physical
>    parts of/locations on the device.
>
>  - Instance ID (IID): A device instance, e.g. for thermal sensors each
>    sensor is at TC=0x03 (thermal) and has a different instance ID.
>
> Those can be used to pretty much uniquely identify a sub-device on the
> EC.
>
> Note the "pretty much". To truly make them unique we can add a function
> ID (FN). With that, we can for example match for TC=0x03, TID=*, IID=*,
> FN=0x00 to load a driver against all thermal sensors. And this is
> basically the device UID that the subsystem uses for matching (modulo
> domain for virtual devices, i.e. device hubs). Sure, we can use some
> string, but that then leads to having to come up with creative names
> once we need some driver specific data, e.g. in the battery driver [1]:
>
>     const struct auxiliary_device_id my_auxiliary_id_table[] = {
>         { .name = "surface_aggregator_registry.battery", .driver_data = x },
>         { .name = "surface_aggregator_registry.battery_sb3", .driver_data = y },
>         { },
>     }
>
> Arguably, not _that_ big of a deal.
>
> What worries me more is that this will block any path of auto-detecting
> devices on a more general/global level. Right now, we hard-code devices
> because we haven't found any way to detect them via some EC query yet
> [2] (FYI the node groups contain all devices that will eventually be
> added to the bus, which are already 11 devices on the Surface Book 3
> without taking missing thermal sensors into account; also they are
> spread across a bunch of subsystems, so not just platform). That's of
> course not an ideal solution and one that I hope we can eventually fix.
> If we can auto-detect devices, it's very likely that we know or can
> easily get to the device UID. A meaningful string is somewhat more
> difficult.
>
> This registry, which is loaded against a platform device that, from what
> we can tell differentiates the models for some driver bindings by
> Windows (that's speculation), is also the reason why we don't register
> client devices directly under the main module, so instead of a nice
> "surface_aggregator.<devicename>", you'll get
> "surface_aggregator_registry.<devicename>". And it may not end there.
>
> Something that's currently not implemented is support for thermal
> sensors on 7th generation devices. With thermal sensors, we can already
> detect which sensors, i.e. which IIDs, are present. Naturally, that's
> part of the EC-API for thermal devices (TC=0x03), so would warrant a
> master driver that registers the individual sensor drivers (that's a
> place where I'd argue that in a normal situation, the auxiliary bus
> makes sense). So with the auxiliary bus we'd now end up with devices
> with "surface_thermal.sensor" for the sensors as well as
> "surface_aggregator_registry.<devicename>", both of type ssam_device
> (which then would be a wrapper around auxiliary_device with UID stored
> in that wrapper). Note that they need to be of type ssam_device (or
> another wrapper around that) as they again need the reference to the
> controller device, their UID for access, etc. With a proper bus, device,
> and the UID for matching, we can just add the sensor devices to the bus
> again, as they will have a meaningful and guaranteed unique UID.
>
> From some reports I've seen it looks like thermal sensors may also be
> available separately on TID=0x01 as well as TID=0x02 on some devices,
> at which point I believe you'd need to introduce some IDA for ID
> allocation to not cause a clash with IDs. At least if you separate the
> base drivers for each TC, which I guess should be preferred due to
> code-reuse. Then again they might use different event registries so you
> may end up needing "surface_thermal.sensor_tc1" and
> "surface_thermal.sensor_tc2" as device names to differentiate those
> for driver loading. Or store the registry in software node properties
> when registering the device.
>
> I'm repeating myself here, but to me it looks cleaner to have a single
> bus type as opposed to spreading the same base auxiliary device type
> over several namespaces.
>
> Which then leads me to the question of how a function like
> "is_ssam_device()", i.e. a function testing if the device is of a given
> type, would be implemented without enforcing and testing against some
> part of the device name. Something that, again, doesn't look clean to
> me. Although the use of such a function could probably avoided, but that
> then feels like working around the auxiliary bus.
>
> Unfortunately, there are a couple more hypotheticals at play than I'd
> like to have (making this not an easy decision), but it's a reverse
> engineered driver so I guess that comes with the territory. All in all,
> I believe it's possible to do this (i.e. use the auxiliary bus), but, to
> me at least, the implementation using a discrete bus feels tidier and
> more true to the hardware (or virtual hardware anyway) behind this. I'm
> happy to hear any arguments against this though.

I'm not so certain why you are so focused on the names and UIDs.
The names are simply the aliases and needed to match between major
device driver that is responsible for the discovery and triggering
sub-driver with implementation.

Nothing prohibits from the logic: "if uid = 123; trigger "sensor""

Different devices and separation between them (*_registry module)
are expected to be in that main driver.

As an outcome of it, you will get: proper module autoload, device
discover and power management.

Thanks

>
> Regards,
> Max
>
> [1]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_battery.c#L1075-L1079
> [2]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_aggregator_registry.c

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Hans de Goede]

Hi,

On 12/6/20 4:58 PM, Maximilian Luz wrote:
> On 12/6/20 8:07 AM, Leon Romanovsky wrote:
>> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
>>> Hello,
>>>
>>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
>>> driver series, adding initial support for the embedded controller on 5th
>>> and later generation Microsoft Surface devices. Initial support includes
>>> the ACPI interface to the controller, via which battery and thermal
>>> information is provided on some of these devices.
>>>
>>> The previous version and cover letter detailing what this series is
>>> about can be found at
>>>
>>>    https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>>>
>>> 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/v2
>>>
>>> Thank you all for the feedback to v1, I hope I have addressed all
>>> comments.
>>
>>
>> I think that it is too far fetched to attempt and expose UAPI headers
>> for some obscure char device that we are all know won't be around in
>> a couple of years from now due to the nature of how this embedded world
>> works.
>>
>> More on that, the whole purpose of proposed interface is to debug and
>> not intended to be used by any user space code.
> 
> I believe this has already been extensively discussed. I want to focus
> more on the part below in this response:
> 
>> Also the idea that you are creating new bus just for this device doesn't
>> really sound right. I recommend you to take a look on auxiliary bus and
>> use it or come with very strong justifications why it is not fit yet.
> 
> I tend to agree that this is a valid concern to bring up, and adding a
> new bus is not something that should be done lightly.
> 
> Let's ignore that this has been merged into -next after I've submitted
> this (and that I only recently became aware of this) for the time being.
> If I would see a clear benefit, I would not hesitate to switch the
> driver and subsystem over to this.
> 
> What does concern me most, is the device/driver matching by string.
> Right now, this subsystem matches those via a device UID. This UID is
> directly tied to the EC functionality provided by the device. A bit of
> background to this:
> 
> Requests sent to the EC contain an address, so to say. This consists of
> 
>  - Target category (TC): Broad group of functionality, e.g. battery/AC,
>    thermal, HID input, ..., i.e. a subsystem of sorts.
> 
>  - Target ID (TID): Some major device, e.g. the dual batteries on the
>    Surface Book 3 are addressed by target ID 1 and 2, some functionality
>    is only available at 2 and some only at 1. May be related to physical
>    parts of/locations on the device.
> 
>  - Instance ID (IID): A device instance, e.g. for thermal sensors each
>    sensor is at TC=0x03 (thermal) and has a different instance ID.
> 
> Those can be used to pretty much uniquely identify a sub-device on the
> EC.

Thank you for this explanation, that is going to be useful to know
when I get around to reviewing this set (although I guess that you
probably also have written this down in one of the commit msgs /
docs I did not check).

> 
> Note the "pretty much". To truly make them unique we can add a function
> ID (FN). With that, we can for example match for TC=0x03, TID=*, IID=*,
> FN=0x00 to load a driver against all thermal sensors. And this is
> basically the device UID that the subsystem uses for matching (modulo
> domain for virtual devices, i.e. device hubs). Sure, we can use some
> string, but that then leads to having to come up with creative names
> once we need some driver specific data, e.g. in the battery driver [1]:
> 
>     const struct auxiliary_device_id my_auxiliary_id_table[] = {
>         { .name = "surface_aggregator_registry.battery", .driver_data = x },
>         { .name = "surface_aggregator_registry.battery_sb3", .driver_data = y },
>         { },
>     }
> 
> Arguably, not _that_ big of a deal.
> 
> What worries me more is that this will block any path of auto-detecting
> devices on a more general/global level. Right now, we hard-code devices
> because we haven't found any way to detect them via some EC query yet
> [2] (FYI the node groups contain all devices that will eventually be
> added to the bus, which are already 11 devices on the Surface Book 3
> without taking missing thermal sensors into account; also they are
> spread across a bunch of subsystems, so not just platform). That's of
> course not an ideal solution and one that I hope we can eventually fix.
> If we can auto-detect devices, it's very likely that we know or can
> easily get to the device UID. A meaningful string is somewhat more
> difficult.
> 
> This registry, which is loaded against a platform device that, from what
> we can tell differentiates the models for some driver bindings by
> Windows (that's speculation), is also the reason why we don't register
> client devices directly under the main module, so instead of a nice
> "surface_aggregator.<devicename>", you'll get
> "surface_aggregator_registry.<devicename>". And it may not end there.
> 
> Something that's currently not implemented is support for thermal
> sensors on 7th generation devices. With thermal sensors, we can already
> detect which sensors, i.e. which IIDs, are present. Naturally, that's
> part of the EC-API for thermal devices (TC=0x03), so would warrant a
> master driver that registers the individual sensor drivers (that's a
> place where I'd argue that in a normal situation, the auxiliary bus
> makes sense). So with the auxiliary bus we'd now end up with devices
> with "surface_thermal.sensor" for the sensors as well as
> "surface_aggregator_registry.<devicename>", both of type ssam_device
> (which then would be a wrapper around auxiliary_device with UID stored
> in that wrapper). Note that they need to be of type ssam_device (or
> another wrapper around that) as they again need the reference to the
> controller device, their UID for access, etc. With a proper bus, device,
> and the UID for matching, we can just add the sensor devices to the bus
> again, as they will have a meaningful and guaranteed unique UID.
> 
> From some reports I've seen it looks like thermal sensors may also be
> available separately on TID=0x01 as well as TID=0x02 on some devices,
> at which point I believe you'd need to introduce some IDA for ID
> allocation to not cause a clash with IDs. At least if you separate the
> base drivers for each TC, which I guess should be preferred due to
> code-reuse. Then again they might use different event registries so you
> may end up needing "surface_thermal.sensor_tc1" and
> "surface_thermal.sensor_tc2" as device names to differentiate those
> for driver loading. Or store the registry in software node properties
> when registering the device.
> 
> I'm repeating myself here, but to me it looks cleaner to have a single
> bus type as opposed to spreading the same base auxiliary device type
> over several namespaces.
> 
> Which then leads me to the question of how a function like
> "is_ssam_device()", i.e. a function testing if the device is of a given
> type, would be implemented without enforcing and testing against some
> part of the device name. Something that, again, doesn't look clean to
> me. Although the use of such a function could probably avoided, but that
> then feels like working around the auxiliary bus.
> 
> Unfortunately, there are a couple more hypotheticals at play than I'd
> like to have (making this not an easy decision), but it's a reverse
> engineered driver so I guess that comes with the territory. All in all,


> I believe it's possible to do this (i.e. use the auxiliary bus), but, to
> me at least, the implementation using a discrete bus feels tidier and
> more true to the hardware (or virtual hardware anyway) behind this. I'm
> happy to hear any arguments against this though.

I agree, the whole setup with the TC + TID + IID feels like the functionality
is nicely (and cleanly) split into separate functions and as with other
busses using a bus + 1 device per function for this is a perfectly clean
way to handle this.

Note if in the future you do see benefit in switching the auxiliary bus
I have no problems with that. But atm I don't really see any benefits of
doing so, so then we would just be switching over for the sake of switching
over which does not seem productive.

Regards,

Hans

Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Greg Kroah-Hartman]

On Mon, Dec 07, 2020 at 09:49:03AM +0100, Hans de Goede wrote:
> Note if in the future you do see benefit in switching the auxiliary bus
> I have no problems with that. But atm I don't really see any benefits of
> doing so, so then we would just be switching over for the sake of switching
> over which does not seem productive.

I too do not see the benefit at this time to switch either.

thanks,

greg k-h

Re: [PATCH v2 6/9] platform/surface: aggregator: Add dedicated bus and device type

[Hans de Goede]

Hi,

On 12/3/20 10:26 PM, Maximilian Luz wrote:
> The Surface Aggregator EC provides varying functionality, depending on
> the Surface device. To manage this functionality, we use dedicated
> client devices for each subsystem or virtual device of the EC. While
> some of these clients are described as standard devices in ACPI and the
> corresponding client drivers can be implemented as platform drivers in
> the kernel (making use of the controller API already present), many
> devices, especially on newer Surface models, cannot be found there.
> 
> To simplify management of these devices, we introduce a new bus and
> client device type for the Surface Aggregator subsystem. The new device
> type takes care of managing the controller reference, essentially
> guaranteeing its validity for as long as the client device exists, thus
> alleviating the need to manually establish device links for that purpose
> in the client driver (as has to be done with the platform devices).
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
> 
> Changes in v1 (from RFC):
>  - add copyright lines
>  - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
>  - remove unnecessary READ_ONCE on multiple occasions
>  - improve documentation of special values for SSAM_DEVICE()
>  - add NULL checks for ssam_device_get, ssam_device_put
> 
> Changes in v2:
>  - return ENODEV instead of ENXIO if controller is not present
>  - use sysfs_emit for sysfs attributes
>  - spell check comments and strings, fix typos
>  - unify comment style
>  - run checkpatch --strict, fix warnings and style issues
> 
> ---
>  drivers/platform/surface/aggregator/Kconfig  |  12 +
>  drivers/platform/surface/aggregator/Makefile |   4 +
>  drivers/platform/surface/aggregator/bus.c    | 415 ++++++++++++++++++
>  drivers/platform/surface/aggregator/bus.h    |  27 ++
>  drivers/platform/surface/aggregator/core.c   |  12 +
>  include/linux/mod_devicetable.h              |  18 +
>  include/linux/surface_aggregator/device.h    | 423 +++++++++++++++++++
>  scripts/mod/devicetable-offsets.c            |   8 +
>  scripts/mod/file2alias.c                     |  23 +
>  9 files changed, 942 insertions(+)
>  create mode 100644 drivers/platform/surface/aggregator/bus.c
>  create mode 100644 drivers/platform/surface/aggregator/bus.h
>  create mode 100644 include/linux/surface_aggregator/device.h
> 
> diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
> index 48f40c345e29..44c2493706bc 100644
> --- a/drivers/platform/surface/aggregator/Kconfig
> +++ b/drivers/platform/surface/aggregator/Kconfig
> @@ -42,6 +42,18 @@ menuconfig SURFACE_AGGREGATOR
>  	  module, y if you want to build it into the kernel and n if you don't
>  	  want it at all.
> 
> +config SURFACE_AGGREGATOR_BUS
> +	bool "Surface System Aggregator Module Bus"
> +	depends on SURFACE_AGGREGATOR
> +	default y
> +	help
> +	  Expands the Surface System Aggregator Module (SSAM) core driver by
> +	  providing a dedicated bus and client-device type.
> +
> +	  This bus and device type are intended to provide and simplify support
> +	  for non-platform and non-ACPI SSAM devices, i.e. SSAM devices that are
> +	  not auto-detectable via the conventional means (e.g. ACPI).
> +
>  config SURFACE_AGGREGATOR_ERROR_INJECTION
>  	bool "Surface System Aggregator Module Error Injection Capabilities"
>  	depends on SURFACE_AGGREGATOR
> diff --git a/drivers/platform/surface/aggregator/Makefile b/drivers/platform/surface/aggregator/Makefile
> index b8b24c8ec310..c112e2c7112b 100644
> --- a/drivers/platform/surface/aggregator/Makefile
> +++ b/drivers/platform/surface/aggregator/Makefile
> @@ -11,3 +11,7 @@ surface_aggregator-objs += ssh_parser.o
>  surface_aggregator-objs += ssh_packet_layer.o
>  surface_aggregator-objs += ssh_request_layer.o
>  surface_aggregator-objs += controller.o
> +
> +ifeq ($(CONFIG_SURFACE_AGGREGATOR_BUS),y)
> +surface_aggregator-objs += bus.o
> +endif
> diff --git a/drivers/platform/surface/aggregator/bus.c b/drivers/platform/surface/aggregator/bus.c
> new file mode 100644
> index 000000000000..9f7b4f91a87e
> --- /dev/null
> +++ b/drivers/platform/surface/aggregator/bus.c
> @@ -0,0 +1,415 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Surface System Aggregator Module bus and device integration.
> + *
> + * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/surface_aggregator/controller.h>
> +#include <linux/surface_aggregator/device.h>
> +
> +#include "bus.h"
> +#include "controller.h"
> +
> +static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	return sysfs_emit(buf, "ssam:d%02Xc%02Xt%02Xi%02Xf%02X\n",
> +			sdev->uid.domain, sdev->uid.category, sdev->uid.target,
> +			sdev->uid.instance, sdev->uid.function);
> +}
> +static DEVICE_ATTR_RO(modalias);
> +
> +static struct attribute *ssam_device_attrs[] = {
> +	&dev_attr_modalias.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ssam_device);
> +
> +static int ssam_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	return add_uevent_var(env, "MODALIAS=ssam:d%02Xc%02Xt%02Xi%02Xf%02X",
> +			      sdev->uid.domain, sdev->uid.category,
> +			      sdev->uid.target, sdev->uid.instance,
> +			      sdev->uid.function);
> +}
> +
> +static void ssam_device_release(struct device *dev)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	ssam_controller_put(sdev->ctrl);
> +	kfree(sdev);
> +}
> +
> +const struct device_type ssam_device_type = {
> +	.name    = "surface_aggregator_device",
> +	.groups  = ssam_device_groups,
> +	.uevent  = ssam_device_uevent,
> +	.release = ssam_device_release,
> +};
> +EXPORT_SYMBOL_GPL(ssam_device_type);
> +
> +/**
> + * ssam_device_alloc() - Allocate and initialize a SSAM client device.
> + * @ctrl: The controller under which the device should be added.
> + * @uid:  The UID of the device to be added.
> + *
> + * Allocates and initializes a new client device. The parent of the device
> + * will be set to the controller device and the name will be set based on the
> + * UID. Note that the device still has to be added via ssam_device_add().
> + * Refer to that function for more details.
> + *
> + * Return: Returns the newly allocated and initialized SSAM client device, or
> + * %NULL if it could not be allocated.
> + */
> +struct ssam_device *ssam_device_alloc(struct ssam_controller *ctrl,
> +				      struct ssam_device_uid uid)
> +{
> +	struct ssam_device *sdev;
> +
> +	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
> +	if (!sdev)
> +		return NULL;
> +
> +	device_initialize(&sdev->dev);
> +	sdev->dev.bus = &ssam_bus_type;
> +	sdev->dev.type = &ssam_device_type;
> +	sdev->dev.parent = ssam_controller_device(ctrl);
> +	sdev->ctrl = ssam_controller_get(ctrl);
> +	sdev->uid = uid;
> +
> +	dev_set_name(&sdev->dev, "%02x:%02x:%02x:%02x:%02x",
> +		     sdev->uid.domain, sdev->uid.category, sdev->uid.target,
> +		     sdev->uid.instance, sdev->uid.function);
> +
> +	return sdev;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_alloc);
> +
> +/**
> + * ssam_device_add() - Add a SSAM client device.
> + * @sdev: The SSAM client device to be added.
> + *
> + * Added client devices must be guaranteed to always have a valid and active
> + * controller. Thus, this function will fail with %-ENODEV if the controller
> + * of the device has not been initialized yet, has been suspended, or has been
> + * shut down.
> + *
> + * The caller of this function should ensure that the corresponding call to
> + * ssam_device_remove() is issued before the controller is shut down. If the
> + * added device is a direct child of the controller device (default), it will
> + * be automatically removed when the controller is shut down.
> + *
> + * By default, the controller device will become the parent of the newly
> + * created client device. The parent may be changed before ssam_device_add is
> + * called, but care must be taken that a) the correct suspend/resume ordering
> + * is guaranteed and b) the client device does not outlive the controller,
> + * i.e. that the device is removed before the controller is being shut down.
> + * In case these guarantees have to be manually enforced, please refer to the
> + * ssam_client_link() and ssam_client_bind() functions, which are intended to
> + * set up device-links for this purpose.
> + *
> + * Return: Returns zero on success, a negative error code on failure.
> + */
> +int ssam_device_add(struct ssam_device *sdev)
> +{
> +	int status;
> +
> +	/*
> +	 * Ensure that we can only add new devices to a controller if it has
> +	 * been started and is not going away soon. This works in combination
> +	 * with ssam_controller_remove_clients to ensure driver presence for the
> +	 * controller device, i.e. it ensures that the controller (sdev->ctrl)
> +	 * is always valid and can be used for requests as long as the client
> +	 * device we add here is registered as child under it. This essentially
> +	 * guarantees that the client driver can always expect the preconditions
> +	 * for functions like ssam_request_sync (controller has to be started
> +	 * and is not suspended) to hold and thus does not have to check for
> +	 * them.
> +	 *
> +	 * Note that for this to work, the controller has to be a parent device.
> +	 * If it is not a direct parent, care has to be taken that the device is
> +	 * removed via ssam_device_remove(), as device_unregister does not
> +	 * remove child devices recursively.
> +	 */
> +	ssam_controller_statelock(sdev->ctrl);
> +
> +	if (sdev->ctrl->state != SSAM_CONTROLLER_STARTED) {
> +		ssam_controller_stateunlock(sdev->ctrl);
> +		return -ENODEV;
> +	}
> +
> +	status = device_add(&sdev->dev);
> +
> +	ssam_controller_stateunlock(sdev->ctrl);
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_add);
> +
> +/**
> + * ssam_device_remove() - Remove a SSAM client device.
> + * @sdev: The device to remove.
> + *
> + * Removes and unregisters the provided SSAM client device.
> + */
> +void ssam_device_remove(struct ssam_device *sdev)
> +{
> +	device_unregister(&sdev->dev);
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_remove);
> +
> +/**
> + * ssam_device_id_compatible() - Check if a device ID matches a UID.
> + * @id:  The device ID as potential match.
> + * @uid: The device UID matching against.
> + *
> + * Check if the given ID is a match for the given UID, i.e. if a device with
> + * the provided UID is compatible to the given ID following the match rules
> + * described in its &ssam_device_id.match_flags member.
> + *
> + * Return: Returns %true iff the given UID is compatible to the match rule
> + * described by the given ID, %false otherwise.
> + */
> +static bool ssam_device_id_compatible(const struct ssam_device_id *id,
> +				      struct ssam_device_uid uid)
> +{
> +	if (id->domain != uid.domain || id->category != uid.category)
> +		return false;
> +
> +	if ((id->match_flags & SSAM_MATCH_TARGET) && id->target != uid.target)
> +		return false;
> +
> +	if ((id->match_flags & SSAM_MATCH_INSTANCE) && id->instance != uid.instance)
> +		return false;
> +
> +	if ((id->match_flags & SSAM_MATCH_FUNCTION) && id->function != uid.function)
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * ssam_device_id_is_null() - Check if a device ID is null.
> + * @id: The device ID to check.
> + *
> + * Check if a given device ID is null, i.e. all zeros. Used to check for the
> + * end of ``MODULE_DEVICE_TABLE(ssam, ...)`` or similar lists.
> + *
> + * Return: Returns %true if the given ID represents a null ID, %false
> + * otherwise.
> + */
> +static bool ssam_device_id_is_null(const struct ssam_device_id *id)
> +{
> +	return id->match_flags == 0 &&
> +		id->domain == 0 &&
> +		id->category == 0 &&
> +		id->target == 0 &&
> +		id->instance == 0 &&
> +		id->function == 0 &&
> +		id->driver_data == 0;
> +}
> +
> +/**
> + * ssam_device_id_match() - Find the matching ID table entry for the given UID.
> + * @table: The table to search in.
> + * @uid:   The UID to matched against the individual table entries.
> + *
> + * Find the first match for the provided device UID in the provided ID table
> + * and return it. Returns %NULL if no match could be found.
> + */
> +const struct ssam_device_id *ssam_device_id_match(const struct ssam_device_id *table,
> +						  const struct ssam_device_uid uid)
> +{
> +	const struct ssam_device_id *id;
> +
> +	for (id = table; !ssam_device_id_is_null(id); ++id)
> +		if (ssam_device_id_compatible(id, uid))
> +			return id;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_id_match);
> +
> +/**
> + * ssam_device_get_match() - Find and return the ID matching the device in the
> + * ID table of the bound driver.
> + * @dev: The device for which to get the matching ID table entry.
> + *
> + * Find the fist match for the UID of the device in the ID table of the
> + * currently bound driver and return it. Returns %NULL if the device does not
> + * have a driver bound to it, the driver does not have match_table (i.e. it is
> + * %NULL), or there is no match in the driver's match_table.
> + *
> + * This function essentially calls ssam_device_id_match() with the ID table of
> + * the bound device driver and the UID of the device.
> + *
> + * Return: Returns the first match for the UID of the device in the device
> + * driver's match table, or %NULL if no such match could be found.
> + */
> +const struct ssam_device_id *ssam_device_get_match(const struct ssam_device *dev)
> +{
> +	const struct ssam_device_driver *sdrv;
> +
> +	sdrv = to_ssam_device_driver(dev->dev.driver);
> +	if (!sdrv)
> +		return NULL;
> +
> +	if (!sdrv->match_table)
> +		return NULL;
> +
> +	return ssam_device_id_match(sdrv->match_table, dev->uid);
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_get_match);
> +
> +/**
> + * ssam_device_get_match_data() - Find the ID matching the device in the
> + * ID table of the bound driver and return its ``driver_data`` member.
> + * @dev: The device for which to get the match data.
> + *
> + * Find the fist match for the UID of the device in the ID table of the
> + * corresponding driver and return its driver_data. Returns %NULL if the
> + * device does not have a driver bound to it, the driver does not have
> + * match_table (i.e. it is %NULL), there is no match in the driver's
> + * match_table, or the match does not have any driver_data.
> + *
> + * This function essentially calls ssam_device_get_match() and, if any match
> + * could be found, returns its ``struct ssam_device_id.driver_data`` member.
> + *
> + * Return: Returns the driver data associated with the first match for the UID
> + * of the device in the device driver's match table, or %NULL if no such match
> + * could be found.
> + */
> +const void *ssam_device_get_match_data(const struct ssam_device *dev)
> +{
> +	const struct ssam_device_id *id;
> +
> +	id = ssam_device_get_match(dev);
> +	if (!id)
> +		return NULL;
> +
> +	return (const void *)id->driver_data;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_get_match_data);
> +
> +static int ssam_bus_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct ssam_device_driver *sdrv = to_ssam_device_driver(drv);
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	if (!is_ssam_device(dev))
> +		return 0;
> +
> +	return !!ssam_device_id_match(sdrv->match_table, sdev->uid);
> +}
> +
> +static int ssam_bus_probe(struct device *dev)
> +{
> +	return to_ssam_device_driver(dev->driver)
> +		->probe(to_ssam_device(dev));
> +}
> +
> +static int ssam_bus_remove(struct device *dev)
> +{
> +	struct ssam_device_driver *sdrv = to_ssam_device_driver(dev->driver);
> +
> +	if (sdrv->remove)
> +		sdrv->remove(to_ssam_device(dev));
> +
> +	return 0;
> +}
> +
> +struct bus_type ssam_bus_type = {
> +	.name   = "surface_aggregator",
> +	.match  = ssam_bus_match,
> +	.probe  = ssam_bus_probe,
> +	.remove = ssam_bus_remove,
> +};
> +EXPORT_SYMBOL_GPL(ssam_bus_type);
> +
> +/**
> + * __ssam_device_driver_register() - Register a SSAM client device driver.
> + * @sdrv:  The driver to register.
> + * @owner: The module owning the provided driver.
> + *
> + * Please refer to the ssam_device_driver_register() macro for the normal way
> + * to register a driver from inside its owning module.
> + */
> +int __ssam_device_driver_register(struct ssam_device_driver *sdrv,
> +				  struct module *owner)
> +{
> +	sdrv->driver.owner = owner;
> +	sdrv->driver.bus = &ssam_bus_type;
> +
> +	/* force drivers to async probe so I/O is possible in probe */
> +	sdrv->driver.probe_type = PROBE_PREFER_ASYNCHRONOUS;
> +
> +	return driver_register(&sdrv->driver);
> +}
> +EXPORT_SYMBOL_GPL(__ssam_device_driver_register);
> +
> +/**
> + * ssam_device_driver_unregister - Unregister a SSAM device driver.
> + * @sdrv: The driver to unregister.
> + */
> +void ssam_device_driver_unregister(struct ssam_device_driver *sdrv)
> +{
> +	driver_unregister(&sdrv->driver);
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_driver_unregister);
> +
> +static int ssam_remove_device(struct device *dev, void *_data)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	if (is_ssam_device(dev))
> +		ssam_device_remove(sdev);
> +
> +	return 0;
> +}
> +
> +/**
> + * ssam_controller_remove_clients() - Remove SSAM client devices registered as
> + * direct children under the given controller.
> + * @ctrl: The controller to remove all direct clients for.
> + *
> + * Remove all SSAM client devices registered as direct children under the
> + * given controller. Note that this only accounts for direct children of the
> + * controller device. This does not take care of any client devices where the
> + * parent device has been manually set before calling ssam_device_add. Refer
> + * to ssam_device_add()/ssam_device_remove() for more details on those cases.
> + *
> + * To avoid new devices being added in parallel to this call, the main
> + * controller lock (not statelock) must be held during this (and if
> + * necessary, any subsequent deinitialization) call.
> + */
> +void ssam_controller_remove_clients(struct ssam_controller *ctrl)
> +{
> +	struct device *dev;
> +
> +	dev = ssam_controller_device(ctrl);
> +	device_for_each_child_reverse(dev, NULL, ssam_remove_device);
> +}
> +
> +/**
> + * ssam_bus_register() - Register and set-up the SSAM client device bus.
> + */
> +int ssam_bus_register(void)
> +{
> +	return bus_register(&ssam_bus_type);
> +}
> +
> +/**
> + * ssam_bus_unregister() - Unregister the SSAM client device bus.
> + */
> +void ssam_bus_unregister(void)
> +{
> +	return bus_unregister(&ssam_bus_type);
> +}
> diff --git a/drivers/platform/surface/aggregator/bus.h b/drivers/platform/surface/aggregator/bus.h
> new file mode 100644
> index 000000000000..7712baaed6a5
> --- /dev/null
> +++ b/drivers/platform/surface/aggregator/bus.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Surface System Aggregator Module bus and device integration.
> + *
> + * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#ifndef _SURFACE_AGGREGATOR_BUS_H
> +#define _SURFACE_AGGREGATOR_BUS_H
> +
> +#include <linux/surface_aggregator/controller.h>
> +
> +#ifdef CONFIG_SURFACE_AGGREGATOR_BUS
> +
> +void ssam_controller_remove_clients(struct ssam_controller *ctrl);
> +
> +int ssam_bus_register(void);
> +void ssam_bus_unregister(void);
> +
> +#else /* CONFIG_SURFACE_AGGREGATOR_BUS */
> +
> +static inline void ssam_controller_remove_clients(struct ssam_controller *ctrl) {}
> +static inline int ssam_bus_register(void) { return 0; }
> +static inline void ssam_bus_unregister(void) {}
> +
> +#endif /* CONFIG_SURFACE_AGGREGATOR_BUS */
> +#endif /* _SURFACE_AGGREGATOR_BUS_H */
> diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
> index b5d44ab61f06..cde66c497d3e 100644
> --- a/drivers/platform/surface/aggregator/core.c
> +++ b/drivers/platform/surface/aggregator/core.c
> @@ -22,6 +22,8 @@
>  #include <linux/sysfs.h>
> 
>  #include <linux/surface_aggregator/controller.h>
> +
> +#include "bus.h"
>  #include "controller.h"
> 
>  #define CREATE_TRACE_POINTS
> @@ -733,6 +735,9 @@ static void ssam_serial_hub_remove(struct serdev_device *serdev)
>  	sysfs_remove_group(&serdev->dev.kobj, &ssam_sam_group);
>  	ssam_controller_lock(ctrl);
> 
> +	/* Remove all client devices. */
> +	ssam_controller_remove_clients(ctrl);
> +
>  	/* Act as if suspending to silence events. */
>  	status = ssam_ctrl_notif_display_off(ctrl);
>  	if (status) {
> @@ -785,6 +790,10 @@ static int __init ssam_core_init(void)
>  {
>  	int status;
> 
> +	status = ssam_bus_register();
> +	if (status)
> +		goto err_bus;
> +
>  	status = ssh_ctrl_packet_cache_init();
>  	if (status)
>  		goto err_cpkg;
> @@ -804,6 +813,8 @@ static int __init ssam_core_init(void)
>  err_evitem:
>  	ssh_ctrl_packet_cache_destroy();
>  err_cpkg:
> +	ssam_bus_unregister();
> +err_bus:
>  	return status;
>  }
>  module_init(ssam_core_init);
> @@ -813,6 +824,7 @@ static void __exit ssam_core_exit(void)
>  	serdev_device_driver_unregister(&ssam_serial_hub);
>  	ssam_event_item_cache_destroy();
>  	ssh_ctrl_packet_cache_destroy();
> +	ssam_bus_unregister();
>  }
>  module_exit(ssam_core_exit);
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5b08a473cdba..0b8f1feefe0e 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -838,4 +838,22 @@ struct mhi_device_id {
>  	kernel_ulong_t driver_data;
>  };
> 
> +/* Surface System Aggregator Module */
> +
> +#define SSAM_MATCH_TARGET	0x1
> +#define SSAM_MATCH_INSTANCE	0x2
> +#define SSAM_MATCH_FUNCTION	0x4
> +
> +struct ssam_device_id {
> +	__u8 match_flags;
> +
> +	__u8 domain;
> +	__u8 category;
> +	__u8 target;
> +	__u8 instance;
> +	__u8 function;
> +
> +	kernel_ulong_t driver_data;
> +};
> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */
> diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
> new file mode 100644
> index 000000000000..7221d4a9c1c1
> --- /dev/null
> +++ b/include/linux/surface_aggregator/device.h
> @@ -0,0 +1,423 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Surface System Aggregator Module (SSAM) bus and client-device subsystem.
> + *
> + * Main interface for the surface-aggregator bus, surface-aggregator client
> + * devices, and respective drivers building on top of the SSAM controller.
> + * Provides support for non-platform/non-ACPI SSAM clients via dedicated
> + * subsystem.
> + *
> + * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#ifndef _LINUX_SURFACE_AGGREGATOR_DEVICE_H
> +#define _LINUX_SURFACE_AGGREGATOR_DEVICE_H
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +
> +#include <linux/surface_aggregator/controller.h>
> +
> +
> +/* -- Surface System Aggregator Module bus. --------------------------------- */
> +
> +/**
> + * enum ssam_device_domain - SAM device domain.
> + * @SSAM_DOMAIN_VIRTUAL:   Virtual device.
> + * @SSAM_DOMAIN_SERIALHUB: Physical device connected via Surface Serial Hub.
> + */
> +enum ssam_device_domain {
> +	SSAM_DOMAIN_VIRTUAL   = 0x00,
> +	SSAM_DOMAIN_SERIALHUB = 0x01,
> +};
> +
> +/**
> + * enum ssam_virtual_tc - Target categories for the virtual SAM domain.
> + * @SSAM_VIRTUAL_TC_HUB: Device hub category.
> + */
> +enum ssam_virtual_tc {
> +	SSAM_VIRTUAL_TC_HUB = 0x00,
> +};
> +
> +/**
> + * struct ssam_device_uid - Unique identifier for SSAM device.
> + * @domain:   Domain of the device.
> + * @category: Target category of the device.
> + * @target:   Target ID of the device.
> + * @instance: Instance ID of the device.
> + * @function: Sub-function of the device. This field can be used to split a
> + *            single SAM device into multiple virtual subdevices to separate
> + *            different functionality of that device and allow one driver per
> + *            such functionality.
> + */
> +struct ssam_device_uid {
> +	u8 domain;
> +	u8 category;
> +	u8 target;
> +	u8 instance;
> +	u8 function;
> +};
> +
> +/*
> + * Special values for device matching.
> + *
> + * These values are intended to be used with SSAM_DEVICE(), SSAM_VDEV(), and
> + * SSAM_SDEV() exclusively. Specifically, they are used to initialize the
> + * match_flags member of the device ID structure. Do not use them directly
> + * with struct ssam_device_id or struct ssam_device_uid.
> + */
> +#define SSAM_ANY_TID		0xffff
> +#define SSAM_ANY_IID		0xffff
> +#define SSAM_ANY_FUN		0xffff
> +
> +/**
> + * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
> + * parameters.
> + * @d:   Domain of the device.
> + * @cat: Target category of the device.
> + * @tid: Target ID of the device.
> + * @iid: Instance ID of the device.
> + * @fun: Sub-function of the device.
> + *
> + * Initializes a &struct ssam_device_id with the given parameters. See &struct
> + * ssam_device_uid for details regarding the parameters. The special values
> + * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify that
> + * matching should ignore target ID, instance ID, and/or sub-function,
> + * respectively. This macro initializes the ``match_flags`` field based on the
> + * given parameters.
> + *
> + * Note: The parameters @d and @cat must be valid &u8 values, the parameters
> + * @tid, @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> + * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * allowed.
> + */
> +#define SSAM_DEVICE(d, cat, tid, iid, fun)					\
> +	.match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0)	\
> +		     | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0)	\
> +		     | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),	\
> +	.domain   = d,								\
> +	.category = cat,							\
> +	.target   = ((tid) != SSAM_ANY_TID) ? (tid) : 0,			\
> +	.instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0,			\
> +	.function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0				\
> +
> +/**
> + * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with
> + * the given parameters.
> + * @cat: Target category of the device.
> + * @tid: Target ID of the device.
> + * @iid: Instance ID of the device.
> + * @fun: Sub-function of the device.
> + *
> + * Initializes a &struct ssam_device_id with the given parameters in the
> + * virtual domain. See &struct ssam_device_uid for details regarding the
> + * parameters. The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and
> + * %SSAM_ANY_FUN can be used to specify that matching should ignore target ID,
> + * instance ID, and/or sub-function, respectively. This macro initializes the
> + * ``match_flags`` field based on the given parameters.
> + *
> + * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
> + * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> + * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * allowed.
> + */
> +#define SSAM_VDEV(cat, tid, iid, fun) \
> +	SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun)
> +
> +/**
> + * SSAM_SDEV() - Initialize a &struct ssam_device_id as physical SSH device
> + * with the given parameters.
> + * @cat: Target category of the device.
> + * @tid: Target ID of the device.
> + * @iid: Instance ID of the device.
> + * @fun: Sub-function of the device.
> + *
> + * Initializes a &struct ssam_device_id with the given parameters in the SSH
> + * domain. See &struct ssam_device_uid for details regarding the parameters.
> + * The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be
> + * used to specify that matching should ignore target ID, instance ID, and/or
> + * sub-function, respectively. This macro initializes the ``match_flags``
> + * field based on the given parameters.
> + *
> + * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
> + * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> + * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * allowed.
> + */
> +#define SSAM_SDEV(cat, tid, iid, fun) \
> +	SSAM_DEVICE(SSAM_DOMAIN_SERIALHUB, SSAM_SSH_TC_##cat, tid, iid, fun)
> +
> +/**
> + * struct ssam_device - SSAM client device.
> + * @dev:  Driver model representation of the device.
> + * @ctrl: SSAM controller managing this device.
> + * @uid:  UID identifying the device.
> + */
> +struct ssam_device {
> +	struct device dev;
> +	struct ssam_controller *ctrl;
> +
> +	struct ssam_device_uid uid;
> +};
> +
> +/**
> + * struct ssam_device_driver - SSAM client device driver.
> + * @driver:      Base driver model structure.
> + * @match_table: Match table specifying which devices the driver should bind to.
> + * @probe:       Called when the driver is being bound to a device.
> + * @remove:      Called when the driver is being unbound from the device.
> + */
> +struct ssam_device_driver {
> +	struct device_driver driver;
> +
> +	const struct ssam_device_id *match_table;
> +
> +	int  (*probe)(struct ssam_device *sdev);
> +	void (*remove)(struct ssam_device *sdev);
> +};
> +
> +extern struct bus_type ssam_bus_type;
> +extern const struct device_type ssam_device_type;
> +
> +/**
> + * is_ssam_device() - Check if the given device is a SSAM client device.
> + * @d: The device to test the type of.
> + *
> + * Return: Returns %true iff the specified device is of type &struct
> + * ssam_device, i.e. the device type points to %ssam_device_type, and %false
> + * otherwise.
> + */
> +static inline bool is_ssam_device(struct device *d)
> +{
> +	return d->type == &ssam_device_type;
> +}
> +
> +/**
> + * to_ssam_device() - Casts the given device to a SSAM client device.
> + * @d: The device to cast.
> + *
> + * Casts the given &struct device to a &struct ssam_device. The caller has to
> + * ensure that the given device is actually enclosed in a &struct ssam_device,
> + * e.g. by calling is_ssam_device().
> + *
> + * Return: Returns a pointer to the &struct ssam_device wrapping the given
> + * device @d.
> + */
> +static inline struct ssam_device *to_ssam_device(struct device *d)
> +{
> +	return container_of(d, struct ssam_device, dev);
> +}
> +
> +/**
> + * to_ssam_device_driver() - Casts the given device driver to a SSAM client
> + * device driver.
> + * @d: The driver to cast.
> + *
> + * Casts the given &struct device_driver to a &struct ssam_device_driver. The
> + * caller has to ensure that the given driver is actually enclosed in a
> + * &struct ssam_device_driver.
> + *
> + * Return: Returns the pointer to the &struct ssam_device_driver wrapping the
> + * given device driver @d.
> + */
> +static inline
> +struct ssam_device_driver *to_ssam_device_driver(struct device_driver *d)
> +{
> +	return container_of(d, struct ssam_device_driver, driver);
> +}
> +
> +const struct ssam_device_id *ssam_device_id_match(const struct ssam_device_id *table,
> +						  const struct ssam_device_uid uid);
> +
> +const struct ssam_device_id *ssam_device_get_match(const struct ssam_device *dev);
> +
> +const void *ssam_device_get_match_data(const struct ssam_device *dev);
> +
> +struct ssam_device *ssam_device_alloc(struct ssam_controller *ctrl,
> +				      struct ssam_device_uid uid);
> +
> +int ssam_device_add(struct ssam_device *sdev);
> +void ssam_device_remove(struct ssam_device *sdev);
> +
> +/**
> + * ssam_device_get() - Increment reference count of SSAM client device.
> + * @sdev: The device to increment the reference count of.
> + *
> + * Increments the reference count of the given SSAM client device by
> + * incrementing the reference count of the enclosed &struct device via
> + * get_device().
> + *
> + * See ssam_device_put() for the counter-part of this function.
> + *
> + * Return: Returns the device provided as input.
> + */
> +static inline struct ssam_device *ssam_device_get(struct ssam_device *sdev)
> +{
> +	return sdev ? to_ssam_device(get_device(&sdev->dev)) : NULL;
> +}
> +
> +/**
> + * ssam_device_put() - Decrement reference count of SSAM client device.
> + * @sdev: The device to decrement the reference count of.
> + *
> + * Decrements the reference count of the given SSAM client device by
> + * decrementing the reference count of the enclosed &struct device via
> + * put_device().
> + *
> + * See ssam_device_get() for the counter-part of this function.
> + */
> +static inline void ssam_device_put(struct ssam_device *sdev)
> +{
> +	if (sdev)
> +		put_device(&sdev->dev);
> +}
> +
> +/**
> + * ssam_device_get_drvdata() - Get driver-data of SSAM client device.
> + * @sdev: The device to get the driver-data from.
> + *
> + * Return: Returns the driver-data of the given device, previously set via
> + * ssam_device_set_drvdata().
> + */
> +static inline void *ssam_device_get_drvdata(struct ssam_device *sdev)
> +{
> +	return dev_get_drvdata(&sdev->dev);
> +}
> +
> +/**
> + * ssam_device_set_drvdata() - Set driver-data of SSAM client device.
> + * @sdev: The device to set the driver-data of.
> + * @data: The data to set the device's driver-data pointer to.
> + */
> +static inline void ssam_device_set_drvdata(struct ssam_device *sdev, void *data)
> +{
> +	dev_set_drvdata(&sdev->dev, data);
> +}
> +
> +int __ssam_device_driver_register(struct ssam_device_driver *d, struct module *o);
> +void ssam_device_driver_unregister(struct ssam_device_driver *d);
> +
> +/**
> + * ssam_device_driver_register() - Register a SSAM client device driver.
> + * @drv: The driver to register.
> + */
> +#define ssam_device_driver_register(drv) \
> +	__ssam_device_driver_register(drv, THIS_MODULE)
> +
> +/**
> + * module_ssam_device_driver() - Helper macro for SSAM device driver
> + * registration.
> + * @drv: The driver managed by this module.
> + *
> + * Helper macro to register a SSAM device driver via module_init() and
> + * module_exit(). This macro may only be used once per module and replaces the
> + * aforementioned definitions.
> + */
> +#define module_ssam_device_driver(drv)			\
> +	module_driver(drv, ssam_device_driver_register,	\
> +		      ssam_device_driver_unregister)
> +
> +
> +/* -- Helpers for client-device requests. ----------------------------------- */
> +
> +/**
> + * SSAM_DEFINE_SYNC_REQUEST_CL_N() - Define synchronous client-device SAM
> + * request function with neither argument nor return value.
> + * @name: Name of the generated function.
> + * @spec: Specification (&struct ssam_request_spec_md) defining the request.
> + *
> + * Defines a function executing the synchronous SAM request specified by
> + * @spec, with the request having neither argument nor return value. Device
> + * specifying parameters are not hard-coded, but instead are provided via the
> + * client device, specifically its UID, supplied when calling this function.
> + * The generated function takes care of setting up the request struct, buffer
> + * allocation, as well as execution of the request itself, returning once the
> + * request has been fully completed. The required transport buffer will be
> + * allocated on the stack.
> + *
> + * The generated function is defined as ``int name(struct ssam_device *sdev)``,
> + * returning the status of the request, which is zero on success and negative
> + * on failure. The ``sdev`` parameter specifies both the target device of the
> + * request and by association the controller via which the request is sent.
> + *
> + * Refer to ssam_request_sync_onstack() for more details on the behavior of
> + * the generated function.
> + */
> +#define SSAM_DEFINE_SYNC_REQUEST_CL_N(name, spec...)			\
> +	SSAM_DEFINE_SYNC_REQUEST_MD_N(__raw_##name, spec)		\
> +	int name(struct ssam_device *sdev)				\
> +	{								\
> +		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
> +				    sdev->uid.instance);		\
> +	}
> +
> +/**
> + * SSAM_DEFINE_SYNC_REQUEST_CL_W() - Define synchronous client-device SAM
> + * request function with argument.
> + * @name:  Name of the generated function.
> + * @atype: Type of the request's argument.
> + * @spec:  Specification (&struct ssam_request_spec_md) defining the request.
> + *
> + * Defines a function executing the synchronous SAM request specified by
> + * @spec, with the request taking an argument of type @atype and having no
> + * return value. Device specifying parameters are not hard-coded, but instead
> + * are provided via the client device, specifically its UID, supplied when
> + * calling this function. The generated function takes care of setting up the
> + * request struct, buffer allocation, as well as execution of the request
> + * itself, returning once the request has been fully completed. The required
> + * transport buffer will be allocated on the stack.
> + *
> + * The generated function is defined as ``int name(struct ssam_device *sdev,
> + * const atype *arg)``, returning the status of the request, which is zero on
> + * success and negative on failure. The ``sdev`` parameter specifies both the
> + * target device of the request and by association the controller via which
> + * the request is sent. The request's argument is specified via the ``arg``
> + * pointer.
> + *
> + * Refer to ssam_request_sync_onstack() for more details on the behavior of
> + * the generated function.
> + */
> +#define SSAM_DEFINE_SYNC_REQUEST_CL_W(name, atype, spec...)		\
> +	SSAM_DEFINE_SYNC_REQUEST_MD_W(__raw_##name, atype, spec)	\
> +	int name(struct ssam_device *sdev, const atype *arg)		\
> +	{								\
> +		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
> +				    sdev->uid.instance, arg);		\
> +	}
> +
> +/**
> + * SSAM_DEFINE_SYNC_REQUEST_CL_R() - Define synchronous client-device SAM
> + * request function with return value.
> + * @name:  Name of the generated function.
> + * @rtype: Type of the request's return value.
> + * @spec:  Specification (&struct ssam_request_spec_md) defining the request.
> + *
> + * Defines a function executing the synchronous SAM request specified by
> + * @spec, with the request taking no argument but having a return value of
> + * type @rtype. Device specifying parameters are not hard-coded, but instead
> + * are provided via the client device, specifically its UID, supplied when
> + * calling this function. The generated function takes care of setting up the
> + * request struct, buffer allocation, as well as execution of the request
> + * itself, returning once the request has been fully completed. The required
> + * transport buffer will be allocated on the stack.
> + *
> + * The generated function is defined as ``int name(struct ssam_device *sdev,
> + * rtype *ret)``, returning the status of the request, which is zero on
> + * success and negative on failure. The ``sdev`` parameter specifies both the
> + * target device of the request and by association the controller via which
> + * the request is sent. The request's return value is written to the memory
> + * pointed to by the ``ret`` parameter.
> + *
> + * Refer to ssam_request_sync_onstack() for more details on the behavior of
> + * the generated function.
> + */
> +#define SSAM_DEFINE_SYNC_REQUEST_CL_R(name, rtype, spec...)		\
> +	SSAM_DEFINE_SYNC_REQUEST_MD_R(__raw_##name, rtype, spec)	\
> +	int name(struct ssam_device *sdev, rtype *ret)			\
> +	{								\
> +		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
> +				    sdev->uid.instance, ret);		\
> +	}
> +
> +#endif /* _LINUX_SURFACE_AGGREGATOR_DEVICE_H */
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index 27007c18e754..4339377ad929 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -243,5 +243,13 @@ int main(void)
>  	DEVID(mhi_device_id);
>  	DEVID_FIELD(mhi_device_id, chan);
> 
> +	DEVID(ssam_device_id);
> +	DEVID_FIELD(ssam_device_id, match_flags);
> +	DEVID_FIELD(ssam_device_id, domain);
> +	DEVID_FIELD(ssam_device_id, category);
> +	DEVID_FIELD(ssam_device_id, target);
> +	DEVID_FIELD(ssam_device_id, instance);
> +	DEVID_FIELD(ssam_device_id, function);
> +
>  	return 0;
>  }
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 2417dd1dee33..5b79fdc42641 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1368,6 +1368,28 @@ static int do_mhi_entry(const char *filename, void *symval, char *alias)
>  	return 1;
>  }
> 
> +/*
> + * Looks like: ssam:dNcNtNiNfN
> + *
> + * N is exactly 2 digits, where each is an upper-case hex digit.
> + */
> +static int do_ssam_entry(const char *filename, void *symval, char *alias)
> +{
> +	DEF_FIELD(symval, ssam_device_id, match_flags);
> +	DEF_FIELD(symval, ssam_device_id, domain);
> +	DEF_FIELD(symval, ssam_device_id, category);
> +	DEF_FIELD(symval, ssam_device_id, target);
> +	DEF_FIELD(symval, ssam_device_id, instance);
> +	DEF_FIELD(symval, ssam_device_id, function);
> +
> +	sprintf(alias, "ssam:d%02Xc%02X", domain, category);
> +	ADD(alias, "t", match_flags & SSAM_MATCH_TARGET, target);
> +	ADD(alias, "i", match_flags & SSAM_MATCH_INSTANCE, instance);
> +	ADD(alias, "f", match_flags & SSAM_MATCH_FUNCTION, function);
> +
> +	return 1;
> +}
> +
>  /* Does namelen bytes of name exactly match the symbol? */
>  static bool sym_is(const char *name, unsigned namelen, const char *symbol)
>  {
> @@ -1442,6 +1464,7 @@ static const struct devtable devtable[] = {
>  	{"tee", SIZE_tee_client_device_id, do_tee_entry},
>  	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
>  	{"mhi", SIZE_mhi_device_id, do_mhi_entry},
> +	{"ssam", SIZE_ssam_device_id, do_ssam_entry},
>  };
> 
>  /* Create MODULE_ALIAS() statements.
> --
> 2.29.2
> 

Re: [PATCH v2 6/9] platform/surface: aggregator: Add dedicated bus and device type

[Hans de Goede]

Hi,

On 12/3/20 10:26 PM, Maximilian Luz wrote:
> The Surface Aggregator EC provides varying functionality, depending on
> the Surface device. To manage this functionality, we use dedicated
> client devices for each subsystem or virtual device of the EC. While
> some of these clients are described as standard devices in ACPI and the
> corresponding client drivers can be implemented as platform drivers in
> the kernel (making use of the controller API already present), many
> devices, especially on newer Surface models, cannot be found there.
> 
> To simplify management of these devices, we introduce a new bus and
> client device type for the Surface Aggregator subsystem. The new device
> type takes care of managing the controller reference, essentially
> guaranteeing its validity for as long as the client device exists, thus
> alleviating the need to manually establish device links for that purpose
> in the client driver (as has to be done with the platform devices).
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
> 
> Changes in v1 (from RFC):
>  - add copyright lines
>  - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
>  - remove unnecessary READ_ONCE on multiple occasions
>  - improve documentation of special values for SSAM_DEVICE()
>  - add NULL checks for ssam_device_get, ssam_device_put
> 
> Changes in v2:
>  - return ENODEV instead of ENXIO if controller is not present
>  - use sysfs_emit for sysfs attributes
>  - spell check comments and strings, fix typos
>  - unify comment style
>  - run checkpatch --strict, fix warnings and style issues
> 
> ---
>  drivers/platform/surface/aggregator/Kconfig  |  12 +
>  drivers/platform/surface/aggregator/Makefile |   4 +
>  drivers/platform/surface/aggregator/bus.c    | 415 ++++++++++++++++++
>  drivers/platform/surface/aggregator/bus.h    |  27 ++
>  drivers/platform/surface/aggregator/core.c   |  12 +
>  include/linux/mod_devicetable.h              |  18 +
>  include/linux/surface_aggregator/device.h    | 423 +++++++++++++++++++
>  scripts/mod/devicetable-offsets.c            |   8 +
>  scripts/mod/file2alias.c                     |  23 +
>  9 files changed, 942 insertions(+)
>  create mode 100644 drivers/platform/surface/aggregator/bus.c
>  create mode 100644 drivers/platform/surface/aggregator/bus.h
>  create mode 100644 include/linux/surface_aggregator/device.h
> 
> diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
> index 48f40c345e29..44c2493706bc 100644
> --- a/drivers/platform/surface/aggregator/Kconfig
> +++ b/drivers/platform/surface/aggregator/Kconfig
> @@ -42,6 +42,18 @@ menuconfig SURFACE_AGGREGATOR
>  	  module, y if you want to build it into the kernel and n if you don't
>  	  want it at all.
> 
> +config SURFACE_AGGREGATOR_BUS
> +	bool "Surface System Aggregator Module Bus"
> +	depends on SURFACE_AGGREGATOR
> +	default y
> +	help
> +	  Expands the Surface System Aggregator Module (SSAM) core driver by
> +	  providing a dedicated bus and client-device type.
> +
> +	  This bus and device type are intended to provide and simplify support
> +	  for non-platform and non-ACPI SSAM devices, i.e. SSAM devices that are
> +	  not auto-detectable via the conventional means (e.g. ACPI).
> +
>  config SURFACE_AGGREGATOR_ERROR_INJECTION
>  	bool "Surface System Aggregator Module Error Injection Capabilities"
>  	depends on SURFACE_AGGREGATOR
> diff --git a/drivers/platform/surface/aggregator/Makefile b/drivers/platform/surface/aggregator/Makefile
> index b8b24c8ec310..c112e2c7112b 100644
> --- a/drivers/platform/surface/aggregator/Makefile
> +++ b/drivers/platform/surface/aggregator/Makefile
> @@ -11,3 +11,7 @@ surface_aggregator-objs += ssh_parser.o
>  surface_aggregator-objs += ssh_packet_layer.o
>  surface_aggregator-objs += ssh_request_layer.o
>  surface_aggregator-objs += controller.o
> +
> +ifeq ($(CONFIG_SURFACE_AGGREGATOR_BUS),y)
> +surface_aggregator-objs += bus.o
> +endif
> diff --git a/drivers/platform/surface/aggregator/bus.c b/drivers/platform/surface/aggregator/bus.c
> new file mode 100644
> index 000000000000..9f7b4f91a87e
> --- /dev/null
> +++ b/drivers/platform/surface/aggregator/bus.c
> @@ -0,0 +1,415 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Surface System Aggregator Module bus and device integration.
> + *
> + * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/surface_aggregator/controller.h>
> +#include <linux/surface_aggregator/device.h>
> +
> +#include "bus.h"
> +#include "controller.h"
> +
> +static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	return sysfs_emit(buf, "ssam:d%02Xc%02Xt%02Xi%02Xf%02X\n",
> +			sdev->uid.domain, sdev->uid.category, sdev->uid.target,
> +			sdev->uid.instance, sdev->uid.function);
> +}
> +static DEVICE_ATTR_RO(modalias);
> +
> +static struct attribute *ssam_device_attrs[] = {
> +	&dev_attr_modalias.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ssam_device);
> +
> +static int ssam_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	return add_uevent_var(env, "MODALIAS=ssam:d%02Xc%02Xt%02Xi%02Xf%02X",
> +			      sdev->uid.domain, sdev->uid.category,
> +			      sdev->uid.target, sdev->uid.instance,
> +			      sdev->uid.function);
> +}
> +
> +static void ssam_device_release(struct device *dev)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	ssam_controller_put(sdev->ctrl);
> +	kfree(sdev);
> +}
> +
> +const struct device_type ssam_device_type = {
> +	.name    = "surface_aggregator_device",
> +	.groups  = ssam_device_groups,
> +	.uevent  = ssam_device_uevent,
> +	.release = ssam_device_release,
> +};
> +EXPORT_SYMBOL_GPL(ssam_device_type);
> +
> +/**
> + * ssam_device_alloc() - Allocate and initialize a SSAM client device.
> + * @ctrl: The controller under which the device should be added.
> + * @uid:  The UID of the device to be added.
> + *
> + * Allocates and initializes a new client device. The parent of the device
> + * will be set to the controller device and the name will be set based on the
> + * UID. Note that the device still has to be added via ssam_device_add().
> + * Refer to that function for more details.
> + *
> + * Return: Returns the newly allocated and initialized SSAM client device, or
> + * %NULL if it could not be allocated.
> + */
> +struct ssam_device *ssam_device_alloc(struct ssam_controller *ctrl,
> +				      struct ssam_device_uid uid)
> +{
> +	struct ssam_device *sdev;
> +
> +	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
> +	if (!sdev)
> +		return NULL;
> +
> +	device_initialize(&sdev->dev);
> +	sdev->dev.bus = &ssam_bus_type;
> +	sdev->dev.type = &ssam_device_type;
> +	sdev->dev.parent = ssam_controller_device(ctrl);
> +	sdev->ctrl = ssam_controller_get(ctrl);
> +	sdev->uid = uid;
> +
> +	dev_set_name(&sdev->dev, "%02x:%02x:%02x:%02x:%02x",
> +		     sdev->uid.domain, sdev->uid.category, sdev->uid.target,
> +		     sdev->uid.instance, sdev->uid.function);
> +
> +	return sdev;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_alloc);
> +
> +/**
> + * ssam_device_add() - Add a SSAM client device.
> + * @sdev: The SSAM client device to be added.
> + *
> + * Added client devices must be guaranteed to always have a valid and active
> + * controller. Thus, this function will fail with %-ENODEV if the controller
> + * of the device has not been initialized yet, has been suspended, or has been
> + * shut down.
> + *
> + * The caller of this function should ensure that the corresponding call to
> + * ssam_device_remove() is issued before the controller is shut down. If the
> + * added device is a direct child of the controller device (default), it will
> + * be automatically removed when the controller is shut down.
> + *
> + * By default, the controller device will become the parent of the newly
> + * created client device. The parent may be changed before ssam_device_add is
> + * called, but care must be taken that a) the correct suspend/resume ordering
> + * is guaranteed and b) the client device does not outlive the controller,
> + * i.e. that the device is removed before the controller is being shut down.
> + * In case these guarantees have to be manually enforced, please refer to the
> + * ssam_client_link() and ssam_client_bind() functions, which are intended to
> + * set up device-links for this purpose.
> + *
> + * Return: Returns zero on success, a negative error code on failure.
> + */
> +int ssam_device_add(struct ssam_device *sdev)
> +{
> +	int status;
> +
> +	/*
> +	 * Ensure that we can only add new devices to a controller if it has
> +	 * been started and is not going away soon. This works in combination
> +	 * with ssam_controller_remove_clients to ensure driver presence for the
> +	 * controller device, i.e. it ensures that the controller (sdev->ctrl)
> +	 * is always valid and can be used for requests as long as the client
> +	 * device we add here is registered as child under it. This essentially
> +	 * guarantees that the client driver can always expect the preconditions
> +	 * for functions like ssam_request_sync (controller has to be started
> +	 * and is not suspended) to hold and thus does not have to check for
> +	 * them.
> +	 *
> +	 * Note that for this to work, the controller has to be a parent device.
> +	 * If it is not a direct parent, care has to be taken that the device is
> +	 * removed via ssam_device_remove(), as device_unregister does not
> +	 * remove child devices recursively.
> +	 */
> +	ssam_controller_statelock(sdev->ctrl);
> +
> +	if (sdev->ctrl->state != SSAM_CONTROLLER_STARTED) {
> +		ssam_controller_stateunlock(sdev->ctrl);
> +		return -ENODEV;
> +	}
> +
> +	status = device_add(&sdev->dev);
> +
> +	ssam_controller_stateunlock(sdev->ctrl);
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_add);
> +
> +/**
> + * ssam_device_remove() - Remove a SSAM client device.
> + * @sdev: The device to remove.
> + *
> + * Removes and unregisters the provided SSAM client device.
> + */
> +void ssam_device_remove(struct ssam_device *sdev)
> +{
> +	device_unregister(&sdev->dev);
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_remove);
> +
> +/**
> + * ssam_device_id_compatible() - Check if a device ID matches a UID.
> + * @id:  The device ID as potential match.
> + * @uid: The device UID matching against.
> + *
> + * Check if the given ID is a match for the given UID, i.e. if a device with
> + * the provided UID is compatible to the given ID following the match rules
> + * described in its &ssam_device_id.match_flags member.
> + *
> + * Return: Returns %true iff the given UID is compatible to the match rule
> + * described by the given ID, %false otherwise.
> + */
> +static bool ssam_device_id_compatible(const struct ssam_device_id *id,
> +				      struct ssam_device_uid uid)
> +{
> +	if (id->domain != uid.domain || id->category != uid.category)
> +		return false;
> +
> +	if ((id->match_flags & SSAM_MATCH_TARGET) && id->target != uid.target)
> +		return false;
> +
> +	if ((id->match_flags & SSAM_MATCH_INSTANCE) && id->instance != uid.instance)
> +		return false;
> +
> +	if ((id->match_flags & SSAM_MATCH_FUNCTION) && id->function != uid.function)
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * ssam_device_id_is_null() - Check if a device ID is null.
> + * @id: The device ID to check.
> + *
> + * Check if a given device ID is null, i.e. all zeros. Used to check for the
> + * end of ``MODULE_DEVICE_TABLE(ssam, ...)`` or similar lists.
> + *
> + * Return: Returns %true if the given ID represents a null ID, %false
> + * otherwise.
> + */
> +static bool ssam_device_id_is_null(const struct ssam_device_id *id)
> +{
> +	return id->match_flags == 0 &&
> +		id->domain == 0 &&
> +		id->category == 0 &&
> +		id->target == 0 &&
> +		id->instance == 0 &&
> +		id->function == 0 &&
> +		id->driver_data == 0;
> +}
> +
> +/**
> + * ssam_device_id_match() - Find the matching ID table entry for the given UID.
> + * @table: The table to search in.
> + * @uid:   The UID to matched against the individual table entries.
> + *
> + * Find the first match for the provided device UID in the provided ID table
> + * and return it. Returns %NULL if no match could be found.
> + */
> +const struct ssam_device_id *ssam_device_id_match(const struct ssam_device_id *table,
> +						  const struct ssam_device_uid uid)
> +{
> +	const struct ssam_device_id *id;
> +
> +	for (id = table; !ssam_device_id_is_null(id); ++id)
> +		if (ssam_device_id_compatible(id, uid))
> +			return id;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_id_match);
> +
> +/**
> + * ssam_device_get_match() - Find and return the ID matching the device in the
> + * ID table of the bound driver.
> + * @dev: The device for which to get the matching ID table entry.
> + *
> + * Find the fist match for the UID of the device in the ID table of the
> + * currently bound driver and return it. Returns %NULL if the device does not
> + * have a driver bound to it, the driver does not have match_table (i.e. it is
> + * %NULL), or there is no match in the driver's match_table.
> + *
> + * This function essentially calls ssam_device_id_match() with the ID table of
> + * the bound device driver and the UID of the device.
> + *
> + * Return: Returns the first match for the UID of the device in the device
> + * driver's match table, or %NULL if no such match could be found.
> + */
> +const struct ssam_device_id *ssam_device_get_match(const struct ssam_device *dev)
> +{
> +	const struct ssam_device_driver *sdrv;
> +
> +	sdrv = to_ssam_device_driver(dev->dev.driver);
> +	if (!sdrv)
> +		return NULL;
> +
> +	if (!sdrv->match_table)
> +		return NULL;
> +
> +	return ssam_device_id_match(sdrv->match_table, dev->uid);
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_get_match);
> +
> +/**
> + * ssam_device_get_match_data() - Find the ID matching the device in the
> + * ID table of the bound driver and return its ``driver_data`` member.
> + * @dev: The device for which to get the match data.
> + *
> + * Find the fist match for the UID of the device in the ID table of the
> + * corresponding driver and return its driver_data. Returns %NULL if the
> + * device does not have a driver bound to it, the driver does not have
> + * match_table (i.e. it is %NULL), there is no match in the driver's
> + * match_table, or the match does not have any driver_data.
> + *
> + * This function essentially calls ssam_device_get_match() and, if any match
> + * could be found, returns its ``struct ssam_device_id.driver_data`` member.
> + *
> + * Return: Returns the driver data associated with the first match for the UID
> + * of the device in the device driver's match table, or %NULL if no such match
> + * could be found.
> + */
> +const void *ssam_device_get_match_data(const struct ssam_device *dev)
> +{
> +	const struct ssam_device_id *id;
> +
> +	id = ssam_device_get_match(dev);
> +	if (!id)
> +		return NULL;
> +
> +	return (const void *)id->driver_data;
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_get_match_data);
> +
> +static int ssam_bus_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct ssam_device_driver *sdrv = to_ssam_device_driver(drv);
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	if (!is_ssam_device(dev))
> +		return 0;
> +
> +	return !!ssam_device_id_match(sdrv->match_table, sdev->uid);
> +}
> +
> +static int ssam_bus_probe(struct device *dev)
> +{
> +	return to_ssam_device_driver(dev->driver)
> +		->probe(to_ssam_device(dev));
> +}
> +
> +static int ssam_bus_remove(struct device *dev)
> +{
> +	struct ssam_device_driver *sdrv = to_ssam_device_driver(dev->driver);
> +
> +	if (sdrv->remove)
> +		sdrv->remove(to_ssam_device(dev));
> +
> +	return 0;
> +}
> +
> +struct bus_type ssam_bus_type = {
> +	.name   = "surface_aggregator",
> +	.match  = ssam_bus_match,
> +	.probe  = ssam_bus_probe,
> +	.remove = ssam_bus_remove,
> +};
> +EXPORT_SYMBOL_GPL(ssam_bus_type);
> +
> +/**
> + * __ssam_device_driver_register() - Register a SSAM client device driver.
> + * @sdrv:  The driver to register.
> + * @owner: The module owning the provided driver.
> + *
> + * Please refer to the ssam_device_driver_register() macro for the normal way
> + * to register a driver from inside its owning module.
> + */
> +int __ssam_device_driver_register(struct ssam_device_driver *sdrv,
> +				  struct module *owner)
> +{
> +	sdrv->driver.owner = owner;
> +	sdrv->driver.bus = &ssam_bus_type;
> +
> +	/* force drivers to async probe so I/O is possible in probe */
> +	sdrv->driver.probe_type = PROBE_PREFER_ASYNCHRONOUS;
> +
> +	return driver_register(&sdrv->driver);
> +}
> +EXPORT_SYMBOL_GPL(__ssam_device_driver_register);
> +
> +/**
> + * ssam_device_driver_unregister - Unregister a SSAM device driver.
> + * @sdrv: The driver to unregister.
> + */
> +void ssam_device_driver_unregister(struct ssam_device_driver *sdrv)
> +{
> +	driver_unregister(&sdrv->driver);
> +}
> +EXPORT_SYMBOL_GPL(ssam_device_driver_unregister);
> +
> +static int ssam_remove_device(struct device *dev, void *_data)
> +{
> +	struct ssam_device *sdev = to_ssam_device(dev);
> +
> +	if (is_ssam_device(dev))
> +		ssam_device_remove(sdev);
> +
> +	return 0;
> +}
> +
> +/**
> + * ssam_controller_remove_clients() - Remove SSAM client devices registered as
> + * direct children under the given controller.
> + * @ctrl: The controller to remove all direct clients for.
> + *
> + * Remove all SSAM client devices registered as direct children under the
> + * given controller. Note that this only accounts for direct children of the
> + * controller device. This does not take care of any client devices where the
> + * parent device has been manually set before calling ssam_device_add. Refer
> + * to ssam_device_add()/ssam_device_remove() for more details on those cases.
> + *
> + * To avoid new devices being added in parallel to this call, the main
> + * controller lock (not statelock) must be held during this (and if
> + * necessary, any subsequent deinitialization) call.
> + */
> +void ssam_controller_remove_clients(struct ssam_controller *ctrl)
> +{
> +	struct device *dev;
> +
> +	dev = ssam_controller_device(ctrl);
> +	device_for_each_child_reverse(dev, NULL, ssam_remove_device);
> +}
> +
> +/**
> + * ssam_bus_register() - Register and set-up the SSAM client device bus.
> + */
> +int ssam_bus_register(void)
> +{
> +	return bus_register(&ssam_bus_type);
> +}
> +
> +/**
> + * ssam_bus_unregister() - Unregister the SSAM client device bus.
> + */
> +void ssam_bus_unregister(void)
> +{
> +	return bus_unregister(&ssam_bus_type);
> +}
> diff --git a/drivers/platform/surface/aggregator/bus.h b/drivers/platform/surface/aggregator/bus.h
> new file mode 100644
> index 000000000000..7712baaed6a5
> --- /dev/null
> +++ b/drivers/platform/surface/aggregator/bus.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Surface System Aggregator Module bus and device integration.
> + *
> + * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#ifndef _SURFACE_AGGREGATOR_BUS_H
> +#define _SURFACE_AGGREGATOR_BUS_H
> +
> +#include <linux/surface_aggregator/controller.h>
> +
> +#ifdef CONFIG_SURFACE_AGGREGATOR_BUS
> +
> +void ssam_controller_remove_clients(struct ssam_controller *ctrl);
> +
> +int ssam_bus_register(void);
> +void ssam_bus_unregister(void);
> +
> +#else /* CONFIG_SURFACE_AGGREGATOR_BUS */
> +
> +static inline void ssam_controller_remove_clients(struct ssam_controller *ctrl) {}
> +static inline int ssam_bus_register(void) { return 0; }
> +static inline void ssam_bus_unregister(void) {}
> +
> +#endif /* CONFIG_SURFACE_AGGREGATOR_BUS */
> +#endif /* _SURFACE_AGGREGATOR_BUS_H */
> diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
> index b5d44ab61f06..cde66c497d3e 100644
> --- a/drivers/platform/surface/aggregator/core.c
> +++ b/drivers/platform/surface/aggregator/core.c
> @@ -22,6 +22,8 @@
>  #include <linux/sysfs.h>
> 
>  #include <linux/surface_aggregator/controller.h>
> +
> +#include "bus.h"
>  #include "controller.h"
> 
>  #define CREATE_TRACE_POINTS
> @@ -733,6 +735,9 @@ static void ssam_serial_hub_remove(struct serdev_device *serdev)
>  	sysfs_remove_group(&serdev->dev.kobj, &ssam_sam_group);
>  	ssam_controller_lock(ctrl);
> 
> +	/* Remove all client devices. */
> +	ssam_controller_remove_clients(ctrl);
> +
>  	/* Act as if suspending to silence events. */
>  	status = ssam_ctrl_notif_display_off(ctrl);
>  	if (status) {
> @@ -785,6 +790,10 @@ static int __init ssam_core_init(void)
>  {
>  	int status;
> 
> +	status = ssam_bus_register();
> +	if (status)
> +		goto err_bus;
> +
>  	status = ssh_ctrl_packet_cache_init();
>  	if (status)
>  		goto err_cpkg;
> @@ -804,6 +813,8 @@ static int __init ssam_core_init(void)
>  err_evitem:
>  	ssh_ctrl_packet_cache_destroy();
>  err_cpkg:
> +	ssam_bus_unregister();
> +err_bus:
>  	return status;
>  }
>  module_init(ssam_core_init);
> @@ -813,6 +824,7 @@ static void __exit ssam_core_exit(void)
>  	serdev_device_driver_unregister(&ssam_serial_hub);
>  	ssam_event_item_cache_destroy();
>  	ssh_ctrl_packet_cache_destroy();
> +	ssam_bus_unregister();
>  }
>  module_exit(ssam_core_exit);
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5b08a473cdba..0b8f1feefe0e 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -838,4 +838,22 @@ struct mhi_device_id {
>  	kernel_ulong_t driver_data;
>  };
> 
> +/* Surface System Aggregator Module */
> +
> +#define SSAM_MATCH_TARGET	0x1
> +#define SSAM_MATCH_INSTANCE	0x2
> +#define SSAM_MATCH_FUNCTION	0x4
> +
> +struct ssam_device_id {
> +	__u8 match_flags;
> +
> +	__u8 domain;
> +	__u8 category;
> +	__u8 target;
> +	__u8 instance;
> +	__u8 function;
> +
> +	kernel_ulong_t driver_data;
> +};
> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */
> diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
> new file mode 100644
> index 000000000000..7221d4a9c1c1
> --- /dev/null
> +++ b/include/linux/surface_aggregator/device.h
> @@ -0,0 +1,423 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Surface System Aggregator Module (SSAM) bus and client-device subsystem.
> + *
> + * Main interface for the surface-aggregator bus, surface-aggregator client
> + * devices, and respective drivers building on top of the SSAM controller.
> + * Provides support for non-platform/non-ACPI SSAM clients via dedicated
> + * subsystem.
> + *
> + * Copyright (C) 2019-2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#ifndef _LINUX_SURFACE_AGGREGATOR_DEVICE_H
> +#define _LINUX_SURFACE_AGGREGATOR_DEVICE_H
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +
> +#include <linux/surface_aggregator/controller.h>
> +
> +
> +/* -- Surface System Aggregator Module bus. --------------------------------- */
> +
> +/**
> + * enum ssam_device_domain - SAM device domain.
> + * @SSAM_DOMAIN_VIRTUAL:   Virtual device.
> + * @SSAM_DOMAIN_SERIALHUB: Physical device connected via Surface Serial Hub.
> + */
> +enum ssam_device_domain {
> +	SSAM_DOMAIN_VIRTUAL   = 0x00,
> +	SSAM_DOMAIN_SERIALHUB = 0x01,
> +};
> +
> +/**
> + * enum ssam_virtual_tc - Target categories for the virtual SAM domain.
> + * @SSAM_VIRTUAL_TC_HUB: Device hub category.
> + */
> +enum ssam_virtual_tc {
> +	SSAM_VIRTUAL_TC_HUB = 0x00,
> +};
> +
> +/**
> + * struct ssam_device_uid - Unique identifier for SSAM device.
> + * @domain:   Domain of the device.
> + * @category: Target category of the device.
> + * @target:   Target ID of the device.
> + * @instance: Instance ID of the device.
> + * @function: Sub-function of the device. This field can be used to split a
> + *            single SAM device into multiple virtual subdevices to separate
> + *            different functionality of that device and allow one driver per
> + *            such functionality.
> + */
> +struct ssam_device_uid {
> +	u8 domain;
> +	u8 category;
> +	u8 target;
> +	u8 instance;
> +	u8 function;
> +};
> +
> +/*
> + * Special values for device matching.
> + *
> + * These values are intended to be used with SSAM_DEVICE(), SSAM_VDEV(), and
> + * SSAM_SDEV() exclusively. Specifically, they are used to initialize the
> + * match_flags member of the device ID structure. Do not use them directly
> + * with struct ssam_device_id or struct ssam_device_uid.
> + */
> +#define SSAM_ANY_TID		0xffff
> +#define SSAM_ANY_IID		0xffff
> +#define SSAM_ANY_FUN		0xffff
> +
> +/**
> + * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
> + * parameters.
> + * @d:   Domain of the device.
> + * @cat: Target category of the device.
> + * @tid: Target ID of the device.
> + * @iid: Instance ID of the device.
> + * @fun: Sub-function of the device.
> + *
> + * Initializes a &struct ssam_device_id with the given parameters. See &struct
> + * ssam_device_uid for details regarding the parameters. The special values
> + * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify that
> + * matching should ignore target ID, instance ID, and/or sub-function,
> + * respectively. This macro initializes the ``match_flags`` field based on the
> + * given parameters.
> + *
> + * Note: The parameters @d and @cat must be valid &u8 values, the parameters
> + * @tid, @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> + * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * allowed.
> + */
> +#define SSAM_DEVICE(d, cat, tid, iid, fun)					\
> +	.match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0)	\
> +		     | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0)	\
> +		     | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),	\
> +	.domain   = d,								\
> +	.category = cat,							\
> +	.target   = ((tid) != SSAM_ANY_TID) ? (tid) : 0,			\
> +	.instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0,			\
> +	.function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0				\
> +
> +/**
> + * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with
> + * the given parameters.
> + * @cat: Target category of the device.
> + * @tid: Target ID of the device.
> + * @iid: Instance ID of the device.
> + * @fun: Sub-function of the device.
> + *
> + * Initializes a &struct ssam_device_id with the given parameters in the
> + * virtual domain. See &struct ssam_device_uid for details regarding the
> + * parameters. The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and
> + * %SSAM_ANY_FUN can be used to specify that matching should ignore target ID,
> + * instance ID, and/or sub-function, respectively. This macro initializes the
> + * ``match_flags`` field based on the given parameters.
> + *
> + * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
> + * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> + * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * allowed.
> + */
> +#define SSAM_VDEV(cat, tid, iid, fun) \
> +	SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun)
> +
> +/**
> + * SSAM_SDEV() - Initialize a &struct ssam_device_id as physical SSH device
> + * with the given parameters.
> + * @cat: Target category of the device.
> + * @tid: Target ID of the device.
> + * @iid: Instance ID of the device.
> + * @fun: Sub-function of the device.
> + *
> + * Initializes a &struct ssam_device_id with the given parameters in the SSH
> + * domain. See &struct ssam_device_uid for details regarding the parameters.
> + * The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be
> + * used to specify that matching should ignore target ID, instance ID, and/or
> + * sub-function, respectively. This macro initializes the ``match_flags``
> + * field based on the given parameters.
> + *
> + * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
> + * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> + * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * allowed.
> + */
> +#define SSAM_SDEV(cat, tid, iid, fun) \
> +	SSAM_DEVICE(SSAM_DOMAIN_SERIALHUB, SSAM_SSH_TC_##cat, tid, iid, fun)
> +
> +/**
> + * struct ssam_device - SSAM client device.
> + * @dev:  Driver model representation of the device.
> + * @ctrl: SSAM controller managing this device.
> + * @uid:  UID identifying the device.
> + */
> +struct ssam_device {
> +	struct device dev;
> +	struct ssam_controller *ctrl;
> +
> +	struct ssam_device_uid uid;
> +};
> +
> +/**
> + * struct ssam_device_driver - SSAM client device driver.
> + * @driver:      Base driver model structure.
> + * @match_table: Match table specifying which devices the driver should bind to.
> + * @probe:       Called when the driver is being bound to a device.
> + * @remove:      Called when the driver is being unbound from the device.
> + */
> +struct ssam_device_driver {
> +	struct device_driver driver;
> +
> +	const struct ssam_device_id *match_table;
> +
> +	int  (*probe)(struct ssam_device *sdev);
> +	void (*remove)(struct ssam_device *sdev);
> +};
> +
> +extern struct bus_type ssam_bus_type;
> +extern const struct device_type ssam_device_type;
> +
> +/**
> + * is_ssam_device() - Check if the given device is a SSAM client device.
> + * @d: The device to test the type of.
> + *
> + * Return: Returns %true iff the specified device is of type &struct
> + * ssam_device, i.e. the device type points to %ssam_device_type, and %false
> + * otherwise.
> + */
> +static inline bool is_ssam_device(struct device *d)
> +{
> +	return d->type == &ssam_device_type;
> +}
> +
> +/**
> + * to_ssam_device() - Casts the given device to a SSAM client device.
> + * @d: The device to cast.
> + *
> + * Casts the given &struct device to a &struct ssam_device. The caller has to
> + * ensure that the given device is actually enclosed in a &struct ssam_device,
> + * e.g. by calling is_ssam_device().
> + *
> + * Return: Returns a pointer to the &struct ssam_device wrapping the given
> + * device @d.
> + */
> +static inline struct ssam_device *to_ssam_device(struct device *d)
> +{
> +	return container_of(d, struct ssam_device, dev);
> +}
> +
> +/**
> + * to_ssam_device_driver() - Casts the given device driver to a SSAM client
> + * device driver.
> + * @d: The driver to cast.
> + *
> + * Casts the given &struct device_driver to a &struct ssam_device_driver. The
> + * caller has to ensure that the given driver is actually enclosed in a
> + * &struct ssam_device_driver.
> + *
> + * Return: Returns the pointer to the &struct ssam_device_driver wrapping the
> + * given device driver @d.
> + */
> +static inline
> +struct ssam_device_driver *to_ssam_device_driver(struct device_driver *d)
> +{
> +	return container_of(d, struct ssam_device_driver, driver);
> +}
> +
> +const struct ssam_device_id *ssam_device_id_match(const struct ssam_device_id *table,
> +						  const struct ssam_device_uid uid);
> +
> +const struct ssam_device_id *ssam_device_get_match(const struct ssam_device *dev);
> +
> +const void *ssam_device_get_match_data(const struct ssam_device *dev);
> +
> +struct ssam_device *ssam_device_alloc(struct ssam_controller *ctrl,
> +				      struct ssam_device_uid uid);
> +
> +int ssam_device_add(struct ssam_device *sdev);
> +void ssam_device_remove(struct ssam_device *sdev);
> +
> +/**
> + * ssam_device_get() - Increment reference count of SSAM client device.
> + * @sdev: The device to increment the reference count of.
> + *
> + * Increments the reference count of the given SSAM client device by
> + * incrementing the reference count of the enclosed &struct device via
> + * get_device().
> + *
> + * See ssam_device_put() for the counter-part of this function.
> + *
> + * Return: Returns the device provided as input.
> + */
> +static inline struct ssam_device *ssam_device_get(struct ssam_device *sdev)
> +{
> +	return sdev ? to_ssam_device(get_device(&sdev->dev)) : NULL;
> +}
> +
> +/**
> + * ssam_device_put() - Decrement reference count of SSAM client device.
> + * @sdev: The device to decrement the reference count of.
> + *
> + * Decrements the reference count of the given SSAM client device by
> + * decrementing the reference count of the enclosed &struct device via
> + * put_device().
> + *
> + * See ssam_device_get() for the counter-part of this function.
> + */
> +static inline void ssam_device_put(struct ssam_device *sdev)
> +{
> +	if (sdev)
> +		put_device(&sdev->dev);
> +}
> +
> +/**
> + * ssam_device_get_drvdata() - Get driver-data of SSAM client device.
> + * @sdev: The device to get the driver-data from.
> + *
> + * Return: Returns the driver-data of the given device, previously set via
> + * ssam_device_set_drvdata().
> + */
> +static inline void *ssam_device_get_drvdata(struct ssam_device *sdev)
> +{
> +	return dev_get_drvdata(&sdev->dev);
> +}
> +
> +/**
> + * ssam_device_set_drvdata() - Set driver-data of SSAM client device.
> + * @sdev: The device to set the driver-data of.
> + * @data: The data to set the device's driver-data pointer to.
> + */
> +static inline void ssam_device_set_drvdata(struct ssam_device *sdev, void *data)
> +{
> +	dev_set_drvdata(&sdev->dev, data);
> +}
> +
> +int __ssam_device_driver_register(struct ssam_device_driver *d, struct module *o);
> +void ssam_device_driver_unregister(struct ssam_device_driver *d);
> +
> +/**
> + * ssam_device_driver_register() - Register a SSAM client device driver.
> + * @drv: The driver to register.
> + */
> +#define ssam_device_driver_register(drv) \
> +	__ssam_device_driver_register(drv, THIS_MODULE)
> +
> +/**
> + * module_ssam_device_driver() - Helper macro for SSAM device driver
> + * registration.
> + * @drv: The driver managed by this module.
> + *
> + * Helper macro to register a SSAM device driver via module_init() and
> + * module_exit(). This macro may only be used once per module and replaces the
> + * aforementioned definitions.
> + */
> +#define module_ssam_device_driver(drv)			\
> +	module_driver(drv, ssam_device_driver_register,	\
> +		      ssam_device_driver_unregister)
> +
> +
> +/* -- Helpers for client-device requests. ----------------------------------- */
> +
> +/**
> + * SSAM_DEFINE_SYNC_REQUEST_CL_N() - Define synchronous client-device SAM
> + * request function with neither argument nor return value.
> + * @name: Name of the generated function.
> + * @spec: Specification (&struct ssam_request_spec_md) defining the request.
> + *
> + * Defines a function executing the synchronous SAM request specified by
> + * @spec, with the request having neither argument nor return value. Device
> + * specifying parameters are not hard-coded, but instead are provided via the
> + * client device, specifically its UID, supplied when calling this function.
> + * The generated function takes care of setting up the request struct, buffer
> + * allocation, as well as execution of the request itself, returning once the
> + * request has been fully completed. The required transport buffer will be
> + * allocated on the stack.
> + *
> + * The generated function is defined as ``int name(struct ssam_device *sdev)``,
> + * returning the status of the request, which is zero on success and negative
> + * on failure. The ``sdev`` parameter specifies both the target device of the
> + * request and by association the controller via which the request is sent.
> + *
> + * Refer to ssam_request_sync_onstack() for more details on the behavior of
> + * the generated function.
> + */
> +#define SSAM_DEFINE_SYNC_REQUEST_CL_N(name, spec...)			\
> +	SSAM_DEFINE_SYNC_REQUEST_MD_N(__raw_##name, spec)		\
> +	int name(struct ssam_device *sdev)				\
> +	{								\
> +		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
> +				    sdev->uid.instance);		\
> +	}
> +
> +/**
> + * SSAM_DEFINE_SYNC_REQUEST_CL_W() - Define synchronous client-device SAM
> + * request function with argument.
> + * @name:  Name of the generated function.
> + * @atype: Type of the request's argument.
> + * @spec:  Specification (&struct ssam_request_spec_md) defining the request.
> + *
> + * Defines a function executing the synchronous SAM request specified by
> + * @spec, with the request taking an argument of type @atype and having no
> + * return value. Device specifying parameters are not hard-coded, but instead
> + * are provided via the client device, specifically its UID, supplied when
> + * calling this function. The generated function takes care of setting up the
> + * request struct, buffer allocation, as well as execution of the request
> + * itself, returning once the request has been fully completed. The required
> + * transport buffer will be allocated on the stack.
> + *
> + * The generated function is defined as ``int name(struct ssam_device *sdev,
> + * const atype *arg)``, returning the status of the request, which is zero on
> + * success and negative on failure. The ``sdev`` parameter specifies both the
> + * target device of the request and by association the controller via which
> + * the request is sent. The request's argument is specified via the ``arg``
> + * pointer.
> + *
> + * Refer to ssam_request_sync_onstack() for more details on the behavior of
> + * the generated function.
> + */
> +#define SSAM_DEFINE_SYNC_REQUEST_CL_W(name, atype, spec...)		\
> +	SSAM_DEFINE_SYNC_REQUEST_MD_W(__raw_##name, atype, spec)	\
> +	int name(struct ssam_device *sdev, const atype *arg)		\
> +	{								\
> +		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
> +				    sdev->uid.instance, arg);		\
> +	}
> +
> +/**
> + * SSAM_DEFINE_SYNC_REQUEST_CL_R() - Define synchronous client-device SAM
> + * request function with return value.
> + * @name:  Name of the generated function.
> + * @rtype: Type of the request's return value.
> + * @spec:  Specification (&struct ssam_request_spec_md) defining the request.
> + *
> + * Defines a function executing the synchronous SAM request specified by
> + * @spec, with the request taking no argument but having a return value of
> + * type @rtype. Device specifying parameters are not hard-coded, but instead
> + * are provided via the client device, specifically its UID, supplied when
> + * calling this function. The generated function takes care of setting up the
> + * request struct, buffer allocation, as well as execution of the request
> + * itself, returning once the request has been fully completed. The required
> + * transport buffer will be allocated on the stack.
> + *
> + * The generated function is defined as ``int name(struct ssam_device *sdev,
> + * rtype *ret)``, returning the status of the request, which is zero on
> + * success and negative on failure. The ``sdev`` parameter specifies both the
> + * target device of the request and by association the controller via which
> + * the request is sent. The request's return value is written to the memory
> + * pointed to by the ``ret`` parameter.
> + *
> + * Refer to ssam_request_sync_onstack() for more details on the behavior of
> + * the generated function.
> + */
> +#define SSAM_DEFINE_SYNC_REQUEST_CL_R(name, rtype, spec...)		\
> +	SSAM_DEFINE_SYNC_REQUEST_MD_R(__raw_##name, rtype, spec)	\
> +	int name(struct ssam_device *sdev, rtype *ret)			\
> +	{								\
> +		return __raw_##name(sdev->ctrl, sdev->uid.target,	\
> +				    sdev->uid.instance, ret);		\
> +	}
> +
> +#endif /* _LINUX_SURFACE_AGGREGATOR_DEVICE_H */
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index 27007c18e754..4339377ad929 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -243,5 +243,13 @@ int main(void)
>  	DEVID(mhi_device_id);
>  	DEVID_FIELD(mhi_device_id, chan);
> 
> +	DEVID(ssam_device_id);
> +	DEVID_FIELD(ssam_device_id, match_flags);
> +	DEVID_FIELD(ssam_device_id, domain);
> +	DEVID_FIELD(ssam_device_id, category);
> +	DEVID_FIELD(ssam_device_id, target);
> +	DEVID_FIELD(ssam_device_id, instance);
> +	DEVID_FIELD(ssam_device_id, function);
> +
>  	return 0;
>  }
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 2417dd1dee33..5b79fdc42641 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1368,6 +1368,28 @@ static int do_mhi_entry(const char *filename, void *symval, char *alias)
>  	return 1;
>  }
> 
> +/*
> + * Looks like: ssam:dNcNtNiNfN
> + *
> + * N is exactly 2 digits, where each is an upper-case hex digit.
> + */
> +static int do_ssam_entry(const char *filename, void *symval, char *alias)
> +{
> +	DEF_FIELD(symval, ssam_device_id, match_flags);
> +	DEF_FIELD(symval, ssam_device_id, domain);
> +	DEF_FIELD(symval, ssam_device_id, category);
> +	DEF_FIELD(symval, ssam_device_id, target);
> +	DEF_FIELD(symval, ssam_device_id, instance);
> +	DEF_FIELD(symval, ssam_device_id, function);
> +
> +	sprintf(alias, "ssam:d%02Xc%02X", domain, category);
> +	ADD(alias, "t", match_flags & SSAM_MATCH_TARGET, target);
> +	ADD(alias, "i", match_flags & SSAM_MATCH_INSTANCE, instance);
> +	ADD(alias, "f", match_flags & SSAM_MATCH_FUNCTION, function);
> +
> +	return 1;
> +}
> +
>  /* Does namelen bytes of name exactly match the symbol? */
>  static bool sym_is(const char *name, unsigned namelen, const char *symbol)
>  {
> @@ -1442,6 +1464,7 @@ static const struct devtable devtable[] = {
>  	{"tee", SIZE_tee_client_device_id, do_tee_entry},
>  	{"wmi", SIZE_wmi_device_id, do_wmi_entry},
>  	{"mhi", SIZE_mhi_device_id, do_mhi_entry},
> +	{"ssam", SIZE_ssam_device_id, do_ssam_entry},
>  };
> 
>  /* Create MODULE_ALIAS() statements.
> --
> 2.29.2
>