diff mbox series

[v1] usb: typec: tcpm: only discover modes the port supports svids for

Message ID 20231016232816.3355132-2-rdbabiera@google.com (mailing list archive)
State New, archived
Headers show
Series [v1] usb: typec: tcpm: only discover modes the port supports svids for | expand

Commit Message

RD Babiera Oct. 16, 2023, 11:28 p.m. UTC 
DisplayPort Alt Mode CTS test 10.3.1 verifies that the device can
recognize the DP SVID in arbitrary locations within the Discover SVIDs
response message. The test expects that the TCPM sends Discover Modes for
the DisplayPort SVID first, but fails because the TCPM sends
Discover Modes for all SVIDs regardless of whether or not the port
supports them.

After discovering the port partner's SVIDs, skip to the first SVID
supported by the device when preparing the Discover Modes request. If
other modes need to be discovered after the first Discover Modes message
is returned, skip over SVIDs not supported by the device.

Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
Cc: stable@vger.kernel.org
Signed-off-by: RD Babiera <rdbabiera@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 46 +++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 5 deletions(-)


base-commit: d0d27ef87e1ca974ed93ed4f7d3c123cbd392ba6

Comments

Heikki Krogerus Oct. 19, 2023, 8:09 a.m. UTC | #1 
Hi,

On Mon, Oct 16, 2023 at 11:28:17PM +0000, RD Babiera wrote:
> DisplayPort Alt Mode CTS test 10.3.1 verifies that the device can
> recognize the DP SVID in arbitrary locations within the Discover SVIDs
> response message. The test expects that the TCPM sends Discover Modes for
> the DisplayPort SVID first, but fails because the TCPM sends
> Discover Modes for all SVIDs regardless of whether or not the port
> supports them.
> 
> After discovering the port partner's SVIDs, skip to the first SVID
> supported by the device when preparing the Discover Modes request. If
> other modes need to be discovered after the first Discover Modes message
> is returned, skip over SVIDs not supported by the device.

I'm confused here. Is the device here the port or partner (or both)?
Why are you skipping the first SVID?

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

> Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")

I think that's wrong commit (perhaps you want 8afe9a3548f9d instead?).

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.

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.


> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 46 +++++++++++++++++++++++++++++++----
>  1 file changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 6e843c511b85..96636a6f1f7c 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1543,6 +1543,38 @@ static bool svdm_consume_svids(struct tcpm_port *port, const u32 *p, int cnt)
>  	return false;
>  }
>  
> +static bool svdm_port_supports_svid(struct tcpm_port *port, u16 svid)
> +{
> +	int i;
> +
> +	for (i = 0; i < ALTMODE_DISCOVERY_MAX; i++) {
> +		struct typec_altmode *altmode = port->port_altmode[i];
> +
> +		if (!altmode)
> +			return false;
> +		if (altmode->svid == svid)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + * This helper will continue to return the next supported SVID that the port
> + * needs to send DISCOVER_MODES to until the pmdata->svid_index is incremented after
> + * svdm_consume_modes() in tcpm_pd_svdm().
> + */
> +static int svdm_get_next_supported_svid(struct tcpm_port *port, struct pd_mode_data *pmdata)
> +{
> +	while (pmdata->svid_index < pmdata->nsvids) {
> +		u16 svid = pmdata->svids[pmdata->svid_index];
> +
> +		if (svdm_port_supports_svid(port, svid))
> +			return svid;
> +		pmdata->svid_index++;
> +	}
> +	return 0;
> +}
> +
>  static void svdm_consume_modes(struct tcpm_port *port, const u32 *p, int cnt)
>  {
>  	struct pd_mode_data *pmdata = &port->mode_data;
> @@ -1705,9 +1737,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>  			if (svdm_consume_svids(port, p, cnt)) {
>  				response[0] = VDO(USB_SID_PD, 1, svdm_version, CMD_DISCOVER_SVID);
>  				rlen = 1;
> -			} else if (modep->nsvids && supports_modal(port)) {
> -				response[0] = VDO(modep->svids[0], 1, svdm_version,
> -						  CMD_DISCOVER_MODES);
> +			} else if (modep->nsvids && supports_modal(port) &&
> +				   svdm_get_next_supported_svid(port, modep)) {
> +				u16 svid = svdm_get_next_supported_svid(port, modep);
> +
> +				response[0] = VDO(svid, 1, svdm_version, CMD_DISCOVER_MODES);
>  				rlen = 1;
>  			}
>  			break;
> @@ -1715,8 +1749,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>  			/* 6.4.4.3.3 */
>  			svdm_consume_modes(port, p, cnt);
>  			modep->svid_index++;
> -			if (modep->svid_index < modep->nsvids) {
> -				u16 svid = modep->svids[modep->svid_index];
> +			if (modep->svid_index < modep->nsvids &&
> +			    svdm_get_next_supported_svid(port, modep)) {
> +				u16 svid = svdm_get_next_supported_svid(port, modep);
> +
>  				response[0] = VDO(svid, 1, svdm_version, CMD_DISCOVER_MODES);
>  				rlen = 1;
>  			} else {
> 
> base-commit: d0d27ef87e1ca974ed93ed4f7d3c123cbd392ba6
> -- 
> 2.42.0.655.g421f12c284-goog
RD Babiera Oct. 19, 2023, 8:44 p.m. UTC | #2 
Hi Heikki,

On Thu, Oct 19, 2023 at 1:09 AM Heikki Krogerus
<heikki.krogerus@linux.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
Heikki Krogerus Oct. 30, 2023, 7:30 a.m. UTC | #3 
Hi,

I'm sorry to keep you waiting.

<snip>

> > 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.

I would like to hear what Guenter thinks. Guenter, do you have time to
take a look at this?

thanks,
diff mbox series

Patch

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 6e843c511b85..96636a6f1f7c 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1543,6 +1543,38 @@  static bool svdm_consume_svids(struct tcpm_port *port, const u32 *p, int cnt)
 	return false;
 }
 
+static bool svdm_port_supports_svid(struct tcpm_port *port, u16 svid)
+{
+	int i;
+
+	for (i = 0; i < ALTMODE_DISCOVERY_MAX; i++) {
+		struct typec_altmode *altmode = port->port_altmode[i];
+
+		if (!altmode)
+			return false;
+		if (altmode->svid == svid)
+			return true;
+	}
+	return false;
+}
+
+/*
+ * This helper will continue to return the next supported SVID that the port
+ * needs to send DISCOVER_MODES to until the pmdata->svid_index is incremented after
+ * svdm_consume_modes() in tcpm_pd_svdm().
+ */
+static int svdm_get_next_supported_svid(struct tcpm_port *port, struct pd_mode_data *pmdata)
+{
+	while (pmdata->svid_index < pmdata->nsvids) {
+		u16 svid = pmdata->svids[pmdata->svid_index];
+
+		if (svdm_port_supports_svid(port, svid))
+			return svid;
+		pmdata->svid_index++;
+	}
+	return 0;
+}
+
 static void svdm_consume_modes(struct tcpm_port *port, const u32 *p, int cnt)
 {
 	struct pd_mode_data *pmdata = &port->mode_data;
@@ -1705,9 +1737,11 @@  static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
 			if (svdm_consume_svids(port, p, cnt)) {
 				response[0] = VDO(USB_SID_PD, 1, svdm_version, CMD_DISCOVER_SVID);
 				rlen = 1;
-			} else if (modep->nsvids && supports_modal(port)) {
-				response[0] = VDO(modep->svids[0], 1, svdm_version,
-						  CMD_DISCOVER_MODES);
+			} else if (modep->nsvids && supports_modal(port) &&
+				   svdm_get_next_supported_svid(port, modep)) {
+				u16 svid = svdm_get_next_supported_svid(port, modep);
+
+				response[0] = VDO(svid, 1, svdm_version, CMD_DISCOVER_MODES);
 				rlen = 1;
 			}
 			break;
@@ -1715,8 +1749,10 @@  static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
 			/* 6.4.4.3.3 */
 			svdm_consume_modes(port, p, cnt);
 			modep->svid_index++;
-			if (modep->svid_index < modep->nsvids) {
-				u16 svid = modep->svids[modep->svid_index];
+			if (modep->svid_index < modep->nsvids &&
+			    svdm_get_next_supported_svid(port, modep)) {
+				u16 svid = svdm_get_next_supported_svid(port, modep);
+
 				response[0] = VDO(svid, 1, svdm_version, CMD_DISCOVER_MODES);
 				rlen = 1;
 			} else {