usb: typec: tcpm: restrict SNK_WAIT_CAPABILITIES_TIMEOUT transitions to non self-powered devices

PATCH In review Amit Sunil Dhamne 4 participants 4 messages Oct 24, 2024 – Oct 25, 2024

Subsystems (4)

Stable branch USB USB Type-C class USB Type-C port controller drivers

lore

Share & tools

lore

Amit Sunil Dhamne

author

2024-10-24 02:22 UTC #0 lore raw

PATCH 1/1 usb: typec: tcpm: restrict SNK_WAIT_CAPABILITIES_TIMEOUT transitions to non self-powered devices

PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates that the policy engine perform a hard reset when SinkWaitCapTimer expires. Instead the code explicitly does a GET_SOURCE_CAP when the timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the following compliance test failures are reported by the compliance tester (added excerpts from the PD Test Spec):

* COMMON.PROC.PD.2#1:
The Tester receives a Get_Source_Cap Message from the UUT. This message is valid except the following conditions: [COMMON.PROC.PD.2#1]
a. The check fails if the UUT sends this message before the Tester
has established an Explicit Contract
...

* TEST.PD.PROT.SNK.4:
...
4. The check fails if the UUT does not send a Hard Reset between tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is between the VBUS present vSafe5V min and the time of the first bit of Preamble of the Hard Reset sent by the UUT.

For the purpose of interoperability, restrict the quirk introduced in
https://lore.kernel.org/all/[email protected]/
to only non self-powered devices as battery powered devices will not have the issue mentioned in that commit.

Cc [email protected]

Fixes 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")

Reported-by Badhri Jagan Sridharan <[email protected]>

Closes https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/ open here

Signed-off-by Amit Sunil Dhamne <[email protected]>

Reviewed-by Badhri Jagan Sridharan <[email protected]>

---
 drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/usb/typec/tcpm/tcpm.c +7 -3

index d6f2412381cf..c8f467d24fbb 100644
@@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
 		return ERROR_RECOVERY;
 	if (port->pwr_role == TYPEC_SOURCE)
 		return SRC_UNATTACHED;
-	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
+	if (port->state == SNK_WAIT_CAPABILITIES ||
+	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
 		return SNK_READY;
 	return SNK_UNATTACHED;
 }

@@ -5043,8 +5044,11 @@ static void run_state_machine(struct tcpm_port *port)
 			tcpm_set_state(port, SNK_SOFT_RESET,
 				       PD_T_SINK_WAIT_CAP);
 		} else {
-			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
-				       PD_T_SINK_WAIT_CAP);
+			if (!port->self_powered)
+				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
+			else
+				upcoming_state = hard_reset_state(port);
+			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
 		}
 		break;
 	case SNK_WAIT_CAPABILITIES_TIMEOUT:

base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8

Signature · 1 line

2.47.0.105.g07ac214952-goog

Xu Yang

Tested-by on v1 1/1

2024-10-25 10:23 UTC #1 lore raw

Quoted 26 lines · Amit Sunil Dhamne

PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates that the policy engine perform a hard reset when SinkWaitCapTimer expires. Instead the code explicitly does a GET_SOURCE_CAP when the timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the following compliance test failures are reported by the compliance tester (added excerpts from the PD Test Spec):
* COMMON.PROC.PD.2#1:
The Tester receives a Get_Source_Cap Message from the UUT. This message is valid except the following conditions: [COMMON.PROC.PD.2#1]
a. The check fails if the UUT sends this message before the Tester
has established an Explicit Contract
...
* TEST.PD.PROT.SNK.4:
...
4. The check fails if the UUT does not send a Hard Reset between tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is between the VBUS present vSafe5V min and the time of the first bit of Preamble of the Hard Reset sent by the UUT.
For the purpose of interoperability, restrict the quirk introduced in
https://lore.kernel.org/all/[email protected]/
to only non self-powered devices as battery powered devices will not have the issue mentioned in that commit.

Quoted 6 lines

Cc [email protected]
Fixes 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")
Reported-by Badhri Jagan Sridharan <[email protected]>
Closes https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/ open here
Signed-off-by Amit Sunil Dhamne <[email protected]>
Reviewed-by Badhri Jagan Sridharan <[email protected]>

Tested-by Xu Yang <[email protected]>

Thanks,
Xu Yang

Quoted 37 lines

---
 drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/usb/typec/tcpm/tcpm.c +7 -3

index d6f2412381cf..c8f467d24fbb 100644
@@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
 		return ERROR_RECOVERY;
 	if (port->pwr_role == TYPEC_SOURCE)
 		return SRC_UNATTACHED;
-	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
+	if (port->state == SNK_WAIT_CAPABILITIES ||
+	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
 		return SNK_READY;
 	return SNK_UNATTACHED;
 }

@@ -5043,8 +5044,11 @@ static void run_state_machine(struct tcpm_port *port)
 			tcpm_set_state(port, SNK_SOFT_RESET,
 				       PD_T_SINK_WAIT_CAP);
 		} else {
-			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
-				       PD_T_SINK_WAIT_CAP);
+			if (!port->self_powered)
+				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
+			else
+				upcoming_state = hard_reset_state(port);
+			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
 		}
 		break;
 	case SNK_WAIT_CAPABILITIES_TIMEOUT:

base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8

Signature · 1 line

2.47.0.105.g07ac214952-goog

Heikki Krogerus

maintainer Reviewed-by on v1 1/1

2024-10-25 10:29 UTC #2 lore raw

In reply to Amit Sunil Dhamne

Quoted 26 lines · Amit Sunil Dhamne

PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates that the policy engine perform a hard reset when SinkWaitCapTimer expires. Instead the code explicitly does a GET_SOURCE_CAP when the timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the following compliance test failures are reported by the compliance tester (added excerpts from the PD Test Spec):
* COMMON.PROC.PD.2#1:
The Tester receives a Get_Source_Cap Message from the UUT. This message is valid except the following conditions: [COMMON.PROC.PD.2#1]
a. The check fails if the UUT sends this message before the Tester
has established an Explicit Contract
...
* TEST.PD.PROT.SNK.4:
...
4. The check fails if the UUT does not send a Hard Reset between tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is between the VBUS present vSafe5V min and the time of the first bit of Preamble of the Hard Reset sent by the UUT.
For the purpose of interoperability, restrict the quirk introduced in
https://lore.kernel.org/all/[email protected]/
to only non self-powered devices as battery powered devices will not have the issue mentioned in that commit.

Quoted 6 lines

Cc [email protected]
Fixes 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")
Reported-by Badhri Jagan Sridharan <[email protected]>
Closes https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/ open here
Signed-off-by Amit Sunil Dhamne <[email protected]>
Reviewed-by Badhri Jagan Sridharan <[email protected]>

Reviewed-by Heikki Krogerus <[email protected]>

Quoted 37 lines

---
 drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/usb/typec/tcpm/tcpm.c +7 -3

index d6f2412381cf..c8f467d24fbb 100644
@@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
 		return ERROR_RECOVERY;
 	if (port->pwr_role == TYPEC_SOURCE)
 		return SRC_UNATTACHED;
-	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
+	if (port->state == SNK_WAIT_CAPABILITIES ||
+	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
 		return SNK_READY;
 	return SNK_UNATTACHED;
 }

@@ -5043,8 +5044,11 @@ static void run_state_machine(struct tcpm_port *port)
 			tcpm_set_state(port, SNK_SOFT_RESET,
 				       PD_T_SINK_WAIT_CAP);
 		} else {
-			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
-				       PD_T_SINK_WAIT_CAP);
+			if (!port->self_powered)
+				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
+			else
+				upcoming_state = hard_reset_state(port);
+			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
 		}
 		break;
 	case SNK_WAIT_CAPABILITIES_TIMEOUT:

base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8

Signature · 1 line

2.47.0.105.g07ac214952-goog

Signature · 1 line

heikki

Sebastian Reichel

Reviewed-by on v1 1/1

2024-10-25 16:55 UTC #3 lore raw

In reply to Amit Sunil Dhamne

Hi,

Quoted 26 lines · Amit Sunil Dhamne

PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates that the policy engine perform a hard reset when SinkWaitCapTimer expires. Instead the code explicitly does a GET_SOURCE_CAP when the timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the following compliance test failures are reported by the compliance tester (added excerpts from the PD Test Spec):
* COMMON.PROC.PD.2#1:
The Tester receives a Get_Source_Cap Message from the UUT. This message is valid except the following conditions: [COMMON.PROC.PD.2#1]
a. The check fails if the UUT sends this message before the Tester
has established an Explicit Contract
...
* TEST.PD.PROT.SNK.4:
...
4. The check fails if the UUT does not send a Hard Reset between tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is between the VBUS present vSafe5V min and the time of the first bit of Preamble of the Hard Reset sent by the UUT.
For the purpose of interoperability, restrict the quirk introduced in
https://lore.kernel.org/all/[email protected]/
to only non self-powered devices as battery powered devices will not have the issue mentioned in that commit.

Quoted 7 lines

Cc [email protected]
Fixes 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")
Reported-by Badhri Jagan Sridharan <[email protected]>
Closes https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/ open here
Signed-off-by Amit Sunil Dhamne <[email protected]>
Reviewed-by Badhri Jagan Sridharan <[email protected]>
---

I actually had this constrained to the !self_powered use-case originally (before sending to the ML). Since I didn't see a good reason for the extra check, I decided to keep the code simple :)

Reviewed-by Sebastian Reichel <[email protected]>

-- Sebastian

Quoted 36 lines

 drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/usb/typec/tcpm/tcpm.c +7 -3

index d6f2412381cf..c8f467d24fbb 100644
@@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
 		return ERROR_RECOVERY;
 	if (port->pwr_role == TYPEC_SOURCE)
 		return SRC_UNATTACHED;
-	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
+	if (port->state == SNK_WAIT_CAPABILITIES ||
+	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
 		return SNK_READY;
 	return SNK_UNATTACHED;
 }

@@ -5043,8 +5044,11 @@ static void run_state_machine(struct tcpm_port *port)
 			tcpm_set_state(port, SNK_SOFT_RESET,
 				       PD_T_SINK_WAIT_CAP);
 		} else {
-			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
-				       PD_T_SINK_WAIT_CAP);
+			if (!port->self_powered)
+				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
+			else
+				upcoming_state = hard_reset_state(port);
+			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
 		}
 		break;
 	case SNK_WAIT_CAPABILITIES_TIMEOUT:

base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8

Signature · 1 line

2.47.0.105.g07ac214952-goog