Re: [PATCH] platform/surface: aggregator: fix a bit test
On 4/20/21 10:44 AM, Dan Carpenter wrote: The "funcs" variable is a u64. If "func" is more than 31 then the BIT() shift will wrap instead of testing the high bits. Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") Reported-by: kernel test robot Signed-off-by: Dan Carpenter Thanks! Reviewed-by: Maximilian Luz --- drivers/platform/surface/aggregator/controller.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c index 00e38284885a..69e86cd599d3 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -1040,7 +1040,7 @@ static int ssam_dsm_load_u32(acpi_handle handle, u64 funcs, u64 func, u32 *ret) union acpi_object *obj; u64 val; - if (!(funcs & BIT(func))) + if (!(funcs & BIT_ULL(func))) return 0; /* Not supported, leave *ret at its default value */ obj = acpi_evaluate_dsm_typed(handle, _SSH_DSM_GUID,
[PATCH] HID: surface-hid: Fix integer endian conversion
We want to convert from 16 bit (unsigned) little endian values contained in a packed struct to CPU native endian values here, not the other way around. So replace cpu_to_le16() with get_unaligned_le16(), using the latter instead of le16_to_cpu() to acknowledge that we are reading from a packed struct. Reported-by: kernel test robot Fixes: b05ff1002a5c ("HID: Add support for Surface Aggregator Module HID transport") Signed-off-by: Maximilian Luz --- drivers/hid/surface-hid/surface_hid_core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/hid/surface-hid/surface_hid_core.c b/drivers/hid/surface-hid/surface_hid_core.c index 7b27ec392232..5571e74abe91 100644 --- a/drivers/hid/surface-hid/surface_hid_core.c +++ b/drivers/hid/surface-hid/surface_hid_core.c @@ -168,9 +168,9 @@ int surface_hid_device_add(struct surface_hid_device *shid) shid->hid->dev.parent = shid->dev; shid->hid->bus = BUS_HOST; - shid->hid->vendor = cpu_to_le16(shid->attrs.vendor); - shid->hid->product = cpu_to_le16(shid->attrs.product); - shid->hid->version = cpu_to_le16(shid->hid_desc.hid_version); + shid->hid->vendor = get_unaligned_le16(>attrs.vendor); + shid->hid->product = get_unaligned_le16(>attrs.product); + shid->hid->version = get_unaligned_le16(>hid_desc.hid_version); shid->hid->country = shid->hid_desc.country_code; snprintf(shid->hid->name, sizeof(shid->hid->name), "Microsoft Surface %04X:%04X", -- 2.31.1
[PATCH v2 2/2] power: supply: Add AC driver for Surface Aggregator Module
On newer Microsoft Surface models (specifically 7th-generation, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go), battery and AC status/information is no longer handled via standard ACPI devices, but instead directly via the Surface System Aggregator Module (SSAM), i.e. the embedded controller on those devices. While on previous generation models, AC status is also handled via SSAM, an ACPI shim was present to translate the standard ACPI AC interface to SSAM requests. The SSAM interface itself, which is modeled closely after the ACPI interface, has not changed. This commit introduces a new SSAM client device driver to support AC status/information via the aforementioned interface on said Surface models. Signed-off-by: Maximilian Luz --- Changes in v2: - Use devm_power_supply_register() - Specify .supplied_to - Fix constness of property arrays - Drop mutex_destroy() call - Inline spwr_ac_unregister() --- MAINTAINERS| 1 + drivers/power/supply/Kconfig | 16 ++ drivers/power/supply/Makefile | 1 + drivers/power/supply/surface_charger.c | 282 + 4 files changed, 300 insertions(+) create mode 100644 drivers/power/supply/surface_charger.c diff --git a/MAINTAINERS b/MAINTAINERS index e989beffde99..bfb0ac2b642f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11867,6 +11867,7 @@ L: linux...@vger.kernel.org L: platform-driver-...@vger.kernel.org S: Maintained F: drivers/power/supply/surface_battery.c +F: drivers/power/supply/surface_charger.c MICROSOFT SURFACE GPE LID SUPPORT DRIVER M: Maximilian Luz diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index 5b5054762194..e696364126f1 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -817,4 +817,20 @@ config BATTERY_SURFACE Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, Surface Book 3, and Surface Laptop Go. +config CHARGER_SURFACE + tristate "AC driver for 7th-generation Microsoft Surface devices" + depends on SURFACE_AGGREGATOR_REGISTRY + help + Driver for AC devices connected via/managed by the Surface System + Aggregator Module (SSAM). + + This driver provides AC-information and -status support for Surface + devices where said data is not exposed via the standard ACPI devices. + On those models (7th-generation), AC-information is instead handled + directly via a SSAM client device and this driver. + + Say M or Y here to include AC status support for 7th-generation + Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, + Surface Book 3, and Surface Laptop Go. + endif # POWER_SUPPLY diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index 134041538d2c..a7309a3d1a47 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -102,3 +102,4 @@ obj-$(CONFIG_CHARGER_WILCO) += wilco-charger.o obj-$(CONFIG_RN5T618_POWER)+= rn5t618_power.o obj-$(CONFIG_BATTERY_ACER_A500)+= acer_a500_battery.o obj-$(CONFIG_BATTERY_SURFACE) += surface_battery.o +obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o diff --git a/drivers/power/supply/surface_charger.c b/drivers/power/supply/surface_charger.c new file mode 100644 index ..c2dd7e604d14 --- /dev/null +++ b/drivers/power/supply/surface_charger.c @@ -0,0 +1,282 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * AC driver for 7th-generation Microsoft Surface devices via Surface System + * Aggregator Module (SSAM). + * + * Copyright (C) 2019-2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include +#include + +#include + + +/* -- SAM interface. */ + +enum sam_event_cid_bat { + SAM_EVENT_CID_BAT_ADP = 0x17, +}; + +enum sam_battery_sta { + SAM_BATTERY_STA_OK = 0x0f, + SAM_BATTERY_STA_PRESENT = 0x10, +}; + +/* Get battery status (_STA). */ +SSAM_DEFINE_SYNC_REQUEST_CL_R(ssam_bat_get_sta, __le32, { + .target_category = SSAM_SSH_TC_BAT, + .command_id = 0x01, +}); + +/* Get platform power source for battery (_PSR / DPTF PSRC). */ +SSAM_DEFINE_SYNC_REQUEST_CL_R(ssam_bat_get_psrc, __le32, { + .target_category = SSAM_SSH_TC_BAT, + .command_id = 0x0d, +}); + + +/* -- Device structures. */ + +struct spwr_psy_properties { + const char *name; + struct ssam_event_registry registry; +}; + +struct spwr_ac_device { + struct ssam_device *sdev; + + char name[32]; + struct power_supply *psy; + struct power_supply_desc psy_desc; + + struct ssam_event_notifier notif; + + struct mutex lock; /* Guards access to state below. */ + + __le32 state; +}; + + +/* -- State
[PATCH v2 1/2] power: supply: Add battery driver for Surface Aggregator Module
On newer Microsoft Surface models (specifically 7th-generation, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go), battery and AC status/information is no longer handled via standard ACPI devices, but instead directly via the Surface System Aggregator Module (SSAM), i.e. the embedded controller on those devices. While on previous generation models, battery status is also handled via SSAM, an ACPI shim was present to translate the standard ACPI battery interface to SSAM requests. The SSAM interface itself, which is modeled closely after the ACPI interface, has not changed. This commit introduces a new SSAM client device driver to support battery status/information via the aforementioned interface on said Surface models. It is in parts based on the standard ACPI battery driver. Signed-off-by: Maximilian Luz --- Changes in v2: - Use devm_power_supply_register() - Use .external_power_changed() callback instead of adapter event - Use DEVICE_ATTR_RW() instead of manually specifying attribute - Don't create device attribute manually, let psy take care of that - Change return values for non-available properties to -ENODATA - Fix constness of property arrays - Drop mutex_destroy() call - Inline spwr_battery_unregister() --- .../ABI/testing/sysfs-class-power-surface | 15 + MAINTAINERS | 7 + drivers/power/supply/Kconfig | 16 + drivers/power/supply/Makefile | 1 + drivers/power/supply/surface_battery.c| 865 ++ 5 files changed, 904 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-power-surface create mode 100644 drivers/power/supply/surface_battery.c diff --git a/Documentation/ABI/testing/sysfs-class-power-surface b/Documentation/ABI/testing/sysfs-class-power-surface new file mode 100644 index ..79cde4dcf2f5 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-power-surface @@ -0,0 +1,15 @@ +What: /sys/class/power_supply//alarm +Date: April 2021 +KernelVersion: 5.13 +Contact: Maximilian Luz +Description: + Battery trip point. When the remaining battery capacity crosses this + value in either direction, the system will be notified and if + necessary woken. + + Set to zero to clear/disable. + + Access: Read, Write + + Valid values: In micro-Wh or micro-Ah, depending on the power unit + of the battery diff --git a/MAINTAINERS b/MAINTAINERS index 5b1416e34256..e989beffde99 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11861,6 +11861,13 @@ F: drivers/scsi/smartpqi/smartpqi*.[ch] F: include/linux/cciss*.h F: include/uapi/linux/cciss*.h +MICROSOFT SURFACE BATTERY AND AC DRIVERS +M: Maximilian Luz +L: linux...@vger.kernel.org +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/power/supply/surface_battery.c + MICROSOFT SURFACE GPE LID SUPPORT DRIVER M: Maximilian Luz L: platform-driver-...@vger.kernel.org diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index b9e10506d6a9..5b5054762194 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -801,4 +801,20 @@ config BATTERY_ACER_A500 help Say Y to include support for Acer Iconia Tab A500 battery fuel gauge. +config BATTERY_SURFACE + tristate "Battery driver for 7th-generation Microsoft Surface devices" + depends on SURFACE_AGGREGATOR_REGISTRY + help + Driver for battery devices connected via/managed by the Surface System + Aggregator Module (SSAM). + + This driver provides battery-information and -status support for + Surface devices where said data is not exposed via the standard ACPI + devices. On those models (7th-generation), battery-information is + instead handled directly via SSAM client devices and this driver. + + Say M or Y here to include battery status support for 7th-generation + Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, + Surface Book 3, and Surface Laptop Go. + endif # POWER_SUPPLY diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index 5e5fdbbef531..134041538d2c 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -101,3 +101,4 @@ obj-$(CONFIG_CHARGER_BD99954) += bd99954-charger.o obj-$(CONFIG_CHARGER_WILCO)+= wilco-charger.o obj-$(CONFIG_RN5T618_POWER)+= rn5t618_power.o obj-$(CONFIG_BATTERY_ACER_A500)+= acer_a500_battery.o +obj-$(CONFIG_BATTERY_SURFACE) += surface_battery.o diff --git a/drivers/power/supply/surface_battery.c b/drivers/power/supply/surface_battery.c new file mode 100644 index ..4116dd839ecd --- /dev/null +++ b/drivers/power/supply/surface_battery.c @@ -0,0 +1,865 @@ +// SPDX-License-Identifie
[PATCH v2 0/2] power: supply: Add battery and AC drivers for Surface devices
This series provides battery and AC drivers for Microsoft Surface devices, where this information is provided via an embedded controller (the Surface System Aggregator Module, SSAM) instead of the usual ACPI interface. Specifically, 7th generation Surface devices, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, as well as the Surface Laptop Go use this new interface. Note: This series depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree and is available as immutable tag at git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git tags/platform-drivers-x86-surface-aggregator-v5.13-1 Maximilian Luz (2): power: supply: Add battery driver for Surface Aggregator Module power: supply: Add AC driver for Surface Aggregator Module .../ABI/testing/sysfs-class-power-surface | 15 + MAINTAINERS | 8 + drivers/power/supply/Kconfig | 32 + drivers/power/supply/Makefile | 2 + drivers/power/supply/surface_battery.c| 865 ++ drivers/power/supply/surface_charger.c| 282 ++ 6 files changed, 1204 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-power-surface create mode 100644 drivers/power/supply/surface_battery.c create mode 100644 drivers/power/supply/surface_charger.c -- 2.31.1
[PATCH] platform/surface: aggregator_registry: Give devices time to set up when connecting
Sometimes, the "base connected" event that we rely on to (re-)attach the device connected to the base is sent a bit too early. When this happens, some devices may not be completely ready yet. Specifically, the battery has been observed to report zero-values for things like full charge capacity, which, however, is only loaded once when the driver for that device probes. This can thus result in battery readings being unavailable. As we cannot easily and reliably discern between devices that are not ready yet and devices that are not connected (i.e. will never be ready), delay adding these devices. This should give them enough time to set up. The delay is set to 2.5 seconds, which should give us a good safety margin based on testing and still be fairly responsive for users. To achieve that delay switch to updating via a delayed work struct, which means that we can also get rid of some locking. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 98 --- 1 file changed, 40 insertions(+), 58 deletions(-) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index eccb9d1007cd..685d37a7add1 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -13,10 +13,10 @@ #include #include #include -#include #include #include #include +#include #include #include @@ -287,6 +287,13 @@ static int ssam_hub_add_devices(struct device *parent, struct ssam_controller *c /* -- SSAM base-hub driver. - */ +/* + * Some devices (especially battery) may need a bit of time to be fully usable + * after being (re-)connected. This delay has been determined via + * experimentation. + */ +#define SSAM_BASE_UPDATE_CONNECT_DELAY msecs_to_jiffies(2500) + enum ssam_base_hub_state { SSAM_BASE_HUB_UNINITIALIZED, SSAM_BASE_HUB_CONNECTED, @@ -296,8 +303,8 @@ enum ssam_base_hub_state { struct ssam_base_hub { struct ssam_device *sdev; - struct mutex lock; /* Guards state update checks and transitions. */ enum ssam_base_hub_state state; + struct delayed_work update_work; struct ssam_event_notifier notif; }; @@ -335,11 +342,7 @@ static ssize_t ssam_base_hub_state_show(struct device *dev, struct device_attrib char *buf) { struct ssam_base_hub *hub = dev_get_drvdata(dev); - bool connected; - - mutex_lock(>lock); - connected = hub->state == SSAM_BASE_HUB_CONNECTED; - mutex_unlock(>lock); + bool connected = hub->state == SSAM_BASE_HUB_CONNECTED; return sysfs_emit(buf, "%d\n", connected); } @@ -356,16 +359,20 @@ static const struct attribute_group ssam_base_hub_group = { .attrs = ssam_base_hub_attrs, }; -static int __ssam_base_hub_update(struct ssam_base_hub *hub, enum ssam_base_hub_state new) +static void ssam_base_hub_update_workfn(struct work_struct *work) { + struct ssam_base_hub *hub = container_of(work, struct ssam_base_hub, update_work.work); struct fwnode_handle *node = dev_fwnode(>sdev->dev); + enum ssam_base_hub_state state; int status = 0; - lockdep_assert_held(>lock); + status = ssam_base_hub_query_state(hub, ); + if (status) + return; - if (hub->state == new) - return 0; - hub->state = new; + if (hub->state == state) + return; + hub->state = state; if (hub->state == SSAM_BASE_HUB_CONNECTED) status = ssam_hub_add_devices(>sdev->dev, hub->sdev->ctrl, node); @@ -374,51 +381,28 @@ static int __ssam_base_hub_update(struct ssam_base_hub *hub, enum ssam_base_hub_ if (status) dev_err(>sdev->dev, "failed to update base-hub devices: %d\n", status); - - return status; -} - -static int ssam_base_hub_update(struct ssam_base_hub *hub) -{ - enum ssam_base_hub_state state; - int status; - - mutex_lock(>lock); - - status = ssam_base_hub_query_state(hub, ); - if (!status) - status = __ssam_base_hub_update(hub, state); - - mutex_unlock(>lock); - return status; } static u32 ssam_base_hub_notif(struct ssam_event_notifier *nf, const struct ssam_event *event) { - struct ssam_base_hub *hub; - struct ssam_device *sdev; - enum ssam_base_hub_state new; - - hub = container_of(nf, struct ssam_base_hub, notif); - sdev = hub->sdev; + struct ssam_base_hub *hub = container_of(nf, struct ssam_base_hub, notif); + unsigned long delay; if (event->command_id != SSAM_EVENT_BAS_CID_CONNECTION) return 0; if (event->length < 1)
Re: [PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module
Hi, On 4/5/21 11:32 PM, Sebastian Reichel wrote: [...] +static void spwr_battery_unregister(struct spwr_battery_device *bat) +{ + ssam_notifier_unregister(bat->sdev->ctrl, >notif); + cancel_delayed_work_sync(>update_work); + device_remove_file(>psy->dev, _attr); + power_supply_unregister(bat->psy); power_supply_unregister being the last function call is a clear sign, that devm_power_supply_register can be used instead. Right, that works here. I normally try to not mix devm code with non-devm code (apart from maybe allocations). well allocations are usually done first and free'd last making them the first targets in the conversion and pretty much a no brainer. Next merge window it's possible to easily go to full devm by using devm_delayed_work_autocancel(), which has been merged by Greg two weeks ago. Then last but not least do the ssam notifier unregister via devm_add_action_or_reset and its fully converted :) Neat, I'll have a look at maybe adding some devm versions for the SSAM notifiers. Should help in at least one other driver apart from these two. Thanks, Max
Re: [PATCH 2/2] power: supply: Add AC driver for Surface Aggregator Module
Hi, On 4/5/21 5:47 PM, Sebastian Reichel wrote: Hi, On Tue, Mar 09, 2021 at 01:05:30AM +0100, Maximilian Luz wrote: On newer Microsoft Surface models (specifically 7th-generation, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go), battery and AC status/information is no longer handled via standard ACPI devices, but instead directly via the Surface System Aggregator Module (SSAM), i.e. the embedded controller on those devices. While on previous generation models, AC status is also handled via SSAM, an ACPI shim was present to translate the standard ACPI AC interface to SSAM requests. The SSAM interface itself, which is modeled closely after the ACPI interface, has not changed. This commit introduces a new SSAM client device driver to support AC status/information via the aforementioned interface on said Surface models. Signed-off-by: Maximilian Luz --- Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde --- MAINTAINERS| 1 + drivers/power/supply/Kconfig | 16 ++ drivers/power/supply/Makefile | 1 + drivers/power/supply/surface_charger.c | 296 + 4 files changed, 314 insertions(+) create mode 100644 drivers/power/supply/surface_charger.c diff --git a/MAINTAINERS b/MAINTAINERS index f44521abe8bf..d6651ba93997 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11867,6 +11867,7 @@ L: linux...@vger.kernel.org L:platform-driver-...@vger.kernel.org S:Maintained F:drivers/power/supply/surface_battery.c +F: drivers/power/supply/surface_charger.c MICROSOFT SURFACE GPE LID SUPPORT DRIVER M:Maximilian Luz diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index cebeff10d543..91f7cf425ac9 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -817,4 +817,20 @@ config BATTERY_SURFACE Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, Surface Book 3, and Surface Laptop Go. +config CHARGER_SURFACE + tristate "AC driver for 7th-generation Microsoft Surface devices" + depends on SURFACE_AGGREGATOR_REGISTRY + help + Driver for AC devices connected via/managed by the Surface System + Aggregator Module (SSAM). + + This driver provides AC-information and -status support for Surface + devices where said data is not exposed via the standard ACPI devices. + On those models (7th-generation), AC-information is instead handled + directly via a SSAM client device and this driver. + + Say M or Y here to include AC status support for 7th-generation + Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, + Surface Book 3, and Surface Laptop Go. + endif # POWER_SUPPLY diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index 134041538d2c..a7309a3d1a47 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -102,3 +102,4 @@ obj-$(CONFIG_CHARGER_WILCO) += wilco-charger.o obj-$(CONFIG_RN5T618_POWER) += rn5t618_power.o obj-$(CONFIG_BATTERY_ACER_A500) += acer_a500_battery.o obj-$(CONFIG_BATTERY_SURFACE) += surface_battery.o +obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o diff --git a/drivers/power/supply/surface_charger.c b/drivers/power/supply/surface_charger.c new file mode 100644 index ..fe484523a2c2 --- /dev/null +++ b/drivers/power/supply/surface_charger.c @@ -0,0 +1,296 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * AC driver for 7th-generation Microsoft Surface devices via Surface System + * Aggregator Module (SSAM). + * + * Copyright (C) 2019-2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include +#include + +#include + + +/* -- SAM interface. */ + +enum sam_event_cid_bat { + SAM_EVENT_CID_BAT_ADP = 0x17, +}; + +enum sam_battery_sta { + SAM_BATTERY_STA_OK = 0x0f, + SAM_BATTERY_STA_PRESENT = 0x10, +}; + +/* Get battery status (_STA). */ +SSAM_DEFINE_SYNC_REQUEST_CL_R(ssam_bat_get_sta, __le32, { + .target_category = SSAM_SSH_TC_BAT, + .command_id = 0x01, +}); + +/* Get platform power source for battery (_PSR / DPTF PSRC). */ +SSAM_DEFINE_SYNC_REQUEST_CL_R(ssam_bat_get_psrc, __le32, { + .target_category = SSAM_SSH_TC_BAT, + .command_id
Re: [PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module
Hi, On 4/5/21 5:37 PM, Sebastian Reichel wrote: Hi, On Tue, Mar 09, 2021 at 01:05:29AM +0100, Maximilian Luz wrote: On newer Microsoft Surface models (specifically 7th-generation, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go), battery and AC status/information is no longer handled via standard ACPI devices, but instead directly via the Surface System Aggregator Module (SSAM), i.e. the embedded controller on those devices. While on previous generation models, battery status is also handled via SSAM, an ACPI shim was present to translate the standard ACPI battery interface to SSAM requests. The SSAM interface itself, which is modeled closely after the ACPI interface, has not changed. This commit introduces a new SSAM client device driver to support battery status/information via the aforementioned interface on said Surface models. It is in parts based on the standard ACPI battery driver. Signed-off-by: Maximilian Luz --- Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde --- MAINTAINERS| 7 + drivers/power/supply/Kconfig | 16 + drivers/power/supply/Makefile | 1 + drivers/power/supply/surface_battery.c | 901 + 4 files changed, 925 insertions(+) create mode 100644 drivers/power/supply/surface_battery.c diff --git a/MAINTAINERS b/MAINTAINERS index d756b9ec236d..f44521abe8bf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11861,6 +11861,13 @@ F: drivers/scsi/smartpqi/smartpqi*.[ch] F:include/linux/cciss*.h F:include/uapi/linux/cciss*.h +MICROSOFT SURFACE BATTERY AND AC DRIVERS +M: Maximilian Luz +L: linux...@vger.kernel.org +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/power/supply/surface_battery.c + MICROSOFT SURFACE GPE LID SUPPORT DRIVER M:Maximilian Luz L:platform-driver-...@vger.kernel.org diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index 006b95eca673..cebeff10d543 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -801,4 +801,20 @@ config BATTERY_ACER_A500 help Say Y to include support for Acer Iconia Tab A500 battery fuel gauge. +config BATTERY_SURFACE + tristate "Battery driver for 7th-generation Microsoft Surface devices" + depends on SURFACE_AGGREGATOR_REGISTRY + help + Driver for battery devices connected via/managed by the Surface System + Aggregator Module (SSAM). + + This driver provides battery-information and -status support for + Surface devices where said data is not exposed via the standard ACPI + devices. On those models (7th-generation), battery-information is + instead handled directly via SSAM client devices and this driver. + + Say M or Y here to include battery status support for 7th-generation + Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, + Surface Book 3, and Surface Laptop Go. + endif # POWER_SUPPLY diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index 5e5fdbbef531..134041538d2c 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -101,3 +101,4 @@ obj-$(CONFIG_CHARGER_BD99954) += bd99954-charger.o obj-$(CONFIG_CHARGER_WILCO) += wilco-charger.o obj-$(CONFIG_RN5T618_POWER) += rn5t618_power.o obj-$(CONFIG_BATTERY_ACER_A500) += acer_a500_battery.o +obj-$(CONFIG_BATTERY_SURFACE) += surface_battery.o diff --git a/drivers/power/supply/surface_battery.c b/drivers/power/supply/surface_battery.c new file mode 100644 index ..b93a4f556b5c --- /dev/null +++ b/drivers/power/supply/surface_battery.c @@ -0,0 +1,901 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Battery driver for 7th-generation Microsoft Surface devices via Surface + * System Aggregator Module (SSAM). + * + * Copyright (C) 2019-2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + + +/* -- SAM interface. */ + +enum sam_event_cid_bat { + SAM_EVENT_CID_BAT_BIX = 0x15, + SAM_EVENT_CID_BAT_BST = 0x16, + SAM_EVENT_CID_BAT_ADP = 0x17, + SAM_EVENT_CID_BAT_PROT= 0x18, + SAM_EVENT_CID_BAT_DPTF= 0x53, +
Re: [PATCH 0/2] HID: Add support for Surface Aggregator Module HID transport
On 3/10/21 11:53 PM, Maximilian Luz wrote: This series adds support for the Surface System Aggregator Module (SSAM) HID transport subsystem. The SSAM is an embedded controller, found on 5th- and later generation Microsoft Surface devices. On some of these devices (specifically Surface Laptops 1, 2, and 3, as well as Surface Book 3), built-in input devices are connected via the SSAM. These devices communicate (mostly) via normal HID reports, so adding support for them is (mostly) just a matter of implementing an HID transport driver. SSAM actually has two different HID interfaces: One (legacy) interface used on Surface Laptops 1 and 2, and a newer interface for the rest. The newer interface allows for multiple HID devices to be addressed and is implemented in the first patch. The older interface only allows a single HID device to be connected and, furthermore, only allows a single output report, specifically one for the caps lock LED. This is implemented in the second patch. See the commit messages of the respective patches for more details. Regards, Max Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde Maximilian Luz (2): HID: Add support for Surface Aggregator Module HID transport HID: surface-hid: Add support for legacy keyboard interface MAINTAINERS| 7 + drivers/hid/Kconfig| 2 + drivers/hid/Makefile | 2 + drivers/hid/surface-hid/Kconfig| 42 +++ drivers/hid/surface-hid/Makefile | 7 + drivers/hid/surface-hid/surface_hid.c | 253 + drivers/hid/surface-hid/surface_hid_core.c | 272 +++ drivers/hid/surface-hid/surface_hid_core.h | 77 ++ drivers/hid/surface-hid/surface_kbd.c | 300 + 9 files changed, 962 insertions(+) create mode 100644 drivers/hid/surface-hid/Kconfig create mode 100644 drivers/hid/surface-hid/Makefile create mode 100644 drivers/hid/surface-hid/surface_hid.c create mode 100644 drivers/hid/surface-hid/surface_hid_core.c create mode 100644 drivers/hid/surface-hid/surface_hid_core.h create mode 100644 drivers/hid/surface-hid/surface_kbd.c Hi, is there any status update on this? Regards, Max
Re: [PATCH 0/2] power: supply: Add battery and AC drivers for Surface devices
On 3/9/21 1:05 AM, Maximilian Luz wrote: This series provides battery and AC drivers for Microsoft Surface devices, where this information is provided via an embedded controller (the Surface System Aggregator Module, SSAM) instead of the usual ACPI interface. Specifically, 7th generation Surface devices, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, as well as the Surface Laptop Go use this new interface. Note: This series depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde Maximilian Luz (2): power: supply: Add battery driver for Surface Aggregator Module power: supply: Add AC driver for Surface Aggregator Module MAINTAINERS| 8 + drivers/power/supply/Kconfig | 32 + drivers/power/supply/Makefile | 2 + drivers/power/supply/surface_battery.c | 901 + drivers/power/supply/surface_charger.c | 296 5 files changed, 1239 insertions(+) create mode 100644 drivers/power/supply/surface_battery.c create mode 100644 drivers/power/supply/surface_charger.c Hi, is there any status update on this? Regards, Max
Re: [PATCH] platform/surface: clean up a variable in surface_dtx_read()
On 3/26/21 1:28 PM, Dan Carpenter wrote: The ">ddev->lock" and ">lock" are the same thing. Let's use ">lock" consistently. Signed-off-by: Dan Carpenter Good catch, thanks! Reviewed-by: Maximilian Luz --- drivers/platform/surface/surface_dtx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/surface/surface_dtx.c b/drivers/platform/surface/surface_dtx.c index 1fedacf74050..63ce587e79e3 100644 --- a/drivers/platform/surface/surface_dtx.c +++ b/drivers/platform/surface/surface_dtx.c @@ -487,7 +487,7 @@ static ssize_t surface_dtx_read(struct file *file, char __user *buf, size_t coun if (status < 0) return status; - if (down_read_killable(>ddev->lock)) + if (down_read_killable(>lock)) return -ERESTARTSYS; /* Need to check that we're not shut down again. */
Re: [PATCH 0/2] power: supply: Add battery and AC drivers for Surface devices
On 3/17/21 6:39 PM, Hans de Goede wrote: Hi, On 3/9/21 1:05 AM, Maximilian Luz wrote: This series provides battery and AC drivers for Microsoft Surface devices, where this information is provided via an embedded controller (the Surface System Aggregator Module, SSAM) instead of the usual ACPI interface. Specifically, 7th generation Surface devices, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, as well as the Surface Laptop Go use this new interface. Note: This series depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde Sebastian, I guess you want a pull-req from an immutable branch from me for that dependend commit and then you will merge these 2 patches ? Maximillian, this only needs that commit right, or would it be better if I send Sebastian a pull-req for a branch with the entire series? The entire series would be better, I think. Strictly speaking, it only requires the mentioned commit to compile successfully, but if anyone would want to test this they'd need the full series (or at least the battery/power subsystem commit) due to the device instantiation. Same reasoning applies to the HID series. Thanks, Max
Re: [PATCH] PCI: PM: Do not read power state in pci_enable_device_flags()
On 3/16/21 4:51 PM, Rafael J. Wysocki wrote: From: Rafael J. Wysocki It should not be necessary to update the current_state field of struct pci_dev in pci_enable_device_flags() before calling do_pci_enable_device() for the device, because none of the code between that point and the pci_set_power_state() call in do_pci_enable_device() invoked later depends on it. Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose _STA method initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from the PCI_PM_CTRL register is D0, the current_state field in the struct pci_dev representing that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward. To avoid such issues it is better to leave the current_state value as is until it is changed to PCI_D0 by do_pci_enable_device() as appropriate. However, the power state of the device is not changed to PCI_D0 if it is already enabled when pci_enable_device_flags() gets called for it, so update its current_state in that case, but use pci_update_current_state() covering platform PM too for that. Link: https://lore.kernel.org/lkml/20210314000439.3138941-1-luzmaximil...@gmail.com/ Reported-by: Maximilian Luz Tested-by: Maximilian Luz Signed-off-by: Rafael J. Wysocki --- Max, I've added a T-by from you even though the patch is slightly different from what you have tested, but the difference shouldn't matter for your case. Thanks! I've tested this now as well, all looks good. Regards, Max --- drivers/pci/pci.c | 16 +++- 1 file changed, 3 insertions(+), 13 deletions(-) Index: linux-pm/drivers/pci/pci.c === --- linux-pm.orig/drivers/pci/pci.c +++ linux-pm/drivers/pci/pci.c @@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(struc int err; int i, bars = 0; - /* -* Power state could be unknown at this point, either due to a fresh -* boot or a device removal call. So get the current power state -* so that things like MSI message writing will behave as expected -* (e.g. if the device really is in D0 at enable time). -*/ - if (dev->pm_cap) { - u16 pmcsr; - pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, ); - dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK); - } - - if (atomic_inc_return(>enable_cnt) > 1) + if (atomic_inc_return(>enable_cnt) > 1) { + pci_update_current_state(dev, dev->current_state); return 0; /* already enabled */ + } bridge = pci_upstream_bridge(dev); if (bridge)
Re: [PATCH v2] PCI: Run platform power transition on initial D0 entry
On 3/16/21 2:36 PM, Rafael J. Wysocki wrote: On Mon, Mar 15, 2021 at 7:28 PM Maximilian Luz wrote: On 3/15/21 4:34 PM, Rafael J. Wysocki wrote: On Sun, Mar 14, 2021 at 1:06 AM Maximilian Luz wrote: On some devices and platforms, the initial platform (e.g. ACPI) power state is not in sync with the power state of the PCI device. This seems like it is, for all intents and purposes, an issue with the device firmware (e.g. ACPI). On some devices, specifically Microsoft Surface Books 2 and 3, we encounter ACPI code akin to the following power resource, corresponding to a PCI device: PowerResource (PRP5, 0x00, 0x) { // Initialized to zero, i.e. off. There is no logic for checking // the actual state dynamically. Name (_STA, Zero) Method (_ON, 0, Serialized) { // ... code omitted ... _STA = One } Method (_OFF, 0, Serialized) { // ... code omitted ... _STA = Zero } } This resource is initialized to 'off' and does not have any logic for checking its actual state, i.e. the state of the corresponding PCI device. The stored state of this resource can only be changed by running the (platform/ACPI) power transition functions (i.e. _ON and _OFF). Well, there is _STA that returns "off" initially, so the OS should set the initial state of the device to D3cold and transition it into D0 as appropriate (i.e. starting with setting all of the power resources used by it to "on"). This means that, at boot, the PCI device power state is out of sync with the power state of the corresponding ACPI resource. During initial bring-up of a PCI device, pci_enable_device_flags() updates its PCI core state (from initially 'unknown') by reading from its PCI_PM_CTRL register. It does, however, not check if the platform (here ACPI) state is in sync with/valid for the actual device state and needs updating. Well, that's inconsistent. Also, it is rather pointless to update the device's power state at this point, because nothing between this point and the later do_pci_enable_device() call in this function requires its current_state to be up to date AFAICS. Have you tried to drop the power state update from pci_enable_device_flags()? [Note that we're talking about relatively old code here and it looks like that code is not necessary any more.] I had not tried this before, as I assumed the comment was still relevant. I did test that now and it works! I can't detect any regressions. Do you want to send this in or should I do that? I'll post it, thanks! Thank you! Feel free to add my tested-by tag. Regards, Max
Re: [PATCH v2] PCI: Run platform power transition on initial D0 entry
On 3/15/21 4:34 PM, Rafael J. Wysocki wrote: On Sun, Mar 14, 2021 at 1:06 AM Maximilian Luz wrote: On some devices and platforms, the initial platform (e.g. ACPI) power state is not in sync with the power state of the PCI device. This seems like it is, for all intents and purposes, an issue with the device firmware (e.g. ACPI). On some devices, specifically Microsoft Surface Books 2 and 3, we encounter ACPI code akin to the following power resource, corresponding to a PCI device: PowerResource (PRP5, 0x00, 0x) { // Initialized to zero, i.e. off. There is no logic for checking // the actual state dynamically. Name (_STA, Zero) Method (_ON, 0, Serialized) { // ... code omitted ... _STA = One } Method (_OFF, 0, Serialized) { // ... code omitted ... _STA = Zero } } This resource is initialized to 'off' and does not have any logic for checking its actual state, i.e. the state of the corresponding PCI device. The stored state of this resource can only be changed by running the (platform/ACPI) power transition functions (i.e. _ON and _OFF). Well, there is _STA that returns "off" initially, so the OS should set the initial state of the device to D3cold and transition it into D0 as appropriate (i.e. starting with setting all of the power resources used by it to "on"). This means that, at boot, the PCI device power state is out of sync with the power state of the corresponding ACPI resource. During initial bring-up of a PCI device, pci_enable_device_flags() updates its PCI core state (from initially 'unknown') by reading from its PCI_PM_CTRL register. It does, however, not check if the platform (here ACPI) state is in sync with/valid for the actual device state and needs updating. Well, that's inconsistent. Also, it is rather pointless to update the device's power state at this point, because nothing between this point and the later do_pci_enable_device() call in this function requires its current_state to be up to date AFAICS. Have you tried to drop the power state update from pci_enable_device_flags()? [Note that we're talking about relatively old code here and it looks like that code is not necessary any more.] I had not tried this before, as I assumed the comment was still relevant. I did test that now and it works! I can't detect any regressions. Do you want to send this in or should I do that? Either it should be possible to do that and all should work, or there is a good reason to make current_state reflect the real current power state of the device upfront, but then that should be done by putting it into D0 diractly at that point rather than later. Calling pci_power_up(dev) instead of pci_set_power_state(dev, PCI_D0) when current_state is already 0 only pokes at the power resources, because pci_raw_set_power_state() will do nothing then, but that is a rather less-than-straightforward way of doing this. Moreover, the ordering of actions mandated by the spec is to set power resources to "on" first and then write to the PMCSR, not the other way around. I don't know much about the PCI core (let alone spec), so that seemed like the least intrusive way to fix this for me. Thanks! Max
[PATCH v2] PCI: Run platform power transition on initial D0 entry
core and platform transitions, i.e. pci_raw_set_power_state() and pci_platform_power_transition() (should) both have checks on the current state. So in case either state is up to date, the corresponding transition should still evaluate to a no-op. The only notable difference made by the suggested replacement is pci_resume_bus() being called for devices where runtime D3cold is enabled. Signed-off-by: Maximilian Luz --- Changes in v2: - No code changes - Improve commit message, add details - Add Rafael and linux-pm to CCs --- drivers/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 16a17215f633..cc42315210e4 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1800,7 +1800,7 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars) u16 cmd; u8 pin; - err = pci_set_power_state(dev, PCI_D0); + err = pci_power_up(dev); if (err < 0 && err != -EIO) return err; -- 2.30.2
[PATCH 1/2] HID: Add support for Surface Aggregator Module HID transport
Add a HID transport driver to support integrated HID devices on newer Microsoft Surface models (specifically 7th-generation, i.e. Surface Laptop 3, Surface Book 3, and later). On those models, the internal keyboard and touchpad (as well as some other HID devices with currently unknown function) are connected via the generic HID subsystem (TC=0x15) of the Surface System Aggregator Module (SSAM). This subsystem provides a generic HID transport layer, support for which is implemented by this driver. Co-developed-by: Blaž Hrastnik Signed-off-by: Blaž Hrastnik Signed-off-by: Maximilian Luz --- Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde --- MAINTAINERS| 7 + drivers/hid/Kconfig| 2 + drivers/hid/Makefile | 2 + drivers/hid/surface-hid/Kconfig| 28 +++ drivers/hid/surface-hid/Makefile | 6 + drivers/hid/surface-hid/surface_hid.c | 253 +++ drivers/hid/surface-hid/surface_hid_core.c | 272 + drivers/hid/surface-hid/surface_hid_core.h | 77 ++ 8 files changed, 647 insertions(+) create mode 100644 drivers/hid/surface-hid/Kconfig create mode 100644 drivers/hid/surface-hid/Makefile create mode 100644 drivers/hid/surface-hid/surface_hid.c create mode 100644 drivers/hid/surface-hid/surface_hid_core.c create mode 100644 drivers/hid/surface-hid/surface_hid_core.h diff --git a/MAINTAINERS b/MAINTAINERS index 240e8a739fd1..bb6a796dc537 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11817,6 +11817,13 @@ S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git F: drivers/platform/surface/ +MICROSOFT SURFACE HID TRANSPORT DRIVER +M: Maximilian Luz +L: linux-in...@vger.kernel.org +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/hid/surface-hid/ + MICROSOFT SURFACE HOT-PLUG DRIVER M: Maximilian Luz L: platform-driver-...@vger.kernel.org diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 786b71ef7738..26e06097ba08 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -1206,4 +1206,6 @@ source "drivers/hid/intel-ish-hid/Kconfig" source "drivers/hid/amd-sfh-hid/Kconfig" +source "drivers/hid/surface-hid/Kconfig" + endmenu diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index c4f6d5c613dc..1044ed238856 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -145,3 +145,5 @@ obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/ obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ish-hid/ obj-$(CONFIG_AMD_SFH_HID) += amd-sfh-hid/ + +obj-$(CONFIG_SURFACE_HID_CORE) += surface-hid/ diff --git a/drivers/hid/surface-hid/Kconfig b/drivers/hid/surface-hid/Kconfig new file mode 100644 index ..642c7f0e64fe --- /dev/null +++ b/drivers/hid/surface-hid/Kconfig @@ -0,0 +1,28 @@ +# SPDX-License-Identifier: GPL-2.0+ +menu "Surface System Aggregator Module HID support" + depends on SURFACE_AGGREGATOR + depends on INPUT + +config SURFACE_HID + tristate "HID transport driver for Surface System Aggregator Module" + depends on SURFACE_AGGREGATOR_REGISTRY + select SURFACE_HID_CORE + help + Driver to support integrated HID devices on newer Microsoft Surface + models. + + This driver provides support for the HID transport protocol provided + by the Surface Aggregator Module (i.e. the embedded controller) on + 7th-generation Microsoft Surface devices, i.e. Surface Book 3 and + Surface Laptop 3. On those models, it is mainly used to connect the + integrated touchpad and keyboard. + + Say M or Y here, if you want support for integrated HID devices, i.e. + integrated touchpad and keyboard, on 7th generation Microsoft Surface + models. + +endmenu + +config SURFACE_HID_CORE + tristate + select HID diff --git a/drivers/hid/surface-hid/Makefile b/drivers/hid/surface-hid/Makefile new file mode 100644 index ..62fc04632d3d --- /dev/null +++ b/drivers/hid/surface-hid/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# Makefile - Surface System Aggregator Module (SSAM) HID transport driver. +# +obj-$(CONFIG_SURFACE_HID_CORE) += surface_hid_core.o +obj-$(CONFIG_SURFACE_HID) += surface_hid.o diff --git a/drivers/hi
[PATCH 2/2] HID: surface-hid: Add support for legacy keyboard interface
Add support for the legacy keyboard (KBD/TC=0x08) HID transport layer of the Surface System Aggregator Module (SSAM) to the Surface HID driver. On Surface Laptops 1 and 2, this interface is used to connect the integrated keyboard. Note that this subsystem interface essentially provides a limited HID transport layer. In contrast to the generic HID interface (TC=0x15) used on newer Surface models, this interface only allows (as far as we know) for a single device to be connected and is otherwise severely limited in terms of support for feature- and output-reports. Specifically, only caps-lock-LED output-reports and a single read-only feature-report are supported. Signed-off-by: Maximilian Luz --- Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde --- drivers/hid/surface-hid/Kconfig | 14 ++ drivers/hid/surface-hid/Makefile | 1 + drivers/hid/surface-hid/surface_kbd.c | 300 ++ 3 files changed, 315 insertions(+) create mode 100644 drivers/hid/surface-hid/surface_kbd.c diff --git a/drivers/hid/surface-hid/Kconfig b/drivers/hid/surface-hid/Kconfig index 642c7f0e64fe..7ce9b5d641eb 100644 --- a/drivers/hid/surface-hid/Kconfig +++ b/drivers/hid/surface-hid/Kconfig @@ -21,6 +21,20 @@ config SURFACE_HID integrated touchpad and keyboard, on 7th generation Microsoft Surface models. +config SURFACE_KBD + tristate "HID keyboard transport driver for Surface System Aggregator Module" + select SURFACE_HID_CORE + help + Driver to support HID keyboards on Surface Laptop 1 and 2 devices. + + This driver provides support for the HID transport protocol provided + by the Surface Aggregator Module (i.e. the embedded controller) on + Microsoft Surface Laptops 1 and 2. It is used to connect the + integrated keyboard on those devices. + + Say M or Y here, if you want support for the integrated keyboard on + Microsoft Surface Laptops 1 and 2. + endmenu config SURFACE_HID_CORE diff --git a/drivers/hid/surface-hid/Makefile b/drivers/hid/surface-hid/Makefile index 62fc04632d3d..4ae11cf09b25 100644 --- a/drivers/hid/surface-hid/Makefile +++ b/drivers/hid/surface-hid/Makefile @@ -4,3 +4,4 @@ # obj-$(CONFIG_SURFACE_HID_CORE) += surface_hid_core.o obj-$(CONFIG_SURFACE_HID) += surface_hid.o +obj-$(CONFIG_SURFACE_KBD) += surface_kbd.o diff --git a/drivers/hid/surface-hid/surface_kbd.c b/drivers/hid/surface-hid/surface_kbd.c new file mode 100644 index ..0635341bc517 --- /dev/null +++ b/drivers/hid/surface-hid/surface_kbd.c @@ -0,0 +1,300 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Surface System Aggregator Module (SSAM) HID transport driver for the legacy + * keyboard interface (KBD/TC=0x08 subsystem). Provides support for the + * integrated HID keyboard on Surface Laptops 1 and 2. + * + * Copyright (C) 2019-2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include +#include + +#include + +#include "surface_hid_core.h" + + +/* -- SAM interface (KBD). -- */ + +#define KBD_FEATURE_REPORT_SIZE7 /* 6 + report ID */ + +enum surface_kbd_cid { + SURFACE_KBD_CID_GET_DESCRIPTOR = 0x00, + SURFACE_KBD_CID_SET_CAPSLOCK_LED= 0x01, + SURFACE_KBD_CID_EVT_INPUT_GENERIC = 0x03, + SURFACE_KBD_CID_EVT_INPUT_HOTKEYS = 0x04, + SURFACE_KBD_CID_GET_FEATURE_REPORT = 0x0b, +}; + +static int ssam_kbd_get_descriptor(struct surface_hid_device *shid, u8 entry, u8 *buf, size_t len) +{ + struct ssam_request rqst; + struct ssam_response rsp; + int status; + + rqst.target_category = shid->uid.category; + rqst.target_id = shid->uid.target; + rqst.command_id = SURFACE_KBD_CID_GET_DESCRIPTOR; + rqst.instance_id = shid->uid.instance; + rqst.flags = SSAM_REQUEST_HAS_RESPONSE; + rqst.length = sizeof(entry); + rqst.payload = + + rsp.capacity = len; + rsp.length = 0; + rsp.pointer = buf; + + status = ssam_retry(ssam_request_sync_onstack, shid->ctrl, , , sizeof(entry)); + if (status) + return status; + + if (rsp.length != len) { + dev_err(shid->dev, "invalid descriptor length: got %zu, expected, %zu\n", + rsp.length, len); +
[PATCH 0/2] HID: Add support for Surface Aggregator Module HID transport
This series adds support for the Surface System Aggregator Module (SSAM) HID transport subsystem. The SSAM is an embedded controller, found on 5th- and later generation Microsoft Surface devices. On some of these devices (specifically Surface Laptops 1, 2, and 3, as well as Surface Book 3), built-in input devices are connected via the SSAM. These devices communicate (mostly) via normal HID reports, so adding support for them is (mostly) just a matter of implementing an HID transport driver. SSAM actually has two different HID interfaces: One (legacy) interface used on Surface Laptops 1 and 2, and a newer interface for the rest. The newer interface allows for multiple HID devices to be addressed and is implemented in the first patch. The older interface only allows a single HID device to be connected and, furthermore, only allows a single output report, specifically one for the caps lock LED. This is implemented in the second patch. See the commit messages of the respective patches for more details. Regards, Max Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde Maximilian Luz (2): HID: Add support for Surface Aggregator Module HID transport HID: surface-hid: Add support for legacy keyboard interface MAINTAINERS| 7 + drivers/hid/Kconfig| 2 + drivers/hid/Makefile | 2 + drivers/hid/surface-hid/Kconfig| 42 +++ drivers/hid/surface-hid/Makefile | 7 + drivers/hid/surface-hid/surface_hid.c | 253 + drivers/hid/surface-hid/surface_hid_core.c | 272 +++ drivers/hid/surface-hid/surface_hid_core.h | 77 ++ drivers/hid/surface-hid/surface_kbd.c | 300 + 9 files changed, 962 insertions(+) create mode 100644 drivers/hid/surface-hid/Kconfig create mode 100644 drivers/hid/surface-hid/Makefile create mode 100644 drivers/hid/surface-hid/surface_hid.c create mode 100644 drivers/hid/surface-hid/surface_hid_core.c create mode 100644 drivers/hid/surface-hid/surface_hid_core.h create mode 100644 drivers/hid/surface-hid/surface_kbd.c -- 2.30.1
[PATCH] platform/surface: aggregator_registry: Add support for Surface Pro 7+
The Surface Pro 7+ is essentially a refresh of the Surface Pro 7 with updated hardware and a new WSID identifier. Signed-off-by: Maximilian Luz --- drivers/platform/surface/surface_aggregator_registry.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index cdb4a95af3e8..c42b97f61a57 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -191,7 +191,7 @@ static const struct software_node *ssam_node_group_sp6[] = { NULL, }; -/* Devices for Surface Pro 7. */ +/* Devices for Surface Pro 7 and Surface Pro 7+. */ static const struct software_node *ssam_node_group_sp7[] = { _node_root, _node_bat_ac, @@ -521,6 +521,9 @@ static const struct acpi_device_id ssam_platform_hub_match[] = { /* Surface Pro 7 */ { "MSHW0116", (unsigned long)ssam_node_group_sp7 }, + /* Surface Pro 7+ */ + { "MSHW0119", (unsigned long)ssam_node_group_sp7 }, + /* Surface Book 2 */ { "MSHW0107", (unsigned long)ssam_node_group_sb2 }, -- 2.30.1
[PATCH 0/2] power: supply: Add battery and AC drivers for Surface devices
This series provides battery and AC drivers for Microsoft Surface devices, where this information is provided via an embedded controller (the Surface System Aggregator Module, SSAM) instead of the usual ACPI interface. Specifically, 7th generation Surface devices, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, as well as the Surface Laptop Go use this new interface. Note: This series depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde Maximilian Luz (2): power: supply: Add battery driver for Surface Aggregator Module power: supply: Add AC driver for Surface Aggregator Module MAINTAINERS| 8 + drivers/power/supply/Kconfig | 32 + drivers/power/supply/Makefile | 2 + drivers/power/supply/surface_battery.c | 901 + drivers/power/supply/surface_charger.c | 296 5 files changed, 1239 insertions(+) create mode 100644 drivers/power/supply/surface_battery.c create mode 100644 drivers/power/supply/surface_charger.c -- 2.30.1
[PATCH 2/2] power: supply: Add AC driver for Surface Aggregator Module
On newer Microsoft Surface models (specifically 7th-generation, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go), battery and AC status/information is no longer handled via standard ACPI devices, but instead directly via the Surface System Aggregator Module (SSAM), i.e. the embedded controller on those devices. While on previous generation models, AC status is also handled via SSAM, an ACPI shim was present to translate the standard ACPI AC interface to SSAM requests. The SSAM interface itself, which is modeled closely after the ACPI interface, has not changed. This commit introduces a new SSAM client device driver to support AC status/information via the aforementioned interface on said Surface models. Signed-off-by: Maximilian Luz --- Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde --- MAINTAINERS| 1 + drivers/power/supply/Kconfig | 16 ++ drivers/power/supply/Makefile | 1 + drivers/power/supply/surface_charger.c | 296 + 4 files changed, 314 insertions(+) create mode 100644 drivers/power/supply/surface_charger.c diff --git a/MAINTAINERS b/MAINTAINERS index f44521abe8bf..d6651ba93997 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11867,6 +11867,7 @@ L: linux...@vger.kernel.org L: platform-driver-...@vger.kernel.org S: Maintained F: drivers/power/supply/surface_battery.c +F: drivers/power/supply/surface_charger.c MICROSOFT SURFACE GPE LID SUPPORT DRIVER M: Maximilian Luz diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index cebeff10d543..91f7cf425ac9 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -817,4 +817,20 @@ config BATTERY_SURFACE Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, Surface Book 3, and Surface Laptop Go. +config CHARGER_SURFACE + tristate "AC driver for 7th-generation Microsoft Surface devices" + depends on SURFACE_AGGREGATOR_REGISTRY + help + Driver for AC devices connected via/managed by the Surface System + Aggregator Module (SSAM). + + This driver provides AC-information and -status support for Surface + devices where said data is not exposed via the standard ACPI devices. + On those models (7th-generation), AC-information is instead handled + directly via a SSAM client device and this driver. + + Say M or Y here to include AC status support for 7th-generation + Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, + Surface Book 3, and Surface Laptop Go. + endif # POWER_SUPPLY diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index 134041538d2c..a7309a3d1a47 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -102,3 +102,4 @@ obj-$(CONFIG_CHARGER_WILCO) += wilco-charger.o obj-$(CONFIG_RN5T618_POWER)+= rn5t618_power.o obj-$(CONFIG_BATTERY_ACER_A500)+= acer_a500_battery.o obj-$(CONFIG_BATTERY_SURFACE) += surface_battery.o +obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o diff --git a/drivers/power/supply/surface_charger.c b/drivers/power/supply/surface_charger.c new file mode 100644 index ..fe484523a2c2 --- /dev/null +++ b/drivers/power/supply/surface_charger.c @@ -0,0 +1,296 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * AC driver for 7th-generation Microsoft Surface devices via Surface System + * Aggregator Module (SSAM). + * + * Copyright (C) 2019-2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include +#include + +#include + + +/* -- SAM interface. */ + +enum sam_event_cid_bat { + SAM_EVENT_CID_BAT_ADP = 0x17, +}; + +enum sam_battery_sta { + SAM_BATTERY_STA_OK = 0x0f, + SAM_BATTERY_STA_PRESENT = 0x10, +}; + +/* Get battery status (_STA). */ +SSAM_DEFINE_SYNC_REQUEST_CL_R(ssam_bat_get_sta, __le32, { + .target_category = SSAM_SSH_TC_BAT, + .command_id = 0x01, +}); + +/* Get platform power source for battery (_PSR / DPTF PSRC). */ +SSAM_DEFINE_SYNC_REQUEST_CL_R(ssam_bat_get_psrc, __le32, { + .target_category = SSAM_SSH_TC_BAT, + .command_id = 0x0d, +}); + + +/* -- Device structures. */ + +struct spwr_psy_properties { +
[PATCH 1/2] power: supply: Add battery driver for Surface Aggregator Module
On newer Microsoft Surface models (specifically 7th-generation, i.e. Surface Pro 7, Surface Book 3, Surface Laptop 3, and Surface Laptop Go), battery and AC status/information is no longer handled via standard ACPI devices, but instead directly via the Surface System Aggregator Module (SSAM), i.e. the embedded controller on those devices. While on previous generation models, battery status is also handled via SSAM, an ACPI shim was present to translate the standard ACPI battery interface to SSAM requests. The SSAM interface itself, which is modeled closely after the ACPI interface, has not changed. This commit introduces a new SSAM client device driver to support battery status/information via the aforementioned interface on said Surface models. It is in parts based on the standard ACPI battery driver. Signed-off-by: Maximilian Luz --- Note: This patch depends on the platform/surface: Add Surface Aggregator device registry series. More specifically patch platform/surface: Set up Surface Aggregator device registry The full series has been merged into the for-next branch of the platform-drivers-x86 tree [1]. The commit in question can be found at [2]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next [2]: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next=fc622b3d36e6d91330fb21506b9ad1e3206a4dde --- MAINTAINERS| 7 + drivers/power/supply/Kconfig | 16 + drivers/power/supply/Makefile | 1 + drivers/power/supply/surface_battery.c | 901 + 4 files changed, 925 insertions(+) create mode 100644 drivers/power/supply/surface_battery.c diff --git a/MAINTAINERS b/MAINTAINERS index d756b9ec236d..f44521abe8bf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11861,6 +11861,13 @@ F: drivers/scsi/smartpqi/smartpqi*.[ch] F: include/linux/cciss*.h F: include/uapi/linux/cciss*.h +MICROSOFT SURFACE BATTERY AND AC DRIVERS +M: Maximilian Luz +L: linux...@vger.kernel.org +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/power/supply/surface_battery.c + MICROSOFT SURFACE GPE LID SUPPORT DRIVER M: Maximilian Luz L: platform-driver-...@vger.kernel.org diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index 006b95eca673..cebeff10d543 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -801,4 +801,20 @@ config BATTERY_ACER_A500 help Say Y to include support for Acer Iconia Tab A500 battery fuel gauge. +config BATTERY_SURFACE + tristate "Battery driver for 7th-generation Microsoft Surface devices" + depends on SURFACE_AGGREGATOR_REGISTRY + help + Driver for battery devices connected via/managed by the Surface System + Aggregator Module (SSAM). + + This driver provides battery-information and -status support for + Surface devices where said data is not exposed via the standard ACPI + devices. On those models (7th-generation), battery-information is + instead handled directly via SSAM client devices and this driver. + + Say M or Y here to include battery status support for 7th-generation + Microsoft Surface devices, i.e. Surface Pro 7, Surface Laptop 3, + Surface Book 3, and Surface Laptop Go. + endif # POWER_SUPPLY diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index 5e5fdbbef531..134041538d2c 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -101,3 +101,4 @@ obj-$(CONFIG_CHARGER_BD99954) += bd99954-charger.o obj-$(CONFIG_CHARGER_WILCO)+= wilco-charger.o obj-$(CONFIG_RN5T618_POWER)+= rn5t618_power.o obj-$(CONFIG_BATTERY_ACER_A500)+= acer_a500_battery.o +obj-$(CONFIG_BATTERY_SURFACE) += surface_battery.o diff --git a/drivers/power/supply/surface_battery.c b/drivers/power/supply/surface_battery.c new file mode 100644 index ..b93a4f556b5c --- /dev/null +++ b/drivers/power/supply/surface_battery.c @@ -0,0 +1,901 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Battery driver for 7th-generation Microsoft Surface devices via Surface + * System Aggregator Module (SSAM). + * + * Copyright (C) 2019-2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + + +/* -- SAM interface. */ + +enum sam_event_cid_bat { + SAM_EVENT_CID_BAT_BIX = 0x15, + SAM_EVENT_CID_BAT_BST = 0x16, + SAM_EVENT_CID_BAT_ADP = 0x17, + SAM_EVENT_CID_BAT_PROT= 0x18, + SAM_EVENT_CID_BAT_DPTF= 0x53, +}; + +enum sam_battery_sta { + SAM_BATTERY_STA_OK= 0x0f, + SAM_BATTERY_STA_PRESENT = 0x10, +}; + +enum sam_bat
[PATCH 3/3] docs: driver-api: Add Surface DTX driver documentation
Add documentation for the user-space interface of the Surface DTX (detachment system) driver, used on Microsoft Surface Book series devices. Signed-off-by: Maximilian Luz --- .../surface_aggregator/clients/dtx.rst| 718 ++ .../surface_aggregator/clients/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 720 insertions(+) create mode 100644 Documentation/driver-api/surface_aggregator/clients/dtx.rst diff --git a/Documentation/driver-api/surface_aggregator/clients/dtx.rst b/Documentation/driver-api/surface_aggregator/clients/dtx.rst new file mode 100644 index ..e7e7c20007f0 --- /dev/null +++ b/Documentation/driver-api/surface_aggregator/clients/dtx.rst @@ -0,0 +1,718 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +.. |__u16| replace:: :c:type:`__u16 <__u16>` +.. |sdtx_event| replace:: :c:type:`struct sdtx_event ` +.. |sdtx_event_code| replace:: :c:type:`enum sdtx_event_code ` +.. |sdtx_base_info| replace:: :c:type:`struct sdtx_base_info ` +.. |sdtx_device_mode| replace:: :c:type:`struct sdtx_device_mode ` + +== +User-Space DTX (Clipboard Detachment System) Interface +== + +The ``surface_dtx`` driver is responsible for proper clipboard detachment +and re-attachment handling. To this end, it provides the ``/dev/surface/dtx`` +device file, through which it can interface with a user-space daemon. This +daemon is then ultimately responsible for determining and taking necessary +actions, such as unmounting devices attached to the base, +unloading/reloading the graphics-driver, user-notifications, etc. + +There are two basic communication principles used in this driver: Commands +(in other parts of the documentation also referred to as requests) and +events. Commands are sent to the EC and may have a different implications in +different contexts. Events are sent by the EC upon some internal state +change. Commands are always driver-initiated, whereas events are always +initiated by the EC. + +.. contents:: + +Nomenclature + + +* **Clipboard:** + The detachable upper part of the Surface Book, housing the screen and CPU. + +* **Base:** + The lower part of the Surface Book from which the clipboard can be + detached, optionally (model dependent) housing the discrete GPU (dGPU). + +* **Latch:** + The mechanism keeping the clipboard attached to the base in normal + operation and allowing it to be detached when requested. + +* **Silently ignored commands:** + The command is accepted by the EC as a valid command and acknowledged + (following the standard communication protocol), but the EC does not act + upon it, i.e. ignores it.e upper part of the + + +Detachment Process +== + +Warning: This part of the documentation is based on reverse engineering and +testing and thus may contain errors or be incomplete. + +Latch States + + +The latch mechanism has two major states: *open* and *closed*. In the +*closed* state (default), the clipboard is secured to the base, whereas in +the *open* state, the clipboard can be removed by a user. + +The latch can additionally be locked and, correspondingly, unlocked, which +can influence the detachment procedure. Specifically, this locking mechanism +is intended to prevent the dGPU, positioned in the base of the device, from +being hot-unplugged while in use. More details can be found in the +documentation for the detachment procedure below. By default, the latch is +unlocked. + +Detachment Procedure + + +Note that the detachment process is governed fully by the EC. The +``surface_dtx`` driver only relays events from the EC to user-space and +commands from user-space to the EC, i.e. it does not influence this process. + +The detachment process is started with the user pressing the *detach* button +on the base of the device or executing the ``SDTX_IOCTL_LATCH_REQUEST`` IOCTL. +Following that: + +1. The EC turns on the indicator led on the detach-button, sends a + *detach-request* event (``SDTX_EVENT_REQUEST``), and awaits further + instructions/commands. In case the latch is unlocked, the led will flash + green. If the latch has been locked, the led will be solid red + +2. The event is, via the ``surface_dtx`` driver, relayed to user-space, where + an appropriate user-space daemon can handle it and send instructions back + to the EC via IOCTLs provided by this driver. + +3. The EC waits for instructions from user-space and acts according to them. + If the EC does not receive any instructions in a given period, it will + time out and continue as follows: + + - If the latch is unlocked, the EC will open the latch and the clipboard + can be detached from the base. This is the exact behavior as without + this driver or any user-space daemon. See the ``SDTX_IOCTL_LATCH_CONFIRM`` + description below for more d
[PATCH 1/3] platform/surface: Add DTX driver
The Microsoft Surface Book series devices consist of a so-called clipboard part (containing the CPU, touchscreen, and primary battery) and a base part (containing keyboard, secondary battery, and optional discrete GPU). These parts can be separated, i.e. the clipboard can be detached and used as tablet. This detachment process is initiated by pressing a button. On the Surface Book 2 and 3 (targeted with this commit), the Surface Aggregator Module (i.e. the embedded controller on those devices) attempts to send a notification to any listening client driver and waits for further instructions (i.e. whether the detachment process should continue or be aborted). If it does not receive a response in a certain time-frame, the detachment process (by default) continues and the clipboard can be physically separated. In other words, (by default and) without a driver, the detachment process takes about 10 seconds to complete. This commit introduces a driver for this detachment system (called DTX). This driver allows a user-space daemon to control and influence the detachment behavior. Specifically, it forwards any detachment requests to user-space, allows user-space to make such requests itself, and allows handling of those requests. Requests can be handled by either aborting, continuing/allowing, or delaying (i.e. resetting the timeout via a heartbeat commend). The user-space API is implemented via the /dev/surface/dtx miscdevice. In addition, user-space can change the default behavior on timeout from allowing detachment to disallowing it, which is useful if the (optional) discrete GPU is in use. Furthermore, this driver allows user-space to receive notifications about the state of the base, specifically when it is physically removed (as opposed to detachment requested), in what manner it is connected (i.e. in reverse-/tent-/studio- or laptop-mode), and what type of base is connected. Based on this information, the driver also provides a simple tablet-mode switch (aliasing all modes without keyboard access, i.e. tablet-mode and studio-mode to its reported tablet-mode). An implementation of such a user-space daemon, allowing configuration of detachment behavior via scripts (e.g. safely unmounting USB devices connected to the base before continuing) can be found at [1]. [1]: https://github.com/linux-surface/surface-dtx-daemon Signed-off-by: Maximilian Luz --- .../userspace-api/ioctl/ioctl-number.rst |2 + MAINTAINERS |7 + drivers/platform/surface/Kconfig | 16 + drivers/platform/surface/Makefile |1 + drivers/platform/surface/surface_dtx.c| 1201 + include/uapi/linux/surface_aggregator/dtx.h | 146 ++ 6 files changed, 1373 insertions(+) create mode 100644 drivers/platform/surface/surface_dtx.c create mode 100644 include/uapi/linux/surface_aggregator/dtx.h diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst index 599bd4493944..1c28b8ef6677 100644 --- a/Documentation/userspace-api/ioctl/ioctl-number.rst +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst @@ -327,6 +327,8 @@ Code Seq#Include File Comments 0xA4 00-1F uapi/asm/sgx.h <mailto:linux-...@vger.kernel.org> 0xA5 01 linux/surface_aggregator/cdev.h Microsoft Surface Platform System Aggregator <mailto:luzmaximil...@gmail.com> +0xA5 20-2F linux/surface_aggregator/dtx.h Microsoft Surface DTX driver + <mailto:luzmaximil...@gmail.com> 0xAA 00-3F linux/uapi/linux/userfaultfd.h 0xAB 00-1F linux/nbd.h 0xAC 00-1F linux/raw.h diff --git a/MAINTAINERS b/MAINTAINERS index cf4cb8892623..adfc3a437db7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11861,6 +11861,13 @@ F: drivers/scsi/smartpqi/smartpqi*.[ch] F: include/linux/cciss*.h F: include/uapi/linux/cciss*.h +MICROSOFT SURFACE DTX DRIVER +M: Maximilian Luz +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/surface/surface_dtx.c +F: include/uapi/linux/surface_aggregator/dtx.h + MICROSOFT SURFACE GPE LID SUPPORT DRIVER M: Maximilian Luz L: platform-driver-...@vger.kernel.org diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index a045425026ae..98cf564fb17a 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -104,6 +104,22 @@ config SURFACE_AGGREGATOR_REGISTRY the respective client devices. Drivers for these devices still need to be selected via the other options. +config SURFACE_DTX + tristate "Surface DTX (Detachment System) Driver" + d
[PATCH 0/3] platform/surface: Add DTX detachment system driver
The Microsoft Surface Book series devices consist of a so-called clipboard part (containing the CPU, touchscreen, and primary battery) and a base part (containing keyboard, secondary battery, and optional discrete GPU). These parts can be separated, i.e. the clipboard can be detached and used as tablet. This detachment process is managed by a subsystem of the Surface System Aggregator Module (SSAM). As that process is a bit more complex, i.e. can involve user interaction, it seems the best way to implement this is to provide a somewhat cleaned-up version of this interface to userspace. This series adds a driver (and documentation) for the detachment system which provides such an interface. See the commit message of the first patch for more details and a link to a user-space daemon using this interface. Support for the Surface Book 3 is added in patch 2, user-space API documentation in patch 3. Regards, Max Maximilian Luz (3): platform/surface: Add DTX driver platform/surface: dtx: Add support for native SSAM devices docs: driver-api: Add Surface DTX driver documentation .../surface_aggregator/clients/dtx.rst| 718 + .../surface_aggregator/clients/index.rst |1 + .../userspace-api/ioctl/ioctl-number.rst |2 + MAINTAINERS |8 + drivers/platform/surface/Kconfig | 20 + drivers/platform/surface/Makefile |1 + drivers/platform/surface/surface_dtx.c| 1289 + include/uapi/linux/surface_aggregator/dtx.h | 146 ++ 8 files changed, 2185 insertions(+) create mode 100644 Documentation/driver-api/surface_aggregator/clients/dtx.rst create mode 100644 drivers/platform/surface/surface_dtx.c create mode 100644 include/uapi/linux/surface_aggregator/dtx.h -- 2.30.1
[PATCH 2/3] platform/surface: dtx: Add support for native SSAM devices
Add support for native SSAM devices to the DTX driver. This allows support for the Surface Book 3, on which the DTX device is not present in ACPI. Signed-off-by: Maximilian Luz --- drivers/platform/surface/Kconfig | 4 ++ drivers/platform/surface/surface_dtx.c | 90 +- 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 98cf564fb17a..3105f651614f 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -120,6 +120,10 @@ config SURFACE_DTX behavior of this process, which includes the option to abort it in case the base is still in use or speed it up in case it is not. + Note that this module can be built without support for the Surface + Aggregator Bus (i.e. CONFIG_SURFACE_AGGREGATOR_BUS=n). In that case, + some devices, specifically the Surface Book 3, will not be supported. + config SURFACE_GPE tristate "Surface GPE/Lid Support Driver" depends on DMI diff --git a/drivers/platform/surface/surface_dtx.c b/drivers/platform/surface/surface_dtx.c index 1301fab0ea14..85451eb94d98 100644 --- a/drivers/platform/surface/surface_dtx.c +++ b/drivers/platform/surface/surface_dtx.c @@ -27,6 +27,7 @@ #include #include +#include #include @@ -1194,7 +1195,94 @@ static struct platform_driver surface_dtx_platform_driver = { .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, }; -module_platform_driver(surface_dtx_platform_driver); + + +/* -- SSAM device driver. --- */ + +#ifdef CONFIG_SURFACE_AGGREGATOR_BUS + +static int surface_dtx_ssam_probe(struct ssam_device *sdev) +{ + struct sdtx_device *ddev; + + ddev = sdtx_device_create(>dev, sdev->ctrl); + if (IS_ERR(ddev)) + return PTR_ERR(ddev); + + ssam_device_set_drvdata(sdev, ddev); + return 0; +} + +static void surface_dtx_ssam_remove(struct ssam_device *sdev) +{ + sdtx_device_destroy(ssam_device_get_drvdata(sdev)); +} + +static const struct ssam_device_id surface_dtx_ssam_match[] = { + { SSAM_SDEV(BAS, 0x01, 0x00, 0x00) }, + { }, +}; +MODULE_DEVICE_TABLE(ssam, surface_dtx_ssam_match); + +static struct ssam_device_driver surface_dtx_ssam_driver = { + .probe = surface_dtx_ssam_probe, + .remove = surface_dtx_ssam_remove, + .match_table = surface_dtx_ssam_match, + .driver = { + .name = "surface_dtx", + .pm = _dtx_pm_ops, + .probe_type = PROBE_PREFER_ASYNCHRONOUS, + }, +}; + +static int ssam_dtx_driver_register(void) +{ + return ssam_device_driver_register(_dtx_ssam_driver); +} + +static void ssam_dtx_driver_unregister(void) +{ + ssam_device_driver_unregister(_dtx_ssam_driver); +} + +#else /* CONFIG_SURFACE_AGGREGATOR_BUS */ + +static int ssam_dtx_driver_register(void) +{ + return 0; +} + +static void ssam_dtx_driver_unregister(void) +{ +} + +#endif /* CONFIG_SURFACE_AGGREGATOR_BUS */ + + +/* -- Module setup. - */ + +static int __init surface_dtx_init(void) +{ + int status; + + status = ssam_dtx_driver_register(); + if (status) + return status; + + status = platform_driver_register(_dtx_platform_driver); + if (status) + ssam_dtx_driver_unregister(); + + return status; +} +module_init(surface_dtx_init); + +static void __exit surface_dtx_exit(void) +{ + platform_driver_unregister(_dtx_platform_driver); + ssam_dtx_driver_unregister(); +} +module_exit(surface_dtx_exit); MODULE_AUTHOR("Maximilian Luz "); MODULE_DESCRIPTION("Detachment-system driver for Surface System Aggregator Module"); -- 2.30.1
Re: [PATCH] Revert "pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"
On 3/8/21 5:46 PM, Andy Shevchenko wrote: On Mon, Mar 8, 2021 at 6:44 PM Maximilian Luz wrote: On 3/8/21 5:31 PM, Andy Shevchenko wrote: On Mon, Mar 08, 2021 at 05:35:59PM +0200, Andy Shevchenko wrote: On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: Following commit 036e126c72eb ("pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"), gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality should be there (they are defined in ACPI and have been accessible previously). Due to this, gpiod_get() fails with -ENOENT. Reverting this commit fixes that issue and the GPIOs in question are accessible again. I would like to have more information. Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg output (when kernel command line has 'ignore_loglevel' option) for both working and non-working cases? Also if it's possible to have DSDT.dsl of the device in question along with output of `grep -H 15 /sys/bus/acpi/devices/*/status`. There is probably a better option than straight up reverting this, so consider this more of a bug-report. Indeed. Can you test if the below helps (probably you have to apply it by editing the file manually): --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1392,6 +1392,7 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, gpps[i].size = min(gpp_size, npins); npins -= gpps[i].size; + gpps[i].gpio_base = gpps[i].base; gpps[i].padown_num = padown_num; That does fix the issue! Thanks for the fast response and fix! I'm about to send the formal patch. I will add the Reported-and-tested-by tag if you are okay with it. Sure thing. Thanks, Max
Re: [PATCH] Revert "pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"
On 3/8/21 5:42 PM, Andy Shevchenko wrote: On Mon, Mar 8, 2021 at 6:34 PM Maximilian Luz wrote: On 3/8/21 4:35 PM, Andy Shevchenko wrote: On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: Following commit 036e126c72eb ("pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"), gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality should be there (they are defined in ACPI and have been accessible previously). Due to this, gpiod_get() fails with -ENOENT. Reverting this commit fixes that issue and the GPIOs in question are accessible again. I would like to have more information. Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg output (when kernel command line has 'ignore_loglevel' option) for both working and non-working cases? Sure. Here are dmesg logs for: - Kernel v5.12-rc2 (not working): https://paste.ubuntu.com/p/HVZybcvQDH/ Thanks! Yeah, yeah... Please, test my patch, I am quite sure it will fix the issue. It does indeed, thanks again! Regards, Max
Re: [PATCH] Revert "pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"
On 3/8/21 5:31 PM, Andy Shevchenko wrote: On Mon, Mar 08, 2021 at 05:35:59PM +0200, Andy Shevchenko wrote: On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: Following commit 036e126c72eb ("pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"), gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality should be there (they are defined in ACPI and have been accessible previously). Due to this, gpiod_get() fails with -ENOENT. Reverting this commit fixes that issue and the GPIOs in question are accessible again. I would like to have more information. Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg output (when kernel command line has 'ignore_loglevel' option) for both working and non-working cases? Also if it's possible to have DSDT.dsl of the device in question along with output of `grep -H 15 /sys/bus/acpi/devices/*/status`. There is probably a better option than straight up reverting this, so consider this more of a bug-report. Indeed. Can you test if the below helps (probably you have to apply it by editing the file manually): --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1392,6 +1392,7 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, gpps[i].size = min(gpp_size, npins); npins -= gpps[i].size; + gpps[i].gpio_base = gpps[i].base; gpps[i].padown_num = padown_num; That does fix the issue! Thanks for the fast response and fix! Regards, Max
Re: [PATCH] Revert "pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"
On 3/8/21 4:35 PM, Andy Shevchenko wrote: On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: Following commit 036e126c72eb ("pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"), gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality should be there (they are defined in ACPI and have been accessible previously). Due to this, gpiod_get() fails with -ENOENT. Reverting this commit fixes that issue and the GPIOs in question are accessible again. I would like to have more information. Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg output (when kernel command line has 'ignore_loglevel' option) for both working and non-working cases? Sure. Here are dmesg logs for: - Kernel v5.12-rc2 (not working): https://paste.ubuntu.com/p/HVZybcvQDH/ - Kernel v5.12-rc2 with 036e126c72eb reverted: https://paste.ubuntu.com/p/hwcXFvhcBd/ Also if it's possible to have DSDT.dsl of the device in question along with output of `grep -H 15 /sys/bus/acpi/devices/*/status`. You can find the DSDT and a full ACPI dump at [1] and GPIOs that fail at [2] and [3]. [1]: https://github.com/linux-surface/acpidumps/tree/master/surface_book_2 [2]: https://github.com/linux-surface/acpidumps/blob/62972f0d806cef45ca01341e3cfbabc04c6dd583/surface_book_2/dsdt.dsl#L15274-L15285 [3]: https://github.com/linux-surface/acpidumps/blob/62972f0d806cef45ca01341e3cfbabc04c6dd583/surface_book_2/dsdt.dsl#L17947-L17982 `grep -H 15 /sys/bus/acpi/devices/*/status` yields /sys/bus/acpi/devices/ACPI0003:00/status:15 /sys/bus/acpi/devices/ACPI000C:00/status:15 /sys/bus/acpi/devices/ACPI000E:00/status:15 /sys/bus/acpi/devices/device:16/status:15 /sys/bus/acpi/devices/device:17/status:15 /sys/bus/acpi/devices/device:31/status:15 /sys/bus/acpi/devices/device:71/status:15 /sys/bus/acpi/devices/INT33A1:00/status:15 /sys/bus/acpi/devices/INT33BE:00/status:15 /sys/bus/acpi/devices/INT3400:00/status:15 /sys/bus/acpi/devices/INT3403:01/status:15 /sys/bus/acpi/devices/INT3403:02/status:15 /sys/bus/acpi/devices/INT3403:06/status:15 /sys/bus/acpi/devices/INT3403:07/status:15 /sys/bus/acpi/devices/INT3403:08/status:15 /sys/bus/acpi/devices/INT3403:09/status:15 /sys/bus/acpi/devices/INT3403:11/status:15 /sys/bus/acpi/devices/INT3407:00/status:15 /sys/bus/acpi/devices/INT344B:00/status:15 /sys/bus/acpi/devices/INT3472:00/status:15 /sys/bus/acpi/devices/INT3472:01/status:15 /sys/bus/acpi/devices/INT3472:02/status:15 /sys/bus/acpi/devices/INT347A:00/status:15 /sys/bus/acpi/devices/INT347E:00/status:15 /sys/bus/acpi/devices/INT3F0D:00/status:15 /sys/bus/acpi/devices/LNXPOWER:07/status:15 /sys/bus/acpi/devices/MSHW0005:00/status:15 /sys/bus/acpi/devices/MSHW0029:00/status:15 /sys/bus/acpi/devices/MSHW0036:00/status:15 /sys/bus/acpi/devices/MSHW0040:00/status:15 /sys/bus/acpi/devices/MSHW0042:00/status:15 /sys/bus/acpi/devices/MSHW0045:00/status:15 /sys/bus/acpi/devices/MSHW0084:00/status:15 /sys/bus/acpi/devices/MSHW0091:00/status:15 /sys/bus/acpi/devices/MSHW0107:00/status:15 /sys/bus/acpi/devices/MSHW0133:00/status:15 /sys/bus/acpi/devices/MSHW0153:00/status:15 /sys/bus/acpi/devices/NTC0103:00/status:15 /sys/bus/acpi/devices/PNP0103:00/status:15 /sys/bus/acpi/devices/PNP0C0D:00/status:15 This output is the same for both versions. There is probably a better option than straight up reverting this, so consider this more of a bug-report. Indeed.
[PATCH] Revert "pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"
Following commit 036e126c72eb ("pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"), gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality should be there (they are defined in ACPI and have been accessible previously). Due to this, gpiod_get() fails with -ENOENT. Reverting this commit fixes that issue and the GPIOs in question are accessible again. Signed-off-by: Maximilian Luz --- There is probably a better option than straight up reverting this, so consider this more of a bug-report. Regards, Max --- drivers/pinctrl/intel/pinctrl-intel.c | 60 +-- 1 file changed, 20 insertions(+), 40 deletions(-) diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 8085782cd8f9..0fe6caf98a8a 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1331,19 +1331,34 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq) return 0; } -static int intel_pinctrl_add_padgroups_by_gpps(struct intel_pinctrl *pctrl, - struct intel_community *community) +static int intel_pinctrl_add_padgroups(struct intel_pinctrl *pctrl, + struct intel_community *community) { struct intel_padgroup *gpps; + unsigned int npins = community->npins; unsigned int padown_num = 0; - size_t i, ngpps = community->ngpps; + size_t ngpps, i; + + if (community->gpps) + ngpps = community->ngpps; + else + ngpps = DIV_ROUND_UP(community->npins, community->gpp_size); gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL); if (!gpps) return -ENOMEM; for (i = 0; i < ngpps; i++) { - gpps[i] = community->gpps[i]; + if (community->gpps) { + gpps[i] = community->gpps[i]; + } else { + unsigned int gpp_size = community->gpp_size; + + gpps[i].reg_num = i; + gpps[i].base = community->pin_base + i * gpp_size; + gpps[i].size = min(gpp_size, npins); + npins -= gpps[i].size; + } if (gpps[i].size > 32) return -EINVAL; @@ -1361,38 +1376,6 @@ static int intel_pinctrl_add_padgroups_by_gpps(struct intel_pinctrl *pctrl, break; } - gpps[i].padown_num = padown_num; - padown_num += DIV_ROUND_UP(gpps[i].size * 4, 32); - } - - community->gpps = gpps; - - return 0; -} - -static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, - struct intel_community *community) -{ - struct intel_padgroup *gpps; - unsigned int npins = community->npins; - unsigned int padown_num = 0; - size_t i, ngpps = DIV_ROUND_UP(npins, community->gpp_size); - - if (community->gpp_size > 32) - return -EINVAL; - - gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL); - if (!gpps) - return -ENOMEM; - - for (i = 0; i < ngpps; i++) { - unsigned int gpp_size = community->gpp_size; - - gpps[i].reg_num = i; - gpps[i].base = community->pin_base + i * gpp_size; - gpps[i].size = min(gpp_size, npins); - npins -= gpps[i].size; - gpps[i].padown_num = padown_num; /* @@ -1529,10 +1512,7 @@ static int intel_pinctrl_probe(struct platform_device *pdev, community->regs = regs; community->pad_regs = regs + offset; - if (community->gpps) - ret = intel_pinctrl_add_padgroups_by_gpps(pctrl, community); - else - ret = intel_pinctrl_add_padgroups_by_size(pctrl, community); + ret = intel_pinctrl_add_padgroups(pctrl, community); if (ret) return ret; } -- 2.30.1
[PATCH] platform/surface: aggregator: Make SSAM_DEFINE_SYNC_REQUEST_x define static functions
The SSAM_DEFINE_SYNC_REQUEST_x() macros are intended to reduce boiler-plate code for SSAM request definitions by defining a wrapper function for the specified request. The client device variants of those macros, i.e. SSAM_DEFINE_SYNC_REQUEST_CL_x() in particular rely on the multi-device (MD) variants, e.g.: #define SSAM_DEFINE_SYNC_REQUEST_CL_R(name, rtype, spec...) \ SSAM_DEFINE_SYNC_REQUEST_MD_R(__raw_##name, rtype, spec) \ int name(struct ssam_device *sdev, rtype *ret)\ { \ return __raw_##name(sdev->ctrl, sdev->uid.target, \ sdev->uid.instance, ret); \ } This now creates the problem that it is not possible to declare the generated functions static via static SSAM_DEFINE_SYNC_REQUEST_CL_R(...) as this will only apply to the function defined by the multi-device macro, i.e. SSAM_DEFINE_SYNC_REQUEST_MD_R(). Thus compiling with `-Wmissing-prototypes' rightfully complains that there is a 'static' keyword missing. To solve this, make all SSAM_DEFINE_SYNC_REQUEST_x() macros define static functions. Non-client-device macros are also changed for consistency. In general, we expect those functions to be only used locally in the respective drivers for the corresponding interfaces, so having to define a wrapper function to be able to export this should be the odd case out. Reported-by: kernel test robot Fixes: 510c8114fc74 ("platform/surface: Add platform profile driver") Signed-off-by: Maximilian Luz --- .../driver-api/surface_aggregator/client.rst | 4 +- .../platform/surface/aggregator/controller.c | 10 +-- .../surface/surface_aggregator_registry.c | 2 +- .../surface/surface_platform_profile.c| 4 +- include/linux/surface_aggregator/controller.h | 74 +-- include/linux/surface_aggregator/device.h | 31 6 files changed, 63 insertions(+), 62 deletions(-) diff --git a/Documentation/driver-api/surface_aggregator/client.rst b/Documentation/driver-api/surface_aggregator/client.rst index 26d13085a117..e519d374c378 100644 --- a/Documentation/driver-api/surface_aggregator/client.rst +++ b/Documentation/driver-api/surface_aggregator/client.rst @@ -248,7 +248,7 @@ This example defines a function .. code-block:: c - int __ssam_tmp_perf_mode_set(struct ssam_controller *ctrl, const __le32 *arg); + static int __ssam_tmp_perf_mode_set(struct ssam_controller *ctrl, const __le32 *arg); executing the specified request, with the controller passed in when calling said function. In this example, the argument is provided via the ``arg`` @@ -296,7 +296,7 @@ This invocation of the macro defines a function .. code-block:: c - int ssam_bat_get_sta(struct ssam_device *sdev, __le32 *ret); + static int ssam_bat_get_sta(struct ssam_device *sdev, __le32 *ret); executing the specified request, using the device IDs and controller given in the client device. The full list of such macros for client devices is: diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c index 5bcb59ed579d..aa6f37b4f46e 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -1750,35 +1750,35 @@ EXPORT_SYMBOL_GPL(ssam_request_sync_with_buffer); /* -- Internal SAM requests. */ -static SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_get_firmware_version, __le32, { +SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_get_firmware_version, __le32, { .target_category = SSAM_SSH_TC_SAM, .target_id = 0x01, .command_id = 0x13, .instance_id = 0x00, }); -static SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_display_off, u8, { +SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_display_off, u8, { .target_category = SSAM_SSH_TC_SAM, .target_id = 0x01, .command_id = 0x15, .instance_id = 0x00, }); -static SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_display_on, u8, { +SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_display_on, u8, { .target_category = SSAM_SSH_TC_SAM, .target_id = 0x01, .command_id = 0x16, .instance_id = 0x00, }); -static SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_d0_exit, u8, { +SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_d0_exit, u8, { .target_category = SSAM_SSH_TC_SAM, .target_id = 0x01, .command_id = 0x33, .instance_id = 0x00, }); -static SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_d0_entry, u8, { +SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_d0_entry, u8, { .target_category = SSAM_SSH_TC_SAM, .target_id = 0x01, .command_id = 0x34, diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregat
[PATCH v3 6/6] platform/surface: aggregator_registry: Add HID subsystem devices
Add HID subsystem (TC=0x15) devices. These devices need to be registered for 7th-generation Surface models. On previous generations, these devices are either provided as platform devices via ACPI (Surface Laptop 1 and 2) or implemented as standard USB device. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 49 +++ 1 file changed, 49 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index dc044d06828b..caee90d135c5 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -77,6 +77,48 @@ static const struct software_node ssam_node_bas_dtx = { .parent = _node_root, }; +/* HID keyboard. */ +static const struct software_node ssam_node_hid_main_keyboard = { + .name = "ssam:01:15:02:01:00", + .parent = _node_root, +}; + +/* HID touchpad. */ +static const struct software_node ssam_node_hid_main_touchpad = { + .name = "ssam:01:15:02:03:00", + .parent = _node_root, +}; + +/* HID device instance 5 (unknown HID device). */ +static const struct software_node ssam_node_hid_main_iid5 = { + .name = "ssam:01:15:02:05:00", + .parent = _node_root, +}; + +/* HID keyboard (base hub). */ +static const struct software_node ssam_node_hid_base_keyboard = { + .name = "ssam:01:15:02:01:00", + .parent = _node_hub_base, +}; + +/* HID touchpad (base hub). */ +static const struct software_node ssam_node_hid_base_touchpad = { + .name = "ssam:01:15:02:03:00", + .parent = _node_hub_base, +}; + +/* HID device instance 5 (unknown HID device, base hub). */ +static const struct software_node ssam_node_hid_base_iid5 = { + .name = "ssam:01:15:02:05:00", + .parent = _node_hub_base, +}; + +/* HID device instance 6 (unknown HID device, base hub). */ +static const struct software_node ssam_node_hid_base_iid6 = { + .name = "ssam:01:15:02:06:00", + .parent = _node_hub_base, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -93,6 +135,10 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_sb3base, _node_tmp_pprof, _node_bas_dtx, + _node_hid_base_keyboard, + _node_hid_base_touchpad, + _node_hid_base_iid5, + _node_hid_base_iid6, NULL, }; @@ -116,6 +162,9 @@ static const struct software_node *ssam_node_group_sl3[] = { _node_bat_ac, _node_bat_main, _node_tmp_pprof, + _node_hid_main_keyboard, + _node_hid_main_touchpad, + _node_hid_main_iid5, NULL, }; -- 2.30.1
[PATCH v3 5/6] platform/surface: aggregator_registry: Add DTX device
Add the detachment system (DTX) SSAM device for the Surface Book 3. This device is accessible under the base (TC=0x11) subsystem. Signed-off-by: Maximilian Luz --- drivers/platform/surface/surface_aggregator_registry.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 33904613dd4b..dc044d06828b 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -71,6 +71,12 @@ static const struct software_node ssam_node_tmp_pprof = { .parent = _node_root, }; +/* DTX / detachment-system device (Surface Book 3). */ +static const struct software_node ssam_node_bas_dtx = { + .name = "ssam:01:11:01:00:00", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -86,6 +92,7 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_main, _node_bat_sb3base, _node_tmp_pprof, + _node_bas_dtx, NULL, }; -- 2.30.1
[PATCH v3 4/6] platform/surface: aggregator_registry: Add platform profile device
Add the SSAM platform profile device to the SSAM device registry. This device is accessible under the thermal subsystem (TC=0x03) and needs to be registered for all Surface models. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 15 +++ 1 file changed, 15 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index cde279692842..33904613dd4b 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -65,9 +65,16 @@ static const struct software_node ssam_node_bat_sb3base = { .parent = _node_hub_base, }; +/* Platform profile / performance-mode device. */ +static const struct software_node ssam_node_tmp_pprof = { + .name = "ssam:01:03:01:00:01", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -78,18 +85,21 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_ac, _node_bat_main, _node_bat_sb3base, + _node_tmp_pprof, NULL, }; /* Devices for Surface Laptop 1. */ static const struct software_node *ssam_node_group_sl1[] = { _node_root, + _node_tmp_pprof, NULL, }; /* Devices for Surface Laptop 2. */ static const struct software_node *ssam_node_group_sl2[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -98,6 +108,7 @@ static const struct software_node *ssam_node_group_sl3[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; @@ -106,18 +117,21 @@ static const struct software_node *ssam_node_group_slg1[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; /* Devices for Surface Pro 5. */ static const struct software_node *ssam_node_group_sp5[] = { _node_root, + _node_tmp_pprof, NULL, }; /* Devices for Surface Pro 6. */ static const struct software_node *ssam_node_group_sp6[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -126,6 +140,7 @@ static const struct software_node *ssam_node_group_sp7[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; -- 2.30.1
[PATCH v3 3/6] platform/surface: aggregator_registry: Add battery subsystem devices
Add battery subsystem (TC=0x02) devices (battery and AC) to the SSAM device registry. These devices need to be registered for 7th-generation Surface models. On 5th- and 6th-generation models, these devices are handled via the standard ACPI battery/AC interface, which in turn accesses the same SSAM interface via the Surface ACPI Notify (SAN) driver. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 27 +++ 1 file changed, 27 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 6c23d75a044c..cde279692842 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -47,6 +47,24 @@ static const struct software_node ssam_node_hub_base = { .parent = _node_root, }; +/* AC adapter. */ +static const struct software_node ssam_node_bat_ac = { + .name = "ssam:01:02:01:01:01", + .parent = _node_root, +}; + +/* Primary battery. */ +static const struct software_node ssam_node_bat_main = { + .name = "ssam:01:02:01:01:00", + .parent = _node_root, +}; + +/* Secondary battery (Surface Book 3). */ +static const struct software_node ssam_node_bat_sb3base = { + .name = "ssam:01:02:02:01:00", + .parent = _node_hub_base, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -57,6 +75,9 @@ static const struct software_node *ssam_node_group_sb2[] = { static const struct software_node *ssam_node_group_sb3[] = { _node_root, _node_hub_base, + _node_bat_ac, + _node_bat_main, + _node_bat_sb3base, NULL, }; @@ -75,12 +96,16 @@ static const struct software_node *ssam_node_group_sl2[] = { /* Devices for Surface Laptop 3. */ static const struct software_node *ssam_node_group_sl3[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; /* Devices for Surface Laptop Go. */ static const struct software_node *ssam_node_group_slg1[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; @@ -99,6 +124,8 @@ static const struct software_node *ssam_node_group_sp6[] = { /* Devices for Surface Pro 7. */ static const struct software_node *ssam_node_group_sp7[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; -- 2.30.1
[PATCH v3 2/6] platform/surface: aggregator_registry: Add base device hub
The Surface Book 3 has a detachable base part. While the top part (so-called clipboard) contains the CPU, touchscreen, and primary battery, the base contains, among other things, a keyboard, touchpad, and secondary battery. Those devices do not react well to being accessed when the base part is detached and should thus be removed and added in sync with the base. To facilitate this, we introduce a virtual base device hub, which automatically removes or adds the devices registered under it. Signed-off-by: Maximilian Luz --- Changes in v3: - Fix use of lockdep_assert_held() --- .../surface/surface_aggregator_registry.c | 261 +- 1 file changed, 260 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index a051d941ad96..6c23d75a044c 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -11,9 +11,12 @@ #include #include +#include #include +#include #include #include +#include #include #include @@ -38,6 +41,12 @@ static const struct software_node ssam_node_root = { .name = "ssam_platform_hub", }; +/* Base device hub (devices attached to Surface Book 3 base). */ +static const struct software_node ssam_node_hub_base = { + .name = "ssam:00:00:02:00:00", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -47,6 +56,7 @@ static const struct software_node *ssam_node_group_sb2[] = { /* Devices for Surface Book 3. */ static const struct software_node *ssam_node_group_sb3[] = { _node_root, + _node_hub_base, NULL, }; @@ -177,6 +187,230 @@ static int ssam_hub_add_devices(struct device *parent, struct ssam_controller *c } +/* -- SSAM base-hub driver. - */ + +enum ssam_base_hub_state { + SSAM_BASE_HUB_UNINITIALIZED, + SSAM_BASE_HUB_CONNECTED, + SSAM_BASE_HUB_DISCONNECTED, +}; + +struct ssam_base_hub { + struct ssam_device *sdev; + + struct mutex lock; /* Guards state update checks and transitions. */ + enum ssam_base_hub_state state; + + struct ssam_event_notifier notif; +}; + +static SSAM_DEFINE_SYNC_REQUEST_R(ssam_bas_query_opmode, u8, { + .target_category = SSAM_SSH_TC_BAS, + .target_id = 0x01, + .command_id = 0x0d, + .instance_id = 0x00, +}); + +#define SSAM_BAS_OPMODE_TABLET 0x00 +#define SSAM_EVENT_BAS_CID_CONNECTION 0x0c + +static int ssam_base_hub_query_state(struct ssam_base_hub *hub, enum ssam_base_hub_state *state) +{ + u8 opmode; + int status; + + status = ssam_retry(ssam_bas_query_opmode, hub->sdev->ctrl, ); + if (status < 0) { + dev_err(>sdev->dev, "failed to query base state: %d\n", status); + return status; + } + + if (opmode != SSAM_BAS_OPMODE_TABLET) + *state = SSAM_BASE_HUB_CONNECTED; + else + *state = SSAM_BASE_HUB_DISCONNECTED; + + return 0; +} + +static ssize_t ssam_base_hub_state_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct ssam_base_hub *hub = dev_get_drvdata(dev); + bool connected; + + mutex_lock(>lock); + connected = hub->state == SSAM_BASE_HUB_CONNECTED; + mutex_unlock(>lock); + + return sysfs_emit(buf, "%d\n", connected); +} + +static struct device_attribute ssam_base_hub_attr_state = + __ATTR(state, 0444, ssam_base_hub_state_show, NULL); + +static struct attribute *ssam_base_hub_attrs[] = { + _base_hub_attr_state.attr, + NULL, +}; + +const struct attribute_group ssam_base_hub_group = { + .attrs = ssam_base_hub_attrs, +}; + +static int __ssam_base_hub_update(struct ssam_base_hub *hub, enum ssam_base_hub_state new) +{ + struct fwnode_handle *node = dev_fwnode(>sdev->dev); + int status = 0; + + lockdep_assert_held(>lock); + + if (hub->state == new) + return 0; + hub->state = new; + + if (hub->state == SSAM_BASE_HUB_CONNECTED) + status = ssam_hub_add_devices(>sdev->dev, hub->sdev->ctrl, node); + else + ssam_hub_remove_devices(>sdev->dev); + + if (status) + dev_err(>sdev->dev, "failed to update base-hub devices: %d\n", status); + + return status; +} + +static int ssam_base_hub_update(struct ssam_base_hub *hub) +{ + enum ssam_base_hub_state state; + int status; + + mutex_lock(>lock); + + status = ssam_base_hub_query_state(hub, ); + if (!status) + status = __ssam_base_hu
[PATCH v3 0/6] platform/surface: Add Surface Aggregator device registry
The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This series introduces a device registry based on software nodes and device hubs to solve this problem. The registry is intended to contain all required non-detectable information. The platform hub driver is loaded against the WSID device and instantiates and manages SSAM devices based on the information provided by the registry for the given WSID HID of the Surface model. All new devices created by this hub added as child devices to this hub. In addition, a base hub is introduced to manage devices associated with the detachable base part of the Surface Book 3, as this requires special handling (i.e. devices need to be removed when the base is removed). Again, all devices created by the base hub (i.e. associated with the base) are added as child devices to this hub. In total, this will yield the following device structure WSID |- SSAM device 1 (physical device) |- SSAM device 2 (physical device) |- SSAM device 3 (physical device) |- ... \- SSAM base hub (virtual device) |- SSAM base device 1 (physical device) |- SSAM base device 2 (physical device) |- ... While software nodes seem to be well suited for this approach due to extensibility, they still need to be hard-coded, so I'm open for ideas on how this could be improved. Changes in v2: - Fix Kconfig dependency Changes in v3: - Fix use of lockdep_assert_held() Maximilian Luz (6): platform/surface: Set up Surface Aggregator device registry platform/surface: aggregator_registry: Add base device hub platform/surface: aggregator_registry: Add battery subsystem devices platform/surface: aggregator_registry: Add platform profile device platform/surface: aggregator_registry: Add DTX device platform/surface: aggregator_registry: Add HID subsystem devices MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 27 + drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 641 ++ 4 files changed, 670 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c -- 2.30.1
[PATCH v3 1/6] platform/surface: Set up Surface Aggregator device registry
The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This commit introduces a registry providing non-detectable device information via software nodes. In addition, a SSAM platform hub driver is introduced, which takes care of creating and managing the SSAM devices specified in this registry. This approach allows for a hierarchical setup akin to ACPI and is easily extendable, e.g. via firmware node properties. Note that this commit only provides the basis for the platform hub and registry, and does not add any content to it. The registry will be expanded in subsequent commits. Signed-off-by: Maximilian Luz --- Changes in v2: - Fix Kconfig dependency --- MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 27 ++ drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 284 ++ 4 files changed, 313 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c diff --git a/MAINTAINERS b/MAINTAINERS index 4fcf3df517a8..000a82f59c76 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11826,6 +11826,7 @@ F: Documentation/driver-api/surface_aggregator/ F: drivers/platform/surface/aggregator/ F: drivers/platform/surface/surface_acpi_notify.c F: drivers/platform/surface/surface_aggregator_cdev.c +F: drivers/platform/surface/surface_aggregator_registry.c F: include/linux/surface_acpi_notify.h F: include/linux/surface_aggregator/ F: include/uapi/linux/surface_aggregator/ diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 0847b2dc97bf..179b8c93d7fd 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -77,6 +77,33 @@ config SURFACE_AGGREGATOR_CDEV The provided interface is intended for debugging and development only, and should not be used otherwise. +config SURFACE_AGGREGATOR_REGISTRY + tristate "Surface System Aggregator Module Device Registry" + depends on SURFACE_AGGREGATOR + depends on SURFACE_AGGREGATOR_BUS + help + Device-registry and device-hubs for Surface System Aggregator Module + (SSAM) devices. + + Provides a module and driver which act as a device-registry for SSAM + client devices that cannot be detected automatically, e.g. via ACPI. + Such devices are instead provided via this registry and attached via + device hubs, also provided in this module. + + Devices provided via this registry are: + - Platform profile (performance-/cooling-mode) device (5th- and later + generations). + - Battery/AC devices (7th-generation). + - HID input devices (7th-generation). + + Select M (recommended) or Y here if you want support for the above + mentioned devices on the corresponding Surface models. Without this + module, the respective devices will not be instantiated and thus any + functionality provided by them will be missing, even when drivers for + these devices are present. In other words, this module only provides + the respective client devices. Drivers for these devices still need to + be selected via the other options. + config SURFACE_GPE tristate "Surface GPE/Lid Support Driver" depends on DMI diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile index 990424c5f0c9..80035ee540bf 100644 --- a/drivers/platform/surface/Makefile +++ b/drivers/platform/surface/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_SURFACE_3_POWER_OPREGION)+= surface3_power.o obj-$(CONFIG_SURFACE_ACPI_NOTIFY) += surface_acpi_notify.o obj-$(CONFIG_SURFACE_AGGREGATOR) += aggregator/ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o +obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
[PATCH v2 4/4] platform/surface: Add platform profile driver
Add a driver to provide platform profile support on 5th- and later generation Microsoft Surface devices with a Surface System Aggregator Module. On those devices, the platform profile can be used to influence cooling behavior and power consumption. For example, the default 'quiet' profile limits fan noise and in turn sacrifices performance of the discrete GPU found on Surface Books. Its full performance can only be unlocked on the 'performance' profile. Signed-off-by: Maximilian Luz --- MAINTAINERS | 6 + drivers/platform/surface/Kconfig | 22 ++ drivers/platform/surface/Makefile | 1 + .../surface/surface_platform_profile.c| 190 ++ 4 files changed, 219 insertions(+) create mode 100644 drivers/platform/surface/surface_platform_profile.c diff --git a/MAINTAINERS b/MAINTAINERS index 000a82f59c76..a08d65f8f0df 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11811,6 +11811,12 @@ L: platform-driver-...@vger.kernel.org S: Maintained F: drivers/platform/surface/surface_hotplug.c +MICROSOFT SURFACE PLATFORM PROFILE DRIVER +M: Maximilian Luz +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/surface/surface_platform_profile.c + MICROSOFT SURFACE PRO 3 BUTTON DRIVER M: Chen Yu L: platform-driver-...@vger.kernel.org diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 179b8c93d7fd..a045425026ae 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -132,6 +132,28 @@ config SURFACE_HOTPLUG Select M or Y here, if you want to (fully) support hot-plugging of dGPU devices on the Surface Book 2 and/or 3 during D3cold. +config SURFACE_PLATFORM_PROFILE + tristate "Surface Platform Profile Driver" + depends on SURFACE_AGGREGATOR_REGISTRY + select ACPI_PLATFORM_PROFILE + help + Provides support for the ACPI platform profile on 5th- and later + generation Microsoft Surface devices. + + More specifically, this driver provides ACPI platform profile support + on Microsoft Surface devices with a Surface System Aggregator Module + (SSAM) connected via the Surface Serial Hub (SSH / SAM-over-SSH). In + other words, this driver provides platform profile support on the + Surface Pro 5, Surface Book 2, Surface Laptop, Surface Laptop Go and + later. On those devices, the platform profile can significantly + influence cooling behavior, e.g. setting it to 'quiet' (default) or + 'low-power' can significantly limit performance of the discrete GPU on + Surface Books, while in turn leading to lower power consumption and/or + less fan noise. + + Select M or Y here, if you want to include ACPI platform profile + support on the above mentioned devices. + config SURFACE_PRO3_BUTTON tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet" depends on INPUT diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile index 80035ee540bf..99372c427b73 100644 --- a/drivers/platform/surface/Makefile +++ b/drivers/platform/surface/Makefile @@ -13,4 +13,5 @@ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o +obj-$(CONFIG_SURFACE_PLATFORM_PROFILE) += surface_platform_profile.o obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c new file mode 100644 index ..0081b01a5b0f --- /dev/null +++ b/drivers/platform/surface/surface_platform_profile.c @@ -0,0 +1,190 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Surface Platform Profile / Performance Mode driver for Surface System + * Aggregator Module (thermal subsystem). + * + * Copyright (C) 2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include + +#include + +enum ssam_tmp_profile { + SSAM_TMP_PROFILE_NORMAL = 1, + SSAM_TMP_PROFILE_BATTERY_SAVER = 2, + SSAM_TMP_PROFILE_BETTER_PERFORMANCE = 3, + SSAM_TMP_PROFILE_BEST_PERFORMANCE = 4, +}; + +struct ssam_tmp_profile_info { + __le32 profile; + __le16 unknown1; + __le16 unknown2; +} __packed; + +struct ssam_tmp_profile_device { + struct ssam_device *sdev; + struct platform_profile_handler handler; +}; + +static SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_profile_get, struct ssam_tmp_profile_info, { + .target_category = SSAM_SSH_TC_TMP, + .command_id = 0x02, +}); + +static SSAM_DEFINE_SYNC_REQUEST_CL_W(__ssam_tmp_profile_set, __le32, { +
[PATCH v2 3/4] ACPI: platform: Add balanced-performance platform profile
Some devices, including most Microsoft Surface devices, have a platform profile somewhere inbetween balanced and performance. More specifically, adding this profile allows the following mapping on Surface devices: Vendor Name Platform Profile -- Battery Saver low-power Recommended balanced Better Performancebalanced-performance Best Performance performance Suggested-by: Hans de Goede Signed-off-by: Maximilian Luz --- .../ABI/testing/sysfs-platform_profile | 18 +++--- drivers/acpi/platform_profile.c| 1 + include/linux/platform_profile.h | 1 + 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-platform_profile b/Documentation/ABI/testing/sysfs-platform_profile index 9d6b89b66cca..dae9c8941905 100644 --- a/Documentation/ABI/testing/sysfs-platform_profile +++ b/Documentation/ABI/testing/sysfs-platform_profile @@ -5,13 +5,17 @@ Description: This file contains a space-separated list of profiles supported for Drivers must use the following standard profile-names: - - low-power Low power consumption - coolCooler operation - quiet Quieter operation - balancedBalance between low power consumption and performance - performance High performance operation - + + low-power Low power consumption + coolCooler operation + quiet Quieter operation + balancedBalance between low power consumption + and performance + balanced-performanceBalance between performance and low + power consumption with a slight bias + towards performance + performance High performance operation + Userspace may expect drivers to offer more than one of these standard profile names. diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c index 4a59c5993bde..dd2fbf38e414 100644 --- a/drivers/acpi/platform_profile.c +++ b/drivers/acpi/platform_profile.c @@ -17,6 +17,7 @@ static const char * const profile_names[] = { [PLATFORM_PROFILE_COOL] = "cool", [PLATFORM_PROFILE_QUIET] = "quiet", [PLATFORM_PROFILE_BALANCED] = "balanced", + [PLATFORM_PROFILE_BALANCED_PERFORMANCE] = "balanced-performance", [PLATFORM_PROFILE_PERFORMANCE] = "performance", }; static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST); diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h index b4794027e759..a6329003aee7 100644 --- a/include/linux/platform_profile.h +++ b/include/linux/platform_profile.h @@ -21,6 +21,7 @@ enum platform_profile_option { PLATFORM_PROFILE_COOL, PLATFORM_PROFILE_QUIET, PLATFORM_PROFILE_BALANCED, + PLATFORM_PROFILE_BALANCED_PERFORMANCE, PLATFORM_PROFILE_PERFORMANCE, PLATFORM_PROFILE_LAST, /*must always be last */ }; -- 2.30.0
[PATCH v2 2/4] ACPI: platform: Fix file references in comment
The referenced files are named slightly different. Replace '-' with '_' and drop the .rst ending. Signed-off-by: Maximilian Luz --- include/linux/platform_profile.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h index a26542d53058..b4794027e759 100644 --- a/include/linux/platform_profile.h +++ b/include/linux/platform_profile.h @@ -12,9 +12,8 @@ #include /* - * If more options are added please update profile_names - * array in platform-profile.c and sysfs-platform-profile.rst - * documentation. + * If more options are added please update profile_names array in + * platform_profile.c and sysfs-platform_profile documentation. */ enum platform_profile_option { -- 2.30.0
[PATCH v2 1/4] ACPI: platform: Hide ACPI_PLATFORM_PROFILE option
The ACPI_PLATFORM_PROFILE option essentially provides a library and not really an independent module. Thus it seems to be more user-friendly to hide this option and simply make drivers depending on it select it. Suggested-by: Hans de Goede Signed-off-by: Maximilian Luz --- drivers/acpi/Kconfig | 16 +--- drivers/platform/x86/Kconfig | 4 ++-- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 5ddff93e38c2..030678965159 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -327,21 +327,7 @@ config ACPI_THERMAL the module will be called thermal. config ACPI_PLATFORM_PROFILE - tristate "ACPI Platform Profile Driver" - default m - help - This driver adds support for platform-profiles on platforms that - support it. - - Platform-profiles can be used to control the platform behaviour. For - example whether to operate in a lower power mode, in a higher - power performance mode or between the two. - - This driver provides the sysfs interface and is used as the registration - point for platform specific drivers. - - Which profiles are supported is determined on a per-platform basis and - should be obtained from the platform specific driver. + tristate config ACPI_CUSTOM_DSDT_FILE string "Custom DSDT Table file to include" diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 56353e8c792a..ad4e630e73e2 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -450,7 +450,7 @@ config IDEAPAD_LAPTOP depends on BACKLIGHT_CLASS_DEVICE depends on ACPI_VIDEO || ACPI_VIDEO = n depends on ACPI_WMI || ACPI_WMI = n - depends on ACPI_PLATFORM_PROFILE + select ACPI_PLATFORM_PROFILE select INPUT_SPARSEKMAP select NEW_LEDS select LEDS_CLASS @@ -484,7 +484,7 @@ config THINKPAD_ACPI depends on RFKILL || RFKILL = n depends on ACPI_VIDEO || ACPI_VIDEO = n depends on BACKLIGHT_CLASS_DEVICE - depends on ACPI_PLATFORM_PROFILE + select ACPI_PLATFORM_PROFILE select HWMON select NVRAM select NEW_LEDS -- 2.30.0
[PATCH v2 0/4] platform/surface: Add platform profile driver for Surface devices
This series adds a driver to provide platform profile support on 5th- and later generation Microsoft Surface devices with a Surface System Aggregator Module. On those devices, the platform profile can be used to influence cooling behavior and power consumption. To achieve this, a new platform profile is introduced: the 'balanced-performance' profile. In addition, a couple of fix-ups are performed: - Hide CONFIG_ACPI_PLATFORM_PROFILE and change drivers so that it is selected instead of depended on. - Fix some references to documentation in a comment. Note: This series (or more specifically "platform/surface: Add platform profile driver") depends on the "platform/surface: Add Surface Aggregator device registry" series. Changes in v2: - Introduce new 'balanced-performance' platform profile and change profile mapping in driver. - Perform some fix-ups for the ACPI platform profile implementation: - Fix some references to documentation in a comment. - Hide CONFIG_ACPI_PLATFORM_PROFILE Maximilian Luz (4): ACPI: platform: Hide ACPI_PLATFORM_PROFILE option ACPI: platform: Fix file references in comment ACPI: platform: Add balanced-performance platform profile platform/surface: Add platform profile driver .../ABI/testing/sysfs-platform_profile| 18 +- MAINTAINERS | 6 + drivers/acpi/Kconfig | 16 +- drivers/acpi/platform_profile.c | 1 + drivers/platform/surface/Kconfig | 22 ++ drivers/platform/surface/Makefile | 1 + .../surface/surface_platform_profile.c| 190 ++ drivers/platform/x86/Kconfig | 4 +- include/linux/platform_profile.h | 6 +- 9 files changed, 237 insertions(+), 27 deletions(-) create mode 100644 drivers/platform/surface/surface_platform_profile.c -- 2.30.0
[PATCH v2 2/6] platform/surface: aggregator_registry: Add base device hub
The Surface Book 3 has a detachable base part. While the top part (so-called clipboard) contains the CPU, touchscreen, and primary battery, the base contains, among other things, a keyboard, touchpad, and secondary battery. Those devices do not react well to being accessed when the base part is detached and should thus be removed and added in sync with the base. To facilitate this, we introduce a virtual base device hub, which automatically removes or adds the devices registered under it. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 261 +- 1 file changed, 260 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index a051d941ad96..0d802804594c 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -11,9 +11,12 @@ #include #include +#include #include +#include #include #include +#include #include #include @@ -38,6 +41,12 @@ static const struct software_node ssam_node_root = { .name = "ssam_platform_hub", }; +/* Base device hub (devices attached to Surface Book 3 base). */ +static const struct software_node ssam_node_hub_base = { + .name = "ssam:00:00:02:00:00", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -47,6 +56,7 @@ static const struct software_node *ssam_node_group_sb2[] = { /* Devices for Surface Book 3. */ static const struct software_node *ssam_node_group_sb3[] = { _node_root, + _node_hub_base, NULL, }; @@ -177,6 +187,230 @@ static int ssam_hub_add_devices(struct device *parent, struct ssam_controller *c } +/* -- SSAM base-hub driver. - */ + +enum ssam_base_hub_state { + SSAM_BASE_HUB_UNINITIALIZED, + SSAM_BASE_HUB_CONNECTED, + SSAM_BASE_HUB_DISCONNECTED, +}; + +struct ssam_base_hub { + struct ssam_device *sdev; + + struct mutex lock; /* Guards state update checks and transitions. */ + enum ssam_base_hub_state state; + + struct ssam_event_notifier notif; +}; + +static SSAM_DEFINE_SYNC_REQUEST_R(ssam_bas_query_opmode, u8, { + .target_category = SSAM_SSH_TC_BAS, + .target_id = 0x01, + .command_id = 0x0d, + .instance_id = 0x00, +}); + +#define SSAM_BAS_OPMODE_TABLET 0x00 +#define SSAM_EVENT_BAS_CID_CONNECTION 0x0c + +static int ssam_base_hub_query_state(struct ssam_base_hub *hub, enum ssam_base_hub_state *state) +{ + u8 opmode; + int status; + + status = ssam_retry(ssam_bas_query_opmode, hub->sdev->ctrl, ); + if (status < 0) { + dev_err(>sdev->dev, "failed to query base state: %d\n", status); + return status; + } + + if (opmode != SSAM_BAS_OPMODE_TABLET) + *state = SSAM_BASE_HUB_CONNECTED; + else + *state = SSAM_BASE_HUB_DISCONNECTED; + + return 0; +} + +static ssize_t ssam_base_hub_state_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct ssam_base_hub *hub = dev_get_drvdata(dev); + bool connected; + + mutex_lock(>lock); + connected = hub->state == SSAM_BASE_HUB_CONNECTED; + mutex_unlock(>lock); + + return sysfs_emit(buf, "%d\n", connected); +} + +static struct device_attribute ssam_base_hub_attr_state = + __ATTR(state, 0444, ssam_base_hub_state_show, NULL); + +static struct attribute *ssam_base_hub_attrs[] = { + _base_hub_attr_state.attr, + NULL, +}; + +const struct attribute_group ssam_base_hub_group = { + .attrs = ssam_base_hub_attrs, +}; + +static int __ssam_base_hub_update(struct ssam_base_hub *hub, enum ssam_base_hub_state new) +{ + struct fwnode_handle *node = dev_fwnode(>sdev->dev); + int status = 0; + + lockdep_assert_held(hub->lock); + + if (hub->state == new) + return 0; + hub->state = new; + + if (hub->state == SSAM_BASE_HUB_CONNECTED) + status = ssam_hub_add_devices(>sdev->dev, hub->sdev->ctrl, node); + else + ssam_hub_remove_devices(>sdev->dev); + + if (status) + dev_err(>sdev->dev, "failed to update base-hub devices: %d\n", status); + + return status; +} + +static int ssam_base_hub_update(struct ssam_base_hub *hub) +{ + enum ssam_base_hub_state state; + int status; + + mutex_lock(>lock); + + status = ssam_base_hub_query_state(hub, ); + if (!status) + status = __ssam_base_hub_update(hub, state); + + mutex_unlock(>lock); +
[PATCH v2 5/6] platform/surface: aggregator_registry: Add DTX device
Add the detachment system (DTX) SSAM device for the Surface Book 3. This device is accessible under the base (TC=0x11) subsystem. Signed-off-by: Maximilian Luz --- drivers/platform/surface/surface_aggregator_registry.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 913fa5cae705..4c74e80dc34a 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -71,6 +71,12 @@ static const struct software_node ssam_node_tmp_pprof = { .parent = _node_root, }; +/* DTX / detachment-system device (Surface Book 3). */ +static const struct software_node ssam_node_bas_dtx = { + .name = "ssam:01:11:01:00:00", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -86,6 +92,7 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_main, _node_bat_sb3base, _node_tmp_pprof, + _node_bas_dtx, NULL, }; -- 2.30.0
[PATCH v2 6/6] platform/surface: aggregator_registry: Add HID subsystem devices
Add HID subsystem (TC=0x15) devices. These devices need to be registered for 7th-generation Surface models. On previous generations, these devices are either provided as platform devices via ACPI (Surface Laptop 1 and 2) or implemented as standard USB device. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 49 +++ 1 file changed, 49 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 4c74e80dc34a..07cadd5a10f2 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -77,6 +77,48 @@ static const struct software_node ssam_node_bas_dtx = { .parent = _node_root, }; +/* HID keyboard. */ +static const struct software_node ssam_node_hid_main_keyboard = { + .name = "ssam:01:15:02:01:00", + .parent = _node_root, +}; + +/* HID touchpad. */ +static const struct software_node ssam_node_hid_main_touchpad = { + .name = "ssam:01:15:02:03:00", + .parent = _node_root, +}; + +/* HID device instance 5 (unknown HID device). */ +static const struct software_node ssam_node_hid_main_iid5 = { + .name = "ssam:01:15:02:05:00", + .parent = _node_root, +}; + +/* HID keyboard (base hub). */ +static const struct software_node ssam_node_hid_base_keyboard = { + .name = "ssam:01:15:02:01:00", + .parent = _node_hub_base, +}; + +/* HID touchpad (base hub). */ +static const struct software_node ssam_node_hid_base_touchpad = { + .name = "ssam:01:15:02:03:00", + .parent = _node_hub_base, +}; + +/* HID device instance 5 (unknown HID device, base hub). */ +static const struct software_node ssam_node_hid_base_iid5 = { + .name = "ssam:01:15:02:05:00", + .parent = _node_hub_base, +}; + +/* HID device instance 6 (unknown HID device, base hub). */ +static const struct software_node ssam_node_hid_base_iid6 = { + .name = "ssam:01:15:02:06:00", + .parent = _node_hub_base, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -93,6 +135,10 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_sb3base, _node_tmp_pprof, _node_bas_dtx, + _node_hid_base_keyboard, + _node_hid_base_touchpad, + _node_hid_base_iid5, + _node_hid_base_iid6, NULL, }; @@ -116,6 +162,9 @@ static const struct software_node *ssam_node_group_sl3[] = { _node_bat_ac, _node_bat_main, _node_tmp_pprof, + _node_hid_main_keyboard, + _node_hid_main_touchpad, + _node_hid_main_iid5, NULL, }; -- 2.30.0
[PATCH v2 4/6] platform/surface: aggregator_registry: Add platform profile device
Add the SSAM platform profile device to the SSAM device registry. This device is accessible under the thermal subsystem (TC=0x03) and needs to be registered for all Surface models. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 15 +++ 1 file changed, 15 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 7e7b801bc606..913fa5cae705 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -65,9 +65,16 @@ static const struct software_node ssam_node_bat_sb3base = { .parent = _node_hub_base, }; +/* Platform profile / performance-mode device. */ +static const struct software_node ssam_node_tmp_pprof = { + .name = "ssam:01:03:01:00:01", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -78,18 +85,21 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_ac, _node_bat_main, _node_bat_sb3base, + _node_tmp_pprof, NULL, }; /* Devices for Surface Laptop 1. */ static const struct software_node *ssam_node_group_sl1[] = { _node_root, + _node_tmp_pprof, NULL, }; /* Devices for Surface Laptop 2. */ static const struct software_node *ssam_node_group_sl2[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -98,6 +108,7 @@ static const struct software_node *ssam_node_group_sl3[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; @@ -106,18 +117,21 @@ static const struct software_node *ssam_node_group_slg1[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; /* Devices for Surface Pro 5. */ static const struct software_node *ssam_node_group_sp5[] = { _node_root, + _node_tmp_pprof, NULL, }; /* Devices for Surface Pro 6. */ static const struct software_node *ssam_node_group_sp6[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -126,6 +140,7 @@ static const struct software_node *ssam_node_group_sp7[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; -- 2.30.0
[PATCH v2 3/6] platform/surface: aggregator_registry: Add battery subsystem devices
Add battery subsystem (TC=0x02) devices (battery and AC) to the SSAM device registry. These devices need to be registered for 7th-generation Surface models. On 5th- and 6th-generation models, these devices are handled via the standard ACPI battery/AC interface, which in turn accesses the same SSAM interface via the Surface ACPI Notify (SAN) driver. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 27 +++ 1 file changed, 27 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 0d802804594c..7e7b801bc606 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -47,6 +47,24 @@ static const struct software_node ssam_node_hub_base = { .parent = _node_root, }; +/* AC adapter. */ +static const struct software_node ssam_node_bat_ac = { + .name = "ssam:01:02:01:01:01", + .parent = _node_root, +}; + +/* Primary battery. */ +static const struct software_node ssam_node_bat_main = { + .name = "ssam:01:02:01:01:00", + .parent = _node_root, +}; + +/* Secondary battery (Surface Book 3). */ +static const struct software_node ssam_node_bat_sb3base = { + .name = "ssam:01:02:02:01:00", + .parent = _node_hub_base, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -57,6 +75,9 @@ static const struct software_node *ssam_node_group_sb2[] = { static const struct software_node *ssam_node_group_sb3[] = { _node_root, _node_hub_base, + _node_bat_ac, + _node_bat_main, + _node_bat_sb3base, NULL, }; @@ -75,12 +96,16 @@ static const struct software_node *ssam_node_group_sl2[] = { /* Devices for Surface Laptop 3. */ static const struct software_node *ssam_node_group_sl3[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; /* Devices for Surface Laptop Go. */ static const struct software_node *ssam_node_group_slg1[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; @@ -99,6 +124,8 @@ static const struct software_node *ssam_node_group_sp6[] = { /* Devices for Surface Pro 7. */ static const struct software_node *ssam_node_group_sp7[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; -- 2.30.0
[PATCH v2 1/6] platform/surface: Set up Surface Aggregator device registry
The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This commit introduces a registry providing non-detectable device information via software nodes. In addition, a SSAM platform hub driver is introduced, which takes care of creating and managing the SSAM devices specified in this registry. This approach allows for a hierarchical setup akin to ACPI and is easily extendable, e.g. via firmware node properties. Note that this commit only provides the basis for the platform hub and registry, and does not add any content to it. The registry will be expanded in subsequent commits. Signed-off-by: Maximilian Luz --- Changes in v2: - Fix Kconfig dependency --- MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 27 ++ drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 284 ++ 4 files changed, 313 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c diff --git a/MAINTAINERS b/MAINTAINERS index 4fcf3df517a8..000a82f59c76 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11826,6 +11826,7 @@ F: Documentation/driver-api/surface_aggregator/ F: drivers/platform/surface/aggregator/ F: drivers/platform/surface/surface_acpi_notify.c F: drivers/platform/surface/surface_aggregator_cdev.c +F: drivers/platform/surface/surface_aggregator_registry.c F: include/linux/surface_acpi_notify.h F: include/linux/surface_aggregator/ F: include/uapi/linux/surface_aggregator/ diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 0847b2dc97bf..179b8c93d7fd 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -77,6 +77,33 @@ config SURFACE_AGGREGATOR_CDEV The provided interface is intended for debugging and development only, and should not be used otherwise. +config SURFACE_AGGREGATOR_REGISTRY + tristate "Surface System Aggregator Module Device Registry" + depends on SURFACE_AGGREGATOR + depends on SURFACE_AGGREGATOR_BUS + help + Device-registry and device-hubs for Surface System Aggregator Module + (SSAM) devices. + + Provides a module and driver which act as a device-registry for SSAM + client devices that cannot be detected automatically, e.g. via ACPI. + Such devices are instead provided via this registry and attached via + device hubs, also provided in this module. + + Devices provided via this registry are: + - Platform profile (performance-/cooling-mode) device (5th- and later + generations). + - Battery/AC devices (7th-generation). + - HID input devices (7th-generation). + + Select M (recommended) or Y here if you want support for the above + mentioned devices on the corresponding Surface models. Without this + module, the respective devices will not be instantiated and thus any + functionality provided by them will be missing, even when drivers for + these devices are present. In other words, this module only provides + the respective client devices. Drivers for these devices still need to + be selected via the other options. + config SURFACE_GPE tristate "Surface GPE/Lid Support Driver" depends on DMI diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile index 990424c5f0c9..80035ee540bf 100644 --- a/drivers/platform/surface/Makefile +++ b/drivers/platform/surface/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_SURFACE_3_POWER_OPREGION)+= surface3_power.o obj-$(CONFIG_SURFACE_ACPI_NOTIFY) += surface_acpi_notify.o obj-$(CONFIG_SURFACE_AGGREGATOR) += aggregator/ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o +obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
[PATCH v2 0/6] platform/surface: Add Surface Aggregator device registry
The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This series introduces a device registry based on software nodes and device hubs to solve this problem. The registry is intended to contain all required non-detectable information. The platform hub driver is loaded against the WSID device and instantiates and manages SSAM devices based on the information provided by the registry for the given WSID HID of the Surface model. All new devices created by this hub added as child devices to this hub. In addition, a base hub is introduced to manage devices associated with the detachable base part of the Surface Book 3, as this requires special handling (i.e. devices need to be removed when the base is removed). Again, all devices created by the base hub (i.e. associated with the base) are added as child devices to this hub. In total, this will yield the following device structure WSID |- SSAM device 1 (physical device) |- SSAM device 2 (physical device) |- SSAM device 3 (physical device) |- ... \- SSAM base hub (virtual device) |- SSAM base device 1 (physical device) |- SSAM base device 2 (physical device) |- ... While software nodes seem to be well suited for this approach due to extensibility, they still need to be hard-coded, so I'm open for ideas on how this could be improved. Changes in v2: - Fix Kconfig dependency Maximilian Luz (6): platform/surface: Set up Surface Aggregator device registry platform/surface: aggregator_registry: Add base device hub platform/surface: aggregator_registry: Add battery subsystem devices platform/surface: aggregator_registry: Add platform profile device platform/surface: aggregator_registry: Add DTX device platform/surface: aggregator_registry: Add HID subsystem devices MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 27 + drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 641 ++ 4 files changed, 670 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c -- 2.30.0
Re: [PATCH] platform/surface: Add platform profile driver
On 2/11/21 5:31 PM, Hans de Goede wrote: Hi, On 2/11/21 5:17 PM, Maximilian Luz wrote: On 2/11/21 4:56 PM, Hans de Goede wrote: Hi, On 2/8/21 10:38 PM, Maximilian Luz wrote: On 2/8/21 9:27 PM, Hans de Goede wrote: +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p) +{ + switch (p) { + case SSAM_TMP_PROFILE_NORMAL: + return PLATFORM_PROFILE_QUIET; + + case SSAM_TMP_PROFILE_BATTERY_SAVER: + return PLATFORM_PROFILE_LOW_POWER; + + case SSAM_TMP_PROFILE_BETTER_PERFORMANCE: + return PLATFORM_PROFILE_BALANCED; + + case SSAM_TMP_PROFILE_BEST_PERFORMANCE: + return PLATFORM_PROFILE_PERFORMANCE; + + default: + dev_err(>dev, "invalid performance profile: %d", p); + return -EINVAL; + } +} I'm not sure about the mapping which you have chosen here. I know that at least for gnome there are plans to make this stuff available in the UI: https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html Thanks for those links! Notice there are only 3 levels in the UI, which will primarily be mapped to: PLATFORM_PROFILE_LOW_POWER PLATFORM_PROFILE_BALANCED PLATFORM_PROFILE_PERFORMANCE (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that mostly is something for userspace to worry about). Interesting, I wasn't aware of that. I was aware of Bastien's work towards implementing user-space support for this but I hadn't yet looked at it in detail (e.g. the "fallback to quiet" is new to me). Note that the fallback stuff would not apply here, since you do provide all 3 of low-power, balanced and performance. But the current way gnome will handle this means that it will be impossible to select "normal" from the GNOME ui which feels wrong. And the power-profile-daemon will likely restore the last used setting on boot, meaning with your mapping that it will always switch the profile away from SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ? Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting the EC does. Same difference though. So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL. I know the ABI docs say that drivers should try to use existing values, but this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum. During the discussion the following 2 options were given because some devices may have more then one balanced profile: PLATFORM_PROFILE_BALANCED_LOW_POWER: balanced-low-power: Balances between low power consumption and performance with a slight bias towards low power PLATFORM_PROFILE_BALANCED_PERFORMANCE: balanced-performance: Balances between performance and low power consumption with a slight bias towards performance I think it would be better to add 1 or both of these, if we add both we could e.g. do the following mappings: SSAM_TMP_PROFILE_BATTERY_SAVER -> PLATFORM_PROFILE_LOW_POWER SSAM_TMP_PROFILE_NORMAL -> PLATFORM_PROFILE_BALANCED_LOW_POWER SSAM_TMP_PROFILE_BETTER_PERFORMANCE -> PLATFORM_PROFILE_BALANCED_PERFORMANCE SSAM_TMP_PROFILE_BEST_PERFORMANCE -> PLATFORM_PROFILE_PERFORMANCE or we could do: SSAM_TMP_PROFILE_BATTERY_SAVER -> PLATFORM_PROFILE_LOW_POWER SSAM_TMP_PROFILE_NORMAL -> PLATFORM_PROFILE_BALANCED SSAM_TMP_PROFILE_BETTER_PERFORMANCE -> PLATFORM_PROFILE_BALANCED_PERFORMANCE SSAM_TMP_PROFILE_BEST_PERFORMANCE -> PLATFORM_PROFILE_PERFORMANCE I'm not sure which is best, I hope you have a better idea of that then me. I might even be wrong here and NORMAL might really be more about being QUIET then it really being BALANCED ? In which case the mapping is fine as is. I can only really speak on the behavior of my Surface Book 2. On that device, the CPU is passively cooled, but the discrete GPU is actively cooled, so I can actually only really talk about active cooling behavior for the dGPU. On that, at least, the normal (Windows calls this 'recommended') profile feels like it targets quiet operation. Using the dGPU with that profile pretty much ensures that the dGPU will be limited in performance by a thermal limiter (around 75°C to 80°C; at least it feels that way), while the fan is somewhat audible but definitely not at maximum speed. Changing the profile to any higher profile (Windows calls those 'better performance' and 'best performance'), the fan becomes significantly more audible. I'm not entirely sure if the performance increase can solely be attributed to cooling though.
Re: [PATCH] platform/surface: Add platform profile driver
On 2/11/21 4:56 PM, Hans de Goede wrote: Hi, On 2/8/21 10:38 PM, Maximilian Luz wrote: On 2/8/21 9:27 PM, Hans de Goede wrote: +static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p) +{ + switch (p) { + case SSAM_TMP_PROFILE_NORMAL: + return PLATFORM_PROFILE_QUIET; + + case SSAM_TMP_PROFILE_BATTERY_SAVER: + return PLATFORM_PROFILE_LOW_POWER; + + case SSAM_TMP_PROFILE_BETTER_PERFORMANCE: + return PLATFORM_PROFILE_BALANCED; + + case SSAM_TMP_PROFILE_BEST_PERFORMANCE: + return PLATFORM_PROFILE_PERFORMANCE; + + default: + dev_err(>dev, "invalid performance profile: %d", p); + return -EINVAL; + } +} I'm not sure about the mapping which you have chosen here. I know that at least for gnome there are plans to make this stuff available in the UI: https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html Thanks for those links! Notice there are only 3 levels in the UI, which will primarily be mapped to: PLATFORM_PROFILE_LOW_POWER PLATFORM_PROFILE_BALANCED PLATFORM_PROFILE_PERFORMANCE (with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that mostly is something for userspace to worry about). Interesting, I wasn't aware of that. I was aware of Bastien's work towards implementing user-space support for this but I hadn't yet looked at it in detail (e.g. the "fallback to quiet" is new to me). Note that the fallback stuff would not apply here, since you do provide all 3 of low-power, balanced and performance. But the current way gnome will handle this means that it will be impossible to select "normal" from the GNOME ui which feels wrong. And the power-profile-daemon will likely restore the last used setting on boot, meaning with your mapping that it will always switch the profile away from SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ? Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting the EC does. Same difference though. So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL. I know the ABI docs say that drivers should try to use existing values, but this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum. During the discussion the following 2 options were given because some devices may have more then one balanced profile: PLATFORM_PROFILE_BALANCED_LOW_POWER: balanced-low-power: Balances between low power consumption and performance with a slight bias towards low power PLATFORM_PROFILE_BALANCED_PERFORMANCE: balanced-performance: Balances between performance and low power consumption with a slight bias towards performance I think it would be better to add 1 or both of these, if we add both we could e.g. do the following mappings: SSAM_TMP_PROFILE_BATTERY_SAVER -> PLATFORM_PROFILE_LOW_POWER SSAM_TMP_PROFILE_NORMAL -> PLATFORM_PROFILE_BALANCED_LOW_POWER SSAM_TMP_PROFILE_BETTER_PERFORMANCE -> PLATFORM_PROFILE_BALANCED_PERFORMANCE SSAM_TMP_PROFILE_BEST_PERFORMANCE -> PLATFORM_PROFILE_PERFORMANCE or we could do: SSAM_TMP_PROFILE_BATTERY_SAVER -> PLATFORM_PROFILE_LOW_POWER SSAM_TMP_PROFILE_NORMAL -> PLATFORM_PROFILE_BALANCED SSAM_TMP_PROFILE_BETTER_PERFORMANCE -> PLATFORM_PROFILE_BALANCED_PERFORMANCE SSAM_TMP_PROFILE_BEST_PERFORMANCE -> PLATFORM_PROFILE_PERFORMANCE I'm not sure which is best, I hope you have a better idea of that then me. I might even be wrong here and NORMAL might really be more about being QUIET then it really being BALANCED ? In which case the mapping is fine as is. I can only really speak on the behavior of my Surface Book 2. On that device, the CPU is passively cooled, but the discrete GPU is actively cooled, so I can actually only really talk about active cooling behavior for the dGPU. On that, at least, the normal (Windows calls this 'recommended') profile feels like it targets quiet operation. Using the dGPU with that profile pretty much ensures that the dGPU will be limited in performance by a thermal limiter (around 75°C to 80°C; at least it feels that way), while the fan is somewhat audible but definitely not at maximum speed. Changing the profile to any higher profile (Windows calls those 'better performance' and 'best performance'), the fan becomes significantly more audible. I'm not entirely sure if the performance increase can solely be attributed to cooling though. As far as I've heard, that behavior seems to be similar on other devices with fans for CPU cooling
[PATCH v2] platform/surface: aggregator: Fix access of unaligned value
The raw message frame length is unaligned and explicitly marked as little endian. It should not be accessed without the appropriate accessor functions. Fix this. Note that payload.len already contains the correct length after parsing via sshp_parse_frame(), so we can simply use that instead. Reported-by: kernel-test-robot Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") Signed-off-by: Maximilian Luz --- Changes in v2: - Use payload.len instead of getting the frame length directly. Note that payload.len equals the frame length and is already correctly set in sshp_parse_frame(), so they are exactly the same thing. Makes it look a bit nicer though. I did drop the ACKs/Reveiewd-by in case you want to check that yourselves and since that's essentially the whole change. --- drivers/platform/surface/aggregator/ssh_packet_layer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c index 583315db8b02..15d96eac6811 100644 --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c @@ -1774,7 +1774,7 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source) break; } - return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(frame->len); + return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(payload.len); } static int ssh_ptl_rx_threadfn(void *data) -- 2.30.0
Re: [PATCH] platform/surface: aggregator: Fix access of unaligned value
On 2/11/21 11:22 AM, Andy Shevchenko wrote: On Thu, Feb 11, 2021 at 12:04:11AM +0100, Maximilian Luz wrote: The raw message frame length is unaligned and explicitly marked as little endian. It should not be accessed without the appropriatte accessor functions. Fix this. Reviewed-by: Andy Shevchenko Though a few nit-picks below. Reported-by: kernel-test-robot Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") Signed-off-by: Maximilian Luz --- drivers/platform/surface/aggregator/ssh_packet_layer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c index 583315db8b02..9a78188d8d1c 100644 --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c @@ -1774,7 +1774,8 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source) break; } - return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(frame->len); + return aligned.ptr - source->ptr + + SSH_MESSAGE_LENGTH(get_unaligned_le16(>len)); I would leave + on previous line. I can fix that if it bugs you. Also it's possible to annotate temporary variable and use it, but it seems not worth to do. Now that you mention it, we already have the correct frame length in payload.len. Let me draft up a new patch with that. Side question: Do you think the below is correct (& operator)? sp.len = get_unaligned_le16(&((struct ssh_frame *)sf.ptr)->len); To me seems like you take an address to len member rather its value. That's the point though, no? The signature is u16 get_unaligned_le16(const void *p) so we do want a pointer to the len member. So I believe that is correct. } static int ssh_ptl_rx_threadfn(void *data)
Re: [PATCH] PCI: Run platform power transition on initial D0 entry
On 2/11/21 12:57 AM, Bjorn Helgaas wrote: [+cc Rafael, linux-pm] On Thu, Feb 04, 2021 at 11:06:40PM +0100, Maximilian Luz wrote: On some devices and platforms, the initial platform power state is not in sync with the power state of the PCI device. pci_enable_device_flags() updates the state of a PCI device by reading from the PCI_PM_CTRL register. This may change the stored power state of the device without running the appropriate platform power transition. At this point in the code, setting dev->current_state based on the value of PCI_PM_CTRL seems reasonable. We're making the pci_dev state match the PCI device hardware state. This paragraph sort of implies we're missing an "appropriate platform power transition" here, but I don't think that's the case. In terms of PCI core, this is fine. But there's no attempt made at checking that the platform state (not the core state) is compatible with what we're setting here. So the core state is correct and shows that the device is on, but unfortunately there's some ACPI code out there that seems to initialize some ACPI power resource to a state that doesn't match this (in this case off). So updating the state without also making sure that the power resource is also updated (e.g. turned on or at least marked as turned on here) leaves both states to be different (emphasis on leaves, the were already out-of-sync before that). E.g. in my case PCI says 'on' (correct) and ACPI says 'off' (wrong). The problem is that when we now later transition the device into D3cold, the PCI core itself will do that transition just fine, but the PCI-ACPI (i.e. platform) part thinks the power resource is already off and won't do anything. And that prevents the device from actually being turned off. Also running pci_set_power_state(..., PCI_D0) later does not fix that because the PCI core (correctly) believes that the device is already on and just returns doing nothing. So it doesn't even attempt to check the platform state, which is reasonable behavior if one assumes that the platform state is always in sync with the PCI core state. It's just that here, I think, we can't assume that they're in sync (mostly because ACPI / platform stuff may be weird and buggy and we may not have control over that). This is why I suggested replacing pci_set_power_state(dev, PCI_D0) with pci_power_up(dev). With PCI_D0, they essentially do the same thing, except for the (first) state check. Also both later call pci_platform_power_transition() and pci_raw_set_power_state(), which (should) have their individual state checks. So if the device is already in D0, this boils down to calling the pci_platform_power_transition() only. But it would be nice if we could combine this bit from pci_enable_device_flags() with the pci_set_power_state() in do_pci_enable_device(). Due to the stored power-state being changed, the later call to pci_set_power_state(..., PCI_D0) in do_pci_enable_device() can evaluate to a no-op if the stored state has been changed to D0 via that. This will then prevent the appropriate platform power transition to be run, which can on some devices and platforms lead to platform and PCI power state being entirely different, i.e. out-of-sync. On ACPI platforms, this can lead to power resources not being turned on, even though they are marked as required for D0. Specifically, on the Microsoft Surface Book 2 and 3, some ACPI power regions that should be "on" for the D0 state (and others) are initialized as "off" in ACPI, whereas the PCI device is in D0. So some ACPI power regions are in fact "on" (because the PCI device that requires them is in D0), but the ACPI core believes them to be "off" (or probably "unknown, treated as 'off'")? Yes, that's pretty much it. The problem I'm dealing with specifically is caused by the ACPI code in [1]. There, _STA gets initialized to 'off' and is only updated when the power transitions run (i.e. the _ON or _OFF methods). There's nothing in this ACPI code that checks the actual state of the PCI device, which causes this problem. So, to me, it seems that this code is expecting the _ON method to be called in PCI bring-up. [1]: https://github.com/linux-surface/acpidumps/blob/1ed05b95df844534229f752ea2267c8dd8ae7f8c/surface_book_2/dsdt.dsl#L19170-L19225 As the state is updated in pci_enable_device_flags() without ensuring that the platform state is also updated, the power resource will never be properly turned on. Instead, it lives in a sort of on-but-marked-as-off zombie-state, which confuses things down the line when attempting to transition the device into D3cold: As the resource is already marked as off, it won't be turned off and the device does not fully enter D3cold, causing increased power consumption during (runtime-)suspend. By replacing pci_set_power_state() in do_pci_enable_device() with pci_power_up(), we can force pci_platform_power_transition() to be called, w
[PATCH] platform/surface: aggregator: Fix access of unaligned value
The raw message frame length is unaligned and explicitly marked as little endian. It should not be accessed without the appropriatte accessor functions. Fix this. Reported-by: kernel-test-robot Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") Signed-off-by: Maximilian Luz --- drivers/platform/surface/aggregator/ssh_packet_layer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c index 583315db8b02..9a78188d8d1c 100644 --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c @@ -1774,7 +1774,8 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source) break; } - return aligned.ptr - source->ptr + SSH_MESSAGE_LENGTH(frame->len); + return aligned.ptr - source->ptr + + SSH_MESSAGE_LENGTH(get_unaligned_le16(>len)); } static int ssh_ptl_rx_threadfn(void *data) -- 2.30.0
Re: [PATCH 1/6] platform/surface: Set up Surface Aggregator device registry
On 2/8/21 8:35 PM, Maximilian Luz wrote: The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This commit introduces a registry providing non-detectable device information via software nodes. In addition, a SSAM platform hub driver is introduced, which takes care of creating and managing the SSAM devices specified in this registry. This approach allows for a hierarchical setup akin to ACPI and is easily extendable, e.g. via firmware node properties. Note that this commit only provides the basis for the platform hub and registry, and does not add any content to it. The registry will be expanded in subsequent commits. Signed-off-by: Maximilian Luz --- MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 26 ++ drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 284 ++ 4 files changed, 312 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c diff --git a/MAINTAINERS b/MAINTAINERS index 4fcf3df517a8..000a82f59c76 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11826,6 +11826,7 @@ F: Documentation/driver-api/surface_aggregator/ F:drivers/platform/surface/aggregator/ F:drivers/platform/surface/surface_acpi_notify.c F:drivers/platform/surface/surface_aggregator_cdev.c +F: drivers/platform/surface/surface_aggregator_registry.c F:include/linux/surface_acpi_notify.h F:include/linux/surface_aggregator/ F:include/uapi/linux/surface_aggregator/ diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 0847b2dc97bf..1cd37c041710 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -77,6 +77,32 @@ config SURFACE_AGGREGATOR_CDEV The provided interface is intended for debugging and development only, and should not be used otherwise. +config SURFACE_AGGREGATOR_REGISTRY + tristate "Surface System Aggregator Module Device Registry" + depends on SURFACE_AGGREGATOR_BUS Just noticed that there should also be a depends on SURFACE_AGGREGATOR here as SURFACE_AGGREGATOR_BUS is a bool. Without that, one could set CONFIG_SURFACE_AGGREGATOR=m CONFIG_SURFACE_AGGREGATOR_BUS=y CONFIG_SURFACE_AGGREGATOR_REGISTRY=y which will probably cause the compiler to complain that it can't find the functions from the Aggregator module. I'll leave it up as-is for a bit longer to maybe gather a couple more comments before I send out a v2 with this fixed. + help + Device-registry and device-hubs for Surface System Aggregator Module + (SSAM) devices. + + Provides a module and driver which act as a device-registry for SSAM + client devices that cannot be detected automatically, e.g. via ACPI. + Such devices are instead provided via this registry and attached via + device hubs, also provided in this module. + + Devices provided via this registry are: + - Platform profile (performance-/cooling-mode) device (5th- and later + generations). + - Battery/AC devices (7th-generation). + - HID input devices (7th-generation). + + Select M (recommended) or Y here if you want support for the above + mentioned devices on the corresponding Surface models. Without this + module, the respective devices will not be instantiated and thus any + functionality provided by them will be missing, even when drivers for + these devices are present. In other words, this module only provides + the respective client devices. Drivers for these devices still need to + be selected via the other options. + config SURFACE_GPE tristate "Surface GPE/Lid Support Driver" depends on DMI diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile index 990424c5f0c9..80035ee540bf 100644 --- a/drivers/platform/surface/Makefile +++ b/drivers/platform/surface/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_SURFACE_3_POWER_OPREGION)+= su
Re: [PATCH] platform/surface: Add platform profile driver
On 2/8/21 9:27 PM, Hans de Goede wrote: Hi, On 2/8/21 8:49 PM, Maximilian Luz wrote: Add a driver to provide platform profile support on 5th- and later generation Microsoft Surface devices with a Surface System Aggregator Module. On those devices, the platform profile can be used to influence cooling behavior and power consumption. For example, the default 'quiet' profile limits fan noise and in turn sacrifices performance of the discrete GPU found on Surface Books. Its full performance can only be unlocked on the 'performance' profile. Signed-off-by: Maximilian Luz --- Note: This patch builds ontop of the platform/surface: Add Surface Aggregator device registry series. While that series is not strictly required for building this driver, it provides the device against which it loads. So (at the moment at least) this patch is essentially useless without that series. Oh, another user of the new platform-profile stuff, great, that means that the time to get this in place was probably well spend :) A few review remarks inline. --- MAINTAINERS | 6 + drivers/platform/surface/Kconfig | 27 +++ drivers/platform/surface/Makefile | 1 + .../surface/surface_platform_profile.c| 190 ++ 4 files changed, 224 insertions(+) create mode 100644 drivers/platform/surface/surface_platform_profile.c diff --git a/MAINTAINERS b/MAINTAINERS index 000a82f59c76..a08d65f8f0df 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11811,6 +11811,12 @@ L: platform-driver-...@vger.kernel.org S:Maintained F:drivers/platform/surface/surface_hotplug.c +MICROSOFT SURFACE PLATFORM PROFILE DRIVER +M: Maximilian Luz +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/surface/surface_platform_profile.c + MICROSOFT SURFACE PRO 3 BUTTON DRIVER M:Chen Yu L:platform-driver-...@vger.kernel.org diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 1cd37c041710..e12c65909bcc 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -131,6 +131,33 @@ config SURFACE_HOTPLUG Select M or Y here, if you want to (fully) support hot-plugging of dGPU devices on the Surface Book 2 and/or 3 during D3cold. +config SURFACE_PLATFORM_PROFILE + tristate "Surface Platform Profile Driver" + depends on SURFACE_AGGREGATOR_BUS + depends on ACPI_PLATFORM_PROFILE Not really about this patch, but it seems to me that it would be better to make ACPI_PLATFORM_PROFILE not user selectable and use select here and in the other 2 Kconfig bits which have depends on ACPI_PLATFORM_PROFILE ATM. I would certainly welcome a patch for this. That sounds like a good idea. Note such a patch should probably sit on top of this one, as it will need some coordination with Rafael to get that upstream. Although we may need some other changes to drivers/acpi/platform_profile.c too, see below. + help + Provides support for the ACPI platform profile on 5th- and later + generation Microsoft Surface devices. + + More specifically, this driver provides ACPI platform profile support + on Microsoft Surface devices with a Surface System Aggregator Module + (SSAM) connected via the Surface Serial Hub (SSH / SAM-over-SSH). In + other words, this driver provides platform profile support on the + Surface Pro 5, Surface Book 2, Surface Laptop, Surface Laptop Go and + later. On those devices, the platform profile can significantly + influence cooling behavior, e.g. setting it to 'quiet' (default) or + 'low-power' can significantly limit performance of the discrete GPU on + Surface Books, while in turn leading to lower power consumption and/or + less fan noise. + + Note that this driver currently relies on the Surface Aggregator + registry (CONFIG_SURFACE_AGGREGATOR_REGISTRY) to provide the device it + loads against. Thus, without that registry, this module is essentially + of no use. I would prefer if you dropped this paragraph and just add a: depends on SURFACE_AGGREGATOR_REGISTRY Technically this could be builtin while SURFACE_AGGREGATOR_REGISTRY is a module, while adding the depends on will disallow that, but I see no reason why someone would want to make this builtin without also having SURFACE_AGGREGATOR_REGISTRY builtin. That's fair, I'll change that. My hope was that we someday may find a way to dynamically query devices, but then we'll have to update the registry anyway. + + Select M or Y here, if you want to include ACPI platform profile + support on the above mentioned devices. + config SURFACE_PRO3_BUTTON tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet" depends on INPUT
Re: [PATCH 0/6] platform/surface: Add Surface Aggregator device registry
On 2/8/21 8:55 PM, Hans de Goede wrote: Hi, On 2/8/21 8:35 PM, Maximilian Luz wrote: The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This series introduces a device registry based on software nodes and device hubs to solve this problem. The registry is intended to contain all required non-detectable information. The platform hub driver is loaded against the WSID device and instantiates and manages SSAM devices based on the information provided by the registry for the given WSID HID of the Surface model. All new devices created by this hub added as child devices to this hub. In addition, a base hub is introduced to manage devices associated with the detachable base part of the Surface Book 3, as this requires special handling (i.e. devices need to be removed when the base is removed). Again, all devices created by the base hub (i.e. associated with the base) are added as child devices to this hub. In total, this will yield the following device structure WSID |- SSAM device 1 (physical device) |- SSAM device 2 (physical device) |- SSAM device 3 (physical device) |- ... \- SSAM base hub (virtual device) |- SSAM base device 1 (physical device) |- SSAM base device 2 (physical device) |- ... While software nodes seem to be well suited for this approach due to extensibility, they still need to be hard-coded, so I'm open for ideas on how this could be improved. This series looks good to me. One question is this 5.12 material, or is this intended for 5.13 (together with drivers attaching to the newly instantiated devices) ? I can drop this into for-next tomorrow, that gives it just about enough "baking" time in -next to still make it into 5.12 (this should be pretty safe to push to for-next despite it being somewhat close to the cutoff point as it is a new driver). Maximilian, do you want me to add this series to for-next tomorrow, or would you prefer for it to go to -next after 5.12-rc1 (and thus end up in 5.13) ? I wouldn't mind giving this a bit more time to maybe collect some more reviews. The only real functionality that depends on this and would be ready for 5.12 right now is the performance profile. Everything else will have to go into 5.13 anyway (still need to cleanup and prepare patches for that), so 5.13 seems to be the better target for me. Regards, Max Regards, Hans Maximilian Luz (6): platform/surface: Set up Surface Aggregator device registry platform/surface: aggregator_registry: Add base device hub platform/surface: aggregator_registry: Add battery subsystem devices platform/surface: aggregator_registry: Add platform profile device platform/surface: aggregator_registry: Add DTX device platform/surface: aggregator_registry: Add HID subsystem devices MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 26 + drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 641 ++ 4 files changed, 669 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c
[PATCH] platform/surface: Add platform profile driver
Add a driver to provide platform profile support on 5th- and later generation Microsoft Surface devices with a Surface System Aggregator Module. On those devices, the platform profile can be used to influence cooling behavior and power consumption. For example, the default 'quiet' profile limits fan noise and in turn sacrifices performance of the discrete GPU found on Surface Books. Its full performance can only be unlocked on the 'performance' profile. Signed-off-by: Maximilian Luz --- Note: This patch builds ontop of the platform/surface: Add Surface Aggregator device registry series. While that series is not strictly required for building this driver, it provides the device against which it loads. So (at the moment at least) this patch is essentially useless without that series. --- MAINTAINERS | 6 + drivers/platform/surface/Kconfig | 27 +++ drivers/platform/surface/Makefile | 1 + .../surface/surface_platform_profile.c| 190 ++ 4 files changed, 224 insertions(+) create mode 100644 drivers/platform/surface/surface_platform_profile.c diff --git a/MAINTAINERS b/MAINTAINERS index 000a82f59c76..a08d65f8f0df 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11811,6 +11811,12 @@ L: platform-driver-...@vger.kernel.org S: Maintained F: drivers/platform/surface/surface_hotplug.c +MICROSOFT SURFACE PLATFORM PROFILE DRIVER +M: Maximilian Luz +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/surface/surface_platform_profile.c + MICROSOFT SURFACE PRO 3 BUTTON DRIVER M: Chen Yu L: platform-driver-...@vger.kernel.org diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 1cd37c041710..e12c65909bcc 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -131,6 +131,33 @@ config SURFACE_HOTPLUG Select M or Y here, if you want to (fully) support hot-plugging of dGPU devices on the Surface Book 2 and/or 3 during D3cold. +config SURFACE_PLATFORM_PROFILE + tristate "Surface Platform Profile Driver" + depends on SURFACE_AGGREGATOR_BUS + depends on ACPI_PLATFORM_PROFILE + help + Provides support for the ACPI platform profile on 5th- and later + generation Microsoft Surface devices. + + More specifically, this driver provides ACPI platform profile support + on Microsoft Surface devices with a Surface System Aggregator Module + (SSAM) connected via the Surface Serial Hub (SSH / SAM-over-SSH). In + other words, this driver provides platform profile support on the + Surface Pro 5, Surface Book 2, Surface Laptop, Surface Laptop Go and + later. On those devices, the platform profile can significantly + influence cooling behavior, e.g. setting it to 'quiet' (default) or + 'low-power' can significantly limit performance of the discrete GPU on + Surface Books, while in turn leading to lower power consumption and/or + less fan noise. + + Note that this driver currently relies on the Surface Aggregator + registry (CONFIG_SURFACE_AGGREGATOR_REGISTRY) to provide the device it + loads against. Thus, without that registry, this module is essentially + of no use. + + Select M or Y here, if you want to include ACPI platform profile + support on the above mentioned devices. + config SURFACE_PRO3_BUTTON tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet" depends on INPUT diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile index 80035ee540bf..99372c427b73 100644 --- a/drivers/platform/surface/Makefile +++ b/drivers/platform/surface/Makefile @@ -13,4 +13,5 @@ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o +obj-$(CONFIG_SURFACE_PLATFORM_PROFILE) += surface_platform_profile.o obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c new file mode 100644 index ..548ad8af9cf1 --- /dev/null +++ b/drivers/platform/surface/surface_platform_profile.c @@ -0,0 +1,190 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Surface Platform Profile / Performance Mode driver for Surface System + * Aggregator Module (thermal subsystem). + * + * Copyright (C) 2021 Maximilian Luz + */ + +#include +#include +#include +#include +#include + +#include + +enum ssam_tmp_profile { + SSAM_TMP_PROFILE_NORMAL = 1, + SSAM_TMP_PROFILE_BATTERY_SAVER = 2, + SSAM_TMP_PROFILE_BE
[PATCH 6/6] platform/surface: aggregator_registry: Add HID subsystem devices
Add HID subsystem (TC=0x15) devices. These devices need to be registered for 7th-generation Surface models. On previous generations, these devices are either provided as platform devices via ACPI (Surface Laptop 1 and 2) or implemented as standard USB device. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 49 +++ 1 file changed, 49 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 4c74e80dc34a..07cadd5a10f2 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -77,6 +77,48 @@ static const struct software_node ssam_node_bas_dtx = { .parent = _node_root, }; +/* HID keyboard. */ +static const struct software_node ssam_node_hid_main_keyboard = { + .name = "ssam:01:15:02:01:00", + .parent = _node_root, +}; + +/* HID touchpad. */ +static const struct software_node ssam_node_hid_main_touchpad = { + .name = "ssam:01:15:02:03:00", + .parent = _node_root, +}; + +/* HID device instance 5 (unknown HID device). */ +static const struct software_node ssam_node_hid_main_iid5 = { + .name = "ssam:01:15:02:05:00", + .parent = _node_root, +}; + +/* HID keyboard (base hub). */ +static const struct software_node ssam_node_hid_base_keyboard = { + .name = "ssam:01:15:02:01:00", + .parent = _node_hub_base, +}; + +/* HID touchpad (base hub). */ +static const struct software_node ssam_node_hid_base_touchpad = { + .name = "ssam:01:15:02:03:00", + .parent = _node_hub_base, +}; + +/* HID device instance 5 (unknown HID device, base hub). */ +static const struct software_node ssam_node_hid_base_iid5 = { + .name = "ssam:01:15:02:05:00", + .parent = _node_hub_base, +}; + +/* HID device instance 6 (unknown HID device, base hub). */ +static const struct software_node ssam_node_hid_base_iid6 = { + .name = "ssam:01:15:02:06:00", + .parent = _node_hub_base, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -93,6 +135,10 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_sb3base, _node_tmp_pprof, _node_bas_dtx, + _node_hid_base_keyboard, + _node_hid_base_touchpad, + _node_hid_base_iid5, + _node_hid_base_iid6, NULL, }; @@ -116,6 +162,9 @@ static const struct software_node *ssam_node_group_sl3[] = { _node_bat_ac, _node_bat_main, _node_tmp_pprof, + _node_hid_main_keyboard, + _node_hid_main_touchpad, + _node_hid_main_iid5, NULL, }; -- 2.30.0
[PATCH 5/6] platform/surface: aggregator_registry: Add DTX device
Add the detachment system (DTX) SSAM device for the Surface Book 3. This device is accessible under the base (TC=0x11) subsystem. Signed-off-by: Maximilian Luz --- drivers/platform/surface/surface_aggregator_registry.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 913fa5cae705..4c74e80dc34a 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -71,6 +71,12 @@ static const struct software_node ssam_node_tmp_pprof = { .parent = _node_root, }; +/* DTX / detachment-system device (Surface Book 3). */ +static const struct software_node ssam_node_bas_dtx = { + .name = "ssam:01:11:01:00:00", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -86,6 +92,7 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_main, _node_bat_sb3base, _node_tmp_pprof, + _node_bas_dtx, NULL, }; -- 2.30.0
[PATCH 4/6] platform/surface: aggregator_registry: Add platform profile device
Add the SSAM platform profile device to the SSAM device registry. This device is accessible under the thermal subsystem (TC=0x03) and needs to be registered for all Surface models. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 15 +++ 1 file changed, 15 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 7e7b801bc606..913fa5cae705 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -65,9 +65,16 @@ static const struct software_node ssam_node_bat_sb3base = { .parent = _node_hub_base, }; +/* Platform profile / performance-mode device. */ +static const struct software_node ssam_node_tmp_pprof = { + .name = "ssam:01:03:01:00:01", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -78,18 +85,21 @@ static const struct software_node *ssam_node_group_sb3[] = { _node_bat_ac, _node_bat_main, _node_bat_sb3base, + _node_tmp_pprof, NULL, }; /* Devices for Surface Laptop 1. */ static const struct software_node *ssam_node_group_sl1[] = { _node_root, + _node_tmp_pprof, NULL, }; /* Devices for Surface Laptop 2. */ static const struct software_node *ssam_node_group_sl2[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -98,6 +108,7 @@ static const struct software_node *ssam_node_group_sl3[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; @@ -106,18 +117,21 @@ static const struct software_node *ssam_node_group_slg1[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; /* Devices for Surface Pro 5. */ static const struct software_node *ssam_node_group_sp5[] = { _node_root, + _node_tmp_pprof, NULL, }; /* Devices for Surface Pro 6. */ static const struct software_node *ssam_node_group_sp6[] = { _node_root, + _node_tmp_pprof, NULL, }; @@ -126,6 +140,7 @@ static const struct software_node *ssam_node_group_sp7[] = { _node_root, _node_bat_ac, _node_bat_main, + _node_tmp_pprof, NULL, }; -- 2.30.0
[PATCH 1/6] platform/surface: Set up Surface Aggregator device registry
The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This commit introduces a registry providing non-detectable device information via software nodes. In addition, a SSAM platform hub driver is introduced, which takes care of creating and managing the SSAM devices specified in this registry. This approach allows for a hierarchical setup akin to ACPI and is easily extendable, e.g. via firmware node properties. Note that this commit only provides the basis for the platform hub and registry, and does not add any content to it. The registry will be expanded in subsequent commits. Signed-off-by: Maximilian Luz --- MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 26 ++ drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 284 ++ 4 files changed, 312 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c diff --git a/MAINTAINERS b/MAINTAINERS index 4fcf3df517a8..000a82f59c76 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11826,6 +11826,7 @@ F: Documentation/driver-api/surface_aggregator/ F: drivers/platform/surface/aggregator/ F: drivers/platform/surface/surface_acpi_notify.c F: drivers/platform/surface/surface_aggregator_cdev.c +F: drivers/platform/surface/surface_aggregator_registry.c F: include/linux/surface_acpi_notify.h F: include/linux/surface_aggregator/ F: include/uapi/linux/surface_aggregator/ diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 0847b2dc97bf..1cd37c041710 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -77,6 +77,32 @@ config SURFACE_AGGREGATOR_CDEV The provided interface is intended for debugging and development only, and should not be used otherwise. +config SURFACE_AGGREGATOR_REGISTRY + tristate "Surface System Aggregator Module Device Registry" + depends on SURFACE_AGGREGATOR_BUS + help + Device-registry and device-hubs for Surface System Aggregator Module + (SSAM) devices. + + Provides a module and driver which act as a device-registry for SSAM + client devices that cannot be detected automatically, e.g. via ACPI. + Such devices are instead provided via this registry and attached via + device hubs, also provided in this module. + + Devices provided via this registry are: + - Platform profile (performance-/cooling-mode) device (5th- and later + generations). + - Battery/AC devices (7th-generation). + - HID input devices (7th-generation). + + Select M (recommended) or Y here if you want support for the above + mentioned devices on the corresponding Surface models. Without this + module, the respective devices will not be instantiated and thus any + functionality provided by them will be missing, even when drivers for + these devices are present. In other words, this module only provides + the respective client devices. Drivers for these devices still need to + be selected via the other options. + config SURFACE_GPE tristate "Surface GPE/Lid Support Driver" depends on DMI diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile index 990424c5f0c9..80035ee540bf 100644 --- a/drivers/platform/surface/Makefile +++ b/drivers/platform/surface/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_SURFACE_3_POWER_OPREGION)+= surface3_power.o obj-$(CONFIG_SURFACE_ACPI_NOTIFY) += surface_acpi_notify.o obj-$(CONFIG_SURFACE_AGGREGATOR) += aggregator/ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o +obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/driv
[PATCH 0/6] platform/surface: Add Surface Aggregator device registry
The Surface System Aggregator Module (SSAM) subsystem provides various functionalities, which are separated by spreading them across multiple devices and corresponding drivers. Parts of that functionality / some of those devices, however, can (as far as we currently know) not be auto-detected by conventional means. While older (specifically 5th- and 6th-)generation models do advertise most of their functionality via standard platform devices in ACPI, newer generations do not. As we are currently also not aware of any feasible way to query said functionalities dynamically, this poses a problem. There is, however, a device in ACPI that seems to be used by Windows for identifying different Surface models: The Windows Surface Integration Device (WSID). This device seems to have a HID corresponding to the overall set of functionalities SSAM provides for the associated model. This series introduces a device registry based on software nodes and device hubs to solve this problem. The registry is intended to contain all required non-detectable information. The platform hub driver is loaded against the WSID device and instantiates and manages SSAM devices based on the information provided by the registry for the given WSID HID of the Surface model. All new devices created by this hub added as child devices to this hub. In addition, a base hub is introduced to manage devices associated with the detachable base part of the Surface Book 3, as this requires special handling (i.e. devices need to be removed when the base is removed). Again, all devices created by the base hub (i.e. associated with the base) are added as child devices to this hub. In total, this will yield the following device structure WSID |- SSAM device 1 (physical device) |- SSAM device 2 (physical device) |- SSAM device 3 (physical device) |- ... \- SSAM base hub (virtual device) |- SSAM base device 1 (physical device) |- SSAM base device 2 (physical device) |- ... While software nodes seem to be well suited for this approach due to extensibility, they still need to be hard-coded, so I'm open for ideas on how this could be improved. Maximilian Luz (6): platform/surface: Set up Surface Aggregator device registry platform/surface: aggregator_registry: Add base device hub platform/surface: aggregator_registry: Add battery subsystem devices platform/surface: aggregator_registry: Add platform profile device platform/surface: aggregator_registry: Add DTX device platform/surface: aggregator_registry: Add HID subsystem devices MAINTAINERS | 1 + drivers/platform/surface/Kconfig | 26 + drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_registry.c | 641 ++ 4 files changed, 669 insertions(+) create mode 100644 drivers/platform/surface/surface_aggregator_registry.c -- 2.30.0
[PATCH 3/6] platform/surface: aggregator_registry: Add battery subsystem devices
Add battery subsystem (TC=0x02) devices (battery and AC) to the SSAM device registry. These devices need to be registered for 7th-generation Surface models. On 5th- and 6th-generation models, these devices are handled via the standard ACPI battery/AC interface, which in turn accesses the same SSAM interface via the Surface ACPI Notify (SAN) driver. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 27 +++ 1 file changed, 27 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 0d802804594c..7e7b801bc606 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -47,6 +47,24 @@ static const struct software_node ssam_node_hub_base = { .parent = _node_root, }; +/* AC adapter. */ +static const struct software_node ssam_node_bat_ac = { + .name = "ssam:01:02:01:01:01", + .parent = _node_root, +}; + +/* Primary battery. */ +static const struct software_node ssam_node_bat_main = { + .name = "ssam:01:02:01:01:00", + .parent = _node_root, +}; + +/* Secondary battery (Surface Book 3). */ +static const struct software_node ssam_node_bat_sb3base = { + .name = "ssam:01:02:02:01:00", + .parent = _node_hub_base, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -57,6 +75,9 @@ static const struct software_node *ssam_node_group_sb2[] = { static const struct software_node *ssam_node_group_sb3[] = { _node_root, _node_hub_base, + _node_bat_ac, + _node_bat_main, + _node_bat_sb3base, NULL, }; @@ -75,12 +96,16 @@ static const struct software_node *ssam_node_group_sl2[] = { /* Devices for Surface Laptop 3. */ static const struct software_node *ssam_node_group_sl3[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; /* Devices for Surface Laptop Go. */ static const struct software_node *ssam_node_group_slg1[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; @@ -99,6 +124,8 @@ static const struct software_node *ssam_node_group_sp6[] = { /* Devices for Surface Pro 7. */ static const struct software_node *ssam_node_group_sp7[] = { _node_root, + _node_bat_ac, + _node_bat_main, NULL, }; -- 2.30.0
[PATCH 2/6] platform/surface: aggregator_registry: Add base device hub
The Surface Book 3 has a detachable base part. While the top part (so-called clipboard) contains the CPU, touchscreen, and primary battery, the base contains, among other things, a keyboard, touchpad, and secondary battery. Those devices do not react well to being accessed when the base part is detached and should thus be removed and added in sync with the base. To facilitate this, we introduce a virtual base device hub, which automatically removes or adds the devices registered under it. Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_registry.c | 261 +- 1 file changed, 260 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index a051d941ad96..0d802804594c 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -11,9 +11,12 @@ #include #include +#include #include +#include #include #include +#include #include #include @@ -38,6 +41,12 @@ static const struct software_node ssam_node_root = { .name = "ssam_platform_hub", }; +/* Base device hub (devices attached to Surface Book 3 base). */ +static const struct software_node ssam_node_hub_base = { + .name = "ssam:00:00:02:00:00", + .parent = _node_root, +}; + /* Devices for Surface Book 2. */ static const struct software_node *ssam_node_group_sb2[] = { _node_root, @@ -47,6 +56,7 @@ static const struct software_node *ssam_node_group_sb2[] = { /* Devices for Surface Book 3. */ static const struct software_node *ssam_node_group_sb3[] = { _node_root, + _node_hub_base, NULL, }; @@ -177,6 +187,230 @@ static int ssam_hub_add_devices(struct device *parent, struct ssam_controller *c } +/* -- SSAM base-hub driver. - */ + +enum ssam_base_hub_state { + SSAM_BASE_HUB_UNINITIALIZED, + SSAM_BASE_HUB_CONNECTED, + SSAM_BASE_HUB_DISCONNECTED, +}; + +struct ssam_base_hub { + struct ssam_device *sdev; + + struct mutex lock; /* Guards state update checks and transitions. */ + enum ssam_base_hub_state state; + + struct ssam_event_notifier notif; +}; + +static SSAM_DEFINE_SYNC_REQUEST_R(ssam_bas_query_opmode, u8, { + .target_category = SSAM_SSH_TC_BAS, + .target_id = 0x01, + .command_id = 0x0d, + .instance_id = 0x00, +}); + +#define SSAM_BAS_OPMODE_TABLET 0x00 +#define SSAM_EVENT_BAS_CID_CONNECTION 0x0c + +static int ssam_base_hub_query_state(struct ssam_base_hub *hub, enum ssam_base_hub_state *state) +{ + u8 opmode; + int status; + + status = ssam_retry(ssam_bas_query_opmode, hub->sdev->ctrl, ); + if (status < 0) { + dev_err(>sdev->dev, "failed to query base state: %d\n", status); + return status; + } + + if (opmode != SSAM_BAS_OPMODE_TABLET) + *state = SSAM_BASE_HUB_CONNECTED; + else + *state = SSAM_BASE_HUB_DISCONNECTED; + + return 0; +} + +static ssize_t ssam_base_hub_state_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct ssam_base_hub *hub = dev_get_drvdata(dev); + bool connected; + + mutex_lock(>lock); + connected = hub->state == SSAM_BASE_HUB_CONNECTED; + mutex_unlock(>lock); + + return sysfs_emit(buf, "%d\n", connected); +} + +static struct device_attribute ssam_base_hub_attr_state = + __ATTR(state, 0444, ssam_base_hub_state_show, NULL); + +static struct attribute *ssam_base_hub_attrs[] = { + _base_hub_attr_state.attr, + NULL, +}; + +const struct attribute_group ssam_base_hub_group = { + .attrs = ssam_base_hub_attrs, +}; + +static int __ssam_base_hub_update(struct ssam_base_hub *hub, enum ssam_base_hub_state new) +{ + struct fwnode_handle *node = dev_fwnode(>sdev->dev); + int status = 0; + + lockdep_assert_held(hub->lock); + + if (hub->state == new) + return 0; + hub->state = new; + + if (hub->state == SSAM_BASE_HUB_CONNECTED) + status = ssam_hub_add_devices(>sdev->dev, hub->sdev->ctrl, node); + else + ssam_hub_remove_devices(>sdev->dev); + + if (status) + dev_err(>sdev->dev, "failed to update base-hub devices: %d\n", status); + + return status; +} + +static int ssam_base_hub_update(struct ssam_base_hub *hub) +{ + enum ssam_base_hub_state state; + int status; + + mutex_lock(>lock); + + status = ssam_base_hub_query_state(hub, ); + if (!status) + status = __ssam_base_hub_update(hub, state); + + mutex_unlock(>lock); +
[PATCH] platform/surface: Add Surface Hot-Plug driver
Some Surface Book 2 and 3 models have a discrete GPU (dGPU) that is hot-pluggable. On those devices, the dGPU is contained in the base, which can be separated from the tablet part (containing CPU and touchscreen) while the device is running. It (in general) is presented as/behaves like a standard PCIe hot-plug capable device, however, this device can also be put into D3cold. In D3cold, the device itself is turned off and can thus not submit any standard PCIe hot-plug events. To properly detect hot-(un)plugging while the dGPU is in D3cold, out-of-band signaling is required. Without this, the device state will only get updated during the next bus-check, eg. via a manually issued lspci call. This commit adds a driver to handle out-of-band PCIe hot-(un)plug events on Microsoft Surface devices. On those devices, said events can be detected via GPIO interrupts, which are then forwarded to the corresponding ACPI DSM calls by this driver. The DSM then takes care of issuing the appropriate bus-/device-check, causing the PCI core to properly pick up the device change. Signed-off-by: Maximilian Luz --- MAINTAINERS| 6 + drivers/platform/surface/Kconfig | 19 ++ drivers/platform/surface/Makefile | 1 + drivers/platform/surface/surface_hotplug.c | 282 + 4 files changed, 308 insertions(+) create mode 100644 drivers/platform/surface/surface_hotplug.c diff --git a/MAINTAINERS b/MAINTAINERS index 34bfa5c1aec5..4fcf3df517a8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11805,6 +11805,12 @@ S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git F: drivers/platform/surface/ +MICROSOFT SURFACE HOT-PLUG DRIVER +M: Maximilian Luz +L: platform-driver-...@vger.kernel.org +S: Maintained +F: drivers/platform/surface/surface_hotplug.c + MICROSOFT SURFACE PRO 3 BUTTON DRIVER M: Chen Yu L: platform-driver-...@vger.kernel.org diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 83b0a4c7b352..0847b2dc97bf 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -86,6 +86,25 @@ config SURFACE_GPE accordingly. It is required on those devices to allow wake-ups from suspend by opening the lid. +config SURFACE_HOTPLUG + tristate "Surface Hot-Plug Driver" + depends on GPIOLIB + help + Driver for out-of-band hot-plug event signaling on Microsoft Surface + devices with hot-pluggable PCIe cards. + + This driver is used on Surface Book (2 and 3) devices with a + hot-pluggable discrete GPU (dGPU). When not in use, the dGPU on those + devices can enter D3cold, which prevents in-band (standard) PCIe + hot-plug signaling. Thus, without this driver, detaching the base + containing the dGPU will not correctly update the state of the + corresponding PCIe device if it is in D3cold. This driver adds support + for out-of-band hot-plug notifications, ensuring that the device state + is properly updated even when the device in question is in D3cold. + + Select M or Y here, if you want to (fully) support hot-plugging of + dGPU devices on the Surface Book 2 and/or 3 during D3cold. + config SURFACE_PRO3_BUTTON tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet" depends on INPUT diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile index 3eb971006877..990424c5f0c9 100644 --- a/drivers/platform/surface/Makefile +++ b/drivers/platform/surface/Makefile @@ -11,4 +11,5 @@ obj-$(CONFIG_SURFACE_ACPI_NOTIFY) += surface_acpi_notify.o obj-$(CONFIG_SURFACE_AGGREGATOR) += aggregator/ obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o +obj-$(CONFIG_SURFACE_HOTPLUG) += surface_hotplug.o obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o diff --git a/drivers/platform/surface/surface_hotplug.c b/drivers/platform/surface/surface_hotplug.c new file mode 100644 index ..cfcc15cfbacb --- /dev/null +++ b/drivers/platform/surface/surface_hotplug.c @@ -0,0 +1,282 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Surface Book (2 and later) hot-plug driver. + * + * Surface Book devices (can) have a hot-pluggable discrete GPU (dGPU). This + * driver is responsible for out-of-band hot-plug event signaling on these + * devices. It is specifically required when the hot-plug device is in D3cold + * and can thus not generate PCIe hot-plug events itself. + * + * Event signaling is handled via ACPI, which will generate the appropriate + * device-check notifications to be picked up by the PCIe hot-plug driver. + * + * Copyright (C) 2019-2021 Maximilian Luz + */ + +#include +#include +#include +#include
[PATCH] PCI: Run platform power transition on initial D0 entry
On some devices and platforms, the initial platform power state is not in sync with the power state of the PCI device. pci_enable_device_flags() updates the state of a PCI device by reading from the PCI_PM_CTRL register. This may change the stored power state of the device without running the appropriate platform power transition. Due to the stored power-state being changed, the later call to pci_set_power_state(..., PCI_D0) in do_pci_enable_device() can evaluate to a no-op if the stored state has been changed to D0 via that. This will then prevent the appropriate platform power transition to be run, which can on some devices and platforms lead to platform and PCI power state being entirely different, i.e. out-of-sync. On ACPI platforms, this can lead to power resources not being turned on, even though they are marked as required for D0. Specifically, on the Microsoft Surface Book 2 and 3, some ACPI power regions that should be "on" for the D0 state (and others) are initialized as "off" in ACPI, whereas the PCI device is in D0. As the state is updated in pci_enable_device_flags() without ensuring that the platform state is also updated, the power resource will never be properly turned on. Instead, it lives in a sort of on-but-marked-as-off zombie-state, which confuses things down the line when attempting to transition the device into D3cold: As the resource is already marked as off, it won't be turned off and the device does not fully enter D3cold, causing increased power consumption during (runtime-)suspend. By replacing pci_set_power_state() in do_pci_enable_device() with pci_power_up(), we can force pci_platform_power_transition() to be called, which will then check if the platform power state needs updating and appropriate actions need to be taken. Signed-off-by: Maximilian Luz --- I'm not entirely sure if this is the best way to do this, so I'm open to alternatives. In a previous version of this, I've tried to run the platform/ACPI transition directly after the pci_read_config_word() in pci_enable_device_flags(), however, that caused some regression in intel-lpss-pci, specifically that then had trouble accessing its config space for initial setup. This version has been tested for a while now on [1/2] without any complaints. As this essentially only drops the initial are-we-already- in-that-state-check, I don't expect any issues to be caused by that. [1]: https://github.com/linux-surface/linux-surface [2]: https://github.com/linux-surface/kernel --- drivers/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b9fecc25d213..eb778e80d8cf 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1802,7 +1802,7 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars) u16 cmd; u8 pin; - err = pci_set_power_state(dev, PCI_D0); + err = pci_power_up(dev); if (err < 0 && err != -EIO) return err; -- 2.30.0
Re: drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used
On 2/4/21 12:36 PM, Hans de Goede wrote: Hi, On 1/4/21 4:24 PM, Maximilian Luz wrote: On 1/4/21 3:52 PM, Hans de Goede wrote: Hi, On 12/29/20 4:58 AM, kernel test robot wrote: Hi Maximilian, FYI, the error/warning still remains. tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: dea8dcf2a9fa8cc540136a6cd885c3beece16ec3 commit: f23027ca3d48b6f93c5994069fb25b73539fdf34 platform/surface: Move Surface 3 WMI driver to platform/surface date: 9 weeks ago config: x86_64-randconfig-r001-20201221 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23027ca3d48b6f93c5994069fb25b73539fdf34 git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git git fetch --no-tags linus master git checkout f23027ca3d48b6f93c5994069fb25b73539fdf34 # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): drivers/platform/surface/surface3-wmi.c: In function 's3_wmi_query_block': drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable] 60 | acpi_status status; | ^~ I guess fixing this would require something like this: From: Hans de Goede Subject: [PATCH] platform/surface: surface3-wmi: Fix variable 'status' set but not used compiler warning Explictly check the status rather then relying on output.pointer staying NULL on an error. This silences the following compiler warning: drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable] Reported-by: kernel test robot Signed-off-by: Hans de Goede --- drivers/platform/surface/surface3-wmi.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c index 130b6f52a600..4b7f79c0b78e 100644 --- a/drivers/platform/surface/surface3-wmi.c +++ b/drivers/platform/surface/surface3-wmi.c @@ -63,6 +63,10 @@ static int s3_wmi_query_block(const char *guid, int instance, int *ret) mutex_lock(_wmi_lock); status = wmi_query_block(guid, instance, ); + if (ACPI_FAILURE(status)) { + error = -EIO; + goto out_free_unlock; + } obj = output.pointer; Maximilian, can you review and/or test this fix please ? Ah, this was on my TODO list (among looking at some other things in this driver), sorry that I haven't gotten to it yet. I'd have proposed pretty much the exact same thing. One thing to note though: You should initialize obj with NULL. Keeping it uninitialized may mess with kfree() under out_free_unlock. Unfortunately I don't have access to a Surface 3 to test, but apart from not initializing obj, this patch looks good to me. You may add my reviewed-by tag once you've fixed that. Thank you, and sorry for being a bit slow with my follow up. No worries. I've added the obj = NULL initialization and added your reviewed-by tag. I'm sending this out as an official patch submission for the archives now and then I'll apply it to my review-hans branch right away, so no need for you to do anything :) Also note that drivers/platform/x86/msi-wmi.c suffers from the same problem in msi_wmi_query_block() and should receive a similar fix. Thanks, I'll prep a patch for that too. Perfect, thanks! Regards, Max Regards, Hans
[PATCH] platform/surface: aggregator: Fix braces in if condition with unlikely() macro
The braces of the unlikely() macro inside the if condition only cover the subtraction part, not the whole statement. This causes the result of the subtraction to be converted to zero or one. While that still works in this context, it causes static analysis tools to complain (and is just plain wrong). Fix the bracket placement and, while at it, simplify the if-condition. Also add a comment to the if-condition explaining what we expect the result to be and what happens on the failure path, as it seems to have caused a bit of confusion. This commit should not cause any difference in behavior or generated code. Reported-by: Dan Carpenter Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") Signed-off-by: Maximilian Luz --- .../surface/aggregator/ssh_packet_layer.c | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c index 74f0faaa2b27..583315db8b02 100644 --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c @@ -1694,7 +1694,24 @@ static size_t ssh_ptl_rx_eval(struct ssh_ptl *ptl, struct ssam_span *source) /* Find SYN. */ syn_found = sshp_find_syn(source, ); - if (unlikely(aligned.ptr - source->ptr) > 0) { + if (unlikely(aligned.ptr != source->ptr)) { + /* +* We expect aligned.ptr == source->ptr. If this is not the +* case, then aligned.ptr > source->ptr and we've encountered +* some unexpected data where we'd expect the start of a new +* message (i.e. the SYN sequence). +* +* This can happen when a CRC check for the previous message +* failed and we start actively searching for the next one +* (via the call to sshp_find_syn() above), or the first bytes +* of a message got dropped or corrupted. +* +* In any case, we issue a warning, send a NAK to the EC to +* request re-transmission of any data we haven't acknowledged +* yet, and finally, skip everything up to the next SYN +* sequence. +*/ + ptl_warn(ptl, "rx: parser: invalid start of frame, skipping\n"); /* -- 2.30.0
[PATCH] platform/surface: aggregator: Fix kernel-doc references
Both, ssh_rtl_rx_start() and ssh_rtl_tx_start() functions, do not exist and have been consolidated into ssh_rtl_start(). Nevertheless, kernel-doc references the former functions. Replace those references with references to ssh_rtl_start(). Signed-off-by: Maximilian Luz --- Note: This patch does not change the kernel doc at the ssh_rtl_start() itself, as there is already another patch for it: "platform/surface: aggregator: fix a kernel-doc markup" https://lore.kernel.org/patchwork/patch/1364953/ --- drivers/platform/surface/aggregator/ssh_request_layer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/platform/surface/aggregator/ssh_request_layer.c b/drivers/platform/surface/aggregator/ssh_request_layer.c index bb1c862411a2..4fbe58265e31 100644 --- a/drivers/platform/surface/aggregator/ssh_request_layer.c +++ b/drivers/platform/surface/aggregator/ssh_request_layer.c @@ -1004,9 +1004,8 @@ int ssh_request_init(struct ssh_request *rqst, enum ssam_request_flags flags, * * Initializes the given request transport layer and associated packet * transport layer. Transmitter and receiver threads must be started - * separately via ssh_rtl_tx_start() and ssh_rtl_rx_start(), after the - * request-layer has been initialized and the lower-level serial device layer - * has been set up. + * separately via ssh_rtl_start(), after the request-layer has been + * initialized and the lower-level serial device layer has been set up. * * Return: Returns zero on success and a nonzero error code on failure. */ -- 2.30.0
Re: [PATCH v6 16/16] platform/surface: aggregator: fix a kernel-doc markup
On 1/14/21 9:04 AM, Mauro Carvalho Chehab wrote: A function has a different name between their prototype and its kernel-doc markup: ../drivers/platform/surface/aggregator/ssh_request_layer.c:1065: warning: expecting prototype for ssh_rtl_tx_start(). Prototype was for ssh_rtl_start() instead Signed-off-by: Mauro Carvalho Chehab --- drivers/platform/surface/aggregator/ssh_request_layer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/surface/aggregator/ssh_request_layer.c b/drivers/platform/surface/aggregator/ssh_request_layer.c index bb1c862411a2..25db4d638cfa 100644 --- a/drivers/platform/surface/aggregator/ssh_request_layer.c +++ b/drivers/platform/surface/aggregator/ssh_request_layer.c @@ -1056,7 +1056,7 @@ void ssh_rtl_destroy(struct ssh_rtl *rtl) } /** - * ssh_rtl_tx_start() - Start request transmitter and receiver. + * ssh_rtl_start() - Start request transmitter and receiver. * @rtl: The request transport layer. * * Return: Returns zero on success, a negative error code on failure. Thanks! Looks good to me. Reviewed-by: Maximilian Luz There seems to be another issue similar to this, specifically the non-existing ssh_rtl_tx_start() and ssh_rtl_tx_start() are referenced. Both should point to to ssh_rtl_start() instead. I'll start working on a patch to fix that right away. Regards, Max
[PATCH -next 1/2] platform/surface: aggregator_cdev: Fix access of uninitialized variables
When copy_struct_from_user() in ssam_cdev_request() fails, we directly jump to the 'out' label. In this case, however 'spec' and 'rsp' are not initialized, but we still access fields of those variables. Fix this by initializing them at the time of their declaration. Reported-by: Colin Ian King Fixes: 178f6ab77e61 ("platform/surface: Add Surface Aggregator user-space interface") Addresses-Coverity: ("Uninitialized pointer read") Signed-off-by: Maximilian Luz --- drivers/platform/surface/surface_aggregator_cdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c index 340d15b148b9..979340cdd9de 100644 --- a/drivers/platform/surface/surface_aggregator_cdev.c +++ b/drivers/platform/surface/surface_aggregator_cdev.c @@ -66,8 +66,8 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg) { struct ssam_cdev_request __user *r; struct ssam_cdev_request rqst; - struct ssam_request spec; - struct ssam_response rsp; + struct ssam_request spec = {}; + struct ssam_response rsp = {}; const void __user *plddata; void __user *rspdata; int status = 0, ret = 0, tmp; -- 2.30.0
[PATCH -next 2/2] platform/surface: aggregator_cdev: Add comments regarding unchecked allocation size
CI static analysis complains about the allocation size in payload and response buffers being unchecked. In general, these allocations should be safe as the user-input is u16 and thus limited to U16_MAX, which is only slightly larger than the theoretical maximum imposed by the underlying SSH protocol. All bounds on these values required by the underlying protocol are enforced in ssam_request_sync() (or rather the functions called by it), thus bounds here are only relevant for allocation. Add comments explaining that this should be safe. Reported-by: Colin Ian King Fixes: 178f6ab77e61 ("platform/surface: Add Surface Aggregator user-space interface") Addresses-Coverity: ("Untrusted allocation size") Signed-off-by: Maximilian Luz --- .../surface/surface_aggregator_cdev.c | 19 +++ 1 file changed, 19 insertions(+) diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c index 979340cdd9de..ccfffe5eadfc 100644 --- a/drivers/platform/surface/surface_aggregator_cdev.c +++ b/drivers/platform/surface/surface_aggregator_cdev.c @@ -106,6 +106,15 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg) goto out; } + /* +* Note: spec.length is limited to U16_MAX bytes via struct +* ssam_cdev_request. This is slightly larger than the +* theoretical maximum (SSH_COMMAND_MAX_PAYLOAD_SIZE) of the +* underlying protocol (note that nothing remotely this size +* should ever be allocated in any normal case). This size is +* validated later in ssam_request_sync(), for allocation the +* bound imposed by u16 should be enough. +*/ spec.payload = kzalloc(spec.length, GFP_KERNEL); if (!spec.payload) { ret = -ENOMEM; @@ -125,6 +134,16 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg) goto out; } + /* +* Note: rsp.capacity is limited to U16_MAX bytes via struct +* ssam_cdev_request. This is slightly larger than the +* theoretical maximum (SSH_COMMAND_MAX_PAYLOAD_SIZE) of the +* underlying protocol (note that nothing remotely this size +* should ever be allocated in any normal case). In later use, +* this capacity does not have to be strictly bounded, as it +* is only used as an output buffer to be written to. For +* allocation the bound imposed by u16 should be enough. +*/ rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL); if (!rsp.pointer) { ret = -ENOMEM; -- 2.30.0
[PATCH -next 0/2] platform/surface: aggregator_cdev: Fixes for CI analysis
Hi, here are some patches addressing two issues in the Surface Aggregator user-space interface reported by Colin Ian King via static analysis. Maximilian Luz (2): platform/surface: aggregator_cdev: Fix access of uninitialized variables platform/surface: aggregator_cdev: Add comments regarding unchecked allocation size .../surface/surface_aggregator_cdev.c | 23 +-- 1 file changed, 21 insertions(+), 2 deletions(-) -- 2.30.0
Re: [PATCH][next] platform/surface: fix potential integer overflow on shift of a int
On 1/11/21 3:46 PM, Colin King wrote: From: Colin Ian King The left shift of int 32 bit integer constant 1 is evaluated using 32 bit arithmetic and then passed as a 64 bit function argument. In the case where func is 32 or more this can lead to an oveflow. Avoid this by shifting using the BIT_ULL macro instead. Addresses-Coverity: ("Unintentional integer overflow") Fixes: fc00bc8ac1da ("platform/surface: Add Surface ACPI Notify driver") Signed-off-by: Colin Ian King --- drivers/platform/surface/surface_acpi_notify.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c index 8cd67a669c86..ef9c1f8e8336 100644 --- a/drivers/platform/surface/surface_acpi_notify.c +++ b/drivers/platform/surface/surface_acpi_notify.c @@ -188,7 +188,7 @@ static int san_acpi_notify_event(struct device *dev, u64 func, union acpi_object *obj; int status = 0; - if (!acpi_check_dsm(san, _DSM_UUID, SAN_DSM_REVISION, 1 << func)) + if (!acpi_check_dsm(san, _DSM_UUID, SAN_DSM_REVISION, BIT_ULL(func))) return 0; dev_dbg(dev, "notify event %#04llx\n", func); Thanks, looks good to me. Reviewed-by: Maximilian Luz Regards, Max
Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
On 1/11/21 3:11 PM, Colin Ian King wrote: On 11/01/2021 13:55, Maximilian Luz wrote: On 1/11/21 1:12 PM, Colin Ian King wrote: Hi Maximilian, Static analysis of linux-next with Coverity has found several issues with the following commit: commit 178f6ab77e617c984d6520b92e747075a12676ff Author: Maximilian Luz Date: Mon Dec 21 19:39:58 2020 +0100 platform/surface: Add Surface Aggregator user-space interface The analysis is as follows: 65static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg) 66{ 67 struct ssam_cdev_request __user *r; 68 struct ssam_cdev_request rqst; 1. var_decl: Declaring variable spec without initializer. 69 struct ssam_request spec; 70 struct ssam_response rsp; 71 const void __user *plddata; 72 void __user *rspdata; 73 int status = 0, ret = 0, tmp; 74 75 r = (struct ssam_cdev_request __user *)arg; 76 ret = copy_struct_from_user(, sizeof(rqst), r, sizeof(*r)); 2. Condition ret, taking true branch. 77 if (ret) 3. Jumping to label out. 78 goto out; 79 80 plddata = u64_to_user_ptr(rqst.payload.data); 81 rspdata = u64_to_user_ptr(rqst.response.data); 82 83 /* Setup basic request fields. */ 84 spec.target_category = rqst.target_category; 85 spec.target_id = rqst.target_id; 86 spec.command_id = rqst.command_id; 87 spec.instance_id = rqst.instance_id; 88 spec.flags = 0; 89 spec.length = rqst.payload.length; 90 spec.payload = NULL; 91 92 if (rqst.flags & SSAM_CDEV_REQUEST_HAS_RESPONSE) 93 spec.flags |= SSAM_REQUEST_HAS_RESPONSE; 94 95 if (rqst.flags & SSAM_CDEV_REQUEST_UNSEQUENCED) 96 spec.flags |= SSAM_REQUEST_UNSEQUENCED; 97 98 rsp.capacity = rqst.response.length; 99 rsp.length = 0; 100 rsp.pointer = NULL; 101 102 /* Get request payload from user-space. */ 103 if (spec.length) { 104 if (!plddata) { 105 ret = -EINVAL; 106 goto out; 107 } 108 CID: Untrusted allocation size (TAINTED_SCALAR) 8. tainted_data: Passing tainted expression spec.length to kzalloc, which uses it as an allocation size 109 spec.payload = kzalloc(spec.length, GFP_KERNEL); I assume a constraint on the maximum length will fix this? I believe so, it's unsigned so just an upper size check will be required to silence this static analysis warning. Mind you, you may want a size that is the full u16 max of 65535, so in that case the check is not required. Right, the theoretical maximum payload (spec.length) and response size allowed by the Surface Aggregator SSH protocol is 'U16_MAX - sizeof(struct ssh_command)' (not that anything this size should ever be allocated in any normal case). Meaning it is (slightly) smaller than U16_MAX, but I'm not sure if it warrants a check here. The payload size is later validated by ssam_request_sync(), so it does only affect the allocation here (the response is just an output buffer and may be of arbitrary size). I think the limit imposed by having u16 as user-input should be enough. I can still add an explicit check here if that is preferred, but I could also add a comment explaining that this should be safe. 110 if (!spec.payload) { 111 ret = -ENOMEM; 112 goto out; 113 } 114 115 if (copy_from_user((void *)spec.payload, plddata, spec.length)) { 116 ret = -EFAULT; 117 goto out; 118 } 119 } 120 121 /* Allocate response buffer. */ 122 if (rsp.capacity) { 123 if (!rspdata) { 124 ret = -EINVAL; 125 goto out; 126 } 127 CID: Untrusted allocation size (TAINTED_SCALAR) 12. tainted_data: Passing tainted expression rsp.capacity to kzalloc, which uses it as an allocation size 128 rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL); 129 if (!rsp.pointer) { 130 ret = -ENOMEM; 131 goto out; 132 } 133 } 134 135 /* Perform request. */ 136 status = ssam_request_sync(cdev->ctrl, , ); 137 if (status) 138 goto out; 139 140 /* Copy response to user-space. */ 141 if (rsp.length && copy_to_user(rspdata, rsp.pointer, rsp.length)) 142 ret = -EFAULT; 143 144out: 145 /* Always try to set response-length and status. */ CID: Uninitialized pointer read (UNINIT) Using uninitialized value rsp.length 146 tmp = put_user(rsp.length, >response.length); 4. Condition tmp, ta
Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
On 1/11/21 1:12 PM, Colin Ian King wrote: Hi Maximilian, Static analysis of linux-next with Coverity has found several issues with the following commit: commit 178f6ab77e617c984d6520b92e747075a12676ff Author: Maximilian Luz Date: Mon Dec 21 19:39:58 2020 +0100 platform/surface: Add Surface Aggregator user-space interface The analysis is as follows: 65static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg) 66{ 67struct ssam_cdev_request __user *r; 68struct ssam_cdev_request rqst; 1. var_decl: Declaring variable spec without initializer. 69struct ssam_request spec; 70struct ssam_response rsp; 71const void __user *plddata; 72void __user *rspdata; 73int status = 0, ret = 0, tmp; 74 75r = (struct ssam_cdev_request __user *)arg; 76ret = copy_struct_from_user(, sizeof(rqst), r, sizeof(*r)); 2. Condition ret, taking true branch. 77if (ret) 3. Jumping to label out. 78goto out; 79 80plddata = u64_to_user_ptr(rqst.payload.data); 81rspdata = u64_to_user_ptr(rqst.response.data); 82 83/* Setup basic request fields. */ 84spec.target_category = rqst.target_category; 85spec.target_id = rqst.target_id; 86spec.command_id = rqst.command_id; 87spec.instance_id = rqst.instance_id; 88spec.flags = 0; 89spec.length = rqst.payload.length; 90spec.payload = NULL; 91 92if (rqst.flags & SSAM_CDEV_REQUEST_HAS_RESPONSE) 93spec.flags |= SSAM_REQUEST_HAS_RESPONSE; 94 95if (rqst.flags & SSAM_CDEV_REQUEST_UNSEQUENCED) 96spec.flags |= SSAM_REQUEST_UNSEQUENCED; 97 98rsp.capacity = rqst.response.length; 99rsp.length = 0; 100rsp.pointer = NULL; 101 102/* Get request payload from user-space. */ 103if (spec.length) { 104if (!plddata) { 105ret = -EINVAL; 106goto out; 107} 108 CID: Untrusted allocation size (TAINTED_SCALAR) 8. tainted_data: Passing tainted expression spec.length to kzalloc, which uses it as an allocation size 109spec.payload = kzalloc(spec.length, GFP_KERNEL); I assume a constraint on the maximum length will fix this? 110if (!spec.payload) { 111ret = -ENOMEM; 112goto out; 113} 114 115if (copy_from_user((void *)spec.payload, plddata, spec.length)) { 116ret = -EFAULT; 117goto out; 118} 119} 120 121/* Allocate response buffer. */ 122if (rsp.capacity) { 123if (!rspdata) { 124ret = -EINVAL; 125goto out; 126} 127 CID: Untrusted allocation size (TAINTED_SCALAR) 12. tainted_data: Passing tainted expression rsp.capacity to kzalloc, which uses it as an allocation size 128rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL); 129if (!rsp.pointer) { 130ret = -ENOMEM; 131goto out; 132} 133} 134 135/* Perform request. */ 136status = ssam_request_sync(cdev->ctrl, , ); 137if (status) 138goto out; 139 140/* Copy response to user-space. */ 141if (rsp.length && copy_to_user(rspdata, rsp.pointer, rsp.length)) 142ret = -EFAULT; 143 144out: 145/* Always try to set response-length and status. */ CID: Uninitialized pointer read (UNINIT) Using uninitialized value rsp.length 146tmp = put_user(rsp.length, >response.length); 4. Condition tmp, taking true branch. 147if (tmp) 148ret = tmp; 149 150tmp = put_user(status, >status); 5. Condition tmp, taking true branch. 151if (tmp) 152ret = tmp; 153 154/* Cleanup. */ CID: Uninitialized pointer read (UNINIT) 6. uninit_use_in_call: Using uninitialized value spec.payload when calling kfree. 155kfree(spec.payload); CID: Uninitialized pointer read (UNINIT) uninit_use_in_call: Using uninitialized value rsp.pointer when calling kfree 156kfree(rsp.pointer); Right, taking the first jump to out leaves rsp and spec uninitialized. I'll fix that. 157 158return ret; Colin Thank you for the analysis. I'll draft up two patches to address these issues. Regards, Max
Re: [PATCH v3 0/9] Add support for Microsoft Surface System Aggregator Module
On 1/7/21 12:40 AM, Hans de Goede wrote: Hi, On 12/21/20 7:39 PM, Maximilian Luz wrote: Hello, Here is version three 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 first version and cover letter detailing what this series is about can be found at https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximil...@gmail.com/ the previous version (v2) at https://lore.kernel.org/platform-driver-x86/20201203212640.663931-1-luzmaximil...@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/v3 Thank you all for the feedback to and reviews of the previous versions, I hope I have addressed all comments. Thank you for your patch-series, I've applied the series to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans This has been applied on top of: "platform/surface: SURFACE_PLATFORMS should depend on ACPI" As requested. Since my review-hans branch has been fast-forwarded to 5.11-rc1 there where some conflicts: 6/9 "platform/surface: aggregator: Add dedicated bus and device type" had conflicts in: scripts/mod/devicetable-offsets.c scripts/mod/file2alias.c 8/9 "platform/surface: Add Surface Aggregator user-space interface" had a conflict in: Documentation/userspace-api/ioctl/ioctl-number.rst This conflict was caused by the addition of documentation of the sgx ioctls, these use 0xA4 as "Code" byte (shared with uapi/linux/tee.h), so the 0xA5 code for the sam cdev ioctl is still unique. I'm pretty sure that I've properly resolved the conflicts, but please double-check. Everything looks good to me. I'll compile your branch and run some tests just in case, but I really don't expect there to be any problems. In the unlikely event that I find anything, I'll submit a patch. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Thank you (and all others involved in reviewing this). Regards, Max Regards, Hans Note: In v3, I have dropped the explicit dependency of the core module and driver on CONFIG_ACPI due to the incoming [PATCH] platform/surface: SURFACE_PLATFORMS should depend on ACPI Thus, this series depends on said patch. This patch can be found at https://www.spinics.net/lists/platform-driver-x86/msg23929.html 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 in v3 (overview): - remove explicit dependency on ACPI as this is going to be covered by CONFIG_SURFACE_PLATFORMS - simplify locking requirements - help enforce locking requirements via lockdep assertions - fix false-positive lockdep warning - warn on event enablement reference counter exhaustion - don't warn about unhandled event if event handling failed - validate flags on request initialization - improve documentation/add comments - replace 'iff' with 'if' in documentation and comments 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 |
Re: drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used
On 1/4/21 3:52 PM, Hans de Goede wrote: Hi, On 12/29/20 4:58 AM, kernel test robot wrote: Hi Maximilian, FYI, the error/warning still remains. tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: dea8dcf2a9fa8cc540136a6cd885c3beece16ec3 commit: f23027ca3d48b6f93c5994069fb25b73539fdf34 platform/surface: Move Surface 3 WMI driver to platform/surface date: 9 weeks ago config: x86_64-randconfig-r001-20201221 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f23027ca3d48b6f93c5994069fb25b73539fdf34 git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git git fetch --no-tags linus master git checkout f23027ca3d48b6f93c5994069fb25b73539fdf34 # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): drivers/platform/surface/surface3-wmi.c: In function 's3_wmi_query_block': drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable] 60 | acpi_status status; | ^~ I guess fixing this would require something like this: From: Hans de Goede Subject: [PATCH] platform/surface: surface3-wmi: Fix variable 'status' set but not used compiler warning Explictly check the status rather then relying on output.pointer staying NULL on an error. This silences the following compiler warning: drivers/platform/surface/surface3-wmi.c:60:14: warning: variable 'status' set but not used [-Wunused-but-set-variable] Reported-by: kernel test robot Signed-off-by: Hans de Goede --- drivers/platform/surface/surface3-wmi.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c index 130b6f52a600..4b7f79c0b78e 100644 --- a/drivers/platform/surface/surface3-wmi.c +++ b/drivers/platform/surface/surface3-wmi.c @@ -63,6 +63,10 @@ static int s3_wmi_query_block(const char *guid, int instance, int *ret) mutex_lock(_wmi_lock); status = wmi_query_block(guid, instance, ); + if (ACPI_FAILURE(status)) { + error = -EIO; + goto out_free_unlock; + } obj = output.pointer; Maximilian, can you review and/or test this fix please ? Ah, this was on my TODO list (among looking at some other things in this driver), sorry that I haven't gotten to it yet. I'd have proposed pretty much the exact same thing. One thing to note though: You should initialize obj with NULL. Keeping it uninitialized may mess with kfree() under out_free_unlock. Unfortunately I don't have access to a Surface 3 to test, but apart from not initializing obj, this patch looks good to me. You may add my reviewed-by tag once you've fixed that. Also note that drivers/platform/x86/msi-wmi.c suffers from the same problem in msi_wmi_query_block() and should receive a similar fix. Regards, Max
Re: [PATCH] platform/surface: mark PM functions as __maybe_unused
On 1/3/21 3:04 PM, Arnd Bergmann wrote: From: Arnd Bergmann When CONFIG_PM is disabled, the compiler produces harmless warnings: drivers/platform/surface/surface_gpe.c:184:12: error: unused function 'surface_gpe_suspend' [-Werror,-Wunused-function] static int surface_gpe_suspend(struct device *dev) drivers/platform/surface/surface_gpe.c:189:12: error: unused function 'surface_gpe_resume' [-Werror,-Wunused-function] static int surface_gpe_resume(struct device *dev) Mark these as __maybe_unused to shut up the warning. Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") Signed-off-by: Arnd Bergmann --- drivers/platform/surface/surface_gpe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/platform/surface/surface_gpe.c b/drivers/platform/surface/surface_gpe.c index e49e5d6d5d4e..86f6991b1215 100644 --- a/drivers/platform/surface/surface_gpe.c +++ b/drivers/platform/surface/surface_gpe.c @@ -181,12 +181,12 @@ static int surface_lid_enable_wakeup(struct device *dev, bool enable) return 0; } -static int surface_gpe_suspend(struct device *dev) +static int __maybe_unused surface_gpe_suspend(struct device *dev) { return surface_lid_enable_wakeup(dev, true); } -static int surface_gpe_resume(struct device *dev) +static int __maybe_unused surface_gpe_resume(struct device *dev) { return surface_lid_enable_wakeup(dev, false); } Hi, a patch doing this exact thing has already been submitted, but has yet to be picked up: https://www.spinics.net/lists/platform-driver-x86/msg23888.html Thanks, Max
[PATCH v3 7/9] docs: driver-api: Add Surface Aggregator subsystem documentation
Add documentation for the Surface Aggregator subsystem and its client drivers, giving an overview of the subsystem, its use-cases, its internal structure and internal API, as well as its external API for writing client drivers. Signed-off-by: Maximilian Luz Reviewed-by: Hans de Goede --- Changes in v1 (from RFC): - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later) - fix typos Changes in v2: - add architectural overview for core driver internals - reorder chapters - improve wording --- Documentation/driver-api/index.rst| 1 + .../surface_aggregator/client-api.rst | 38 ++ .../driver-api/surface_aggregator/client.rst | 393 .../surface_aggregator/clients/index.rst | 10 + .../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 +++ MAINTAINERS | 1 + 10 files changed, 1529 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/index.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 diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst index f357f3eb400c..699dc7cac0fb 100644 --- a/Documentation/driver-api/index.rst +++ b/Documentation/driver-api/index.rst @@ -97,6 +97,7 @@ available subsections can be seen below. rfkill serial/index sm501 + surface_aggregator/index switchtec sync_file vfio-mediated-device diff --git a/Documentation/driver-api/surface_aggregator/client-api.rst b/Documentation/driver-api/surface_aggregator/client-api.rst new file mode 100644 index ..8e0b000d0e64 --- /dev/null +++ b/Documentation/driver-api/surface_aggregator/client-api.rst @@ -0,0 +1,38 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +=== +Client Driver API Documentation +=== + +.. contents:: +:depth: 2 + + +Serial Hub Communication + + +.. kernel-doc:: include/linux/surface_aggregator/serial_hub.h + +.. kernel-doc:: drivers/platform/surface/aggregator/ssh_packet_layer.c +:export: + + +Controller and Core Interface += + +.. kernel-doc:: include/linux/surface_aggregator/controller.h + +.. kernel-doc:: drivers/platform/surface/aggregator/controller.c +:export: + +.. kernel-doc:: drivers/platform/surface/aggregator/core.c +:export: + + +Client Bus and Client Device API + + +.. kernel-doc:: include/linux/surface_aggregator/device.h + +.. kernel-doc:: drivers/platform/surface/aggregator/bus.c +:export: diff --git a/Documentation/driver-api/surface_aggregator/client.rst b/Documentation/driver-api/surface_aggregator/client.rst new file mode 100644 index ..26d13085a117 --- /dev/null +++ b/Documentation/driver-api/surface_aggregator/client.rst @@ -0,0 +1,393 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +.. |ssam_controller| replace:: :c:type:`struct ssam_controller ` +.. |ssam_device| replace:: :c:type:`struct ssam_device ` +.. |ssam_device_driver| replace:: :c:type:`struct ssam_device_driver ` +.. |ssam_client_bind| replace:: :c:func:`ssam_client_bind` +.. |ssam_client_link| replace:: :c:func:`ssam_client_link` +.. |ssam_get_controller| replace:: :c:func:`ssam_get_controller` +.. |ssam_controller_get| replace:: :c:func:`ssam_controller_get` +.. |ssam_controller_put| replace:: :c:func:`ssam_controller_put` +.. |ssam_device_alloc| replace:: :c:func:`ssam_device_alloc` +.. |ssam_device_add| replace:: :c:func:`ssam_device_add` +.. |ssam_device_remove| replace:: :c:func:`ssam_device_remove` +.. |ssam_device_driver_register| replace:: :c:func:`ssam_device_driver_register` +.. |ssam_device_driver_unregister| replace:: :c:func:`ssam_device_driver_unregister` +.. |module_ssam_device_driver| replace:: :c:func:`module_ssam_device_driver` +.. |SSAM_DEVICE| replace:: :c:func:`SSAM_DEVICE` +.. |ssam_notifier_register| replace:: :c:func:`ssam_notifier_register` +.. |ssam_notifier_unregister| replace:: :c:func:`ssam_notifier_unregister` +.. |ssam_request_sync| replace:: :c:func:`ssam_request_sync` +.. |ssam_event_mask| replace:: :c:type:`enum ssam_event_mask ` + + +== +Writing Client Drivers +== + +For the API documentation
[PATCH v3 6/9] platform/surface: aggregator: Add dedicated bus and device type
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 Reviewed-by: Hans de Goede --- 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 Changes in v3: - replace 'iff' with 'if' in documentation/comments --- 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 e417bac67088..3aaeea9f0433 100644 --- a/drivers/platform/surface/aggregator/Kconfig +++ b/drivers/platform/surface/aggregator/Kconfig @@ -41,6 +41,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 ..a9b660af0917 --- /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 + */ + +#include +#include + +#include +#include + +#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[] = { + _attr_modalias.attr
[PATCH v3 9/9] platform/surface: Add Surface ACPI Notify driver
The Surface ACPI Notify (SAN) device provides an ACPI interface to the Surface Aggregator EC, specifically the Surface Serial Hub interface. This interface allows EC requests to be made from ACPI code and can convert a subset of EC events back to ACPI notifications. Specifically, this interface provides a GenericSerialBus operation region ACPI code can execute a request by writing the request command data and payload to this operation region and reading back the corresponding response via a write-then-read operation. Furthermore, this interface provides a _DSM method to be called when certain events from the EC have been received, essentially turning them into ACPI notifications. The driver provided in this commit essentially takes care of translating the request data written to the operation region, executing the request, waiting for it to finish, and finally writing and translating back the response (if the request has one). Furthermore, this driver takes care of enabling the events handled via ACPI _DSM calls. Lastly, this driver also exposes an interface providing discrete GPU (dGPU) power-on notifications on the Surface Book 2, which are also received via the operation region interface (but not handled by the SAN driver directly), making them accessible to other drivers (such as a dGPU hot-plug driver that may be added later on). On 5th and 6th generation Surface devices (Surface Pro 5/2017, Pro 6, Book 2, Laptop 1 and 2), the SAN interface provides full battery and thermal subsystem access, as well as other EC based functionality. On those models, battery and thermal sensor devices are implemented as standard ACPI devices of that type, however, forward ACPI calls to the corresponding Surface Aggregator EC request via the SAN interface and receive corresponding notifications (e.g. battery information change) from it. This interface is therefore required to provide said functionality on those devices. Signed-off-by: Maximilian Luz Reviewed-by: Hans de Goede --- Changes in v1 (from RFC): - add copyright lines - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later) - fix invalid pointer calculation in san_evt_tmp - add explicit dependency on CONFIG_ACPI - remove default in Kconfig Changes in v2: - drop explicit dependency on ACPI in Kconfig - 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 warnings and style issues --- .../surface_aggregator/clients/index.rst | 1 + .../surface_aggregator/clients/san.rst| 44 + MAINTAINERS | 2 + drivers/platform/surface/Kconfig | 19 + drivers/platform/surface/Makefile | 1 + .../platform/surface/surface_acpi_notify.c| 886 ++ include/linux/surface_acpi_notify.h | 39 + 7 files changed, 992 insertions(+) create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst create mode 100644 drivers/platform/surface/surface_acpi_notify.c create mode 100644 include/linux/surface_acpi_notify.h diff --git a/Documentation/driver-api/surface_aggregator/clients/index.rst b/Documentation/driver-api/surface_aggregator/clients/index.rst index ab260ec82cfb..3ccabce23271 100644 --- a/Documentation/driver-api/surface_aggregator/clients/index.rst +++ b/Documentation/driver-api/surface_aggregator/clients/index.rst @@ -11,6 +11,7 @@ This is the documentation for client drivers themselves. Refer to :maxdepth: 1 cdev + san .. only:: subproject and html diff --git a/Documentation/driver-api/surface_aggregator/clients/san.rst b/Documentation/driver-api/surface_aggregator/clients/san.rst new file mode 100644 index ..38c2580e7758 --- /dev/null +++ b/Documentation/driver-api/surface_aggregator/clients/san.rst @@ -0,0 +1,44 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +.. |san_client_link| replace:: :c:func:`san_client_link` +.. |san_dgpu_notifier_register| replace:: :c:func:`san_dgpu_notifier_register` +.. |san_dgpu_notifier_unregister| replace:: :c:func:`san_dgpu_notifier_unregister` + +=== +Surface ACPI Notify +=== + +The Surface ACPI Notify (SAN) device provides the bridge between ACPI and +SAM controller. Specifically, ACPI code can execute requests and handle +battery and thermal events via this interface. In addition to this, events +relating to the discrete GPU (dGPU) of the Surface Book 2 can be sent from +ACPI code (note: the Surface Book 3 uses a different method for this). The +only currently known event sent via this interface is a dGPU power-on +notification. While this driver handles the former part internally, it only +relays the dGPU events to any other driver interested via its public API and +does not handle them. + +The public interface of this driver is split into two parts: Client +registration and notifier-block registration. + +A client to the SAN
[PATCH v3 3/9] platform/surface: aggregator: Add event item allocation caching
Event items are used for completing Surface Aggregator EC events, i.e. placing event command data and payload on a workqueue for later processing to avoid doing said processing directly on the receiver thread. This means that event items are allocated for each incoming event, regardless of that event being transmitted via sequenced or unsequenced packets. On the Surface Book 3 and Surface Laptop 3, touchpad HID input events (unsequenced), can constitute a larger amount of traffic, and therefore allocation of event items. This warrants caching event items to reduce memory fragmentation. The size of the cached objects is specifically tuned to accommodate keyboard and touchpad input events and their payloads on those devices. As a result, this effectively also covers most other event types. In case of a larger event payload, event item allocation will fall back to kzalloc(). Signed-off-by: Maximilian Luz Reviewed-by: Hans de Goede --- .../platform/surface/aggregator/controller.c | 86 +-- .../platform/surface/aggregator/controller.h | 9 ++ drivers/platform/surface/aggregator/core.c| 16 +++- 3 files changed, 101 insertions(+), 10 deletions(-) diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c index 488318cf2098..775a4509bece 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -513,14 +513,74 @@ static void ssam_nf_destroy(struct ssam_nf *nf) */ #define SSAM_CPLT_WQ_BATCH 10 +/* + * SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN - Maximum payload length for a cached + * ssam_event_item. + * + * This length has been chosen to be accommodate standard touchpad and + * keyboard input events. Events with larger payloads will be allocated + * separately. + */ +#define SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN 32 + +static struct kmem_cache *ssam_event_item_cache; + +/** + * ssam_event_item_cache_init() - Initialize the event item cache. + */ +int ssam_event_item_cache_init(void) +{ + const unsigned int size = sizeof(struct ssam_event_item) + + SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN; + const unsigned int align = __alignof__(struct ssam_event_item); + struct kmem_cache *cache; + + cache = kmem_cache_create("ssam_event_item", size, align, 0, NULL); + if (!cache) + return -ENOMEM; + + ssam_event_item_cache = cache; + return 0; +} + +/** + * ssam_event_item_cache_destroy() - Deinitialize the event item cache. + */ +void ssam_event_item_cache_destroy(void) +{ + kmem_cache_destroy(ssam_event_item_cache); + ssam_event_item_cache = NULL; +} + +static void __ssam_event_item_free_cached(struct ssam_event_item *item) +{ + kmem_cache_free(ssam_event_item_cache, item); +} + +static void __ssam_event_item_free_generic(struct ssam_event_item *item) +{ + kfree(item); +} + +/** + * ssam_event_item_free() - Free the provided event item. + * @item: The event item to free. + */ +static void ssam_event_item_free(struct ssam_event_item *item) +{ + item->ops.free(item); +} + /** * ssam_event_item_alloc() - Allocate an event item with the given payload size. * @len: The event payload length. * @flags: The flags used for allocation. * - * Allocate an event item with the given payload size. Sets the item - * operations and payload length values. The item free callback (``ops.free``) - * should not be overwritten after this call. + * Allocate an event item with the given payload size, preferring allocation + * from the event item cache if the payload is small enough (i.e. smaller than + * %SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN). Sets the item operations and payload + * length values. The item free callback (``ops.free``) should not be + * overwritten after this call. * * Return: Returns the newly allocated event item. */ @@ -528,9 +588,19 @@ static struct ssam_event_item *ssam_event_item_alloc(size_t len, gfp_t flags) { struct ssam_event_item *item; - item = kzalloc(struct_size(item, event.data, len), flags); - if (!item) - return NULL; + if (len <= SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN) { + item = kmem_cache_alloc(ssam_event_item_cache, flags); + if (!item) + return NULL; + + item->ops.free = __ssam_event_item_free_cached; + } else { + item = kzalloc(struct_size(item, event.data, len), flags); + if (!item) + return NULL; + + item->ops.free = __ssam_event_item_free_generic; + } item->event.length = len; return item; @@ -692,7 +762,7 @@ static void ssam_event_queue_work_fn(struct work_struct *work) return; ssam_nf_call(nf, dev, item->rqid, >event); - kfree(item); +
[PATCH v3 8/9] platform/surface: Add Surface Aggregator user-space interface
Add a misc-device providing user-space access to the Surface Aggregator EC, mainly intended for debugging, testing, and reverse-engineering. This interface gives user-space applications the ability to send requests to the EC and receive the corresponding responses. The device-file is managed by a pseudo platform-device and corresponding driver to avoid dependence on the dedicated bus, allowing it to be loaded in a minimal configuration. A python library and scripts to access this device can be found at [1]. [1]: https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam Signed-off-by: Maximilian Luz --- Changes in v1 (from RFC): - add copyright lines - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later) - move from debugfs to proper misc-device - move interface definitions to uapi header - remove versioning - allocate platform device dynamically - prioritize EFAULT error in IOCTL return - use ENOTTY error for unknown IOCTLs - fix IOCTL compatibility for 32-bit user-space on 64-bit kernels - mark request struct as __attribute__((__packed__)) - address issues when driver is unbound while file still open Changes in v2: - add ioctl-number.rst entry - improve documentation - spell check comments and strings, fix typos - unify comment style - run checkpatch --strict, fix warnings and style issues Changes in v3: - export request flags in uapi header - return ENOMEM instead of EFAULT on memory allocation failure - add link to example tools to documentation and commit message --- .../surface_aggregator/clients/cdev.rst | 87 + .../surface_aggregator/clients/index.rst | 12 +- .../userspace-api/ioctl/ioctl-number.rst | 2 + MAINTAINERS | 2 + drivers/platform/surface/Kconfig | 17 + drivers/platform/surface/Makefile | 1 + .../surface/surface_aggregator_cdev.c | 303 ++ include/uapi/linux/surface_aggregator/cdev.h | 78 + 8 files changed, 501 insertions(+), 1 deletion(-) create mode 100644 Documentation/driver-api/surface_aggregator/clients/cdev.rst create mode 100644 drivers/platform/surface/surface_aggregator_cdev.c create mode 100644 include/uapi/linux/surface_aggregator/cdev.h diff --git a/Documentation/driver-api/surface_aggregator/clients/cdev.rst b/Documentation/driver-api/surface_aggregator/clients/cdev.rst new file mode 100644 index ..248c1372d879 --- /dev/null +++ b/Documentation/driver-api/surface_aggregator/clients/cdev.rst @@ -0,0 +1,87 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +.. |u8| replace:: :c:type:`u8 ` +.. |u16| replace:: :c:type:`u16 ` +.. |ssam_cdev_request| replace:: :c:type:`struct ssam_cdev_request ` +.. |ssam_cdev_request_flags| replace:: :c:type:`enum ssam_cdev_request_flags ` + +== +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. + +The provided interface is accessible through the ``/dev/surface/aggregator`` +device-file. All functionality of this interface is provided via IOCTLs. +These IOCTLs and their respective input/output parameter structs are defined in +``include/uapi/linux/surface_aggregator/cdev.h``. + +A small python library and scripts for accessing this interface can be found +at https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam. + + +Controller IOCTLs += + +The following IOCTLs are provided: + +.. flat-table:: Controller IOCTLs + :widths: 1 1 1 1 4 + :header-rows: 1 + + * - Type + - Number + - Direction + - Name + - Description + + * - ``0xA5`` + - ``1`` + - ``WR`` + - ``REQUEST`` + - Perform synchronous SAM request. + + +``REQUEST`` +--- + +Defined as ``_IOWR(0xA5, 1, struct ssam_cdev_request)``. + +Executes a synchronous SAM request. The request specification is passed in +as argument of type |ssam_cdev_request|, which is then written to/modified +by the IOCTL to return status and result of the request. + +Request payload data must be allocated separately and is passed in via the +``payload.data`` and ``payload.length`` members. If a response is required, +the response buffer must be allocated by the caller and passed in via the +``response.data`` member. The ``response.length`` member must be set to the +capacity of this buffer, or if no response is required, zero. Upon +completion of the request, the call will write the response to the response +buffer (if its capacity allows it) and overwrite the length field with the +actual size of the response, in bytes
[PATCH v3 5/9] platform/surface: aggregator: Add error injection capabilities
This commit adds error injection hooks to the Surface Serial Hub communication protocol implementation, to: - simulate simple serial transmission errors, - drop packets, requests, and responses, simulating communication failures and potentially trigger retransmission timeouts, as well as - inject invalid data into submitted and received packets. Together with the trace points introduced in the previous commit, these facilities are intended to aid in testing, validation, and debugging of the Surface Aggregator communication layer. Signed-off-by: Maximilian Luz Reviewed-by: Hans de Goede --- Changes in v1 (from RFC): - remove unnecessary default in Kconfig entry Changes in v2: - use dedicated trace event class for data error injection - spell check comments and strings, fix typos - unify comment style - run checkpatch --strict, fix warnings and style issues --- drivers/platform/surface/aggregator/Kconfig | 14 + .../surface/aggregator/ssh_packet_layer.c | 296 +- .../surface/aggregator/ssh_request_layer.c| 35 +++ drivers/platform/surface/aggregator/trace.h | 31 ++ 4 files changed, 375 insertions(+), 1 deletion(-) diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig index e9f4ad96e40a..e417bac67088 100644 --- a/drivers/platform/surface/aggregator/Kconfig +++ b/drivers/platform/surface/aggregator/Kconfig @@ -40,3 +40,17 @@ menuconfig SURFACE_AGGREGATOR Choose m if you want to build the SAM subsystem core and SSH driver as module, y if you want to build it into the kernel and n if you don't want it at all. + +config SURFACE_AGGREGATOR_ERROR_INJECTION + bool "Surface System Aggregator Module Error Injection Capabilities" + depends on SURFACE_AGGREGATOR + depends on FUNCTION_ERROR_INJECTION + help + Provides error-injection capabilities for the Surface System + Aggregator Module subsystem and Surface Serial Hub driver. + + Specifically, exports error injection hooks to be used with the + kernel's function error injection capabilities to simulate underlying + transport and communication problems, such as invalid data sent to or + received from the EC, dropped data, and communication timeouts. + Intended for development and debugging. diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c index c4f082e57372..74f0faaa2b27 100644 --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -226,6 +227,286 @@ */ #define SSH_PTL_RX_FIFO_LEN4096 +#ifdef CONFIG_SURFACE_AGGREGATOR_ERROR_INJECTION + +/** + * ssh_ptl_should_drop_ack_packet() - Error injection hook to drop ACK packets. + * + * Useful to test detection and handling of automated re-transmits by the EC. + * Specifically of packets that the EC considers not-ACKed but the driver + * already considers ACKed (due to dropped ACK). In this case, the EC + * re-transmits the packet-to-be-ACKed and the driver should detect it as + * duplicate/already handled. Note that the driver should still send an ACK + * for the re-transmitted packet. + */ +static noinline bool ssh_ptl_should_drop_ack_packet(void) +{ + return false; +} +ALLOW_ERROR_INJECTION(ssh_ptl_should_drop_ack_packet, TRUE); + +/** + * ssh_ptl_should_drop_nak_packet() - Error injection hook to drop NAK packets. + * + * Useful to test/force automated (timeout-based) re-transmit by the EC. + * Specifically, packets that have not reached the driver completely/with valid + * checksums. Only useful in combination with receival of (injected) bad data. + */ +static noinline bool ssh_ptl_should_drop_nak_packet(void) +{ + return false; +} +ALLOW_ERROR_INJECTION(ssh_ptl_should_drop_nak_packet, TRUE); + +/** + * ssh_ptl_should_drop_dsq_packet() - Error injection hook to drop sequenced + * data packet. + * + * Useful to test re-transmit timeout of the driver. If the data packet has not + * been ACKed after a certain time, the driver should re-transmit the packet up + * to limited number of times defined in SSH_PTL_MAX_PACKET_TRIES. + */ +static noinline bool ssh_ptl_should_drop_dsq_packet(void) +{ + return false; +} +ALLOW_ERROR_INJECTION(ssh_ptl_should_drop_dsq_packet, TRUE); + +/** + * ssh_ptl_should_fail_write() - Error injection hook to make + * serdev_device_write() fail. + * + * Hook to simulate errors in serdev_device_write when transmitting packets. + */ +static noinline int ssh_ptl_should_fail_write(void) +{ + return 0; +} +ALLOW_ERROR_INJECTION(ssh_ptl_should_fail_write, ERRNO); + +/** + * ssh_ptl_should_corrupt_tx_data() - Error injection hook to simulate invalid + * data being sent to the EC. + * + * Hook to simulate corru
[PATCH v3 2/9] platform/surface: aggregator: Add control packet allocation caching
Surface Serial Hub communication is, in its core, packet based. Each sequenced packet requires to be acknowledged, via an ACK-type control packet. In case invalid data has been received by the driver, a NAK-type (not-acknowledge/negative acknowledge) control packet is sent, triggering retransmission. Control packets are therefore a core communication primitive and used frequently enough (with every sequenced packet transmission sent by the embedded controller, including events and request responses) that it may warrant caching their allocations to reduce possible memory fragmentation. Signed-off-by: Maximilian Luz Reviewed-by: Hans de Goede --- drivers/platform/surface/aggregator/core.c| 27 ++- .../surface/aggregator/ssh_packet_layer.c | 47 +++ .../surface/aggregator/ssh_packet_layer.h | 3 ++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c index 18e0e9e34e7b..60d312f71436 100644 --- a/drivers/platform/surface/aggregator/core.c +++ b/drivers/platform/surface/aggregator/core.c @@ -780,7 +780,32 @@ static struct serdev_device_driver ssam_serial_hub = { .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, }; -module_serdev_device_driver(ssam_serial_hub); + + +/* -- Module setup. - */ + +static int __init ssam_core_init(void) +{ + int status; + + status = ssh_ctrl_packet_cache_init(); + if (status) + return status; + + status = serdev_device_driver_register(_serial_hub); + if (status) + ssh_ctrl_packet_cache_destroy(); + + return status; +} +module_init(ssam_core_init); + +static void __exit ssam_core_exit(void) +{ + serdev_device_driver_unregister(_serial_hub); + ssh_ctrl_packet_cache_destroy(); +} +module_exit(ssam_core_exit); MODULE_AUTHOR("Maximilian Luz "); MODULE_DESCRIPTION("Subsystem and Surface Serial Hub driver for Surface System Aggregator Module"); diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c index 66e38fdc7963..23c2e31e7d0e 100644 --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c @@ -303,24 +303,53 @@ void ssh_packet_init(struct ssh_packet *packet, unsigned long type, packet->ops = ops; } +static struct kmem_cache *ssh_ctrl_packet_cache; + +/** + * ssh_ctrl_packet_cache_init() - Initialize the control packet cache. + */ +int ssh_ctrl_packet_cache_init(void) +{ + const unsigned int size = sizeof(struct ssh_packet) + SSH_MSG_LEN_CTRL; + const unsigned int align = __alignof__(struct ssh_packet); + struct kmem_cache *cache; + + cache = kmem_cache_create("ssam_ctrl_packet", size, align, 0, NULL); + if (!cache) + return -ENOMEM; + + ssh_ctrl_packet_cache = cache; + return 0; +} + +/** + * ssh_ctrl_packet_cache_destroy() - Deinitialize the control packet cache. + */ +void ssh_ctrl_packet_cache_destroy(void) +{ + kmem_cache_destroy(ssh_ctrl_packet_cache); + ssh_ctrl_packet_cache = NULL; +} + /** - * ssh_ctrl_packet_alloc() - Allocate control packet. + * ssh_ctrl_packet_alloc() - Allocate packet from control packet cache. * @packet: Where the pointer to the newly allocated packet should be stored. * @buffer: The buffer corresponding to this packet. * @flags: Flags used for allocation. * - * Allocates a packet and corresponding transport buffer. Sets the packet's - * buffer reference to the allocated buffer. The packet must be freed via - * ssh_ctrl_packet_free(), which will also free the corresponding buffer. The - * corresponding buffer must not be freed separately. Intended to be used with - * %ssh_ptl_ctrl_packet_ops as packet operations. + * Allocates a packet and corresponding transport buffer from the control + * packet cache. Sets the packet's buffer reference to the allocated buffer. + * The packet must be freed via ssh_ctrl_packet_free(), which will also free + * the corresponding buffer. The corresponding buffer must not be freed + * separately. Intended to be used with %ssh_ptl_ctrl_packet_ops as packet + * operations. * * Return: Returns zero on success, %-ENOMEM if the allocation failed. */ static int ssh_ctrl_packet_alloc(struct ssh_packet **packet, struct ssam_span *buffer, gfp_t flags) { - *packet = kzalloc(sizeof(**packet) + SSH_MSG_LEN_CTRL, flags); + *packet = kmem_cache_alloc(ssh_ctrl_packet_cache, flags); if (!*packet) return -ENOMEM; @@ -331,12 +360,12 @@ static int ssh_ctrl_packet_alloc(struct ssh_packet **packet, } /** - * ssh_ctrl_packet_free() - Free control packet. + * ssh_ctrl_packet_free() - Free packet allocated from control
[PATCH v3 0/9] Add support for Microsoft Surface System Aggregator Module
Hello, Here is version three 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 first version and cover letter detailing what this series is about can be found at https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximil...@gmail.com/ the previous version (v2) at https://lore.kernel.org/platform-driver-x86/20201203212640.663931-1-luzmaximil...@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/v3 Thank you all for the feedback to and reviews of the previous versions, I hope I have addressed all comments. Regards, Max Note: In v3, I have dropped the explicit dependency of the core module and driver on CONFIG_ACPI due to the incoming [PATCH] platform/surface: SURFACE_PLATFORMS should depend on ACPI Thus, this series depends on said patch. This patch can be found at https://www.spinics.net/lists/platform-driver-x86/msg23929.html 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 in v3 (overview): - remove explicit dependency on ACPI as this is going to be covered by CONFIG_SURFACE_PLATFORMS - simplify locking requirements - help enforce locking requirements via lockdep assertions - fix false-positive lockdep warning - warn on event enablement reference counter exhaustion - don't warn about unhandled event if event handling failed - validate flags on request initialization - improve documentation/add comments - replace 'iff' with 'if' in documentation and comments 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 | 87 + .../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 | 68 + 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 | 2579 + .../platform/surface/aggregator/controller.h | 285 ++ drivers/platform/surface/aggregator/core.c| 839 ++ .../platform/surface/aggregator/ssh_msgb.h| 205 ++ .../surface/aggregator/ssh_packet_layer.c | 2057 + .../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| 1264 .../surface/aggregator/ssh_request_layer.h| 143 + drivers/platform/surface/aggregator/trace.h | 632 .../platform/surface/surface_acpi_notify.c| 886
[PATCH v3 4/9] platform/surface: aggregator: Add trace points
Add trace points to the Surface Aggregator subsystem core. These trace points can be used to track packets, requests, and allocations. They are further intended for debugging and testing/validation, specifically in combination with the error injection capabilities introduced in the subsequent commit. Signed-off-by: Maximilian Luz Reviewed-by: Hans de Goede --- Changes in v1 (from RFC): - add copyright line - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later) - pack tracing structs Changes in v2: - add compiletime check on SSAM_PTR_UID_LEN - fix ssam_trace_get_command_field_u8() macro - use dedicated trace event class for timeout reaper - use printk specifier for hex prefix instead of hard-coding it - unify comment style - run checkpatch --strict, fix warnings and style issues --- drivers/platform/surface/aggregator/Makefile | 3 + .../platform/surface/aggregator/controller.c | 5 + drivers/platform/surface/aggregator/core.c| 3 + .../surface/aggregator/ssh_packet_layer.c | 26 +- .../surface/aggregator/ssh_request_layer.c| 18 + drivers/platform/surface/aggregator/trace.h | 601 ++ 6 files changed, 655 insertions(+), 1 deletion(-) create mode 100644 drivers/platform/surface/aggregator/trace.h diff --git a/drivers/platform/surface/aggregator/Makefile b/drivers/platform/surface/aggregator/Makefile index faad18d4a7f2..b8b24c8ec310 100644 --- a/drivers/platform/surface/aggregator/Makefile +++ b/drivers/platform/surface/aggregator/Makefile @@ -1,6 +1,9 @@ # SPDX-License-Identifier: GPL-2.0+ # Copyright (C) 2019-2020 Maximilian Luz +# For include/trace/define_trace.h to include trace.h +CFLAGS_core.o = -I$(src) + obj-$(CONFIG_SURFACE_AGGREGATOR) += surface_aggregator.o surface_aggregator-objs := core.o diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c index 775a4509bece..5bcb59ed579d 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -32,6 +32,8 @@ #include "ssh_msgb.h" #include "ssh_request_layer.h" +#include "trace.h" + /* -- Safe counters. */ @@ -568,6 +570,7 @@ static void __ssam_event_item_free_generic(struct ssam_event_item *item) */ static void ssam_event_item_free(struct ssam_event_item *item) { + trace_ssam_event_item_free(item); item->ops.free(item); } @@ -603,6 +606,8 @@ static struct ssam_event_item *ssam_event_item_alloc(size_t len, gfp_t flags) } item->event.length = len; + + trace_ssam_event_item_alloc(item, len); return item; } diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c index 37593234fb31..b6a9dea53592 100644 --- a/drivers/platform/surface/aggregator/core.c +++ b/drivers/platform/surface/aggregator/core.c @@ -24,6 +24,9 @@ #include #include "controller.h" +#define CREATE_TRACE_POINTS +#include "trace.h" + /* -- Static controller reference. -- */ diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c index 23c2e31e7d0e..c4f082e57372 100644 --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c @@ -26,6 +26,8 @@ #include "ssh_packet_layer.h" #include "ssh_parser.h" +#include "trace.h" + /* * To simplify reasoning about the code below, we define a few concepts. The * system below is similar to a state-machine for packets, however, there are @@ -228,6 +230,8 @@ static void __ssh_ptl_packet_release(struct kref *kref) { struct ssh_packet *p = container_of(kref, struct ssh_packet, refcnt); + trace_ssam_packet_release(p); + ptl_dbg_cond(p->ptl, "ptl: releasing packet %p\n", p); p->ops->release(p); } @@ -356,6 +360,7 @@ static int ssh_ctrl_packet_alloc(struct ssh_packet **packet, buffer->ptr = (u8 *)(*packet + 1); buffer->len = SSH_MSG_LEN_CTRL; + trace_ssam_ctrl_packet_alloc(*packet, buffer->len); return 0; } @@ -365,6 +370,7 @@ static int ssh_ctrl_packet_alloc(struct ssh_packet **packet, */ static void ssh_ctrl_packet_free(struct ssh_packet *p) { + trace_ssam_ctrl_packet_free(p); kmem_cache_free(ssh_ctrl_packet_cache, p); } @@ -398,7 +404,12 @@ static void ssh_packet_next_try(struct ssh_packet *p) lockdep_assert_held(>ptl->queue.lock); - p->priority = __SSH_PACKET_PRIORITY(base, try + 1); + /* +* Ensure that we write the priority in one go via WRITE_ONCE() so we +* can access it via READ_ONCE() for tracing. Note that other access +* is guarded by the queue
Re: [PATCH] platform/surface: SURFACE_PLATFORMS should depend on ACPI
On 12/16/20 3:01 PM, Hans de Goede wrote: Hi, On 12/16/20 2:37 PM, Geert Uytterhoeven wrote: All Microsoft Surface platform-specific device drivers depend on ACPI, but the gatekeeper symbol SURFACE_PLATFORMS does not. Hence when the user is configuring a kernel without ACPI support, he is still asked about Microsoft Surface drivers, even though this question is irrelevant. Fix this by moving the dependency on ACPI from the individual driver symbols to SURFACE_PLATFORMS. Signed-off-by: Geert Uytterhoeven Thanks, patch looks good to me: Reviewed-by: Hans de Goede Maximilian, can I have your ack or reviewed-by for this (assuming you agree with this change) ? Sure, looks good to me. Reviewed-by: Maximilian Luz Regards, Max Regards, Hans --- drivers/platform/surface/Kconfig | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig index 33040b0b3b799c2d..2c941cdac9eedc6f 100644 --- a/drivers/platform/surface/Kconfig +++ b/drivers/platform/surface/Kconfig @@ -5,6 +5,7 @@ menuconfig SURFACE_PLATFORMS bool "Microsoft Surface Platform-Specific Device Drivers" + depends on ACPI default y help Say Y here to get to see options for platform-specific device drivers @@ -29,20 +30,19 @@ config SURFACE3_WMI config SURFACE_3_BUTTON tristate "Power/home/volume buttons driver for Microsoft Surface 3 tablet" - depends on ACPI && KEYBOARD_GPIO && I2C + depends on KEYBOARD_GPIO && I2C help This driver handles the power/home/volume buttons on the Microsoft Surface 3 tablet. config SURFACE_3_POWER_OPREGION tristate "Surface 3 battery platform operation region support" - depends on ACPI && I2C + depends on I2C help This driver provides support for ACPI operation region of the Surface 3 battery platform driver. config SURFACE_GPE tristate "Surface GPE/Lid Support Driver" - depends on ACPI depends on DMI help This driver marks the GPEs related to the ACPI lid device found on @@ -52,7 +52,7 @@ config SURFACE_GPE config SURFACE_PRO3_BUTTON tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3/4 tablet" - depends on ACPI && INPUT + depends on INPUT help This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3/4 tablet.
Re: [PATCH v2 8/9] platform/surface: Add Surface Aggregator user-space interface
On 12/15/20 5:35 PM, Hans de Goede wrote: Hi, On 12/3/20 10:26 PM, Maximilian Luz wrote: Add a misc-device providing user-space access to the Surface Aggregator EC, mainly intended for debugging, testing, and reverse-engineering. This interface gives user-space applications the ability to send requests to the EC and receive the corresponding responses. The device-file is managed by a pseudo platform-device and corresponding driver to avoid dependence on the dedicated bus, allowing it to be loaded in a minimal configuration. Signed-off-by: Maximilian Luz 1 review comment inline: [...] +static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg) +{ + struct ssam_cdev_request __user *r; + struct ssam_cdev_request rqst; + struct ssam_request spec; + struct ssam_response rsp; + const void __user *plddata; + void __user *rspdata; + int status = 0, ret = 0, tmp; + + r = (struct ssam_cdev_request __user *)arg; + ret = copy_struct_from_user(, sizeof(rqst), r, sizeof(*r)); + if (ret) + goto out; + + plddata = u64_to_user_ptr(rqst.payload.data); + rspdata = u64_to_user_ptr(rqst.response.data); + + /* Setup basic request fields. */ + spec.target_category = rqst.target_category; + spec.target_id = rqst.target_id; + spec.command_id = rqst.command_id; + spec.instance_id = rqst.instance_id; + spec.flags = rqst.flags; + spec.length = rqst.payload.length; + spec.payload = NULL; + + rsp.capacity = rqst.response.length; + rsp.length = 0; + rsp.pointer = NULL; + + /* Get request payload from user-space. */ + if (spec.length) { + if (!plddata) { + ret = -EINVAL; + goto out; + } + + spec.payload = kzalloc(spec.length, GFP_KERNEL); + if (!spec.payload) { + status = -ENOMEM; + ret = -EFAULT; + goto out; + } + + if (copy_from_user((void *)spec.payload, plddata, spec.length)) { + ret = -EFAULT; + goto out; + } + } + + /* Allocate response buffer. */ + if (rsp.capacity) { + if (!rspdata) { + ret = -EINVAL; + goto out; + } + + rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL); + if (!rsp.pointer) { + status = -ENOMEM; + ret = -EFAULT; This is weird, -EFAULT should only be used if a SEGFAULT would have been raised if the code was running in userspace rather then in kernelspace, IOW if userspace has provided an invalid pointer (or a too small buffer, causing the pointer to become invalid at some point in the buffer). Oh, right. IMHO you should simply do ret = -ENOMEM here. Yes. that looks better. I will change that as suggested. Otherwise this looks good to me. Thanks, Max
Re: [PATCH v2 -next] platform: surface: fix non-PM_SLEEP build warnings
On 12/15/20 12:33 AM, Randy Dunlap wrote: Fix build warnings when CONFIG_PM_SLEEP is not enabled and these functions are not used: ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] static int surface_gpe_resume(struct device *dev) ^~ ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] static int surface_gpe_suspend(struct device *dev) ^~~ Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") Signed-off-by: Randy Dunlap Cc: Maximilian Luz Cc: Hans de Goede Cc: platform-driver-...@vger.kernel.org --- v2: dropped Maximilian's RVB tag since the patch changed use preferred __maybe_unused instead of ifdeffery: https://lore.kernel.org/patchwork/patch/732981/ drivers/platform/surface/surface_gpe.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- linux-next-20201214.orig/drivers/platform/surface/surface_gpe.c +++ linux-next-20201214/drivers/platform/surface/surface_gpe.c @@ -181,12 +181,12 @@ static int surface_lid_enable_wakeup(str return 0; } -static int surface_gpe_suspend(struct device *dev) +static int __maybe_unused surface_gpe_suspend(struct device *dev) { return surface_lid_enable_wakeup(dev, true); } -static int surface_gpe_resume(struct device *dev) +static int __maybe_unused surface_gpe_resume(struct device *dev) { return surface_lid_enable_wakeup(dev, false); } Code looks good to me. Reviewed-by: Maximilian Luz As already mentioned before, I'd prefer the subject line to be "platform/surface: gpe: ...", or at least "platform/surface: ..." for consistency with other commits. May just be a personal preference though, so nothing that should prevent it from being applied. Thanks, Max
Re: [PATCH -next] platform: surface: fix non-PM_SLEEP build warnings
On 12/11/20 9:41 PM, Randy Dunlap wrote: On 12/11/20 12:23 PM, Maximilian Luz wrote: On 12/11/20 8:03 PM, Randy Dunlap wrote: Fix build warnings when CONFIG_PM_SLEEP is not enabled and these functions are not used: ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] static int surface_gpe_resume(struct device *dev) ^~ ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] static int surface_gpe_suspend(struct device *dev) ^~~ Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") Signed-off-by: Randy Dunlap Cc: Maximilian Luz Cc: Hans de Goede Cc: platform-driver-...@vger.kernel.org --- drivers/platform/surface/surface_gpe.c | 2 ++ 1 file changed, 2 insertions(+) --- linux-next-20201210.orig/drivers/platform/surface/surface_gpe.c +++ linux-next-20201210/drivers/platform/surface/surface_gpe.c @@ -181,6 +181,7 @@ static int surface_lid_enable_wakeup(str return 0; } +#ifdef CONFIG_PM_SLEEP static int surface_gpe_suspend(struct device *dev) { return surface_lid_enable_wakeup(dev, true); @@ -190,6 +191,7 @@ static int surface_gpe_resume(struct dev { return surface_lid_enable_wakeup(dev, false); } +#endif static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume); Right, thanks. I assume this covers all instances of this warning in platform/surface? Otherwise, a "platform: surface: gpe: ..." subject would make more sense. It should cover all of surface/. It was an allmodconfig and then I disabled CONFIG_PM and CONFIG_PM_SLEEP etc. Perfect, thanks! As for prefixes, how many levels do we want to use? (that's mostly rhetorical, although I would read answers :) Looking at platform/x86 and past commit messages, I'd prefer something like platform/surface: : This would be similar to the platform/x86 style. So two or three, depending on how you count "platform/surface". I agree that this probably tends to get a bit long, so I propose we drop the surface_ prefix on the component part to help with that. So, for example, "surface_gpe" will become "gpe". As for the rest: Reviewed-by: Maximilian Luz thanks. Regards, Max
Re: [PATCH -next] platform: surface: fix non-PM_SLEEP build warnings
On 12/11/20 8:03 PM, Randy Dunlap wrote: Fix build warnings when CONFIG_PM_SLEEP is not enabled and these functions are not used: ../drivers/platform/surface/surface_gpe.c:189:12: warning: ‘surface_gpe_resume’ defined but not used [-Wunused-function] static int surface_gpe_resume(struct device *dev) ^~ ../drivers/platform/surface/surface_gpe.c:184:12: warning: ‘surface_gpe_suspend’ defined but not used [-Wunused-function] static int surface_gpe_suspend(struct device *dev) ^~~ Fixes: 274335f1c557 ("platform/surface: Add Driver to set up lid GPEs on MS Surface device") Signed-off-by: Randy Dunlap Cc: Maximilian Luz Cc: Hans de Goede Cc: platform-driver-...@vger.kernel.org --- drivers/platform/surface/surface_gpe.c |2 ++ 1 file changed, 2 insertions(+) --- linux-next-20201210.orig/drivers/platform/surface/surface_gpe.c +++ linux-next-20201210/drivers/platform/surface/surface_gpe.c @@ -181,6 +181,7 @@ static int surface_lid_enable_wakeup(str return 0; } +#ifdef CONFIG_PM_SLEEP static int surface_gpe_suspend(struct device *dev) { return surface_lid_enable_wakeup(dev, true); @@ -190,6 +191,7 @@ static int surface_gpe_resume(struct dev { return surface_lid_enable_wakeup(dev, false); } +#endif static SIMPLE_DEV_PM_OPS(surface_gpe_pm, surface_gpe_suspend, surface_gpe_resume); Right, thanks. I assume this covers all instances of this warning in platform/surface? Otherwise, a "platform: surface: gpe: ..." subject would make more sense. As for the rest: Reviewed-by: Maximilian Luz Regards, Max
Re: [PATCH v2 1/9] platform/surface: Add Surface Aggregator subsystem
On 12/8/20 3:43 PM, Hans de Goede wrote: Hi, On 12/8/20 3:37 PM, Maximilian Luz wrote: + + obj = acpi_evaluate_dsm_typed(handle, _SSH_DSM_GUID, + SSAM_SSH_DSM_REVISION, func, NULL, + ACPI_TYPE_INTEGER); + if (!obj) + return -EIO; + + val = obj->integer.value; + ACPI_FREE(obj); + + if (val > U32_MAX) + return -ERANGE; + + *ret = val; + return 0; +} [...] +/** + * ssam_controller_start() - Start the receiver and transmitter threads of the + * controller. + * @ctrl: The controller. + * + * Note: When this function is called, the controller should be properly + * hooked up to the serdev core via serdev_device_ops. Please refer + * to ssam_controller_init() for more details on controller initialization. + * + * This function must be called from an exclusive context with regards to the + * state, if necessary, by locking the controller via ssam_controller_lock(). Again you are being a bit hand-wavy (I assume you know what I mean by that) wrt the locking requirements. If possible I would prefer clearly spelled out locking requirements in the form of "this and that lock must be held when calling this function". Preferably backed-up by lockdep_assert-s asserting these conditions. The reason for this is that this function specifically is currently only called during initialization, when the controller has not been published yet, i.e. when we have an exclusive reference to the controller. I'll change this to fully enforce locking (with lockdep_assert). And maybe if you are a bit stricter with always holding the lock when calling this, you can also drop the WRITE_ONCE and the comment about it (in all places where you do this). The WRITE_ONCE is only there to ensure that the basic test in ssam_request_sync_submit() can be done. I always try to be explicit about access that can happen without the respective locks being held. Yes I saw the matching READ_ONCE later on (as the comment indicated I would), which made it more obvious to me why the WRITE_ONCE is here,' so maybe I should have gone back and updated this comment. No worries, always good to have another look at these kinds of things. Anyways, keeping the WRITE_ONCE + READ_ONCE for this is fine. Unfortunately it's not feasible to hold the reader lock in ssam_request_sync_submit() due to reentrancy. Specifically, as the lock, if at all (i.e. if this is not a client driver bound to the controller), must be held not only during submission but until the request has been completed. Note that if we would hold the lock during submission, this is just a smoke-test. Ack. Regards, Hans