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@xxxxxxxxx>
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
