Message-ID: <CALzBnUF-EZjFEHCc4XRLdFr5yP8dCq7De4SaNif32LcL5=tMYA@mail.gmail.com>
Date:   Thu, 19 Oct 2023 13:44:59 -0700
From:   RD Babiera <rdbabiera@...gle.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     gregkh@...uxfoundation.org, linux@...ck-us.net,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        badhri@...gle.com, stable@...r.kernel.org
Subject: Re: [PATCH v1] usb: typec: tcpm: only discover modes the port
 supports svids for

Hi Heikki,

On Thu, Oct 19, 2023 at 1:09 AM Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
> I'm confused here. Is the device here the port or partner (or both)?

The port, I'll make sure to be more precise when describing.

> Why are you skipping the first SVID?

Skipping to the first SVID supported by the port when preparing
the first Discover Modes message.

> Please note that the Type-C specification puts priority on TBT over DP.
> Is this in conflict with that?

Not in this case. Assuming the port supports both TBT and DP, a Discover
Modes message will be sent to both regardless of what order they return
in the Discover SVIDs ACK message.

> > Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
>
> I think that's wrong commit (perhaps you want 8afe9a3548f9d instead?).

8afe9a3548f9d looks to be more concerned with the consumption and
processing of the received payload, I had put f0690a25a140 because it
contained the logic to determine if the Discover Mode message was being
sent at all as well as preparing the response. 5e1d4c49fbc86 does touch
the response formation but only the svdm_version and not the SVID.

> Right now I'm not convinced that this should be considered as a fix at
> all. I don't know anything about the test you are talking about, but
> if this patch is just about making it pass, then there is something
> seriously wrong.

I use the VESA DisplayPort Alt Mode on USB Type-C CTS as a reference.
In regards to this being a fix, if this ends up being more optional (discussed
below), then I'll remove the fix tag.

> If you need the modes to be discovered in some specific order, then we
> need the framework to allow you to do that. So perhaps the tcpci
> drivers should be able to supply the preferred order to the tcpm?
>
> But as such, unless I'm mistaken, this patch will change the logic so
> that only the partner alt modes that the port supports get registered,
> and that way are exposed to the user. You can't do that - right now
> it's the only way we can inform the user about them. All partner
> alternate modes (at least the SVIDs) must be exposed to the user one
> way or the other, regardless does the port support them or not.

The test this patch tries to fix could just be written without consideration
of this. My guess is that the test was designed such that the SVIDs before
the DisplayPort SVID are unknown to the port under test so the mentality
could have been "why should a port care about SVIDs it doesn't know
about?"

A defense I could make for it is that the USB PD CTS doesn't test
to see if a port under test sends Discover Modes for every SVID returned
in a Discover SVIDs ACK, so the interpretation isn't invalid. I've seen other
tcpm implementations handle Discover Modes this way as well.

Regardless, you're definitely right that the user should know about all
Alt Modes/SVIDs - the port would lose SVID information without
registering a partner altmode for it. Currently I think the approaches to pass
this test look like:
    1. Your suggestion - let the tcpci decide if there should be a
discovery order.
Alternatively, let the tcpci decide if it wants to opt into this
patch's behavior of
only discovering modes for known SVIDs - a strict discovery flag.
    2. Send a Discover Mode message to known SVIDs first in the order
they come in, and then to unknown SVIDs. The test passes and no information
is lost, but it's unnecessary refactoring just to pass one test for
one Alt Mode.
    3. Don't send a Discover Mode message to unknown SVIDs, but do register
an Alt Mode with blank info for that SVID. It passes the test without having to
do any reordering compared to the first option and it preserves supported
SVIDs. But, the port would lose information such as each SVID's Alt Modes
VDO plus each SVID can support more than one Alt Mode.

Let me know if any of these approaches sound worth pursuing; I do think
Option 1 does make more sense than the others.

---
Thanks for the feedback,
RD

Powered by blists - more mailing lists