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@xxxxxxxxxx>
---
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
