Re: [PATCH] usb: typec: tcpm: Complete discovery when the partner lacks SVDM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: typec: tcpm: SOP' Not_Supported in tcpm_pd_ctrl_request() halts discovery
--

commit f5763c417e2367d11a45635b9af7ac9ca073d24a
Author: Alexey Charkov <alchark@flipper.net>

usb: typec: tcpm: Complete discovery when the partner lacks SVDM support

This commit marks VDM discovery complete when a port partner responds to
Discover Identity or Discover SVIDs over SOP with PD_CTRL_NOT_SUPP,
avoiding an infinite loop of discovery requests.

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d721baef52104..e21fe544b2d4b 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3908,6 +3908,17 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  		case SNK_READY:
>  			if (port->vdm_state > VDM_STATE_READY) {
>  				port->vdm_state = VDM_STATE_DONE;
> +				/*
> +				 * A partner without Structured VDM support answers
> +				 * discovery with Not_Supported: there is nothing left
> +				 * to discover, just as after a NAK.
> +				 */
> +				if (type == PD_CTRL_NOT_SUPP &&
> +				    rx_sop_type == TCPC_TX_SOP &&
> +				    (port->ams == DISCOVER_IDENTITY ||
> +				     port->ams == DISCOVER_SVIDS))
> +					tcpm_update_vdm_discovery_state(port,
> +									VDM_DISCOVERY_COMPLETE);

[Severity: Medium]
The commit message notes that a Not_Supported reply from the cable plug is
left alone so that it cannot cut short discovery of the partner. However, if a
cable plug replies with PD_CTRL_NOT_SUPP (rx_sop_type == TCPC_TX_SOP_PRIME) in
tcpm_pd_ctrl_request(), doesn't leaving port->vdm_discovery_state unchanged
still stall partner discovery and repeatedly re-send the SOP' request?

Specifically, after the partner ACKs SOP CMD_DISCOVER_IDENT, tcpm_pd_svdm()
sets port->vdm_discovery_state to VDM_DISCOVERY_PARTNER_IDENT and probes SOP'
before discovering the partner's SVIDs:

drivers/usb/typec/tcpm/tcpm.c:tcpm_pd_svdm() {
    ...
	tcpm_update_vdm_discovery_state(port, VDM_DISCOVERY_PARTNER_IDENT);
    ...
	if (IS_ERR_OR_NULL(port->cable) &&
	    tcpm_can_communicate_sop_prime(port)) {
		*response_tx_sop_type = TCPC_TX_SOP_PRIME;
		response[0] = VDO(USB_SID_PD, 1,
				  typec_get_negotiated_svdm_version(typec),
				  CMD_DISCOVER_IDENT);
		rlen = 1;
	} else {
		*response_tx_sop_type = TCPC_TX_SOP;
		response[0] = VDO(USB_SID_PD, 1,
				  typec_get_negotiated_svdm_version(typec),
				  CMD_DISCOVER_SVID);
		rlen = 1;
	}
}

If the cable plug replies to SOP' CMD_DISCOVER_IDENT with PD_CTRL_NOT_SUPP,
tcpm_pd_ctrl_request() finishes the AMS without advancing
port->vdm_discovery_state or queueing SOP CMD_DISCOVER_SVID. Because
port->cable is still NULL, every subsequent run of tcpm_vdm_discovery_work()
will re-send SOP' CMD_DISCOVER_IDENT instead of moving on to partner SVID
discovery:

drivers/usb/typec/tcpm/tcpm.c:tcpm_vdm_discovery_work() {
    ...
	case VDM_DISCOVERY_PARTNER_IDENT:
		if (tcpm_can_communicate_sop_prime(port) && !port->cable) {
			tx_sop_type = TCPC_TX_SOP_PRIME;
			msg[0] = VDO(USB_SID_PD, 1, svdm_version, CMD_DISCOVER_IDENT);
		} else {
			if (tcpm_can_communicate_sop_prime(port))
				tcpm_update_vdm_discovery_state(port, VDM_DISCOVERY_CABLE_IDENT);
			msg[0] = VDO(USB_SID_PD, 1, svdm_version, CMD_DISCOVER_SVID);
		}
		break;
}

Similarly, when tcpm_handle_discover_mode() finishes discovering partner modes
and sends SOP' CMD_DISCOVER_SVID in VDM_DISCOVERY_PARTNER_MODES, a
PD_CTRL_NOT_SUPP on SOP' leaves port->vdm_discovery_state in
VDM_DISCOVERY_PARTNER_MODES without calling tcpm_register_partner_altmodes(),
causing tcpm_vdm_discovery_work() to repeatedly re-send SOP'
CMD_DISCOVER_SVID.

Should a PD_CTRL_NOT_SUPP on TCPC_TX_SOP_PRIME fall back to SOP discovery in
the same way vdm_run_state_machine() handles SOP' transmission failures in
VDM_STATE_ERR_SEND (advancing to VDM_DISCOVERY_CABLE_IDENT and queueing SOP
CMD_DISCOVER_SVID on DISCOVER_IDENTITY, or calling
tcpm_register_partner_altmodes() and marking VDM_DISCOVERY_COMPLETE on
DISCOVER_SVIDS and DISCOVER_MODES)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-tcpm-discovery-no-svdm-v1-1-54bb9b613065@flipper.net?part=1