Message ID | 20221202223327.690880-1-luzmaximilian@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | platform/surface: aggregator: Improve target/source handling in SSH messages | expand |
Hi Maximilian, On 12/2/22 23:33, Maximilian Luz wrote: > We have some new insights into the Serial Hub protocol, obtained through > reverse engineering. In particular, regarding the command structure. The > input/output target IDs actually represent source and target IDs of > (what looks like) physical entities (specifically: host, SAM EC, KIP EC, > debug connector, and SurfLink connector). > > This series aims to improve handling of messages with regards to those > new findings and, mainly, improve clarity of the documentation and usage > around those fields. > > See the discussion in > > https://github.com/linux-surface/surface-aggregator-module/issues/64 > > for more details. > > There are a couple of standouts: > > - Patch 1 ensures that we only handle commands actually intended for us. > It's possible that we receive messages not intended for us when we > enable debugging. I've kept it intentionally minimal to simplify > backporting. The rest of the series patch 9 focuses more on clarity > and documentation, which is probably too much to backport. > > - Patch 8 touches on multiple subsystems. The intention is to enforce > proper usage and documentation of target IDs in the SSAM_SDEV() / > SSAM_VDEV() macros. As it directly touches those macros I > unfortunately can't split it up by subsystem. > > - Patch 9 is a loosely connected cleanup for consistency. Thank you for the patches. Unfortunately I don't have time atm to review this. And the next 2 weeks are the merge window, followed by 2 weeks of christmas vacation. So I'm afraid that I likely won't get around to reviewing this until the week of January 9th. > Hans, Jiri, Benjamin, Sebastian: While patch 8 ("platform/surface: > aggregator: Enforce use of target-ID enum in device ID macros") touches > multiple subsystems, it should be possible to take the whole series > through the pdx86 tree. The changes in other subsystems are fairly > limited. I agree that it will be best to take all of this upstream through the pdx86 tree. Sebastian thank you for the ack for patch 8/9. Jiri or Benjamin may we have your ack for merging patch 7/9 + 8/9 through the pdx86 tree ? Regards, Hans > Maximilian Luz (9): > platform/surface: aggregator: Ignore command messages not intended for > us > platform/surface: aggregator: Improve documentation and handling of > message target and source IDs > platform/surface: aggregator: Add target and source IDs to command > trace events > platform/surface: aggregator_hub: Use target-ID enum instead of > hard-coding values > platform/surface: aggregator_tabletsw: Use target-ID enum instead of > hard-coding values > platform/surface: dtx: Use target-ID enum instead of hard-coding > values > HID: surface-hid: Use target-ID enum instead of hard-coding values > platform/surface: aggregator: Enforce use of target-ID enum in device > ID macros > platform/surface: aggregator_registry: Fix target-ID of base-hub > > .../driver-api/surface_aggregator/client.rst | 4 +- > .../driver-api/surface_aggregator/ssh.rst | 36 ++++----- > drivers/hid/surface-hid/surface_hid.c | 2 +- > drivers/hid/surface-hid/surface_kbd.c | 2 +- > .../platform/surface/aggregator/controller.c | 12 +-- > .../platform/surface/aggregator/ssh_msgb.h | 4 +- > .../surface/aggregator/ssh_request_layer.c | 15 ++++ > drivers/platform/surface/aggregator/trace.h | 73 +++++++++++++++++-- > .../platform/surface/surface_aggregator_hub.c | 8 +- > .../surface/surface_aggregator_registry.c | 2 +- > .../surface/surface_aggregator_tabletsw.c | 10 +-- > drivers/platform/surface/surface_dtx.c | 20 ++--- > .../surface/surface_platform_profile.c | 2 +- > drivers/power/supply/surface_battery.c | 4 +- > drivers/power/supply/surface_charger.c | 2 +- > include/linux/surface_aggregator/controller.h | 4 +- > include/linux/surface_aggregator/device.h | 50 ++++++------- > include/linux/surface_aggregator/serial_hub.h | 40 ++++++---- > 18 files changed, 191 insertions(+), 99 deletions(-) >
On 12/8/22 17:03, Hans de Goede wrote: > Hi Maximilian, > > On 12/2/22 23:33, Maximilian Luz wrote: >> We have some new insights into the Serial Hub protocol, obtained through >> reverse engineering. In particular, regarding the command structure. The >> input/output target IDs actually represent source and target IDs of >> (what looks like) physical entities (specifically: host, SAM EC, KIP EC, >> debug connector, and SurfLink connector). >> >> This series aims to improve handling of messages with regards to those >> new findings and, mainly, improve clarity of the documentation and usage >> around those fields. >> >> See the discussion in >> >> https://github.com/linux-surface/surface-aggregator-module/issues/64 >> >> for more details. >> >> There are a couple of standouts: >> >> - Patch 1 ensures that we only handle commands actually intended for us. >> It's possible that we receive messages not intended for us when we >> enable debugging. I've kept it intentionally minimal to simplify >> backporting. The rest of the series patch 9 focuses more on clarity >> and documentation, which is probably too much to backport. >> >> - Patch 8 touches on multiple subsystems. The intention is to enforce >> proper usage and documentation of target IDs in the SSAM_SDEV() / >> SSAM_VDEV() macros. As it directly touches those macros I >> unfortunately can't split it up by subsystem. >> >> - Patch 9 is a loosely connected cleanup for consistency. > > Thank you for the patches. Unfortunately I don't have time atm to > review this. > > And the next 2 weeks are the merge window, followed by 2 weeks > of christmas vacation. > > So I'm afraid that I likely won't get around to reviewing > this until the week of January 9th. Sure, no worries and no rush. Thanks for the heads-up. Just as a note: While patch 1 is a "fix", I don't consider it time-critical in any way. The underlying issue only appears if you explicitly enable debug mode on the SAM EC. So no need to hurry. Happy holidays. Regards, Max
On Thu, Dec 8, 2022 at 5:03 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Maximilian, > > On 12/2/22 23:33, Maximilian Luz wrote: > > We have some new insights into the Serial Hub protocol, obtained through > > reverse engineering. In particular, regarding the command structure. The > > input/output target IDs actually represent source and target IDs of > > (what looks like) physical entities (specifically: host, SAM EC, KIP EC, > > debug connector, and SurfLink connector). > > > > This series aims to improve handling of messages with regards to those > > new findings and, mainly, improve clarity of the documentation and usage > > around those fields. > > > > See the discussion in > > > > https://github.com/linux-surface/surface-aggregator-module/issues/64 > > > > for more details. > > > > There are a couple of standouts: > > > > - Patch 1 ensures that we only handle commands actually intended for us. > > It's possible that we receive messages not intended for us when we > > enable debugging. I've kept it intentionally minimal to simplify > > backporting. The rest of the series patch 9 focuses more on clarity > > and documentation, which is probably too much to backport. > > > > - Patch 8 touches on multiple subsystems. The intention is to enforce > > proper usage and documentation of target IDs in the SSAM_SDEV() / > > SSAM_VDEV() macros. As it directly touches those macros I > > unfortunately can't split it up by subsystem. > > > > - Patch 9 is a loosely connected cleanup for consistency. > > Thank you for the patches. Unfortunately I don't have time atm to > review this. > > And the next 2 weeks are the merge window, followed by 2 weeks > of christmas vacation. > > So I'm afraid that I likely won't get around to reviewing > this until the week of January 9th. > > > Hans, Jiri, Benjamin, Sebastian: While patch 8 ("platform/surface: > > aggregator: Enforce use of target-ID enum in device ID macros") touches > > multiple subsystems, it should be possible to take the whole series > > through the pdx86 tree. The changes in other subsystems are fairly > > limited. > > I agree that it will be best to take all of this upstream through the > pdx86 tree. Sebastian thank you for the ack for patch 8/9. > > Jiri or Benjamin may we have your ack for merging patch 7/9 + 8/9 > through the pdx86 tree ? I can give you an ack for taking those through your tree, but I can not review the patches themselves because I was only CC-ed to those 2, and so was linux-input. Given that SSAM_SSH_TID_KIP is not in my current tree I assume it comes from this series. Anyway, enough ranting :) If you think the patches are OK, they are really small concerning the HID part, so feel free to take them through your tree Hans. Cheers, Benjamin > > Regards, > > Hans > > > > > > Maximilian Luz (9): > > platform/surface: aggregator: Ignore command messages not intended for > > us > > platform/surface: aggregator: Improve documentation and handling of > > message target and source IDs > > platform/surface: aggregator: Add target and source IDs to command > > trace events > > platform/surface: aggregator_hub: Use target-ID enum instead of > > hard-coding values > > platform/surface: aggregator_tabletsw: Use target-ID enum instead of > > hard-coding values > > platform/surface: dtx: Use target-ID enum instead of hard-coding > > values > > HID: surface-hid: Use target-ID enum instead of hard-coding values > > platform/surface: aggregator: Enforce use of target-ID enum in device > > ID macros > > platform/surface: aggregator_registry: Fix target-ID of base-hub > > > > .../driver-api/surface_aggregator/client.rst | 4 +- > > .../driver-api/surface_aggregator/ssh.rst | 36 ++++----- > > drivers/hid/surface-hid/surface_hid.c | 2 +- > > drivers/hid/surface-hid/surface_kbd.c | 2 +- > > .../platform/surface/aggregator/controller.c | 12 +-- > > .../platform/surface/aggregator/ssh_msgb.h | 4 +- > > .../surface/aggregator/ssh_request_layer.c | 15 ++++ > > drivers/platform/surface/aggregator/trace.h | 73 +++++++++++++++++-- > > .../platform/surface/surface_aggregator_hub.c | 8 +- > > .../surface/surface_aggregator_registry.c | 2 +- > > .../surface/surface_aggregator_tabletsw.c | 10 +-- > > drivers/platform/surface/surface_dtx.c | 20 ++--- > > .../surface/surface_platform_profile.c | 2 +- > > drivers/power/supply/surface_battery.c | 4 +- > > drivers/power/supply/surface_charger.c | 2 +- > > include/linux/surface_aggregator/controller.h | 4 +- > > include/linux/surface_aggregator/device.h | 50 ++++++------- > > include/linux/surface_aggregator/serial_hub.h | 40 ++++++---- > > 18 files changed, 191 insertions(+), 99 deletions(-) > > >
Hi, On 12/8/22 17:24, Benjamin Tissoires wrote: > On Thu, Dec 8, 2022 at 5:03 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Maximilian, >> >> On 12/2/22 23:33, Maximilian Luz wrote: >>> We have some new insights into the Serial Hub protocol, obtained through >>> reverse engineering. In particular, regarding the command structure. The >>> input/output target IDs actually represent source and target IDs of >>> (what looks like) physical entities (specifically: host, SAM EC, KIP EC, >>> debug connector, and SurfLink connector). >>> >>> This series aims to improve handling of messages with regards to those >>> new findings and, mainly, improve clarity of the documentation and usage >>> around those fields. >>> >>> See the discussion in >>> >>> https://github.com/linux-surface/surface-aggregator-module/issues/64 >>> >>> for more details. >>> >>> There are a couple of standouts: >>> >>> - Patch 1 ensures that we only handle commands actually intended for us. >>> It's possible that we receive messages not intended for us when we >>> enable debugging. I've kept it intentionally minimal to simplify >>> backporting. The rest of the series patch 9 focuses more on clarity >>> and documentation, which is probably too much to backport. >>> >>> - Patch 8 touches on multiple subsystems. The intention is to enforce >>> proper usage and documentation of target IDs in the SSAM_SDEV() / >>> SSAM_VDEV() macros. As it directly touches those macros I >>> unfortunately can't split it up by subsystem. >>> >>> - Patch 9 is a loosely connected cleanup for consistency. >> >> Thank you for the patches. Unfortunately I don't have time atm to >> review this. >> >> And the next 2 weeks are the merge window, followed by 2 weeks >> of christmas vacation. >> >> So I'm afraid that I likely won't get around to reviewing >> this until the week of January 9th. >> >>> Hans, Jiri, Benjamin, Sebastian: While patch 8 ("platform/surface: >>> aggregator: Enforce use of target-ID enum in device ID macros") touches >>> multiple subsystems, it should be possible to take the whole series >>> through the pdx86 tree. The changes in other subsystems are fairly >>> limited. >> >> I agree that it will be best to take all of this upstream through the >> pdx86 tree. Sebastian thank you for the ack for patch 8/9. >> >> Jiri or Benjamin may we have your ack for merging patch 7/9 + 8/9 >> through the pdx86 tree ? > > I can give you an ack for taking those through your tree, but I can > not review the patches themselves because I was only CC-ed to those 2, > and so was linux-input. Given that SSAM_SSH_TID_KIP is not in my > current tree I assume it comes from this series. > > Anyway, enough ranting :) > > If you think the patches are OK, they are really small concerning the > HID part, so feel free to take them through your tree Hans. Thank you. Regards, Hans >>> Maximilian Luz (9): >>> platform/surface: aggregator: Ignore command messages not intended for >>> us >>> platform/surface: aggregator: Improve documentation and handling of >>> message target and source IDs >>> platform/surface: aggregator: Add target and source IDs to command >>> trace events >>> platform/surface: aggregator_hub: Use target-ID enum instead of >>> hard-coding values >>> platform/surface: aggregator_tabletsw: Use target-ID enum instead of >>> hard-coding values >>> platform/surface: dtx: Use target-ID enum instead of hard-coding >>> values >>> HID: surface-hid: Use target-ID enum instead of hard-coding values >>> platform/surface: aggregator: Enforce use of target-ID enum in device >>> ID macros >>> platform/surface: aggregator_registry: Fix target-ID of base-hub >>> >>> .../driver-api/surface_aggregator/client.rst | 4 +- >>> .../driver-api/surface_aggregator/ssh.rst | 36 ++++----- >>> drivers/hid/surface-hid/surface_hid.c | 2 +- >>> drivers/hid/surface-hid/surface_kbd.c | 2 +- >>> .../platform/surface/aggregator/controller.c | 12 +-- >>> .../platform/surface/aggregator/ssh_msgb.h | 4 +- >>> .../surface/aggregator/ssh_request_layer.c | 15 ++++ >>> drivers/platform/surface/aggregator/trace.h | 73 +++++++++++++++++-- >>> .../platform/surface/surface_aggregator_hub.c | 8 +- >>> .../surface/surface_aggregator_registry.c | 2 +- >>> .../surface/surface_aggregator_tabletsw.c | 10 +-- >>> drivers/platform/surface/surface_dtx.c | 20 ++--- >>> .../surface/surface_platform_profile.c | 2 +- >>> drivers/power/supply/surface_battery.c | 4 +- >>> drivers/power/supply/surface_charger.c | 2 +- >>> include/linux/surface_aggregator/controller.h | 4 +- >>> include/linux/surface_aggregator/device.h | 50 ++++++------- >>> include/linux/surface_aggregator/serial_hub.h | 40 ++++++---- >>> 18 files changed, 191 insertions(+), 99 deletions(-) >>> >> >
On 12/8/22 17:24, Benjamin Tissoires wrote: > On Thu, Dec 8, 2022 at 5:03 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Maximilian, >> >> On 12/2/22 23:33, Maximilian Luz wrote: >>> We have some new insights into the Serial Hub protocol, obtained through >>> reverse engineering. In particular, regarding the command structure. The >>> input/output target IDs actually represent source and target IDs of >>> (what looks like) physical entities (specifically: host, SAM EC, KIP EC, >>> debug connector, and SurfLink connector). >>> >>> This series aims to improve handling of messages with regards to those >>> new findings and, mainly, improve clarity of the documentation and usage >>> around those fields. >>> >>> See the discussion in >>> >>> https://github.com/linux-surface/surface-aggregator-module/issues/64 >>> >>> for more details. >>> >>> There are a couple of standouts: >>> >>> - Patch 1 ensures that we only handle commands actually intended for us. >>> It's possible that we receive messages not intended for us when we >>> enable debugging. I've kept it intentionally minimal to simplify >>> backporting. The rest of the series patch 9 focuses more on clarity >>> and documentation, which is probably too much to backport. >>> >>> - Patch 8 touches on multiple subsystems. The intention is to enforce >>> proper usage and documentation of target IDs in the SSAM_SDEV() / >>> SSAM_VDEV() macros. As it directly touches those macros I >>> unfortunately can't split it up by subsystem. >>> >>> - Patch 9 is a loosely connected cleanup for consistency. >> >> Thank you for the patches. Unfortunately I don't have time atm to >> review this. >> >> And the next 2 weeks are the merge window, followed by 2 weeks >> of christmas vacation. >> >> So I'm afraid that I likely won't get around to reviewing >> this until the week of January 9th. >> >>> Hans, Jiri, Benjamin, Sebastian: While patch 8 ("platform/surface: >>> aggregator: Enforce use of target-ID enum in device ID macros") touches >>> multiple subsystems, it should be possible to take the whole series >>> through the pdx86 tree. The changes in other subsystems are fairly >>> limited. >> >> I agree that it will be best to take all of this upstream through the >> pdx86 tree. Sebastian thank you for the ack for patch 8/9. >> >> Jiri or Benjamin may we have your ack for merging patch 7/9 + 8/9 >> through the pdx86 tree ? > > I can give you an ack for taking those through your tree, but I can > not review the patches themselves because I was only CC-ed to those 2, > and so was linux-input. Given that SSAM_SSH_TID_KIP is not in my > current tree I assume it comes from this series. > > Anyway, enough ranting :) Apologies for that. I should have included you in the CC on at least patch 2 as well, which introduces this symbol. FWIW, here's the patchwork link to this series: https://patchwork.kernel.org/project/platform-driver-x86/list/?series=701392 Regards, Max
On Thu, Dec 8, 2022 at 5:49 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > On 12/8/22 17:24, Benjamin Tissoires wrote: > > On Thu, Dec 8, 2022 at 5:03 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi Maximilian, > >> > >> On 12/2/22 23:33, Maximilian Luz wrote: > >>> We have some new insights into the Serial Hub protocol, obtained through > >>> reverse engineering. In particular, regarding the command structure. The > >>> input/output target IDs actually represent source and target IDs of > >>> (what looks like) physical entities (specifically: host, SAM EC, KIP EC, > >>> debug connector, and SurfLink connector). > >>> > >>> This series aims to improve handling of messages with regards to those > >>> new findings and, mainly, improve clarity of the documentation and usage > >>> around those fields. > >>> > >>> See the discussion in > >>> > >>> https://github.com/linux-surface/surface-aggregator-module/issues/64 > >>> > >>> for more details. > >>> > >>> There are a couple of standouts: > >>> > >>> - Patch 1 ensures that we only handle commands actually intended for us. > >>> It's possible that we receive messages not intended for us when we > >>> enable debugging. I've kept it intentionally minimal to simplify > >>> backporting. The rest of the series patch 9 focuses more on clarity > >>> and documentation, which is probably too much to backport. > >>> > >>> - Patch 8 touches on multiple subsystems. The intention is to enforce > >>> proper usage and documentation of target IDs in the SSAM_SDEV() / > >>> SSAM_VDEV() macros. As it directly touches those macros I > >>> unfortunately can't split it up by subsystem. > >>> > >>> - Patch 9 is a loosely connected cleanup for consistency. > >> > >> Thank you for the patches. Unfortunately I don't have time atm to > >> review this. > >> > >> And the next 2 weeks are the merge window, followed by 2 weeks > >> of christmas vacation. > >> > >> So I'm afraid that I likely won't get around to reviewing > >> this until the week of January 9th. > >> > >>> Hans, Jiri, Benjamin, Sebastian: While patch 8 ("platform/surface: > >>> aggregator: Enforce use of target-ID enum in device ID macros") touches > >>> multiple subsystems, it should be possible to take the whole series > >>> through the pdx86 tree. The changes in other subsystems are fairly > >>> limited. > >> > >> I agree that it will be best to take all of this upstream through the > >> pdx86 tree. Sebastian thank you for the ack for patch 8/9. > >> > >> Jiri or Benjamin may we have your ack for merging patch 7/9 + 8/9 > >> through the pdx86 tree ? > > > > I can give you an ack for taking those through your tree, but I can > > not review the patches themselves because I was only CC-ed to those 2, > > and so was linux-input. Given that SSAM_SSH_TID_KIP is not in my > > current tree I assume it comes from this series. > > > > Anyway, enough ranting :) > > Apologies for that. I should have included you in the CC on at least > patch 2 as well, which introduces this symbol. No need to apologize. There is a tight balance between not annoying people with too many emails and then having those people wanting to have all of the series :) I have enough trust in Hans to know that when he reviewed the series, he did it correctly. > > FWIW, here's the patchwork link to this series: > > https://patchwork.kernel.org/project/platform-driver-x86/list/?series=701392 thanks! Cheers, Benjamin
Hi, On 12/2/22 23:33, Maximilian Luz wrote: > We have some new insights into the Serial Hub protocol, obtained through > reverse engineering. In particular, regarding the command structure. The > input/output target IDs actually represent source and target IDs of > (what looks like) physical entities (specifically: host, SAM EC, KIP EC, > debug connector, and SurfLink connector). > > This series aims to improve handling of messages with regards to those > new findings and, mainly, improve clarity of the documentation and usage > around those fields. > > See the discussion in > > https://github.com/linux-surface/surface-aggregator-module/issues/64 > > for more details. > > There are a couple of standouts: > > - Patch 1 ensures that we only handle commands actually intended for us. > It's possible that we receive messages not intended for us when we > enable debugging. I've kept it intentionally minimal to simplify > backporting. The rest of the series patch 9 focuses more on clarity > and documentation, which is probably too much to backport. > > - Patch 8 touches on multiple subsystems. The intention is to enforce > proper usage and documentation of target IDs in the SSAM_SDEV() / > SSAM_VDEV() macros. As it directly touches those macros I > unfortunately can't split it up by subsystem. > > - Patch 9 is a loosely connected cleanup for consistency. > > Hans, Jiri, Benjamin, Sebastian: While patch 8 ("platform/surface: > aggregator: Enforce use of target-ID enum in device ID macros") touches > multiple subsystems, it should be possible to take the whole series > through the pdx86 tree. The changes in other subsystems are fairly > limited. 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 Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. 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. Regards, Hans > > > Maximilian Luz (9): > platform/surface: aggregator: Ignore command messages not intended for > us > platform/surface: aggregator: Improve documentation and handling of > message target and source IDs > platform/surface: aggregator: Add target and source IDs to command > trace events > platform/surface: aggregator_hub: Use target-ID enum instead of > hard-coding values > platform/surface: aggregator_tabletsw: Use target-ID enum instead of > hard-coding values > platform/surface: dtx: Use target-ID enum instead of hard-coding > values > HID: surface-hid: Use target-ID enum instead of hard-coding values > platform/surface: aggregator: Enforce use of target-ID enum in device > ID macros > platform/surface: aggregator_registry: Fix target-ID of base-hub > > .../driver-api/surface_aggregator/client.rst | 4 +- > .../driver-api/surface_aggregator/ssh.rst | 36 ++++----- > drivers/hid/surface-hid/surface_hid.c | 2 +- > drivers/hid/surface-hid/surface_kbd.c | 2 +- > .../platform/surface/aggregator/controller.c | 12 +-- > .../platform/surface/aggregator/ssh_msgb.h | 4 +- > .../surface/aggregator/ssh_request_layer.c | 15 ++++ > drivers/platform/surface/aggregator/trace.h | 73 +++++++++++++++++-- > .../platform/surface/surface_aggregator_hub.c | 8 +- > .../surface/surface_aggregator_registry.c | 2 +- > .../surface/surface_aggregator_tabletsw.c | 10 +-- > drivers/platform/surface/surface_dtx.c | 20 ++--- > .../surface/surface_platform_profile.c | 2 +- > drivers/power/supply/surface_battery.c | 4 +- > drivers/power/supply/surface_charger.c | 2 +- > include/linux/surface_aggregator/controller.h | 4 +- > include/linux/surface_aggregator/device.h | 50 ++++++------- > include/linux/surface_aggregator/serial_hub.h | 40 ++++++---- > 18 files changed, 191 insertions(+), 99 deletions(-) >