This series adds support for the type cover of the Surface Pro 8. On the
Pro 8, the type cover is (unlike on previous generations) handled via
the Surface System Aggregator Module (SSAM). As the type cover is
detachable, care needs to be taken and the respective SSAM (HID) client
devices need to be properly removed when detached and re-initialized
when attached.
Therefore, this series does three things:
1. Improve hot-removal support for SSAM client devices. When
hot-removing clients, subsequent communication may time out.
In the worst case, this can lead to problems when devices are
detached and re-attached quickly, before we can remove their
respective kernel representations. This can then lead to devices
being in an uninitialized state, preventing, for example, touchpad
gestures from working properly as the required HID feature report
has not been sent.
Therefore, handle hot-removal of devices more gracefully by avoiding
communication once it has been detected and ensure that devices are
actually removed.
2. Generify SSAM subsystem hubs and add a KIP hub. On the Surface Pro
8, the KIP subsystem (only that abbreviation is known) is
responsible for managing type-cover devices. This hub acts as the
controller for device removal similar to the BAS (detachable base)
subsystem hub on the Surface Book 3 (therefore we can share most of
the code between them).
3. Add the (HID) type-cover clients of the Surface Pro 8 to the
aggregator registry.
Regards,
Max
Maximilian Luz (10):
platform/surface: aggregator: Allow devices to be marked as
hot-removed
platform/surface: aggregator: Allow notifiers to avoid communication
on unregistering
platform/surface: aggregator_registry: Use client device wrappers for
notifier registration
power/supply: surface_charger: Use client device wrappers for notifier
registration
power/supply: surface_battery: Use client device wrappers for notifier
registration
HID: surface-hid: Add support for hot-removal
platform/surface: aggregator: Add comment for KIP subsystem category
platform/surface: aggregator_registry: Generify subsystem hub
functionality
platform/surface: aggregator_registry: Add KIP device hub
platform/surface: aggregator_registry: Add support for keyboard cover
on Surface Pro 8
.../driver-api/surface_aggregator/client.rst | 6 +-
drivers/hid/surface-hid/surface_hid_core.c | 38 +-
.../platform/surface/aggregator/controller.c | 53 ++-
.../surface/surface_aggregator_registry.c | 401 +++++++++++++-----
drivers/power/supply/surface_battery.c | 4 +-
drivers/power/supply/surface_charger.c | 4 +-
include/linux/surface_aggregator/controller.h | 24 +-
include/linux/surface_aggregator/device.h | 114 ++++-
include/linux/surface_aggregator/serial_hub.h | 2 +-
9 files changed, 501 insertions(+), 145 deletions(-)
--
2.36.1
Use newly introduced client device wrapper functions for notifier
registration and unregistration.
Signed-off-by: Maximilian Luz <[email protected]>
---
drivers/power/supply/surface_battery.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/power/supply/surface_battery.c b/drivers/power/supply/surface_battery.c
index 5ec2e6bb2465..540707882bb0 100644
--- a/drivers/power/supply/surface_battery.c
+++ b/drivers/power/supply/surface_battery.c
@@ -802,7 +802,7 @@ static int spwr_battery_register(struct spwr_battery_device *bat)
if (IS_ERR(bat->psy))
return PTR_ERR(bat->psy);
- return ssam_notifier_register(bat->sdev->ctrl, &bat->notif);
+ return ssam_device_notifier_register(bat->sdev, &bat->notif);
}
@@ -837,7 +837,7 @@ static void surface_battery_remove(struct ssam_device *sdev)
{
struct spwr_battery_device *bat = ssam_device_get_drvdata(sdev);
- ssam_notifier_unregister(sdev->ctrl, &bat->notif);
+ ssam_device_notifier_unregister(sdev, &bat->notif);
cancel_delayed_work_sync(&bat->update_work);
}
--
2.36.1
The KIP subsystem (full name unknown, abbreviation has been obtained
through reverse engineering) handles detachable peripherals such as the
keyboard cover on the Surface Pro X and Surface Pro 8.
It is currently not entirely clear what this subsystem entails, but at
the very least it provides event notifications for when the keyboard
cover on the Surface Pro X and Surface Pro 8 have been detached or
re-attached, as well as the state that the keyboard cover is currently
in (e.g. folded-back, folded laptop-like, closed, etc.).
Signed-off-by: Maximilian Luz <[email protected]>
---
include/linux/surface_aggregator/serial_hub.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/surface_aggregator/serial_hub.h b/include/linux/surface_aggregator/serial_hub.h
index c3de43edcffa..26b95ec12733 100644
--- a/include/linux/surface_aggregator/serial_hub.h
+++ b/include/linux/surface_aggregator/serial_hub.h
@@ -306,7 +306,7 @@ enum ssam_ssh_tc {
SSAM_SSH_TC_LPC = 0x0b,
SSAM_SSH_TC_TCL = 0x0c,
SSAM_SSH_TC_SFL = 0x0d,
- SSAM_SSH_TC_KIP = 0x0e,
+ SSAM_SSH_TC_KIP = 0x0e, /* Manages detachable peripherals (Pro X/8 keyboard cover) */
SSAM_SSH_TC_EXT = 0x0f,
SSAM_SSH_TC_BLD = 0x10,
SSAM_SSH_TC_BAS = 0x11, /* Detachment system (Surface Book 2/3). */
--
2.36.1
When SSAM client devices have been (physically) hot-removed,
communication attempts with those devices may fail and time out. This
can even extend to event notifiers, due to which timeouts may occur
during device removal, slowing down that process.
Add a parameter to the notifier unregister function that allows skipping
communication with the EC to prevent this. Furthermore, add wrappers for
registering and unregistering notifiers belonging to SSAM client devices
that automatically check if the device has been marked as hot-removed
and communication should be avoided.
Note that non-SSAM client devices can generally not be hot-removed, so
also add a convenience wrapper for those, defaulting to allow
communication.
Signed-off-by: Maximilian Luz <[email protected]>
---
.../driver-api/surface_aggregator/client.rst | 6 +-
.../platform/surface/aggregator/controller.c | 53 ++++++++++-----
include/linux/surface_aggregator/controller.h | 24 ++++++-
include/linux/surface_aggregator/device.h | 66 +++++++++++++++++++
4 files changed, 128 insertions(+), 21 deletions(-)
diff --git a/Documentation/driver-api/surface_aggregator/client.rst b/Documentation/driver-api/surface_aggregator/client.rst
index e519d374c378..27f95abdbe99 100644
--- a/Documentation/driver-api/surface_aggregator/client.rst
+++ b/Documentation/driver-api/surface_aggregator/client.rst
@@ -17,6 +17,8 @@
.. |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_device_notifier_register| replace:: :c:func:`ssam_device_notifier_register`
+.. |ssam_device_notifier_unregister| replace:: :c:func:`ssam_device_notifier_unregister`
.. |ssam_request_sync| replace:: :c:func:`ssam_request_sync`
.. |ssam_event_mask| replace:: :c:type:`enum ssam_event_mask <ssam_event_mask>`
@@ -312,7 +314,9 @@ Handling Events
To receive events from the SAM EC, an event notifier must be registered for
the desired event via |ssam_notifier_register|. The notifier must be
unregistered via |ssam_notifier_unregister| once it is not required any
-more.
+more. For |ssam_device| type clients, the |ssam_device_notifier_register| and
+|ssam_device_notifier_unregister| wrappers should be preferred as they properly
+handle hot-removal of client devices.
Event notifiers are registered by providing (at minimum) a callback to call
in case an event has been received, the registry specifying how the event
diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
index b8c377b3f932..6de834b52b63 100644
--- a/drivers/platform/surface/aggregator/controller.c
+++ b/drivers/platform/surface/aggregator/controller.c
@@ -2199,16 +2199,26 @@ static int ssam_nf_refcount_enable(struct ssam_controller *ctrl,
}
/**
- * ssam_nf_refcount_disable_free() - Disable event for reference count entry if it is
- * no longer in use and free the corresponding entry.
+ * ssam_nf_refcount_disable_free() - Disable event for reference count entry if
+ * it is no longer in use and free the corresponding entry.
* @ctrl: The controller to disable the event on.
* @entry: The reference count entry for the event to be disabled.
* @flags: The flags used for enabling the event on the EC.
+ * @ec: Flag specifying if the event should actually be disabled on the EC.
*
- * If the reference count equals zero, i.e. the event is no longer requested by
- * any client, the event will be disabled and the corresponding reference count
- * entry freed. The reference count entry must not be used any more after a
- * call to this function.
+ * If ``ec`` equals ``true`` and the reference count equals zero (i.e. the
+ * event is no longer requested by any client), the specified event will be
+ * disabled on the EC via the corresponding request.
+ *
+ * If ``ec`` equals ``false``, no request will be sent to the EC and the event
+ * can be considered in a detached state (i.e. no longer used but still
+ * enabled). Disabling an event via this method may be required for
+ * hot-removable devices, where event disable requests may time out after the
+ * device has been physically removed.
+ *
+ * In both cases, if the reference count equals zero, the corresponding
+ * reference count entry will be freed. The reference count entry must not be
+ * used any more after a call to this function.
*
* Also checks if the flags used for disabling the event match the flags used
* for enabling the event and warns if they do not (regardless of reference
@@ -2223,7 +2233,7 @@ static int ssam_nf_refcount_enable(struct ssam_controller *ctrl,
* returns the status of the event-enable EC command.
*/
static int ssam_nf_refcount_disable_free(struct ssam_controller *ctrl,
- struct ssam_nf_refcount_entry *entry, u8 flags)
+ struct ssam_nf_refcount_entry *entry, u8 flags, bool ec)
{
const struct ssam_event_registry reg = entry->key.reg;
const struct ssam_event_id id = entry->key.id;
@@ -2232,8 +2242,9 @@ static int ssam_nf_refcount_disable_free(struct ssam_controller *ctrl,
lockdep_assert_held(&nf->lock);
- ssam_dbg(ctrl, "disabling event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n",
- reg.target_category, id.target_category, id.instance, entry->refcount);
+ ssam_dbg(ctrl, "%s event (reg: %#04x, tc: %#04x, iid: %#04x, rc: %d)\n",
+ ec ? "disabling" : "detaching", reg.target_category, id.target_category,
+ id.instance, entry->refcount);
if (entry->flags != flags) {
ssam_warn(ctrl,
@@ -2242,7 +2253,7 @@ static int ssam_nf_refcount_disable_free(struct ssam_controller *ctrl,
id.instance);
}
- if (entry->refcount == 0) {
+ if (ec && entry->refcount == 0) {
status = ssam_ssh_event_disable(ctrl, reg, id, flags);
kfree(entry);
}
@@ -2322,20 +2333,26 @@ int ssam_notifier_register(struct ssam_controller *ctrl, struct ssam_event_notif
EXPORT_SYMBOL_GPL(ssam_notifier_register);
/**
- * ssam_notifier_unregister() - Unregister an event notifier.
- * @ctrl: The controller the notifier has been registered on.
- * @n: The event notifier to unregister.
+ * __ssam_notifier_unregister() - Unregister an event notifier.
+ * @ctrl: The controller the notifier has been registered on.
+ * @n: The event notifier to unregister.
+ * @disable: Whether to disable the corresponding event on the EC.
*
* Unregister an event notifier. Decrement the usage counter of the associated
* SAM event if the notifier is not marked as an observer. If the usage counter
- * reaches zero, the event will be disabled.
+ * reaches zero and ``disable`` equals ``true``, the event will be disabled.
+ *
+ * Useful for hot-removable devices, where communication may fail once the
+ * device has been physically removed. In that case, specifying ``disable`` as
+ * ``false`` avoids communication with the EC.
*
* Return: Returns zero on success, %-ENOENT if the given notifier block has
* not been registered on the controller. If the given notifier block was the
* last one associated with its specific event, returns the status of the
* event-disable EC-command.
*/
-int ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_notifier *n)
+int __ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_notifier *n,
+ bool disable)
{
u16 rqid = ssh_tc_to_rqid(n->event.id.target_category);
struct ssam_nf_refcount_entry *entry;
@@ -2373,7 +2390,7 @@ int ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_not
goto remove;
}
- status = ssam_nf_refcount_disable_free(ctrl, entry, n->event.flags);
+ status = ssam_nf_refcount_disable_free(ctrl, entry, n->event.flags, disable);
}
remove:
@@ -2383,7 +2400,7 @@ int ssam_notifier_unregister(struct ssam_controller *ctrl, struct ssam_event_not
return status;
}
-EXPORT_SYMBOL_GPL(ssam_notifier_unregister);
+EXPORT_SYMBOL_GPL(__ssam_notifier_unregister);
/**
* ssam_controller_event_enable() - Enable the specified event.
@@ -2477,7 +2494,7 @@ int ssam_controller_event_disable(struct ssam_controller *ctrl,
return -ENOENT;
}
- status = ssam_nf_refcount_disable_free(ctrl, entry, flags);
+ status = ssam_nf_refcount_disable_free(ctrl, entry, flags, true);
mutex_unlock(&nf->lock);
return status;
diff --git a/include/linux/surface_aggregator/controller.h b/include/linux/surface_aggregator/controller.h
index 74bfdffaf7b0..50a2b4926c06 100644
--- a/include/linux/surface_aggregator/controller.h
+++ b/include/linux/surface_aggregator/controller.h
@@ -835,8 +835,28 @@ struct ssam_event_notifier {
int ssam_notifier_register(struct ssam_controller *ctrl,
struct ssam_event_notifier *n);
-int ssam_notifier_unregister(struct ssam_controller *ctrl,
- struct ssam_event_notifier *n);
+int __ssam_notifier_unregister(struct ssam_controller *ctrl,
+ struct ssam_event_notifier *n, bool disable);
+
+/**
+ * ssam_notifier_unregister() - Unregister an event notifier.
+ * @ctrl: The controller the notifier has been registered on.
+ * @n: The event notifier to unregister.
+ *
+ * Unregister an event notifier. Decrement the usage counter of the associated
+ * SAM event if the notifier is not marked as an observer. If the usage counter
+ * reaches zero, the event will be disabled.
+ *
+ * Return: Returns zero on success, %-ENOENT if the given notifier block has
+ * not been registered on the controller. If the given notifier block was the
+ * last one associated with its specific event, returns the status of the
+ * event-disable EC-command.
+ */
+static inline int ssam_notifier_unregister(struct ssam_controller *ctrl,
+ struct ssam_event_notifier *n)
+{
+ return __ssam_notifier_unregister(ctrl, n, true);
+}
int ssam_controller_event_enable(struct ssam_controller *ctrl,
struct ssam_event_registry reg,
diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
index 491aa7e9f4bc..ad245c6b00d0 100644
--- a/include/linux/surface_aggregator/device.h
+++ b/include/linux/surface_aggregator/device.h
@@ -472,4 +472,70 @@ static inline void ssam_remove_clients(struct device *dev) {}
sdev->uid.instance, ret); \
}
+
+/* -- Helpers for client-device notifiers. ---------------------------------- */
+
+/**
+ * ssam_device_notifier_register() - Register an event notifier for the
+ * specified client device.
+ * @sdev: The device the notifier should be registered on.
+ * @n: The event notifier to register.
+ *
+ * Register an event notifier. Increment the usage counter of the associated
+ * SAM event if the notifier is not marked as an observer. If the event is not
+ * marked as an observer and is currently not enabled, it will be enabled
+ * during this call. If the notifier is marked as an observer, no attempt will
+ * be made at enabling any event and no reference count will be modified.
+ *
+ * Notifiers marked as observers do not need to be associated with one specific
+ * event, i.e. as long as no event matching is performed, only the event target
+ * category needs to be set.
+ *
+ * Return: Returns zero on success, %-ENOSPC if there have already been
+ * %INT_MAX notifiers for the event ID/type associated with the notifier block
+ * registered, %-ENOMEM if the corresponding event entry could not be
+ * allocated, %-ENODEV if the device is marked as hot-removed. If this is the
+ * first time that a notifier block is registered for the specific associated
+ * event, returns the status of the event-enable EC-command.
+ */
+static inline int ssam_device_notifier_register(struct ssam_device *sdev,
+ struct ssam_event_notifier *n)
+{
+ /*
+ * Note that this check does not provide any guarantees whatsoever as
+ * hot-removal could happen at any point and we can't protect against
+ * it. Nevertheless, if we can detect hot-removal, bail early to avoid
+ * communication timeouts.
+ */
+ if (ssam_device_is_hot_removed(sdev))
+ return -ENODEV;
+
+ return ssam_notifier_register(sdev->ctrl, n);
+}
+
+/**
+ * ssam_device_notifier_unregister() - Unregister an event notifier for the
+ * specified client device.
+ * @sdev: The device the notifier has been registered on.
+ * @n: The event notifier to unregister.
+ *
+ * Unregister an event notifier. Decrement the usage counter of the associated
+ * SAM event if the notifier is not marked as an observer. If the usage counter
+ * reaches zero, the event will be disabled.
+ *
+ * In case the device has been marked as hot-removed, the event will not be
+ * disabled on the EC, as in those cases any attempt at doing so may time out.
+ *
+ * Return: Returns zero on success, %-ENOENT if the given notifier block has
+ * not been registered on the controller. If the given notifier block was the
+ * last one associated with its specific event, returns the status of the
+ * event-disable EC-command.
+ */
+static inline int ssam_device_notifier_unregister(struct ssam_device *sdev,
+ struct ssam_event_notifier *n)
+{
+ return __ssam_notifier_unregister(sdev->ctrl, n,
+ !ssam_device_is_hot_removed(sdev));
+}
+
#endif /* _LINUX_SURFACE_AGGREGATOR_DEVICE_H */
--
2.36.1
Use newly introduced client device wrapper functions for notifier
registration and unregistration.
Signed-off-by: Maximilian Luz <[email protected]>
---
drivers/power/supply/surface_charger.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/power/supply/surface_charger.c b/drivers/power/supply/surface_charger.c
index a060c36c7766..59182d55742d 100644
--- a/drivers/power/supply/surface_charger.c
+++ b/drivers/power/supply/surface_charger.c
@@ -216,7 +216,7 @@ static int spwr_ac_register(struct spwr_ac_device *ac)
if (IS_ERR(ac->psy))
return PTR_ERR(ac->psy);
- return ssam_notifier_register(ac->sdev->ctrl, &ac->notif);
+ return ssam_device_notifier_register(ac->sdev, &ac->notif);
}
@@ -251,7 +251,7 @@ static void surface_ac_remove(struct ssam_device *sdev)
{
struct spwr_ac_device *ac = ssam_device_get_drvdata(sdev);
- ssam_notifier_unregister(sdev->ctrl, &ac->notif);
+ ssam_device_notifier_unregister(sdev, &ac->notif);
}
static const struct spwr_psy_properties spwr_psy_props_adp1 = {
--
2.36.1
The Surface System Aggregator Module (SSAM) has multiple subsystems that
can manage detachable devices. At the moment, we only support the "base"
(BAS/0x11) subsystem, which is used on the Surface Book 3 to manage
devices (including keyboard, touchpad, and secondary battery) connected
to the base of the device.
The Surface Pro 8 has a new type-cover with keyboard and touchpad, which
is managed via the KIP/0x0e subsystem. The general procedure is the
same, but with slightly different events and setup. To make
implementation of the KIP hub easier and prevent duplication, generify
the parts of the base hub that we can use for the KIP hub (or any
potential future subsystem hubs).
This also switches over to use the newly introduced "hot-remove"
functionality, which should prevent communication issues when devices
have been detached.
Lastly, also drop the undocumented and unused sysfs "state" attribute of
the base hub. It has at best been useful for debugging.
Signed-off-by: Maximilian Luz <[email protected]>
---
.../surface/surface_aggregator_registry.c | 269 ++++++++++--------
1 file changed, 153 insertions(+), 116 deletions(-)
diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 9f630e890ff7..09cbeee2428b 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -308,30 +308,159 @@ static int ssam_hub_register_clients(struct device *parent, struct ssam_controll
}
-/* -- SSAM base-hub driver. ------------------------------------------------- */
+/* -- SSAM generic subsystem hub driver framework. -------------------------- */
-/*
- * 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_hub_state {
+ SSAM_HUB_UNINITIALIZED, /* Only set during initialization. */
+ SSAM_HUB_CONNECTED,
+ SSAM_HUB_DISCONNECTED,
+};
-enum ssam_base_hub_state {
- SSAM_BASE_HUB_UNINITIALIZED,
- SSAM_BASE_HUB_CONNECTED,
- SSAM_BASE_HUB_DISCONNECTED,
+enum ssam_hub_flags {
+ SSAM_HUB_HOT_REMOVED,
};
-struct ssam_base_hub {
+struct ssam_hub {
struct ssam_device *sdev;
- enum ssam_base_hub_state state;
+ enum ssam_hub_state state;
+ unsigned long flags;
+
struct delayed_work update_work;
+ unsigned long connect_delay;
struct ssam_event_notifier notif;
+
+ int (*get_state)(struct ssam_hub *hub, enum ssam_hub_state *state);
};
+static void ssam_hub_update_workfn(struct work_struct *work)
+{
+ struct ssam_hub *hub = container_of(work, struct ssam_hub, update_work.work);
+ struct fwnode_handle *node = dev_fwnode(&hub->sdev->dev);
+ enum ssam_hub_state state;
+ int status = 0;
+
+ status = hub->get_state(hub, &state);
+ if (status)
+ return;
+
+ /*
+ * There is a small possibility that hub devices were hot-removed and
+ * re-added before we were able to remove them here. In that case, both
+ * the state returned by get_state() and the state of the hub will
+ * equal SSAM_HUB_CONNECTED and we would bail early below, which would
+ * leave child devices without proper (re-)initialization and the
+ * hot-remove flag set.
+ *
+ * Therefore, we check whether devices have been hot-removed via an
+ * additional flag on the hub and, in this case, override the returned
+ * hub state. In case of a missed disconnect (i.e. get_state returned
+ * "connected"), we further need to re-schedule this work (with the
+ * appropriate delay) as the actual connect work submission might have
+ * been merged with this one.
+ *
+ * This then leads to one of two cases: Either we submit an unnecessary
+ * work item (which will get ignored via either the queue or the state
+ * checks) or, in the unlikely case that the work is actually required,
+ * double the normal connect delay.
+ */
+ if (test_and_clear_bit(SSAM_HUB_HOT_REMOVED, &hub->flags)) {
+ if (state == SSAM_HUB_CONNECTED)
+ schedule_delayed_work(&hub->update_work, hub->connect_delay);
+
+ state = SSAM_HUB_DISCONNECTED;
+ }
+
+ if (hub->state == state)
+ return;
+ hub->state = state;
+
+ if (hub->state == SSAM_HUB_CONNECTED)
+ status = ssam_hub_register_clients(&hub->sdev->dev, hub->sdev->ctrl, node);
+ else
+ ssam_remove_clients(&hub->sdev->dev);
+
+ if (status)
+ dev_err(&hub->sdev->dev, "failed to update hub child devices: %d\n", status);
+}
+
+static int ssam_hub_mark_hot_removed(struct device *dev, void *_data)
+{
+ struct ssam_device *sdev = to_ssam_device(dev);
+
+ if (is_ssam_device(dev))
+ ssam_device_mark_hot_removed(sdev);
+
+ return 0;
+}
+
+static void ssam_hub_update(struct ssam_hub *hub, bool connected)
+{
+ unsigned long delay;
+
+ /* Mark devices as hot-removed before we remove any. */
+ if (!connected) {
+ set_bit(SSAM_HUB_HOT_REMOVED, &hub->flags);
+ device_for_each_child_reverse(&hub->sdev->dev, NULL, ssam_hub_mark_hot_removed);
+ }
+
+ /*
+ * Delay update when the base/keyboard cover is being connected to give
+ * devices/EC some time to set up.
+ */
+ delay = connected ? hub->connect_delay : 0;
+
+ schedule_delayed_work(&hub->update_work, delay);
+}
+
+static int __maybe_unused ssam_hub_resume(struct device *dev)
+{
+ struct ssam_hub *hub = dev_get_drvdata(dev);
+
+ schedule_delayed_work(&hub->update_work, 0);
+ return 0;
+}
+static SIMPLE_DEV_PM_OPS(ssam_hub_pm_ops, NULL, ssam_hub_resume);
+
+static int ssam_hub_setup(struct ssam_device *sdev, struct ssam_hub *hub)
+{
+ int status;
+
+ hub->sdev = sdev;
+ hub->state = SSAM_HUB_UNINITIALIZED;
+
+ INIT_DELAYED_WORK(&hub->update_work, ssam_hub_update_workfn);
+
+ ssam_device_set_drvdata(sdev, hub);
+
+ status = ssam_device_notifier_register(sdev, &hub->notif);
+ if (status)
+ return status;
+
+ schedule_delayed_work(&hub->update_work, 0);
+ return 0;
+}
+
+static void ssam_hub_remove(struct ssam_device *sdev)
+{
+ struct ssam_hub *hub = ssam_device_get_drvdata(sdev);
+
+ ssam_device_notifier_unregister(sdev, &hub->notif);
+ cancel_delayed_work_sync(&hub->update_work);
+ ssam_remove_clients(&sdev->dev);
+}
+
+
+/* -- 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)
+
SSAM_DEFINE_SYNC_REQUEST_R(ssam_bas_query_opmode, u8, {
.target_category = SSAM_SSH_TC_BAS,
.target_id = 0x01,
@@ -342,7 +471,7 @@ SSAM_DEFINE_SYNC_REQUEST_R(ssam_bas_query_opmode, u8, {
#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)
+static int ssam_base_hub_query_state(struct ssam_hub *hub, enum ssam_hub_state *state)
{
u8 opmode;
int status;
@@ -354,62 +483,16 @@ static int ssam_base_hub_query_state(struct ssam_base_hub *hub, enum ssam_base_h
}
if (opmode != SSAM_BAS_OPMODE_TABLET)
- *state = SSAM_BASE_HUB_CONNECTED;
+ *state = SSAM_HUB_CONNECTED;
else
- *state = SSAM_BASE_HUB_DISCONNECTED;
+ *state = SSAM_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 = hub->state == SSAM_BASE_HUB_CONNECTED;
-
- 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[] = {
- &ssam_base_hub_attr_state.attr,
- NULL,
-};
-
-static const struct attribute_group ssam_base_hub_group = {
- .attrs = ssam_base_hub_attrs,
-};
-
-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(&hub->sdev->dev);
- enum ssam_base_hub_state state;
- int status = 0;
-
- status = ssam_base_hub_query_state(hub, &state);
- if (status)
- return;
-
- if (hub->state == state)
- return;
- hub->state = state;
-
- if (hub->state == SSAM_BASE_HUB_CONNECTED)
- status = ssam_hub_register_clients(&hub->sdev->dev, hub->sdev->ctrl, node);
- else
- ssam_remove_clients(&hub->sdev->dev);
-
- if (status)
- dev_err(&hub->sdev->dev, "failed to update base-hub devices: %d\n", status);
-}
-
static u32 ssam_base_hub_notif(struct ssam_event_notifier *nf, const struct ssam_event *event)
{
- struct ssam_base_hub *hub = container_of(nf, struct ssam_base_hub, notif);
- unsigned long delay;
+ struct ssam_hub *hub = container_of(nf, struct ssam_hub, notif);
if (event->command_id != SSAM_EVENT_BAS_CID_CONNECTION)
return 0;
@@ -419,13 +502,7 @@ static u32 ssam_base_hub_notif(struct ssam_event_notifier *nf, const struct ssam
return 0;
}
- /*
- * Delay update when the base is being connected to give devices/EC
- * some time to set up.
- */
- delay = event->data[0] ? SSAM_BASE_UPDATE_CONNECT_DELAY : 0;
-
- schedule_delayed_work(&hub->update_work, delay);
+ ssam_hub_update(hub, event->data[0]);
/*
* Do not return SSAM_NOTIF_HANDLED: The event should be picked up and
@@ -435,27 +512,14 @@ static u32 ssam_base_hub_notif(struct ssam_event_notifier *nf, const struct ssam
return 0;
}
-static int __maybe_unused ssam_base_hub_resume(struct device *dev)
-{
- struct ssam_base_hub *hub = dev_get_drvdata(dev);
-
- schedule_delayed_work(&hub->update_work, 0);
- return 0;
-}
-static SIMPLE_DEV_PM_OPS(ssam_base_hub_pm_ops, NULL, ssam_base_hub_resume);
-
static int ssam_base_hub_probe(struct ssam_device *sdev)
{
- struct ssam_base_hub *hub;
- int status;
+ struct ssam_hub *hub;
hub = devm_kzalloc(&sdev->dev, sizeof(*hub), GFP_KERNEL);
if (!hub)
return -ENOMEM;
- hub->sdev = sdev;
- hub->state = SSAM_BASE_HUB_UNINITIALIZED;
-
hub->notif.base.priority = INT_MAX; /* This notifier should run first. */
hub->notif.base.fn = ssam_base_hub_notif;
hub->notif.event.reg = SSAM_EVENT_REGISTRY_SAM;
@@ -464,37 +528,10 @@ static int ssam_base_hub_probe(struct ssam_device *sdev)
hub->notif.event.mask = SSAM_EVENT_MASK_NONE;
hub->notif.event.flags = SSAM_EVENT_SEQUENCED;
- INIT_DELAYED_WORK(&hub->update_work, ssam_base_hub_update_workfn);
-
- ssam_device_set_drvdata(sdev, hub);
-
- status = ssam_device_notifier_register(sdev, &hub->notif);
- if (status)
- return status;
-
- status = sysfs_create_group(&sdev->dev.kobj, &ssam_base_hub_group);
- if (status)
- goto err;
-
- schedule_delayed_work(&hub->update_work, 0);
- return 0;
+ hub->connect_delay = SSAM_BASE_UPDATE_CONNECT_DELAY;
+ hub->get_state = ssam_base_hub_query_state;
-err:
- ssam_device_notifier_unregister(sdev, &hub->notif);
- cancel_delayed_work_sync(&hub->update_work);
- ssam_remove_clients(&sdev->dev);
- return status;
-}
-
-static void ssam_base_hub_remove(struct ssam_device *sdev)
-{
- struct ssam_base_hub *hub = ssam_device_get_drvdata(sdev);
-
- sysfs_remove_group(&sdev->dev.kobj, &ssam_base_hub_group);
-
- ssam_device_notifier_unregister(sdev, &hub->notif);
- cancel_delayed_work_sync(&hub->update_work);
- ssam_remove_clients(&sdev->dev);
+ return ssam_hub_setup(sdev, hub);
}
static const struct ssam_device_id ssam_base_hub_match[] = {
@@ -504,12 +541,12 @@ static const struct ssam_device_id ssam_base_hub_match[] = {
static struct ssam_device_driver ssam_base_hub_driver = {
.probe = ssam_base_hub_probe,
- .remove = ssam_base_hub_remove,
+ .remove = ssam_hub_remove,
.match_table = ssam_base_hub_match,
.driver = {
.name = "surface_aggregator_base_hub",
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
- .pm = &ssam_base_hub_pm_ops,
+ .pm = &ssam_hub_pm_ops,
},
};
--
2.36.1
Use newly introduced client device wrapper functions for notifier
registration and unregistration.
Signed-off-by: Maximilian Luz <[email protected]>
---
drivers/platform/surface/surface_aggregator_registry.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index ce2bd88feeaa..9f630e890ff7 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -468,7 +468,7 @@ static int ssam_base_hub_probe(struct ssam_device *sdev)
ssam_device_set_drvdata(sdev, hub);
- status = ssam_notifier_register(sdev->ctrl, &hub->notif);
+ status = ssam_device_notifier_register(sdev, &hub->notif);
if (status)
return status;
@@ -480,7 +480,7 @@ static int ssam_base_hub_probe(struct ssam_device *sdev)
return 0;
err:
- ssam_notifier_unregister(sdev->ctrl, &hub->notif);
+ ssam_device_notifier_unregister(sdev, &hub->notif);
cancel_delayed_work_sync(&hub->update_work);
ssam_remove_clients(&sdev->dev);
return status;
@@ -492,7 +492,7 @@ static void ssam_base_hub_remove(struct ssam_device *sdev)
sysfs_remove_group(&sdev->dev.kobj, &ssam_base_hub_group);
- ssam_notifier_unregister(sdev->ctrl, &hub->notif);
+ ssam_device_notifier_unregister(sdev, &hub->notif);
cancel_delayed_work_sync(&hub->update_work);
ssam_remove_clients(&sdev->dev);
}
--
2.36.1
Add support for hot-removal of SSAM HID client devices.
Once a device has been hot-removed, further communication with it should
be avoided as it may fail and time out. While the device will be removed
as soon as we detect hot-removal, communication may still occur during
teardown, especially when unregistering notifiers.
While hot-removal is a surprise event that can happen at any time, try
to avoid communication as much as possible once it has been detected to
prevent timeouts that can slow down device removal and cause issues,
e.g. when quickly re-attaching the device.
Signed-off-by: Maximilian Luz <[email protected]>
---
drivers/hid/surface-hid/surface_hid_core.c | 38 +++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/surface-hid/surface_hid_core.c b/drivers/hid/surface-hid/surface_hid_core.c
index e46330b2e561..87637f813de2 100644
--- a/drivers/hid/surface-hid/surface_hid_core.c
+++ b/drivers/hid/surface-hid/surface_hid_core.c
@@ -19,12 +19,30 @@
#include "surface_hid_core.h"
+/* -- Utility functions. ---------------------------------------------------- */
+
+static bool surface_hid_is_hot_removed(struct surface_hid_device *shid)
+{
+ /*
+ * Non-ssam client devices, i.e. platform client devices, cannot be
+ * hot-removed.
+ */
+ if (!is_ssam_device(shid->dev))
+ return false;
+
+ return ssam_device_is_hot_removed(to_ssam_device(shid->dev));
+}
+
+
/* -- Device descriptor access. --------------------------------------------- */
static int surface_hid_load_hid_descriptor(struct surface_hid_device *shid)
{
int status;
+ if (surface_hid_is_hot_removed(shid))
+ return -ENODEV;
+
status = shid->ops.get_descriptor(shid, SURFACE_HID_DESC_HID,
(u8 *)&shid->hid_desc, sizeof(shid->hid_desc));
if (status)
@@ -61,6 +79,9 @@ static int surface_hid_load_device_attributes(struct surface_hid_device *shid)
{
int status;
+ if (surface_hid_is_hot_removed(shid))
+ return -ENODEV;
+
status = shid->ops.get_descriptor(shid, SURFACE_HID_DESC_ATTRS,
(u8 *)&shid->attrs, sizeof(shid->attrs));
if (status)
@@ -88,9 +109,18 @@ static int surface_hid_start(struct hid_device *hid)
static void surface_hid_stop(struct hid_device *hid)
{
struct surface_hid_device *shid = hid->driver_data;
+ bool hot_removed;
+
+ /*
+ * Communication may fail for devices that have been hot-removed. This
+ * also includes unregistration of HID events, so we need to check this
+ * here. Only if the device has not been marked as hot-removed, we can
+ * safely disable events.
+ */
+ hot_removed = surface_hid_is_hot_removed(shid);
/* Note: This call will log errors for us, so ignore them here. */
- ssam_notifier_unregister(shid->ctrl, &shid->notif);
+ __ssam_notifier_unregister(shid->ctrl, &shid->notif, !hot_removed);
}
static int surface_hid_open(struct hid_device *hid)
@@ -109,6 +139,9 @@ static int surface_hid_parse(struct hid_device *hid)
u8 *buf;
int status;
+ if (surface_hid_is_hot_removed(shid))
+ return -ENODEV;
+
buf = kzalloc(len, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -126,6 +159,9 @@ static int surface_hid_raw_request(struct hid_device *hid, unsigned char reportn
{
struct surface_hid_device *shid = hid->driver_data;
+ if (surface_hid_is_hot_removed(shid))
+ return -ENODEV;
+
if (rtype == HID_OUTPUT_REPORT && reqtype == HID_REQ_SET_REPORT)
return shid->ops.output_report(shid, reportnum, buf, len);
--
2.36.1
Add a Surface System Aggregator Module (SSAM) client device hub for
hot-removable devices managed via the KIP subsystem.
The KIP subsystem (full name unknown, abbreviation has been obtained
through reverse engineering) is a subsystem that manages hot-removable
SSAM client devices. Specifically, it manages HID input devices
contained in the detachable keyboard cover of the Surface Pro 8 and
Surface Pro X.
The KIP subsystem handles a single group of devices (e.g. all devices
contained in the keyboard cover) and cannot handle devices individually.
Thus we model it as a client device hub, which (hot-)removes all devices
contained under it once removal of the hub (e.g. keyboard cover) has
been detected and (re-)adds all devices once the physical hub device has
been (re-)attached. To do this, use the previously generified SSAM
subsystem hub framework.
Signed-off-by: Maximilian Luz <[email protected]>
---
.../surface/surface_aggregator_registry.c | 103 +++++++++++++++++-
1 file changed, 101 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 09cbeee2428b..1e60435c7cce 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -551,6 +551,93 @@ static struct ssam_device_driver ssam_base_hub_driver = {
};
+/* -- SSAM KIP-subsystem hub driver. ---------------------------------------- */
+
+/*
+ * Some devices may need a bit of time to be fully usable after being
+ * (re-)connected. This delay has been determined via experimentation.
+ */
+#define SSAM_KIP_UPDATE_CONNECT_DELAY msecs_to_jiffies(250)
+
+#define SSAM_EVENT_KIP_CID_CONNECTION 0x2c
+
+SSAM_DEFINE_SYNC_REQUEST_R(__ssam_kip_get_connection_state, u8, {
+ .target_category = SSAM_SSH_TC_KIP,
+ .target_id = 0x01,
+ .command_id = 0x2c,
+ .instance_id = 0x00,
+});
+
+static int ssam_kip_get_connection_state(struct ssam_hub *hub, enum ssam_hub_state *state)
+{
+ int status;
+ u8 connected;
+
+ status = ssam_retry(__ssam_kip_get_connection_state, hub->sdev->ctrl, &connected);
+ if (status < 0) {
+ dev_err(&hub->sdev->dev, "failed to query KIP connection state: %d\n", status);
+ return status;
+ }
+
+ *state = connected ? SSAM_HUB_CONNECTED : SSAM_HUB_DISCONNECTED;
+ return 0;
+}
+
+static u32 ssam_kip_hub_notif(struct ssam_event_notifier *nf, const struct ssam_event *event)
+{
+ struct ssam_hub *hub = container_of(nf, struct ssam_hub, notif);
+
+ if (event->command_id != SSAM_EVENT_KIP_CID_CONNECTION)
+ return 0; /* Return "unhandled". */
+
+ if (event->length < 1) {
+ dev_err(&hub->sdev->dev, "unexpected payload size: %u\n", event->length);
+ return 0;
+ }
+
+ ssam_hub_update(hub, event->data[0]);
+ return SSAM_NOTIF_HANDLED;
+}
+
+static int ssam_kip_hub_probe(struct ssam_device *sdev)
+{
+ struct ssam_hub *hub;
+
+ hub = devm_kzalloc(&sdev->dev, sizeof(*hub), GFP_KERNEL);
+ if (!hub)
+ return -ENOMEM;
+
+ hub->notif.base.priority = INT_MAX; /* This notifier should run first. */
+ hub->notif.base.fn = ssam_kip_hub_notif;
+ hub->notif.event.reg = SSAM_EVENT_REGISTRY_SAM;
+ hub->notif.event.id.target_category = SSAM_SSH_TC_KIP,
+ hub->notif.event.id.instance = 0,
+ hub->notif.event.mask = SSAM_EVENT_MASK_TARGET;
+ hub->notif.event.flags = SSAM_EVENT_SEQUENCED;
+
+ hub->connect_delay = SSAM_KIP_UPDATE_CONNECT_DELAY;
+ hub->get_state = ssam_kip_get_connection_state;
+
+ return ssam_hub_setup(sdev, hub);
+}
+
+static const struct ssam_device_id ssam_kip_hub_match[] = {
+ { SSAM_SDEV(KIP, 0x01, 0x00, 0x00) },
+ { },
+};
+
+static struct ssam_device_driver ssam_kip_hub_driver = {
+ .probe = ssam_kip_hub_probe,
+ .remove = ssam_hub_remove,
+ .match_table = ssam_kip_hub_match,
+ .driver = {
+ .name = "surface_kip_hub",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ .pm = &ssam_hub_pm_ops,
+ },
+};
+
+
/* -- SSAM platform/meta-hub driver. ---------------------------------------- */
static const struct acpi_device_id ssam_platform_hub_match[] = {
@@ -673,18 +760,30 @@ static int __init ssam_device_hub_init(void)
status = platform_driver_register(&ssam_platform_hub_driver);
if (status)
- return status;
+ goto err_platform;
status = ssam_device_driver_register(&ssam_base_hub_driver);
if (status)
- platform_driver_unregister(&ssam_platform_hub_driver);
+ goto err_base;
+
+ status = ssam_device_driver_register(&ssam_kip_hub_driver);
+ if (status)
+ goto err_kip;
+ return 0;
+
+err_kip:
+ ssam_device_driver_unregister(&ssam_base_hub_driver);
+err_base:
+ platform_driver_unregister(&ssam_platform_hub_driver);
+err_platform:
return status;
}
module_init(ssam_device_hub_init);
static void __exit ssam_device_hub_exit(void)
{
+ ssam_device_driver_unregister(&ssam_kip_hub_driver);
ssam_device_driver_unregister(&ssam_base_hub_driver);
platform_driver_unregister(&ssam_platform_hub_driver);
}
--
2.36.1
Hi Maximilian,
I love your patch! Yet something to improve:
[auto build test ERROR on sre-power-supply/for-next]
[also build test ERROR on hid/for-next linus/master v5.18 next-20220526]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Maximilian-Luz/platform-surface-aggregator-Add-support-for-client-hot-removal/20220521-024312
base: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
config: i386-randconfig-r014-20220516 (https://download.01.org/0day-ci/archive/20220527/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/314a7da4f5af820a0475695017585a83226b05b5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Maximilian-Luz/platform-surface-aggregator-Add-support-for-client-hot-removal/20220521-024312
git checkout 314a7da4f5af820a0475695017585a83226b05b5
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
ld: drivers/hid/surface-hid/surface_hid_core.o: in function `surface_hid_is_hot_removed.isra.0':
>> surface_hid_core.c:(.text+0x1e9): undefined reference to `ssam_device_type'
--
0-DAY CI Kernel Test Service
https://01.org/lkp