linux-kernel - Re: [PATCH] usb: typec: fusb302: cache PD RX state

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aH4MEsYX43afRO79@kuha.fi.intel.com>
Date: Mon, 21 Jul 2025 12:44:50 +0300
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Guenter Roeck <linux@...ck-us.net>, Yueyao Zhu <yueyao@...gle.com>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel@...labora.com, stable@...r.kernel.org
Subject: Re: [PATCH] usb: typec: fusb302: cache PD RX state

On Fri, Jul 04, 2025 at 07:55:06PM +0200, Sebastian Reichel wrote:
> This patch fixes a race condition communication error, which ends up in
> PD hard resets when losing the race. Some systems, like the Radxa ROCK
> 5B are powered through USB-C without any backup power source and use a
> FUSB302 chip to do the PD negotiation. This means it is quite important
> to avoid hard resets, since that effectively kills the system's
> power-supply.
> 
> I've found the following race condition while debugging unplanned power
> loss during booting the board every now and then:
> 
> 1. lots of TCPM/FUSB302/PD initialization stuff
> 2. TCPM ends up in SNK_WAIT_CAPABILITIES (tcpm_set_pd_rx is enabled here)
> 3. the remote PD source does not send anything, so TCPM does a SOFT RESET
> 4. TCPM ends up in SNK_WAIT_CAPABILITIES for the second time
>    (tcpm_set_pd_rx is enabled again, even though it is still on)
> 
> At this point I've seen broken CRC good messages being send by the
> FUSB302 with a logic analyzer sniffing the CC lines. Also it looks like
> messages are being lost and things generally going haywire with one of
> the two sides doing a hard reset once a broken CRC good message was send
> to the bus.
> 
> I think the system is running into a race condition, that the FIFOs are
> being cleared and/or the automatic good CRC message generation flag is
> being updated while a message is already arriving.
> 
> Let's avoid this by caching the PD RX enabled state, as we have already
> processed anything in the FIFOs and are in a good state. As a side
> effect that this also optimizes I2C bus usage :)
> 
> As far as I can tell the problem theoretically also exists when TCPM
> enters SNK_WAIT_CAPABILITIES the first time, but I believe this is less
> critical for the following reason:
> 
> On devices like the ROCK 5B, which are powered through a TCPM backed
> USB-C port, the bootloader must have done some prior PD communication
> (initial communication must happen within 5 seconds after plugging the
> USB-C plug). This means the first time the kernel TCPM state machine
> reaches SNK_WAIT_CAPABILITIES, the remote side is not sending messages
> actively. On other devices a hard reset simply adds some extra delay and
> things should be good afterwards.
> 
> Fixes: c034a43e72dda ("staging: typec: Fairchild FUSB302 Type-c chip driver")
> Cc: stable@...r.kernel.org
> Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>

> ---
>  drivers/usb/typec/tcpm/fusb302.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index f15c63d3a8f441569ec98302f5b241430d8e4547..870a71f953f6cd8dfc618caea56f72782e40ee1c 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -104,6 +104,7 @@ struct fusb302_chip {
>  	bool vconn_on;
>  	bool vbus_on;
>  	bool charge_on;
> +	bool pd_rx_on;
>  	bool vbus_present;
>  	enum typec_cc_polarity cc_polarity;
>  	enum typec_cc_status cc1;
> @@ -841,6 +842,11 @@ static int tcpm_set_pd_rx(struct tcpc_dev *dev, bool on)
>  	int ret = 0;
>  
>  	mutex_lock(&chip->lock);
> +	if (chip->pd_rx_on == on) {
> +		fusb302_log(chip, "pd is already %s", str_on_off(on));
> +		goto done;
> +	}
> +
>  	ret = fusb302_pd_rx_flush(chip);
>  	if (ret < 0) {
>  		fusb302_log(chip, "cannot flush pd rx buffer, ret=%d", ret);
> @@ -863,6 +869,8 @@ static int tcpm_set_pd_rx(struct tcpc_dev *dev, bool on)
>  			    str_on_off(on), ret);
>  		goto done;
>  	}
> +
> +	chip->pd_rx_on = on;
>  	fusb302_log(chip, "pd := %s", str_on_off(on));
>  done:
>  	mutex_unlock(&chip->lock);
> 
> ---
> base-commit: c435a4f487e8c6a3b23dafbda87d971d4fd14e0b
> change-id: 20250704-fusb302-race-condition-fix-9cc9de73f05d
> 
> Best regards,
> -- 
> Sebastian Reichel <sre@...nel.org>

-- 
heikki