Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7f906ecf-f115-c102-35b6-0974d98d65e1@redhat.com>
Date:   Tue, 15 Nov 2022 17:23:19 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH 1/2] platform/surface: aggregator: Do not check for
 repeated unsequenced packets
Content-Language: en-US, nl
To:     Maximilian Luz <luzmaximilian@gmail.com>
Cc:     Mark Gross <markgross@kernel.org>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20221113185951.224759-1-luzmaximilian@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20221113185951.224759-1-luzmaximilian@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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