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(-)
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
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
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
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,
linux-stable-mirror@lists.linaro.org
-
Heikki Krogerus -
RD Babiera