usb: typec: tcpm: ignore data role mismatch with a GoodCRC Message

PATCH In review Gianni Pisetta 2 participants 4 messages Oct 8, 2021 – Oct 9, 2021

Subsystems (3)

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

lore

Share & tools

lore

Gianni Pisetta

author

2021-10-08 20:16 UTC #0 lore raw

PATCH 1/1 usb: typec: tcpm: ignore data role mismatch with a GoodCRC Message

The data role check in tcpm.c cause the port manager to enter a loop with a hard reset on some chargers.
By the spec in a GoodCRC Message the other end does not need to comply with the data role, so ignore a data role mismatch in a request when the message type is GoodCRC.
From the USB PD spec:
"If a USB Type-C Port receive a Message with the Port Data Role field set to the same Data Role, except for the GoodCRC Message, USB Type-C error recovery..."

Below are the log of a Pinebook Pro attached to a PinePower Desktop Supply before the patch:

[226057.970532] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
[226057.975891] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 310 ms [rev3 NONE_AMS]
[226058.134384] PD RX, header: 0x53a1 [1]
[226058.134389] Data role mismatch, initiating error recovery
[226058.134392] state change SNK_WAIT_CAPABILITIES -> ERROR_RECOVERY [rev3 NONE_AMS]
[226058.134404] state change ERROR_RECOVERY -> PORT_RESET [rev3 NONE_AMS]

Fixes f0690a25a140b

cc <[email protected]>

Signed-off-by Gianni Pisetta <[email protected]>

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

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

index 5d05de666597..02ebf7e03c41 100644
@@ -2887,11 +2887,11 @@ static void tcpm_pd_rx_handler(struct kthread_work *work)
 
 		/*
 		 * If both ends believe to be DFP/host, we have a data role
-		 * mismatch.
+		 * mismatch. Must perform error recovery actions, except for
+		 * a GoodCRC Message(USB PD standard, 6.2.1.6).
 		 */
-		if (!!(le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
-		    (port->data_role == TYPEC_HOST)) {
+		if (!!((le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
+		       (port->data_role == TYPEC_HOST) && type != PD_CTRL_GOOD_CRC)) {
 			tcpm_log(port,
 				 "Data role mismatch, initiating error recovery");
 			tcpm_set_state(port, ERROR_RECOVERY, 0);

Signature · 1 line

2.20.1

Guenter Roeck

2021-10-08 20:47 UTC #1 lore raw

Quoted 34 lines · Gianni Pisetta

The data role check in tcpm.c cause the port manager to enter a loop with a hard reset on some chargers.
By the spec in a GoodCRC Message the other end does not need to comply with the data role, so ignore a data role mismatch in a request when the message type is GoodCRC.
Quoted 1 line
From the USB PD spec:
"If a USB Type-C Port receive a Message with the Port Data Role field set to the same Data Role, except for the GoodCRC Message, USB Type-C error recovery..."
Below are the log of a Pinebook Pro attached to a PinePower Desktop Supply before the patch:
[226057.970532] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
[226057.975891] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 310 ms [rev3 NONE_AMS]
[226058.134384] PD RX, header: 0x53a1 [1]
[226058.134389] Data role mismatch, initiating error recovery
[226058.134392] state change SNK_WAIT_CAPABILITIES -> ERROR_RECOVERY [rev3 NONE_AMS]
[226058.134404] state change ERROR_RECOVERY -> PORT_RESET [rev3 NONE_AMS]
Fixes f0690a25a140b
cc <[email protected]>
Signed-off-by Gianni Pisetta <[email protected]>
---
  drivers/usb/typec/tcpm/tcpm.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +0 -0
index 5d05de666597..02ebf7e03c41 100644
@@ -2887,11 +2887,11 @@ static void tcpm_pd_rx_handler(struct kthread_work *work)
 

drivers/usb/typec/tcpm/tcpm.c static void tcpm_pd_rx_handler(struct kthread_work *work) · line 2894

Quoted 10 lines

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

  		/*
  		 * If both ends believe to be DFP/host, we have a data role
-		 * mismatch.
+		 * mismatch. Must perform error recovery actions, except for
+		 * a GoodCRC Message(USB PD standard, 6.2.1.6).
  		 */
-		if (!!(le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
-		    (port->data_role == TYPEC_HOST)) {
+		if (!!((le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
+		       (port->data_role == TYPEC_HOST) && type != PD_CTRL_GOOD_CRC)) {

Unless I am missing something, this could also be a PD_DATA_SOURCE_CAP or PD_EXT_SOURCE_CAP_EXT message.

Guenter

Gianni Pisetta

author

2021-10-09 06:25 UTC #2 lore raw

Il ven 8 ott 2021, 22:47 Guenter Roeck <[email protected]> ha scritto:

Quoted 50 lines · Gianni Pisetta

Quoted 34 lines
The data role check in tcpm.c cause the port manager to enter a loop with a hard reset on some chargers.
By the spec in a GoodCRC Message the other end does not need to comply with the data role, so ignore a data role mismatch in a request when the message type is GoodCRC.
Quoted 1 line
From the USB PD spec:
"If a USB Type-C Port receive a Message with the Port Data Role field set to the same Data Role, except for the GoodCRC Message, USB Type-C error recovery..."
Below are the log of a Pinebook Pro attached to a PinePower Desktop Supply before the patch:
[226057.970532] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
[226057.975891] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 310 ms [rev3 NONE_AMS]
[226058.134384] PD RX, header: 0x53a1 [1]
[226058.134389] Data role mismatch, initiating error recovery
[226058.134392] state change SNK_WAIT_CAPABILITIES -> ERROR_RECOVERY [rev3 NONE_AMS]
[226058.134404] state change ERROR_RECOVERY -> PORT_RESET [rev3 NONE_AMS]
Fixes f0690a25a140b
cc <[email protected]>
Signed-off-by Gianni Pisetta <[email protected]>
---
  drivers/usb/typec/tcpm/tcpm.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +0 -0
index 5d05de666597..02ebf7e03c41 100644
@@ -2887,11 +2887,11 @@ static void tcpm_pd_rx_handler(struct kthread_work *work)
 
drivers/usb/typec/tcpm/tcpm.c static void tcpm_pd_rx_handler(struct kthread_work *work) · line 2894
Quoted 10 lines
drivers/usb/typec/tcpm/tcpm.c +4 -3
              /*
               * If both ends believe to be DFP/host, we have a data role
-              * mismatch.
+              * mismatch. Must perform error recovery actions, except for
+              * a GoodCRC Message(USB PD standard, 6.2.1.6).
               */
-             if (!!(le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
-                 (port->data_role == TYPEC_HOST)) {
+             if (!!((le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
+                    (port->data_role == TYPEC_HOST) && type != PD_CTRL_GOOD_CRC)) {
Unless I am missing something, this could also be a PD_DATA_SOURCE_CAP or PD_EXT_SOURCE_CAP_EXT message.

Quoted 1 line

Guenter

Yes, you are definitely right about that. I thought I had checked. Please discard this patch.
Would you accept a v2 with a proper check also on PD_HEADER_EXT_HDR and pd_header_cnt?

Thanks,
Gianni

Guenter Roeck

2021-10-09 06:43 UTC #3 lore raw

Quoted 24 lines · Gianni Pisetta

Il ven 8 ott 2021, 22:47 Guenter Roeck <[email protected]> ha scritto:
Quoted 23 lines · Gianni Pisetta
Quoted 21 lines
The data role check in tcpm.c cause the port manager to enter a loop with a hard reset on some chargers.
By the spec in a GoodCRC Message the other end does not need to comply with the data role, so ignore a data role mismatch in a request when the message type is GoodCRC.
Quoted 1 line
From the USB PD spec:
"If a USB Type-C Port receive a Message with the Port Data Role field set to the same Data Role, except for the GoodCRC Message, USB Type-C error recovery..."
Below are the log of a Pinebook Pro attached to a PinePower Desktop Supply before the patch:
[226057.970532] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES [rev3 NONE_AMS]
[226057.975891] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 310 ms [rev3 NONE_AMS]
[226058.134384] PD RX, header: 0x53a1 [1]
[226058.134389] Data role mismatch, initiating error recovery
[226058.134392] state change SNK_WAIT_CAPABILITIES -> ERROR_RECOVERY [rev3 NONE_AMS]
[226058.134404] state change ERROR_RECOVERY -> PORT_RESET [rev3 NONE_AMS]
Fixes f0690a25a140b

This is not a correct Fixes: tag. The correct tag would be:

Fixes f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")

Please run checkpatch on your patches; it should tell you.

Quoted 27 lines

Quoted 13 lines

cc <[email protected]>
Signed-off-by Gianni Pisetta <[email protected]>
---
   drivers/usb/typec/tcpm/tcpm.c | 7 ++++---
   1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +0 -0
index 5d05de666597..02ebf7e03c41 100644
@@ -2887,11 +2887,11 @@ static void tcpm_pd_rx_handler(struct kthread_work *work)
 

drivers/usb/typec/tcpm/tcpm.c static void tcpm_pd_rx_handler(struct kthread_work *work) · line 2894

Quoted 10 lines

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

               /*
                * If both ends believe to be DFP/host, we have a data role
-              * mismatch.
+              * mismatch. Must perform error recovery actions, except for
+              * a GoodCRC Message(USB PD standard, 6.2.1.6).
                */
-             if (!!(le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
-                 (port->data_role == TYPEC_HOST)) {
+             if (!!((le16_to_cpu(msg->header) & PD_HEADER_DATA_ROLE) ==
+                    (port->data_role == TYPEC_HOST) && type != PD_CTRL_GOOD_CRC)) {

Unless I am missing something, this could also be a PD_DATA_SOURCE_CAP or PD_EXT_SOURCE_CAP_EXT message.

Quoted 6 lines

Quoted 1 line
Guenter
Yes, you are definitely right about that. I thought I had checked. Please discard this patch.
Would you accept a v2 with a proper check also on PD_HEADER_EXT_HDR and pd_header_cnt?

Sure, assuming it is correct, but please remember that there should only be one logical change per patch.

Guenter

Quoted 4 lines

Thanks,
Gianni