Re: [PATCH v2 00/12] platform/surface: aggregator: Add support for client hot-removal

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/27/22 04:34, Maximilian Luz wrote:
> Summary:
> 
>   Add support for the HID type cover input devices on the Pro 8 and all
>   requirements for that.
> 
> 
> Blurb from v1:
> 
>   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.
> 
> 
> Changes in v2:
> 
>  - Introduce "platform/surface: aggregator: Allow is_ssam_device() to be
>    used when CONFIG_SURFACE_AGGREGATOR_BUS is disabled" to fix an
>    undefined reference  build issue when CONFIG_SURFACE_AGGREGATOR_BUS
>    is disabled.
> 
>  - Make SSAM hub device UIDs consistent.
>     - Introduce "platform/surface: aggregator_registry: Change device ID
>       for base hub" to make association between hub and subsystem target
>       category more obvious.
>     - Change hub device ID for KIP subsystem hub to be consistent with
>       the id of the already existing BAS hub.

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

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.

Jiri, Benjamin, note I've also taken the one small(ish) HID patch
which is a part of this series, despite it lacking an Ack from
either of you. I hope this is ok, if not let me know.

Regards,

Hans




> Maximilian Luz (12):
>   platform/surface: aggregator: Allow is_ssam_device() to be used when
>     CONFIG_SURFACE_AGGREGATOR_BUS is disabled
>   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: Change device ID for base hub
>   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     | 403 +++++++++++++-----
>  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     | 125 +++++-
>  include/linux/surface_aggregator/serial_hub.h |   2 +-
>  9 files changed, 513 insertions(+), 146 deletions(-)
>