Re: [PATCH v4] phy: Add USB3 PHY support to Google Tensor SoC USB PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Data race on `gphy->orientation` causes redundant hardware programming and a 2.5-second timeout.

Pre-existing issues:
- [High] TOCTOU race on `pm_runtime_suspended()` allows hardware access while suspended, causing a bus fault.
--

--- Patch [1]: [PATCH v4] phy: Add USB3 PHY support to Google Tensor SoC USB PHY driver ---
commit 1008e9b0470bdcef99e7408c0e43896673f90ba3
Author: RD Babiera <rdbabiera@google.com>

phy: Add USB3 PHY support to Google Tensor SoC USB PHY driver

This patch adds USB3 PHY support for the Google Tensor G5 USB PHY driver,
including the usb3_tca register, clock, and reset functionality. It also
refactors the probe sequence and implements USB3 phy_ops.

> diff --git a/drivers/phy/phy-google-usb.c b/drivers/phy/phy-google-usb.c
> index ab20bc20f19ea..19256588d50ec 100644
> --- a/drivers/phy/phy-google-usb.c
> +++ b/drivers/phy/phy-google-usb.c
[ ... ]
> @@ -92,6 +261,9 @@ static int google_usb_set_orientation(struct typec_switch_dev *sw,
>  
>  	set_vbus_valid(gphy);
>  
> +	if (gphy->phy_state == COMBO_PHY_TCA_READY && orientation != TYPEC_ORIENTATION_NONE)
> +		return program_tca_locked(gphy);
> +

[Severity: High]
This is a pre-existing issue, but does this function have a race condition
with power management that could cause a bus fault?

google_usb_set_orientation() checks pm_runtime_suspended(gphy->dev) without
holding a PM runtime reference. If the USB core releases its PM reference
immediately after this check, the device could suspend and power off the PHY.

The function would then acquire the lock and call set_vbus_valid() and
program_tca_locked() on unclocked and unpowered MMIO registers, resulting
in a synchronous external abort.

Would it be safer to use pm_runtime_get_sync() to ensure the hardware
remains active during these accesses?

>  	return 0;
>  }
[ ... ]
> +static int google_usb3_phy_power_on(struct phy *_phy)
> +{
> +	struct google_usb_phy_instance *inst = phy_get_drvdata(_phy);
> +	struct google_usb_phy *gphy = inst->parent;
> +	int ret;
> +
> +	dev_dbg(gphy->dev, "power on usb3 phy\n");
> +
> +	guard(mutex)(&gphy->phy_mutex);
> +
> +	if (gphy->phy_state != COMBO_PHY_TCA_READY) {
> +		/* Wait for PoR -> NC transitions*/
> +		ret = wait_tca_xa_ack(gphy);
> +		if (ret) {
> +			dev_err(gphy->dev, "PoR->NC transition timeout");
> +			return ret;
> +		}
> +		gphy->phy_state = COMBO_PHY_TCA_READY;
> +	}
> +
> +	if (gphy->orientation != TYPEC_ORIENTATION_NONE)
> +		return program_tca_locked(gphy);

[Severity: High]
Can this lead to redundant TCA programming and a 2.5-second timeout?

Looking at google_usb_set_orientation(), gphy->orientation is updated before
the mutex is acquired:

google_usb_set_orientation() {
        gphy->orientation = orientation;

        if (pm_runtime_suspended(gphy->dev))
                return 0;

        guard(mutex)(&gphy->phy_mutex);
        ...

If a Type-C plug event (Thread A) sets gphy->orientation but is preempted
before taking the lock, and the USB core concurrently calls
google_usb3_phy_power_on() (Thread B), Thread B will see the new
orientation, program the TCA, and release the lock.

When Thread A resumes, it will acquire the mutex, see COMBO_PHY_TCA_READY,
and call program_tca_locked() a second time with the identical configuration.
Since writing the same configuration to the TCA hardware doesn't trigger a
new XA_ACT_EVT interrupt, wait_tca_xa_ack() will stall for its full timeout.

Should the orientation update be moved inside the lock?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706231633.700676-2-rdbabiera@google.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy