Re: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low

From: Amit Sunil Dhamne
Date: Wed Oct 23 2024 - 23:13:56 EST


Hi Yanik,

On 10/22/24 10:28 AM, Yanik Fuchs wrote:
Good Evening

Here is a Patch to resolve an issue with TCPM if Vbus was never low.
Please consider that this is one of my first kernel pull requests, I may have missunderstood the process.

Freundliche Grüsse
Best regards


Yanik Fuchs

---

From 604b97b6394b5643394bc63d4ac691c098c99c40 Mon Sep 17 00:00:00 2001
From: yfu <yanikfuchs@xxxxxx>
Date: Tue, 22 Oct 2024 18:23:18 +0200
Subject: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low

Before this patch, tcpm went into SOFT_RESET state, if Vbus was never low
resulting in Hard_Reset, if power supply does not support USB_PD Soft_Reset.

In order to prevent this, I remove the Vbus check completely, so that
we go as well into the SNK_WAIT_CAPABILITIES_TIMEOUT state. There, we send
PD_CTRL_GET_SOURCE_CAP which many power supply do support.
(122968f8dda8 usb: typec: tcpm: avoid resets for missing source capability messages)

Please refer to https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@xxxxxxxxxx/ as
122968f8dda8 is causing USB Type-C PD compliance failures.


Additionally, I added SOFT_RESET (instead of Hard_Reset) as Fallback solution
if we still not have gotten any capabilities. Hard_Reset is now only done,
if PD_CTRL_GET_SOURCE_CAP and SOFT_RESET fail to get capabilities.
---
drivers/usb/typec/tcpm/tcpm.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index fc619478200f..c7a59c9f78ee 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -5038,14 +5038,8 @@ static void run_state_machine(struct tcpm_port *port)
* were already in a stable contract before this boot.
* Do this only once.
*/
- if (port->vbus_never_low) {
- port->vbus_never_low = false;
- tcpm_set_state(port, SNK_SOFT_RESET,
+ tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
PD_T_SINK_WAIT_CAP);
- } else {
- tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
- PD_T_SINK_WAIT_CAP);
- }

Instead of deleting code, please restrict this behavior to non self powered battery case as this most likely break compliance and

may break actual use-cases for other users as a result. If you want you can move stuff around after

https://lore.kernel.org/all/20241024022233.3276995-1-amitsd@xxxxxxxxxx/

gets accepted in the following way:

```

if (!port->self_powered) {

        tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT, PD_T_SINK_WAIT_CAP);

        break;

}


if (port->vbus_never_low) {

        tcpm_set_state(port, SNK_SOFT_RESET, PD_T_SINK_WAIT_CAP);

} else {

        tcpm_set_state(port, hard_reset_state(..), PD_T_SINK_WAIT_CAP);

}

```

This way you don't have to execute the SNK_SOFT_RESET flow for non self powered use-case.

Thanks,

Amit

break;
case SNK_WAIT_CAPABILITIES_TIMEOUT:
/*
@@ -5064,7 +5058,7 @@ static void run_state_machine(struct tcpm_port *port)
* according to the specification.
*/
if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
- tcpm_set_state_cond(port, hard_reset_state(port), 0);
+ tcpm_set_state_cond(port, SNK_SOFT_RESET, 0);
else
tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
break;