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: <4051dd46-7e9e-455c-27d3-9516fa3c5fa6@redhat.com>
Date:   Thu, 12 Jan 2023 19:12:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH 1/9] platform/surface: aggregator: Ignore command messages
 not intended for us
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: <20221202223327.690880-1-luzmaximilian@gmail.com>
 <20221202223327.690880-2-luzmaximilian@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20221202223327.690880-2-luzmaximilian@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 12/2/22 23:33, Maximilian Luz wrote:
> It is possible that we (the host/kernel driver) receive command messages
> that are not intended for us. Ignore those for now.
> 
> The whole story is a bit more complicated: It is possible to enable
> debug output on SAM, which is sent via SSH command messages. By default
> this output is sent to a debug connector, with its own target ID
> (TID=0x03). It is possible to override the target of the debug output
> and set it to the host/kernel driver. This, however, does not change the
> original target ID of the message. Meaning, we receive messages with
> TID=0x03 (debug) but expect to only receive messages with TID=0x00
> (host).
> 
> The problem is that the different target ID also comes with a different
> scope of request IDs. In particular, these do not follow the standard
> event rules (i.e. do not fall into a set of small reserved values).
> Therefore, current message handling interprets them as responses to
> pending requests and tries to match them up via the request ID. However,
> these debug output messages are not in fact responses, and therefore
> this will at best fail to find the request and at worst pass on the
> wrong data as response for a request.
> 
> Therefore ignore any command messages not intended for us (host) for
> now. We can implement support for the debug messages once we have a
> better understanding of them.
> 
> Note that this may also provide a bit more stability and avoid some
> driver confusion in case any other targets want to talk to us in the
> future, since we don't yet know what to do with those as well. A warning
> for the dropped messages should suffice for now and also give us a
> chance of discovering new targets if they come along without any
> potential for bugs/instabilities.
> 
> Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

Thank you for your patch, I've applied this patch 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 patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans


> ---
> Note: I've simplified this commit so that it can be applied
> independently of the rest of the series for easier backporting.
> ---
>  .../surface/aggregator/ssh_request_layer.c         | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/platform/surface/aggregator/ssh_request_layer.c b/drivers/platform/surface/aggregator/ssh_request_layer.c
> index f5565570f16c..69132976d297 100644
> --- a/drivers/platform/surface/aggregator/ssh_request_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_request_layer.c
> @@ -916,6 +916,20 @@ static void ssh_rtl_rx_command(struct ssh_ptl *p, const struct ssam_span *data)
>  	if (sshp_parse_command(dev, data, &command, &command_data))
>  		return;
>  
> +	/*
> +	 * Check if the message was intended for us. If not, drop it.
> +	 *
> +	 * Note: We will need to change this to handle debug messages. On newer
> +	 * generation devices, these seem to be sent to tid_out=0x03. We as
> +	 * host can still receive them as they can be forwarded via an override
> +	 * option on SAM, but doing so does not change tid_out=0x00.
> +	 */
> +	if (command->tid_out != 0x00) {
> +		rtl_warn(rtl, "rtl: dropping message not intended for us (tid = %#04x)\n",
> +			 command->tid_out);
> +		return;
> +	}
> +
>  	if (ssh_rqid_is_event(get_unaligned_le16(&command->rqid)))
>  		ssh_rtl_rx_event(rtl, command, &command_data);
>  	else