| Message ID | 20190315144219.5914-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tcpm: Try PD-2.0 if sink does not respond to 3.0 source-caps | expand |
On Fri, Mar 15, 2019 at 03:42:19PM +0100, Hans de Goede wrote: > PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and > simply ignore any src PDOs which the sink does not understand such as PPS > but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, > causing contract negotiation to fail. > > This commit fixes such sinks not working by re-trying the contract > negotiation with PD-2.0 source-caps messages if we don't have a contract > after PD_N_HARD_RESET_COUNT hard-reset attempts. > > The problem fixed by this commit was noticed with a Type-C to VGA dongle. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > The Type-C to VGA dongle on which this encountered looks like this one: > https://www.aliexpress.com/item/Male-USB-3-1-Type-C-USB-C-to-Female-VGA-Adapter-Cable-10Gbps-for-New/32898274476.html > --- > drivers/usb/typec/tcpm/tcpm.c | 34 +++++++++++++++++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index f1c39a3c7534..3f8df845d1a5 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -37,6 +37,7 @@ > S(SRC_ATTACHED), \ > S(SRC_STARTUP), \ > S(SRC_SEND_CAPABILITIES), \ > + S(SRC_SEND_CAP_LOWER_PD_REVISION), \ > S(SRC_NEGOTIATE_CAPABILITIES), \ > S(SRC_TRANSITION_SUPPLY), \ > S(SRC_READY), \ > @@ -2792,6 +2793,29 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port) > return SNK_UNATTACHED; > } > > +/* > + * PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and > + * simply ignore any src PDOs which the sink does not understand such as PPS > + * but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, > + * causing contract negotiation to fail. > + * > + * This function is used by the SRC_SEND_CAPABILITIES state in > + * run_state_machine() to work around this. > + * > + * After PD_N_HARD_RESET_COUNT hard-reset attempts this function selects > + * SRC_SEND_CAP_LOWER_PD_REVISION as state to set after the next timeout, > + * this state will fallback to a lower PD revision and then try sending the > + * src-capabilities again. > + */ > +static inline enum tcpm_state src_send_cap_timeout_state(struct tcpm_port *port) > +{ > + if (port->hard_reset_count < PD_N_HARD_RESET_COUNT) > + return HARD_RESET_SEND; > + if (port->negotiated_rev > PD_REV20) > + return SRC_SEND_CAP_LOWER_PD_REVISION; > + return hard_reset_state(port); > +} > + > static inline enum tcpm_state unattached_state(struct tcpm_port *port) > { > if (port->port_type == TYPEC_PORT_DRP) { > @@ -2966,10 +2990,18 @@ static void run_state_machine(struct tcpm_port *port) > /* port->hard_reset_count = 0; */ > port->caps_count = 0; > port->pd_capable = true; > - tcpm_set_state_cond(port, hard_reset_state(port), > + tcpm_set_state_cond(port, > + src_send_cap_timeout_state(port), > PD_T_SEND_SOURCE_CAP); > } > break; > + case SRC_SEND_CAP_LOWER_PD_REVISION: > + if (WARN_ON(port->negotiated_rev <= PD_REV20)) > + break; I really dislike the WARN_ON here. A bad remote can potentially trigger this, which on systems with crash on warning enabled can result in a reboot. Just revert to the original behavior here, and maybe add a tcpm log message. Guenter > + port->negotiated_rev--; > + port->hard_reset_count = 0; > + tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0); > + break; > case SRC_NEGOTIATE_CAPABILITIES: > ret = tcpm_pd_check_request(port); > if (ret < 0) { > -- > 2.20.1 >
Hi, On 3/15/19 3:57 PM, Guenter Roeck wrote: > On Fri, Mar 15, 2019 at 03:42:19PM +0100, Hans de Goede wrote: >> PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and >> simply ignore any src PDOs which the sink does not understand such as PPS >> but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, >> causing contract negotiation to fail. >> >> This commit fixes such sinks not working by re-trying the contract >> negotiation with PD-2.0 source-caps messages if we don't have a contract >> after PD_N_HARD_RESET_COUNT hard-reset attempts. >> >> The problem fixed by this commit was noticed with a Type-C to VGA dongle. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> The Type-C to VGA dongle on which this encountered looks like this one: >> https://www.aliexpress.com/item/Male-USB-3-1-Type-C-USB-C-to-Female-VGA-Adapter-Cable-10Gbps-for-New/32898274476.html >> --- >> drivers/usb/typec/tcpm/tcpm.c | 34 +++++++++++++++++++++++++++++++++- >> 1 file changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >> index f1c39a3c7534..3f8df845d1a5 100644 >> --- a/drivers/usb/typec/tcpm/tcpm.c >> +++ b/drivers/usb/typec/tcpm/tcpm.c >> @@ -37,6 +37,7 @@ >> S(SRC_ATTACHED), \ >> S(SRC_STARTUP), \ >> S(SRC_SEND_CAPABILITIES), \ >> + S(SRC_SEND_CAP_LOWER_PD_REVISION), \ >> S(SRC_NEGOTIATE_CAPABILITIES), \ >> S(SRC_TRANSITION_SUPPLY), \ >> S(SRC_READY), \ >> @@ -2792,6 +2793,29 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port) >> return SNK_UNATTACHED; >> } >> >> +/* >> + * PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and >> + * simply ignore any src PDOs which the sink does not understand such as PPS >> + * but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, >> + * causing contract negotiation to fail. >> + * >> + * This function is used by the SRC_SEND_CAPABILITIES state in >> + * run_state_machine() to work around this. >> + * >> + * After PD_N_HARD_RESET_COUNT hard-reset attempts this function selects >> + * SRC_SEND_CAP_LOWER_PD_REVISION as state to set after the next timeout, >> + * this state will fallback to a lower PD revision and then try sending the >> + * src-capabilities again. >> + */ >> +static inline enum tcpm_state src_send_cap_timeout_state(struct tcpm_port *port) >> +{ >> + if (port->hard_reset_count < PD_N_HARD_RESET_COUNT) >> + return HARD_RESET_SEND; >> + if (port->negotiated_rev > PD_REV20) >> + return SRC_SEND_CAP_LOWER_PD_REVISION; >> + return hard_reset_state(port); >> +} >> + >> static inline enum tcpm_state unattached_state(struct tcpm_port *port) >> { >> if (port->port_type == TYPEC_PORT_DRP) { >> @@ -2966,10 +2990,18 @@ static void run_state_machine(struct tcpm_port *port) >> /* port->hard_reset_count = 0; */ >> port->caps_count = 0; >> port->pd_capable = true; >> - tcpm_set_state_cond(port, hard_reset_state(port), >> + tcpm_set_state_cond(port, >> + src_send_cap_timeout_state(port), >> PD_T_SEND_SOURCE_CAP); >> } >> break; >> + case SRC_SEND_CAP_LOWER_PD_REVISION: >> + if (WARN_ON(port->negotiated_rev <= PD_REV20)) >> + break; > > I really dislike the WARN_ON here. A bad remote can potentially trigger > this, which on systems with crash on warning enabled can result in a > reboot. Just revert to the original behavior here, and maybe add > a tcpm log message. How would a bad remote trigger this? We only ever call set_state with SRC_SEND_CAP_LOWER_PD_REVISION in the new src_send_cap_timeout_state which has: if (port->negotiated_rev > PD_REV20) return SRC_SEND_CAP_LOWER_PD_REVISION; So we really should never hit the WARN_ON, of we do hit the WARN_ON something is seriously wrong. Regards, Hans > > Guenter > >> + port->negotiated_rev--; >> + port->hard_reset_count = 0; >> + tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0); >> + break; >> case SRC_NEGOTIATE_CAPABILITIES: >> ret = tcpm_pd_check_request(port); >> if (ret < 0) { >> -- >> 2.20.1 >>
On Fri, Mar 15, 2019 at 05:43:05PM +0100, Hans de Goede wrote: > Hi, > > On 3/15/19 3:57 PM, Guenter Roeck wrote: > >On Fri, Mar 15, 2019 at 03:42:19PM +0100, Hans de Goede wrote: > >>PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and > >>simply ignore any src PDOs which the sink does not understand such as PPS > >>but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, > >>causing contract negotiation to fail. > >> > >>This commit fixes such sinks not working by re-trying the contract > >>negotiation with PD-2.0 source-caps messages if we don't have a contract > >>after PD_N_HARD_RESET_COUNT hard-reset attempts. > >> > >>The problem fixed by this commit was noticed with a Type-C to VGA dongle. > >> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>--- > >>The Type-C to VGA dongle on which this encountered looks like this one: > >>https://www.aliexpress.com/item/Male-USB-3-1-Type-C-USB-C-to-Female-VGA-Adapter-Cable-10Gbps-for-New/32898274476.html > >>--- > >> drivers/usb/typec/tcpm/tcpm.c | 34 +++++++++++++++++++++++++++++++++- > >> 1 file changed, 33 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > >>index f1c39a3c7534..3f8df845d1a5 100644 > >>--- a/drivers/usb/typec/tcpm/tcpm.c > >>+++ b/drivers/usb/typec/tcpm/tcpm.c > >>@@ -37,6 +37,7 @@ > >> S(SRC_ATTACHED), \ > >> S(SRC_STARTUP), \ > >> S(SRC_SEND_CAPABILITIES), \ > >>+ S(SRC_SEND_CAP_LOWER_PD_REVISION), \ > >> S(SRC_NEGOTIATE_CAPABILITIES), \ > >> S(SRC_TRANSITION_SUPPLY), \ > >> S(SRC_READY), \ > >>@@ -2792,6 +2793,29 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port) > >> return SNK_UNATTACHED; > >> } > >>+/* > >>+ * PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and > >>+ * simply ignore any src PDOs which the sink does not understand such as PPS > >>+ * but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, > >>+ * causing contract negotiation to fail. > >>+ * > >>+ * This function is used by the SRC_SEND_CAPABILITIES state in > >>+ * run_state_machine() to work around this. > >>+ * > >>+ * After PD_N_HARD_RESET_COUNT hard-reset attempts this function selects > >>+ * SRC_SEND_CAP_LOWER_PD_REVISION as state to set after the next timeout, > >>+ * this state will fallback to a lower PD revision and then try sending the > >>+ * src-capabilities again. > >>+ */ > >>+static inline enum tcpm_state src_send_cap_timeout_state(struct tcpm_port *port) > >>+{ > >>+ if (port->hard_reset_count < PD_N_HARD_RESET_COUNT) > >>+ return HARD_RESET_SEND; > >>+ if (port->negotiated_rev > PD_REV20) > >>+ return SRC_SEND_CAP_LOWER_PD_REVISION; > >>+ return hard_reset_state(port); > >>+} > >>+ > >> static inline enum tcpm_state unattached_state(struct tcpm_port *port) > >> { > >> if (port->port_type == TYPEC_PORT_DRP) { > >>@@ -2966,10 +2990,18 @@ static void run_state_machine(struct tcpm_port *port) > >> /* port->hard_reset_count = 0; */ > >> port->caps_count = 0; > >> port->pd_capable = true; > >>- tcpm_set_state_cond(port, hard_reset_state(port), > >>+ tcpm_set_state_cond(port, > >>+ src_send_cap_timeout_state(port), > >> PD_T_SEND_SOURCE_CAP); > >> } > >> break; > >>+ case SRC_SEND_CAP_LOWER_PD_REVISION: > >>+ if (WARN_ON(port->negotiated_rev <= PD_REV20)) > >>+ break; > > > >I really dislike the WARN_ON here. A bad remote can potentially trigger > >this, which on systems with crash on warning enabled can result in a > >reboot. Just revert to the original behavior here, and maybe add > >a tcpm log message. > > How would a bad remote trigger this? > > We only ever call set_state with SRC_SEND_CAP_LOWER_PD_REVISION in the new > src_send_cap_timeout_state which has: > > if (port->negotiated_rev > PD_REV20) > return SRC_SEND_CAP_LOWER_PD_REVISION; > > So we really should never hit the WARN_ON, of we do hit the WARN_ON > something is seriously wrong. > If that situation can't happen, the check should not be there in the first place. Otherwise you could litter the implementation with WARN_ON all over the place, and make it all but unreadable. I am not in favor of code like that. Guenter > Regards, > > Hans > > > > > > >Guenter > > > >>+ port->negotiated_rev--; > >>+ port->hard_reset_count = 0; > >>+ tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0); > >>+ break; > >> case SRC_NEGOTIATE_CAPABILITIES: > >> ret = tcpm_pd_check_request(port); > >> if (ret < 0) { > >>-- > >>2.20.1 > >>
Hi, On 15-03-19 17:53, Guenter Roeck wrote: > On Fri, Mar 15, 2019 at 05:43:05PM +0100, Hans de Goede wrote: >> Hi, >> >> On 3/15/19 3:57 PM, Guenter Roeck wrote: >>> On Fri, Mar 15, 2019 at 03:42:19PM +0100, Hans de Goede wrote: >>>> PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and >>>> simply ignore any src PDOs which the sink does not understand such as PPS >>>> but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, >>>> causing contract negotiation to fail. >>>> >>>> This commit fixes such sinks not working by re-trying the contract >>>> negotiation with PD-2.0 source-caps messages if we don't have a contract >>>> after PD_N_HARD_RESET_COUNT hard-reset attempts. >>>> >>>> The problem fixed by this commit was noticed with a Type-C to VGA dongle. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> The Type-C to VGA dongle on which this encountered looks like this one: >>>> https://www.aliexpress.com/item/Male-USB-3-1-Type-C-USB-C-to-Female-VGA-Adapter-Cable-10Gbps-for-New/32898274476.html >>>> --- >>>> drivers/usb/typec/tcpm/tcpm.c | 34 +++++++++++++++++++++++++++++++++- >>>> 1 file changed, 33 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >>>> index f1c39a3c7534..3f8df845d1a5 100644 >>>> --- a/drivers/usb/typec/tcpm/tcpm.c >>>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>>> @@ -37,6 +37,7 @@ >>>> S(SRC_ATTACHED), \ >>>> S(SRC_STARTUP), \ >>>> S(SRC_SEND_CAPABILITIES), \ >>>> + S(SRC_SEND_CAP_LOWER_PD_REVISION), \ >>>> S(SRC_NEGOTIATE_CAPABILITIES), \ >>>> S(SRC_TRANSITION_SUPPLY), \ >>>> S(SRC_READY), \ >>>> @@ -2792,6 +2793,29 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port) >>>> return SNK_UNATTACHED; >>>> } >>>> +/* >>>> + * PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and >>>> + * simply ignore any src PDOs which the sink does not understand such as PPS >>>> + * but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, >>>> + * causing contract negotiation to fail. >>>> + * >>>> + * This function is used by the SRC_SEND_CAPABILITIES state in >>>> + * run_state_machine() to work around this. >>>> + * >>>> + * After PD_N_HARD_RESET_COUNT hard-reset attempts this function selects >>>> + * SRC_SEND_CAP_LOWER_PD_REVISION as state to set after the next timeout, >>>> + * this state will fallback to a lower PD revision and then try sending the >>>> + * src-capabilities again. >>>> + */ >>>> +static inline enum tcpm_state src_send_cap_timeout_state(struct tcpm_port *port) >>>> +{ >>>> + if (port->hard_reset_count < PD_N_HARD_RESET_COUNT) >>>> + return HARD_RESET_SEND; >>>> + if (port->negotiated_rev > PD_REV20) >>>> + return SRC_SEND_CAP_LOWER_PD_REVISION; >>>> + return hard_reset_state(port); >>>> +} >>>> + >>>> static inline enum tcpm_state unattached_state(struct tcpm_port *port) >>>> { >>>> if (port->port_type == TYPEC_PORT_DRP) { >>>> @@ -2966,10 +2990,18 @@ static void run_state_machine(struct tcpm_port *port) >>>> /* port->hard_reset_count = 0; */ >>>> port->caps_count = 0; >>>> port->pd_capable = true; >>>> - tcpm_set_state_cond(port, hard_reset_state(port), >>>> + tcpm_set_state_cond(port, >>>> + src_send_cap_timeout_state(port), >>>> PD_T_SEND_SOURCE_CAP); >>>> } >>>> break; >>>> + case SRC_SEND_CAP_LOWER_PD_REVISION: >>>> + if (WARN_ON(port->negotiated_rev <= PD_REV20)) >>>> + break; >>> >>> I really dislike the WARN_ON here. A bad remote can potentially trigger >>> this, which on systems with crash on warning enabled can result in a >>> reboot. Just revert to the original behavior here, and maybe add >>> a tcpm log message. >> >> How would a bad remote trigger this? >> >> We only ever call set_state with SRC_SEND_CAP_LOWER_PD_REVISION in the new >> src_send_cap_timeout_state which has: >> >> if (port->negotiated_rev > PD_REV20) >> return SRC_SEND_CAP_LOWER_PD_REVISION; >> >> So we really should never hit the WARN_ON, of we do hit the WARN_ON >> something is seriously wrong. >> > > If that situation can't happen, the check should not be there in the first > place. Otherwise you could litter the implementation with WARN_ON all over > the place, and make it all but unreadable. I am not in favor of code like > that. So thinking more about this, the WARN_ON can actually trigger if we receive a pd packet with a lower revision during the timeout window and this does not cause the state to change. This is an obvious time of check vs time of use problem and the fix is to check if we should try to fallback / check the negotiated_rev after the timeout, rather then when determining the next state to use after the timeout, so that both the check and the decrement of negotiated_rev are done in one go while holding the lock. I will rework the patch accordingly and submit a new version. Regards, Hans >>>> + port->negotiated_rev--; >>>> + port->hard_reset_count = 0; >>>> + tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0); >>>> + break; >>>> case SRC_NEGOTIATE_CAPABILITIES: >>>> ret = tcpm_pd_check_request(port); >>>> if (ret < 0) { >>>> -- >>>> 2.20.1 >>>>
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index f1c39a3c7534..3f8df845d1a5 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -37,6 +37,7 @@ S(SRC_ATTACHED), \ S(SRC_STARTUP), \ S(SRC_SEND_CAPABILITIES), \ + S(SRC_SEND_CAP_LOWER_PD_REVISION), \ S(SRC_NEGOTIATE_CAPABILITIES), \ S(SRC_TRANSITION_SUPPLY), \ S(SRC_READY), \ @@ -2792,6 +2793,29 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port) return SNK_UNATTACHED; } +/* + * PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and + * simply ignore any src PDOs which the sink does not understand such as PPS + * but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, + * causing contract negotiation to fail. + * + * This function is used by the SRC_SEND_CAPABILITIES state in + * run_state_machine() to work around this. + * + * After PD_N_HARD_RESET_COUNT hard-reset attempts this function selects + * SRC_SEND_CAP_LOWER_PD_REVISION as state to set after the next timeout, + * this state will fallback to a lower PD revision and then try sending the + * src-capabilities again. + */ +static inline enum tcpm_state src_send_cap_timeout_state(struct tcpm_port *port) +{ + if (port->hard_reset_count < PD_N_HARD_RESET_COUNT) + return HARD_RESET_SEND; + if (port->negotiated_rev > PD_REV20) + return SRC_SEND_CAP_LOWER_PD_REVISION; + return hard_reset_state(port); +} + static inline enum tcpm_state unattached_state(struct tcpm_port *port) { if (port->port_type == TYPEC_PORT_DRP) { @@ -2966,10 +2990,18 @@ static void run_state_machine(struct tcpm_port *port) /* port->hard_reset_count = 0; */ port->caps_count = 0; port->pd_capable = true; - tcpm_set_state_cond(port, hard_reset_state(port), + tcpm_set_state_cond(port, + src_send_cap_timeout_state(port), PD_T_SEND_SOURCE_CAP); } break; + case SRC_SEND_CAP_LOWER_PD_REVISION: + if (WARN_ON(port->negotiated_rev <= PD_REV20)) + break; + port->negotiated_rev--; + port->hard_reset_count = 0; + tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0); + break; case SRC_NEGOTIATE_CAPABILITIES: ret = tcpm_pd_check_request(port); if (ret < 0) {
PD 2.0 sinks are supposed to accept src-capabilities with a 3.0 header and simply ignore any src PDOs which the sink does not understand such as PPS but some 2.0 sinks instead ignore the entire PD_DATA_SOURCE_CAP message, causing contract negotiation to fail. This commit fixes such sinks not working by re-trying the contract negotiation with PD-2.0 source-caps messages if we don't have a contract after PD_N_HARD_RESET_COUNT hard-reset attempts. The problem fixed by this commit was noticed with a Type-C to VGA dongle. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- The Type-C to VGA dongle on which this encountered looks like this one: https://www.aliexpress.com/item/Male-USB-3-1-Type-C-USB-C-to-Female-VGA-Adapter-Cable-10Gbps-for-New/32898274476.html --- drivers/usb/typec/tcpm/tcpm.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)