| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<GVAP278MB066241A50CBD35448CB11DE7974E2@GVAP278MB0662.CHEP278.PROD.OUTLOOK.COM>
Date: Thu, 24 Oct 2024 08:54:03 +0000
From: Yanik Fuchs <Yanik.fuchs@....ch>
To: Amit Sunil Dhamne <amitsd@...gle.com>, "heikki.krogerus@...ux.intel.com"
<heikki.krogerus@...ux.intel.com>, "gregkh@...uxfoundation.org"
<gregkh@...uxfoundation.org>, "rdbabiera@...gle.com" <rdbabiera@...gle.com>,
"linux@...ck-us.net" <linux@...ck-us.net>, "kyletso@...gle.com"
<kyletso@...gle.com>, "badhri@...gle.com" <badhri@...gle.com>,
"xu.yang_2@....com" <xu.yang_2@....com>, "sebastian.reichel@...labora.com"
<sebastian.reichel@...labora.com>
CC: "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: AW: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never
low
Hi Amit
If the setting "bus-powered" is not set, there is only one way, that Vbus does not go down in init (where we force a Hard_Reset, which sets CC to Open if not bus-powerded). The case where Vbus never goes down and setting Bus-powered is not set is only with a legacy cable/adabter. In that case, we do not have PD compatibility anyway, and have to end up in SNK_READY eventually.
So we do the SOFT_RESET if Vbus was never down out of historical reasons and is not needed anymore since tcpm is sending Get_Capabilities (the only reason we send SOFT_RESET if Vbus was never down, is to get capabilities of Source, and that we still do, if Get_Capabilities does not work).
As you correctly have pointed out, I have to fix a issue I spotted yesterday as well, that with this Patch, after two hard_resets and then after 3 SoftReset we seem not to end up in SNK_READY, but in SNK_UNNCONECTED -> I have to find a way to check if we try hard reset in SOFT_RESET state after leaving the WAIT_SINK_CAPABILITIES_TIMOUT and in that case send it to SNK_CONNECTED (same as if we would be in state WAIT_SINK_CAPABILITIES or WAIT_SINK_CAPABILITIES_TIMOUT).
Maybe we have to count softresets after requesing capabilities and reset it as soon as we are in SNK_READY
BTW:
I have a patch which I use in the 5.15 version of the driver, where I can get ptn5110 to init correctly without forcing the Hardreset and the manual CC_Change trigger (I changed default state of sink to SNK_DEBOUNCED (where Vbus is checked and if not true sends us to SNK_UNCONNECTED) and added a delay of one second to be shure that IRQ is running when state machine starts)
Freundliche Grüsse
Best regards
Freundliche Grüsse
Best regards
Freundliche Grüsse
Best regards
________________________________________
Von: Amit Sunil Dhamne <amitsd@...gle.com>
Gesendet: Donnerstag, 24. Oktober 2024 05:13
An: Yanik Fuchs <Yanik.fuchs@....ch>; heikki.krogerus@...ux.intel.com <heikki.krogerus@...ux.intel.com>; gregkh@...uxfoundation.org <gregkh@...uxfoundation.org>; rdbabiera@...gle.com <rdbabiera@...gle.com>; linux@...ck-us.net <linux@...ck-us.net>; kyletso@...gle.com <kyletso@...gle.com>; badhri@...gle.com <badhri@...gle.com>; xu.yang_2@....com <xu.yang_2@....com>; sebastian.reichel@...labora.com <sebastian.reichel@...labora.com>
Cc: linux-usb@...r.kernel.org <linux-usb@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
Betreff: Re: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
ACHTUNG: Dieses E-Mail stammt von einem externen Absender. Prüfe Anhänge oder Links sehr genau!
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@...com>
> 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@google.com/ 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@google.com/
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;
Powered by blists - more mailing lists