[PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Maximilian Luz]

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.


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(-)

-- 
2.38.1

[PATCH 7/9] HID: surface-hid: Use target-ID enum instead of hard-coding values

[Maximilian Luz]

Instead of hard-coding the target ID, use the respective enum
ssam_ssh_tid value.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/hid/surface-hid/surface_kbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/surface-hid/surface_kbd.c b/drivers/hid/surface-hid/surface_kbd.c
index 0635341bc517..42933bf3e925 100644
--- a/drivers/hid/surface-hid/surface_kbd.c
+++ b/drivers/hid/surface-hid/surface_kbd.c
@@ -250,7 +250,7 @@ static int surface_kbd_probe(struct platform_device *pdev)
 
 	shid->uid.domain = SSAM_DOMAIN_SERIALHUB;
 	shid->uid.category = SSAM_SSH_TC_KBD;
-	shid->uid.target = 2;
+	shid->uid.target = SSAM_SSH_TID_KIP;
 	shid->uid.instance = 0;
 	shid->uid.function = 0;
 
-- 
2.38.1

[PATCH 8/9] platform/surface: aggregator: Enforce use of target-ID enum in device ID macros

[Maximilian Luz]

Similar to the target category (TC), the target ID (TID) can be one
value out of a small number of choices, given in enum ssam_ssh_tid.

In the device ID macros, SSAM_SDEV() and SSAM_VDEV() we already use text
expansion to, both, remove some textual clutter for the target category
values and enforce that the value belongs to the known set. Now that we
know the names for the target IDs, use the same trick for them as well.

Also rename the SSAM_ANY_x macros to SSAM_SSH_x_ANY to better fit in.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/hid/surface-hid/surface_hid.c         |  2 +-
 .../platform/surface/surface_aggregator_hub.c |  4 +-
 .../surface/surface_aggregator_tabletsw.c     |  4 +-
 drivers/platform/surface/surface_dtx.c        |  2 +-
 .../surface/surface_platform_profile.c        |  2 +-
 drivers/power/supply/surface_battery.c        |  4 +-
 drivers/power/supply/surface_charger.c        |  2 +-
 include/linux/surface_aggregator/device.h     | 50 +++++++++----------
 8 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/hid/surface-hid/surface_hid.c b/drivers/hid/surface-hid/surface_hid.c
index d4aa8c81903a..aa80d83a83d1 100644
--- a/drivers/hid/surface-hid/surface_hid.c
+++ b/drivers/hid/surface-hid/surface_hid.c
@@ -230,7 +230,7 @@ static void surface_hid_remove(struct ssam_device *sdev)
 }
 
 static const struct ssam_device_id surface_hid_match[] = {
-	{ SSAM_SDEV(HID, SSAM_ANY_TID, SSAM_ANY_IID, 0x00) },
+	{ SSAM_SDEV(HID, ANY, SSAM_SSH_IID_ANY, 0x00) },
 	{ },
 };
 MODULE_DEVICE_TABLE(ssam, surface_hid_match);
diff --git a/drivers/platform/surface/surface_aggregator_hub.c b/drivers/platform/surface/surface_aggregator_hub.c
index 62f27cdb6ca8..6abd1efe2088 100644
--- a/drivers/platform/surface/surface_aggregator_hub.c
+++ b/drivers/platform/surface/surface_aggregator_hub.c
@@ -348,8 +348,8 @@ static const struct ssam_hub_desc kip_hub = {
 /* -- Driver registration. -------------------------------------------------- */
 
 static const struct ssam_device_id ssam_hub_match[] = {
-	{ SSAM_VDEV(HUB, 0x01, SSAM_SSH_TC_KIP, 0x00), (unsigned long)&kip_hub  },
-	{ SSAM_VDEV(HUB, 0x02, SSAM_SSH_TC_BAS, 0x00), (unsigned long)&base_hub },
+	{ SSAM_VDEV(HUB, SAM, SSAM_SSH_TC_KIP, 0x00), (unsigned long)&kip_hub  },
+	{ SSAM_VDEV(HUB, KIP, SSAM_SSH_TC_BAS, 0x00), (unsigned long)&base_hub },
 	{ }
 };
 MODULE_DEVICE_TABLE(ssam, ssam_hub_match);
diff --git a/drivers/platform/surface/surface_aggregator_tabletsw.c b/drivers/platform/surface/surface_aggregator_tabletsw.c
index bd8cd453c393..6147aa887939 100644
--- a/drivers/platform/surface/surface_aggregator_tabletsw.c
+++ b/drivers/platform/surface/surface_aggregator_tabletsw.c
@@ -510,8 +510,8 @@ static const struct ssam_tablet_sw_desc ssam_pos_sw_desc = {
 /* -- Driver registration. -------------------------------------------------- */
 
 static const struct ssam_device_id ssam_tablet_sw_match[] = {
-	{ SSAM_SDEV(KIP, 0x01, 0x00, 0x01), (unsigned long)&ssam_kip_sw_desc },
-	{ SSAM_SDEV(POS, 0x01, 0x00, 0x01), (unsigned long)&ssam_pos_sw_desc },
+	{ SSAM_SDEV(KIP, SAM, 0x00, 0x01), (unsigned long)&ssam_kip_sw_desc },
+	{ SSAM_SDEV(POS, SAM, 0x00, 0x01), (unsigned long)&ssam_pos_sw_desc },
 	{ },
 };
 MODULE_DEVICE_TABLE(ssam, ssam_tablet_sw_match);
diff --git a/drivers/platform/surface/surface_dtx.c b/drivers/platform/surface/surface_dtx.c
index 0de76a784a35..30cbde278c59 100644
--- a/drivers/platform/surface/surface_dtx.c
+++ b/drivers/platform/surface/surface_dtx.c
@@ -1214,7 +1214,7 @@ static void surface_dtx_ssam_remove(struct ssam_device *sdev)
 }
 
 static const struct ssam_device_id surface_dtx_ssam_match[] = {
-	{ SSAM_SDEV(BAS, 0x01, 0x00, 0x00) },
+	{ SSAM_SDEV(BAS, SAM, 0x00, 0x00) },
 	{ },
 };
 MODULE_DEVICE_TABLE(ssam, surface_dtx_ssam_match);
diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index fbf2e11fd6ce..f433a13c3689 100644
--- a/drivers/platform/surface/surface_platform_profile.c
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -169,7 +169,7 @@ static void surface_platform_profile_remove(struct ssam_device *sdev)
 }
 
 static const struct ssam_device_id ssam_platform_profile_match[] = {
-	{ SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
+	{ SSAM_SDEV(TMP, SAM, 0x00, 0x01) },
 	{ },
 };
 MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
diff --git a/drivers/power/supply/surface_battery.c b/drivers/power/supply/surface_battery.c
index 540707882bb0..19d2f8834e56 100644
--- a/drivers/power/supply/surface_battery.c
+++ b/drivers/power/supply/surface_battery.c
@@ -852,8 +852,8 @@ static const struct spwr_psy_properties spwr_psy_props_bat2_sb3 = {
 };
 
 static const struct ssam_device_id surface_battery_match[] = {
-	{ SSAM_SDEV(BAT, 0x01, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat1     },
-	{ SSAM_SDEV(BAT, 0x02, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat2_sb3 },
+	{ SSAM_SDEV(BAT, SAM, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat1     },
+	{ SSAM_SDEV(BAT, KIP, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat2_sb3 },
 	{ },
 };
 MODULE_DEVICE_TABLE(ssam, surface_battery_match);
diff --git a/drivers/power/supply/surface_charger.c b/drivers/power/supply/surface_charger.c
index 59182d55742d..cabdd8da12d0 100644
--- a/drivers/power/supply/surface_charger.c
+++ b/drivers/power/supply/surface_charger.c
@@ -260,7 +260,7 @@ static const struct spwr_psy_properties spwr_psy_props_adp1 = {
 };
 
 static const struct ssam_device_id surface_ac_match[] = {
-	{ SSAM_SDEV(BAT, 0x01, 0x01, 0x01), (unsigned long)&spwr_psy_props_adp1 },
+	{ SSAM_SDEV(BAT, SAM, 0x01, 0x01), (unsigned long)&spwr_psy_props_adp1 },
 	{ },
 };
 MODULE_DEVICE_TABLE(ssam, surface_ac_match);
diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
index 46c45d1b6368..4da20b7a0ee5 100644
--- a/include/linux/surface_aggregator/device.h
+++ b/include/linux/surface_aggregator/device.h
@@ -68,9 +68,9 @@ struct ssam_device_uid {
  * match_flags member of the device ID structure. Do not use them directly
  * with struct ssam_device_id or struct ssam_device_uid.
  */
-#define SSAM_ANY_TID		0xffff
-#define SSAM_ANY_IID		0xffff
-#define SSAM_ANY_FUN		0xffff
+#define SSAM_SSH_TID_ANY	0xffff
+#define SSAM_SSH_IID_ANY	0xffff
+#define SSAM_SSH_FUN_ANY	0xffff
 
 /**
  * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
@@ -83,25 +83,25 @@ struct ssam_device_uid {
  *
  * Initializes a &struct ssam_device_id with the given parameters. See &struct
  * ssam_device_uid for details regarding the parameters. The special values
- * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify that
+ * %SSAM_SSH_TID_ANY, %SSAM_SSH_IID_ANY, and %SSAM_SSH_FUN_ANY can be used to specify that
  * matching should ignore target ID, instance ID, and/or sub-function,
  * respectively. This macro initializes the ``match_flags`` field based on the
  * given parameters.
  *
  * Note: The parameters @d and @cat must be valid &u8 values, the parameters
- * @tid, @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
- * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
+ * @tid, @iid, and @fun must be either valid &u8 values or %SSAM_SSH_TID_ANY,
+ * %SSAM_SSH_IID_ANY, or %SSAM_SSH_FUN_ANY, respectively. Other non-&u8 values are not
  * allowed.
  */
 #define SSAM_DEVICE(d, cat, tid, iid, fun)					\
-	.match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0)	\
-		     | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0)	\
-		     | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),	\
+	.match_flags = (((tid) != SSAM_SSH_TID_ANY) ? SSAM_MATCH_TARGET : 0)	\
+		     | (((iid) != SSAM_SSH_IID_ANY) ? SSAM_MATCH_INSTANCE : 0)	\
+		     | (((fun) != SSAM_SSH_FUN_ANY) ? SSAM_MATCH_FUNCTION : 0),	\
 	.domain   = d,								\
 	.category = cat,							\
-	.target   = __builtin_choose_expr((tid) != SSAM_ANY_TID, (tid), 0),	\
-	.instance = __builtin_choose_expr((iid) != SSAM_ANY_IID, (iid), 0),	\
-	.function = __builtin_choose_expr((fun) != SSAM_ANY_FUN, (fun), 0)
+	.target   = __builtin_choose_expr((tid) != SSAM_SSH_TID_ANY, (tid), 0),	\
+	.instance = __builtin_choose_expr((iid) != SSAM_SSH_IID_ANY, (iid), 0),	\
+	.function = __builtin_choose_expr((fun) != SSAM_SSH_FUN_ANY, (fun), 0)
 
 /**
  * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with
@@ -113,18 +113,18 @@ struct ssam_device_uid {
  *
  * Initializes a &struct ssam_device_id with the given parameters in the
  * virtual domain. See &struct ssam_device_uid for details regarding the
- * parameters. The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and
- * %SSAM_ANY_FUN can be used to specify that matching should ignore target ID,
+ * parameters. The special values %SSAM_SSH_TID_ANY, %SSAM_SSH_IID_ANY, and
+ * %SSAM_SSH_FUN_ANY can be used to specify that matching should ignore target ID,
  * instance ID, and/or sub-function, respectively. This macro initializes the
  * ``match_flags`` field based on the given parameters.
  *
  * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
- * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
- * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
+ * @iid, and @fun must be either valid &u8 values or %SSAM_SSH_TID_ANY,
+ * %SSAM_SSH_IID_ANY, or %SSAM_SSH_FUN_ANY, respectively. Other non-&u8 values are not
  * allowed.
  */
 #define SSAM_VDEV(cat, tid, iid, fun) \
-	SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun)
+	SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, SSAM_SSH_TID_##tid, iid, fun)
 
 /**
  * SSAM_SDEV() - Initialize a &struct ssam_device_id as physical SSH device
@@ -136,18 +136,18 @@ struct ssam_device_uid {
  *
  * Initializes a &struct ssam_device_id with the given parameters in the SSH
  * domain. See &struct ssam_device_uid for details regarding the parameters.
- * The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be
- * used to specify that matching should ignore target ID, instance ID, and/or
- * sub-function, respectively. This macro initializes the ``match_flags``
- * field based on the given parameters.
+ * The special values %SSAM_SSH_TID_ANY, %SSAM_SSH_IID_ANY, and
+ * %SSAM_SSH_FUN_ANY can be used to specify that matching should ignore target
+ * ID, instance ID, and/or sub-function, respectively. This macro initializes
+ * the ``match_flags`` field based on the given parameters.
  *
  * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
- * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
- * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
- * allowed.
+ * @iid, and @fun must be either valid &u8 values or %SSAM_SSH_TID_ANY,
+ * %SSAM_SSH_IID_ANY, or %SSAM_SSH_FUN_ANY, respectively. Other non-&u8 values
+ * are not allowed.
  */
 #define SSAM_SDEV(cat, tid, iid, fun) \
-	SSAM_DEVICE(SSAM_DOMAIN_SERIALHUB, SSAM_SSH_TC_##cat, tid, iid, fun)
+	SSAM_DEVICE(SSAM_DOMAIN_SERIALHUB, SSAM_SSH_TC_##cat, SSAM_SSH_TID_##tid, iid, fun)
 
 /*
  * enum ssam_device_flags - Flags for SSAM client devices.
-- 
2.38.1

Re: [PATCH 8/9] platform/surface: aggregator: Enforce use of target-ID enum in device ID macros

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 12075 bytes --]

Hi,

On Fri, Dec 02, 2022 at 11:33:26PM +0100, Maximilian Luz wrote:
> Similar to the target category (TC), the target ID (TID) can be one
> value out of a small number of choices, given in enum ssam_ssh_tid.
> 
> In the device ID macros, SSAM_SDEV() and SSAM_VDEV() we already use text
> expansion to, both, remove some textual clutter for the target category
> values and enforce that the value belongs to the known set. Now that we
> know the names for the target IDs, use the same trick for them as well.
> 
> Also rename the SSAM_ANY_x macros to SSAM_SSH_x_ANY to better fit in.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/hid/surface-hid/surface_hid.c         |  2 +-
>  .../platform/surface/surface_aggregator_hub.c |  4 +-
>  .../surface/surface_aggregator_tabletsw.c     |  4 +-
>  drivers/platform/surface/surface_dtx.c        |  2 +-
>  .../surface/surface_platform_profile.c        |  2 +-
>  drivers/power/supply/surface_battery.c        |  4 +-
>  drivers/power/supply/surface_charger.c        |  2 +-
>  include/linux/surface_aggregator/device.h     | 50 +++++++++----------
>  8 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hid/surface-hid/surface_hid.c b/drivers/hid/surface-hid/surface_hid.c
> index d4aa8c81903a..aa80d83a83d1 100644
> --- a/drivers/hid/surface-hid/surface_hid.c
> +++ b/drivers/hid/surface-hid/surface_hid.c
> @@ -230,7 +230,7 @@ static void surface_hid_remove(struct ssam_device *sdev)
>  }
>  
>  static const struct ssam_device_id surface_hid_match[] = {
> -	{ SSAM_SDEV(HID, SSAM_ANY_TID, SSAM_ANY_IID, 0x00) },
> +	{ SSAM_SDEV(HID, ANY, SSAM_SSH_IID_ANY, 0x00) },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(ssam, surface_hid_match);
> diff --git a/drivers/platform/surface/surface_aggregator_hub.c b/drivers/platform/surface/surface_aggregator_hub.c
> index 62f27cdb6ca8..6abd1efe2088 100644
> --- a/drivers/platform/surface/surface_aggregator_hub.c
> +++ b/drivers/platform/surface/surface_aggregator_hub.c
> @@ -348,8 +348,8 @@ static const struct ssam_hub_desc kip_hub = {
>  /* -- Driver registration. -------------------------------------------------- */
>  
>  static const struct ssam_device_id ssam_hub_match[] = {
> -	{ SSAM_VDEV(HUB, 0x01, SSAM_SSH_TC_KIP, 0x00), (unsigned long)&kip_hub  },
> -	{ SSAM_VDEV(HUB, 0x02, SSAM_SSH_TC_BAS, 0x00), (unsigned long)&base_hub },
> +	{ SSAM_VDEV(HUB, SAM, SSAM_SSH_TC_KIP, 0x00), (unsigned long)&kip_hub  },
> +	{ SSAM_VDEV(HUB, KIP, SSAM_SSH_TC_BAS, 0x00), (unsigned long)&base_hub },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(ssam, ssam_hub_match);
> diff --git a/drivers/platform/surface/surface_aggregator_tabletsw.c b/drivers/platform/surface/surface_aggregator_tabletsw.c
> index bd8cd453c393..6147aa887939 100644
> --- a/drivers/platform/surface/surface_aggregator_tabletsw.c
> +++ b/drivers/platform/surface/surface_aggregator_tabletsw.c
> @@ -510,8 +510,8 @@ static const struct ssam_tablet_sw_desc ssam_pos_sw_desc = {
>  /* -- Driver registration. -------------------------------------------------- */
>  
>  static const struct ssam_device_id ssam_tablet_sw_match[] = {
> -	{ SSAM_SDEV(KIP, 0x01, 0x00, 0x01), (unsigned long)&ssam_kip_sw_desc },
> -	{ SSAM_SDEV(POS, 0x01, 0x00, 0x01), (unsigned long)&ssam_pos_sw_desc },
> +	{ SSAM_SDEV(KIP, SAM, 0x00, 0x01), (unsigned long)&ssam_kip_sw_desc },
> +	{ SSAM_SDEV(POS, SAM, 0x00, 0x01), (unsigned long)&ssam_pos_sw_desc },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(ssam, ssam_tablet_sw_match);
> diff --git a/drivers/platform/surface/surface_dtx.c b/drivers/platform/surface/surface_dtx.c
> index 0de76a784a35..30cbde278c59 100644
> --- a/drivers/platform/surface/surface_dtx.c
> +++ b/drivers/platform/surface/surface_dtx.c
> @@ -1214,7 +1214,7 @@ static void surface_dtx_ssam_remove(struct ssam_device *sdev)
>  }
>  
>  static const struct ssam_device_id surface_dtx_ssam_match[] = {
> -	{ SSAM_SDEV(BAS, 0x01, 0x00, 0x00) },
> +	{ SSAM_SDEV(BAS, SAM, 0x00, 0x00) },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(ssam, surface_dtx_ssam_match);
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index fbf2e11fd6ce..f433a13c3689 100644
> --- a/drivers/platform/surface/surface_platform_profile.c
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -169,7 +169,7 @@ static void surface_platform_profile_remove(struct ssam_device *sdev)
>  }
>  
>  static const struct ssam_device_id ssam_platform_profile_match[] = {
> -	{ SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
> +	{ SSAM_SDEV(TMP, SAM, 0x00, 0x01) },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
> diff --git a/drivers/power/supply/surface_battery.c b/drivers/power/supply/surface_battery.c
> index 540707882bb0..19d2f8834e56 100644
> --- a/drivers/power/supply/surface_battery.c
> +++ b/drivers/power/supply/surface_battery.c
> @@ -852,8 +852,8 @@ static const struct spwr_psy_properties spwr_psy_props_bat2_sb3 = {
>  };
>  
>  static const struct ssam_device_id surface_battery_match[] = {
> -	{ SSAM_SDEV(BAT, 0x01, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat1     },
> -	{ SSAM_SDEV(BAT, 0x02, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat2_sb3 },
> +	{ SSAM_SDEV(BAT, SAM, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat1     },
> +	{ SSAM_SDEV(BAT, KIP, 0x01, 0x00), (unsigned long)&spwr_psy_props_bat2_sb3 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(ssam, surface_battery_match);
> diff --git a/drivers/power/supply/surface_charger.c b/drivers/power/supply/surface_charger.c
> index 59182d55742d..cabdd8da12d0 100644
> --- a/drivers/power/supply/surface_charger.c
> +++ b/drivers/power/supply/surface_charger.c
> @@ -260,7 +260,7 @@ static const struct spwr_psy_properties spwr_psy_props_adp1 = {
>  };
>  
>  static const struct ssam_device_id surface_ac_match[] = {
> -	{ SSAM_SDEV(BAT, 0x01, 0x01, 0x01), (unsigned long)&spwr_psy_props_adp1 },
> +	{ SSAM_SDEV(BAT, SAM, 0x01, 0x01), (unsigned long)&spwr_psy_props_adp1 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(ssam, surface_ac_match);
> diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h
> index 46c45d1b6368..4da20b7a0ee5 100644
> --- a/include/linux/surface_aggregator/device.h
> +++ b/include/linux/surface_aggregator/device.h
> @@ -68,9 +68,9 @@ struct ssam_device_uid {
>   * match_flags member of the device ID structure. Do not use them directly
>   * with struct ssam_device_id or struct ssam_device_uid.
>   */
> -#define SSAM_ANY_TID		0xffff
> -#define SSAM_ANY_IID		0xffff
> -#define SSAM_ANY_FUN		0xffff
> +#define SSAM_SSH_TID_ANY	0xffff
> +#define SSAM_SSH_IID_ANY	0xffff
> +#define SSAM_SSH_FUN_ANY	0xffff
>  
>  /**
>   * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
> @@ -83,25 +83,25 @@ struct ssam_device_uid {
>   *
>   * Initializes a &struct ssam_device_id with the given parameters. See &struct
>   * ssam_device_uid for details regarding the parameters. The special values
> - * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify that
> + * %SSAM_SSH_TID_ANY, %SSAM_SSH_IID_ANY, and %SSAM_SSH_FUN_ANY can be used to specify that
>   * matching should ignore target ID, instance ID, and/or sub-function,
>   * respectively. This macro initializes the ``match_flags`` field based on the
>   * given parameters.
>   *
>   * Note: The parameters @d and @cat must be valid &u8 values, the parameters
> - * @tid, @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> - * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * @tid, @iid, and @fun must be either valid &u8 values or %SSAM_SSH_TID_ANY,
> + * %SSAM_SSH_IID_ANY, or %SSAM_SSH_FUN_ANY, respectively. Other non-&u8 values are not
>   * allowed.
>   */
>  #define SSAM_DEVICE(d, cat, tid, iid, fun)					\
> -	.match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0)	\
> -		     | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0)	\
> -		     | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0),	\
> +	.match_flags = (((tid) != SSAM_SSH_TID_ANY) ? SSAM_MATCH_TARGET : 0)	\
> +		     | (((iid) != SSAM_SSH_IID_ANY) ? SSAM_MATCH_INSTANCE : 0)	\
> +		     | (((fun) != SSAM_SSH_FUN_ANY) ? SSAM_MATCH_FUNCTION : 0),	\
>  	.domain   = d,								\
>  	.category = cat,							\
> -	.target   = __builtin_choose_expr((tid) != SSAM_ANY_TID, (tid), 0),	\
> -	.instance = __builtin_choose_expr((iid) != SSAM_ANY_IID, (iid), 0),	\
> -	.function = __builtin_choose_expr((fun) != SSAM_ANY_FUN, (fun), 0)
> +	.target   = __builtin_choose_expr((tid) != SSAM_SSH_TID_ANY, (tid), 0),	\
> +	.instance = __builtin_choose_expr((iid) != SSAM_SSH_IID_ANY, (iid), 0),	\
> +	.function = __builtin_choose_expr((fun) != SSAM_SSH_FUN_ANY, (fun), 0)
>  
>  /**
>   * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with
> @@ -113,18 +113,18 @@ struct ssam_device_uid {
>   *
>   * Initializes a &struct ssam_device_id with the given parameters in the
>   * virtual domain. See &struct ssam_device_uid for details regarding the
> - * parameters. The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and
> - * %SSAM_ANY_FUN can be used to specify that matching should ignore target ID,
> + * parameters. The special values %SSAM_SSH_TID_ANY, %SSAM_SSH_IID_ANY, and
> + * %SSAM_SSH_FUN_ANY can be used to specify that matching should ignore target ID,
>   * instance ID, and/or sub-function, respectively. This macro initializes the
>   * ``match_flags`` field based on the given parameters.
>   *
>   * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
> - * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> - * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> + * @iid, and @fun must be either valid &u8 values or %SSAM_SSH_TID_ANY,
> + * %SSAM_SSH_IID_ANY, or %SSAM_SSH_FUN_ANY, respectively. Other non-&u8 values are not
>   * allowed.
>   */
>  #define SSAM_VDEV(cat, tid, iid, fun) \
> -	SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun)
> +	SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, SSAM_SSH_TID_##tid, iid, fun)
>  
>  /**
>   * SSAM_SDEV() - Initialize a &struct ssam_device_id as physical SSH device
> @@ -136,18 +136,18 @@ struct ssam_device_uid {
>   *
>   * Initializes a &struct ssam_device_id with the given parameters in the SSH
>   * domain. See &struct ssam_device_uid for details regarding the parameters.
> - * The special values %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be
> - * used to specify that matching should ignore target ID, instance ID, and/or
> - * sub-function, respectively. This macro initializes the ``match_flags``
> - * field based on the given parameters.
> + * The special values %SSAM_SSH_TID_ANY, %SSAM_SSH_IID_ANY, and
> + * %SSAM_SSH_FUN_ANY can be used to specify that matching should ignore target
> + * ID, instance ID, and/or sub-function, respectively. This macro initializes
> + * the ``match_flags`` field based on the given parameters.
>   *
>   * Note: The parameter @cat must be a valid &u8 value, the parameters @tid,
> - * @iid, and @fun must be either valid &u8 values or %SSAM_ANY_TID,
> - * %SSAM_ANY_IID, or %SSAM_ANY_FUN, respectively. Other non-&u8 values are not
> - * allowed.
> + * @iid, and @fun must be either valid &u8 values or %SSAM_SSH_TID_ANY,
> + * %SSAM_SSH_IID_ANY, or %SSAM_SSH_FUN_ANY, respectively. Other non-&u8 values
> + * are not allowed.
>   */
>  #define SSAM_SDEV(cat, tid, iid, fun) \
> -	SSAM_DEVICE(SSAM_DOMAIN_SERIALHUB, SSAM_SSH_TC_##cat, tid, iid, fun)
> +	SSAM_DEVICE(SSAM_DOMAIN_SERIALHUB, SSAM_SSH_TC_##cat, SSAM_SSH_TID_##tid, iid, fun)
>  
>  /*
>   * enum ssam_device_flags - Flags for SSAM client devices.
> -- 
> 2.38.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Hans de Goede]

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(-)
> 

Re: [PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Maximilian Luz]

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

Re: [PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Benjamin Tissoires]

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(-)
> >
>

Re: [PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Hans de Goede]

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(-)
>>>
>>
> 

Re: [PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Maximilian Luz]

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

Re: [PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Benjamin Tissoires]

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

Re: [PATCH 0/9] platform/surface: aggregator: Improve target/source handling in SSH messages

[Hans de Goede]

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(-)
>