On 10/8/21 11:25 PM, Gianni Pisetta wrote:
Il ven 8 ott 2021, 22:47 Guenter Roeck <linux@xxxxxxxxxxxx> ha scritto:
On 10/8/21 1:14 PM, Gianni Pisetta wrote:
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
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.
cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Gianni Pisetta <gianni.pisetta@xxxxxxxxx>
---
drivers/usb/typec/tcpm/tcpm.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 5d05de666597..02ebf7e03c41 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -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)) {
Unless I am missing something, this could also be a PD_DATA_SOURCE_CAP
or PD_EXT_SOURCE_CAP_EXT message.
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
Thanks,
Gianni
