[PATCH 1/2] platform/surface: aggregator: Do not check for repeated unsequenced packets

[PATCH 1/2] platform/surface: aggregator: Do not check for repeated unsequenced packets

[Maximilian Luz]

Currently, we check any received packet whether we have already seen it
previously, regardless of the packet type (sequenced / unsequenced). We
do this by checking the sequence number. This assumes that sequence
numbers are valid for both sequenced and unsequenced packets. However,
this assumption appears to be incorrect.

On some devices, the sequence number field of unsequenced packets (in
particular HID input events on the Surface Pro 9) is always zero. As a
result, the current retransmission check kicks in and discards all but
the first unsequenced packet, breaking (among other things) keyboard and
touchpad input.

Note that we have, so far, only seen packets being retransmitted in
sequenced communication. In particular, this happens when there is an
ACK timeout, causing the EC (or us) to re-send the packet waiting for an
ACK. Arguably, retransmission / duplication of unsequenced packets
should not be an issue as there is no logical condition (such as an ACK
timeout) to determine when a packet should be sent again.

Therefore, remove the retransmission check for unsequenced packets
entirely to resolve the issue.

Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 .../surface/aggregator/ssh_packet_layer.c     | 24 +++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
index 6748fe4ac5d5..def8d7ac541f 100644
--- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
+++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
@@ -1596,16 +1596,32 @@ static void ssh_ptl_timeout_reap(struct work_struct *work)
 		ssh_ptl_tx_wakeup_packet(ptl);
 }
 
-static bool ssh_ptl_rx_retransmit_check(struct ssh_ptl *ptl, u8 seq)
+static bool ssh_ptl_rx_retransmit_check(struct ssh_ptl *ptl, const struct ssh_frame *frame)
 {
 	int i;
 
+	/*
+	 * Ignore unsequenced packets. On some devices (notably Surface Pro 9),
+	 * unsequenced events will always be sent with SEQ=0x00. Attempting to
+	 * detect retransmission would thus just block all events.
+	 *
+	 * While sequence numbers would also allow detection of retransmitted
+	 * packets in unsequenced communication, they have only ever been used
+	 * to cover edge-cases in sequenced transmission. In particular, the
+	 * only instance of packets being retransmitted (that we are aware of)
+	 * is due to an ACK timeout. As this does not happen in unsequenced
+	 * communication, skip the retransmission check for those packets
+	 * entirely.
+	 */
+	if (frame->type == SSH_FRAME_TYPE_DATA_NSQ)
+		return false;
+
 	/*
 	 * Check if SEQ has been seen recently (i.e. packet was
 	 * re-transmitted and we should ignore it).
 	 */
 	for (i = 0; i < ARRAY_SIZE(ptl->rx.blocked.seqs); i++) {
-		if (likely(ptl->rx.blocked.seqs[i] != seq))
+		if (likely(ptl->rx.blocked.seqs[i] != frame->seq))
 			continue;
 
 		ptl_dbg(ptl, "ptl: ignoring repeated data packet\n");
@@ -1613,7 +1629,7 @@ static bool ssh_ptl_rx_retransmit_check(struct ssh_ptl *ptl, u8 seq)
 	}
 
 	/* Update list of blocked sequence IDs. */
-	ptl->rx.blocked.seqs[ptl->rx.blocked.offset] = seq;
+	ptl->rx.blocked.seqs[ptl->rx.blocked.offset] = frame->seq;
 	ptl->rx.blocked.offset = (ptl->rx.blocked.offset + 1)
 				  % ARRAY_SIZE(ptl->rx.blocked.seqs);
 
@@ -1624,7 +1640,7 @@ static void ssh_ptl_rx_dataframe(struct ssh_ptl *ptl,
 				 const struct ssh_frame *frame,
 				 const struct ssam_span *payload)
 {
-	if (ssh_ptl_rx_retransmit_check(ptl, frame->seq))
+	if (ssh_ptl_rx_retransmit_check(ptl, frame))
 		return;
 
 	ptl->ops.data_received(ptl, payload);
-- 
2.38.1

[PATCH 2/2] platform/surface: aggregator_registry: Add support for Surface Pro 9

[Maximilian Luz]

Add device nodes to enable support for battery and charger status, the
ACPI platform profile, as well as internal and type-cover HID devices
(including sensors, touchpad, keyboard, and other miscellaneous devices)
on the Surface Pro 9.

This does not include support for a tablet-mode switch yet, as that is
now handled via the POS subsystem (unlike the Surface Pro 8, where it is
handled via the KIP subsystem) and therefore needs further changes.

While we're at it, also add the missing comment for the Surface Pro 8.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 .../surface/surface_aggregator_registry.c     | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 585911020cea..db82c2a7c567 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -268,6 +268,7 @@ static const struct software_node *ssam_node_group_sp7[] = {
 	NULL,
 };
 
+/* Devices for Surface Pro 8 */
 static const struct software_node *ssam_node_group_sp8[] = {
 	&ssam_node_root,
 	&ssam_node_hub_kip,
@@ -284,6 +285,23 @@ static const struct software_node *ssam_node_group_sp8[] = {
 	NULL,
 };
 
+/* Devices for Surface Pro 9 */
+static const struct software_node *ssam_node_group_sp9[] = {
+	&ssam_node_root,
+	&ssam_node_hub_kip,
+	&ssam_node_bat_ac,
+	&ssam_node_bat_main,
+	&ssam_node_tmp_pprof,
+	/* TODO: Tablet mode switch (via POS subsystem) */
+	&ssam_node_hid_kip_keyboard,
+	&ssam_node_hid_kip_penstash,
+	&ssam_node_hid_kip_touchpad,
+	&ssam_node_hid_kip_fwupd,
+	&ssam_node_hid_sam_sensors,
+	&ssam_node_hid_sam_ucm_ucsi,
+	NULL,
+};
+
 
 /* -- SSAM platform/meta-hub driver. ---------------------------------------- */
 
@@ -303,6 +321,9 @@ static const struct acpi_device_id ssam_platform_hub_match[] = {
 	/* Surface Pro 8 */
 	{ "MSHW0263", (unsigned long)ssam_node_group_sp8 },
 
+	/* Surface Pro 9 */
+	{ "MSHW0343", (unsigned long)ssam_node_group_sp9 },
+
 	/* Surface Book 2 */
 	{ "MSHW0107", (unsigned long)ssam_node_group_gen5 },
 
-- 
2.38.1

Re: [PATCH 1/2] platform/surface: aggregator: Do not check for repeated unsequenced packets

[Hans de Goede]

Hi,

On 11/13/22 19:59, Maximilian Luz wrote:
> Currently, we check any received packet whether we have already seen it
> previously, regardless of the packet type (sequenced / unsequenced). We
> do this by checking the sequence number. This assumes that sequence
> numbers are valid for both sequenced and unsequenced packets. However,
> this assumption appears to be incorrect.
> 
> On some devices, the sequence number field of unsequenced packets (in
> particular HID input events on the Surface Pro 9) is always zero. As a
> result, the current retransmission check kicks in and discards all but
> the first unsequenced packet, breaking (among other things) keyboard and
> touchpad input.
> 
> Note that we have, so far, only seen packets being retransmitted in
> sequenced communication. In particular, this happens when there is an
> ACK timeout, causing the EC (or us) to re-send the packet waiting for an
> ACK. Arguably, retransmission / duplication of unsequenced packets
> should not be an issue as there is no logical condition (such as an ACK
> timeout) to determine when a packet should be sent again.
> 
> Therefore, remove the retransmission check for unsequenced packets
> entirely to resolve the issue.
> 
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

Thank you for your patch-series, I've applied this series to my
fixes branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes

Note it will show up in my fixes branch once I've pushed my
local branch there, which might take a while.

I will include this series in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans

> ---
>  .../surface/aggregator/ssh_packet_layer.c     | 24 +++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index 6748fe4ac5d5..def8d7ac541f 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -1596,16 +1596,32 @@ static void ssh_ptl_timeout_reap(struct work_struct *work)
>  		ssh_ptl_tx_wakeup_packet(ptl);
>  }
>  
> -static bool ssh_ptl_rx_retransmit_check(struct ssh_ptl *ptl, u8 seq)
> +static bool ssh_ptl_rx_retransmit_check(struct ssh_ptl *ptl, const struct ssh_frame *frame)
>  {
>  	int i;
>  
> +	/*
> +	 * Ignore unsequenced packets. On some devices (notably Surface Pro 9),
> +	 * unsequenced events will always be sent with SEQ=0x00. Attempting to
> +	 * detect retransmission would thus just block all events.
> +	 *
> +	 * While sequence numbers would also allow detection of retransmitted
> +	 * packets in unsequenced communication, they have only ever been used
> +	 * to cover edge-cases in sequenced transmission. In particular, the
> +	 * only instance of packets being retransmitted (that we are aware of)
> +	 * is due to an ACK timeout. As this does not happen in unsequenced
> +	 * communication, skip the retransmission check for those packets
> +	 * entirely.
> +	 */
> +	if (frame->type == SSH_FRAME_TYPE_DATA_NSQ)
> +		return false;
> +
>  	/*
>  	 * Check if SEQ has been seen recently (i.e. packet was
>  	 * re-transmitted and we should ignore it).
>  	 */
>  	for (i = 0; i < ARRAY_SIZE(ptl->rx.blocked.seqs); i++) {
> -		if (likely(ptl->rx.blocked.seqs[i] != seq))
> +		if (likely(ptl->rx.blocked.seqs[i] != frame->seq))
>  			continue;
>  
>  		ptl_dbg(ptl, "ptl: ignoring repeated data packet\n");
> @@ -1613,7 +1629,7 @@ static bool ssh_ptl_rx_retransmit_check(struct ssh_ptl *ptl, u8 seq)
>  	}
>  
>  	/* Update list of blocked sequence IDs. */
> -	ptl->rx.blocked.seqs[ptl->rx.blocked.offset] = seq;
> +	ptl->rx.blocked.seqs[ptl->rx.blocked.offset] = frame->seq;
>  	ptl->rx.blocked.offset = (ptl->rx.blocked.offset + 1)
>  				  % ARRAY_SIZE(ptl->rx.blocked.seqs);
>  
> @@ -1624,7 +1640,7 @@ static void ssh_ptl_rx_dataframe(struct ssh_ptl *ptl,
>  				 const struct ssh_frame *frame,
>  				 const struct ssam_span *payload)
>  {
> -	if (ssh_ptl_rx_retransmit_check(ptl, frame->seq))
> +	if (ssh_ptl_rx_retransmit_check(ptl, frame))
>  		return;
>  
>  	ptl->ops.data_received(ptl, payload);