Re: [PATCH v3] 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 3 potential issue(s) to consider:

New issues:
- [High] The teardown sequence in `google_usb3_phy_exit` violates LIFO ordering, causing a hardware resource sequence mismatch.
- [High] Unconditional TCA programming in `google_usb3_phy_power_on` misconfigures the PHY for a disconnected port.

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in `google_usb_set_orientation` leads to unclocked MMIO accesses.
--

commit cefbad787f1b94bc99ee246cf94e5430283fce0c
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.
It implements the USB3 phy_ops for phy_init, phy_exit, and phy_power_on,
and adds a combo_phy_state enum to track the PHY bringup state across
API calls.

diff --git a/drivers/phy/phy-google-usb.c b/drivers/phy/phy-google-usb.c
--- 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);

[Severity: High]
This is a pre-existing issue, but does checking pm_runtime_suspended()
earlier in this function without holding a PM reference introduce a race
condition here?

If the PM core suspends the device immediately after that check returns
false, the function will proceed to acquire the mutex and write to MMIO
registers via set_vbus_valid() and program_tca_locked() after the power
domain and clocks have been disabled.

Could this lead to a synchronous external abort when accessing the unclocked
registers? Should this use pm_runtime_get_if_active() instead?

> +	if (gphy->phy_state == COMBO_PHY_TCA_READY && orientation != TYPEC_ORIENTATION_NONE)
> +		return program_tca_locked(gphy);
> +
>  	return 0;
>  }

[ ... ]

> +static int google_usb3_phy_exit(struct phy *_phy)
> +{
> +	struct google_usb_phy_instance *inst = phy_get_drvdata(_phy);
> +	struct google_usb_phy *gphy = inst->parent;
> +
> +	dev_dbg(gphy->dev, "exiting usb3 phy\n");
> +
> +	guard(mutex)(&gphy->phy_mutex);
> +
> +	set_pmgt_ref_clk_req_n(gphy, false);
> +	reset_control_bulk_assert(inst->num_rsts, inst->rsts);
> +	clk_bulk_disable_unprepare(inst->num_clks, inst->clks);

[Severity: High]
Is the manual teardown sequence here in the correct order?

In google_usb3_phy_init(), resources are acquired in this order:
1) set_pmgt_ref_clk_req_n(true)
2) clock enable
3) reset deassert

The __free blocks in the init function correctly establish the error cleanup
sequence in reverse order (LIFO). However, here the upstream reference clock
request is disabled before placing the hardware block into reset and
disabling clocks.

Could disabling the clock request before asserting reset freeze the IP in an
invalid state and prevent subsequent wakeups?

> +
> +	gphy->phy_state = COMBO_PHY_IDLE;
> +
> +	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;
> +	}
> +
> +	return program_tca_locked(gphy);

[Severity: High]
Should this check if the Type-C port is actually connected before calling
program_tca_locked()?

If the port is disconnected (gphy->orientation == TYPEC_ORIENTATION_NONE),
program_tca_locked() falls through its else branch, configuring the registers
for TYPEC_ORIENTATION_NORMAL and setting TCA_TCPC_VALID.

Since the Type-C switch callback google_usb_set_orientation() correctly skips
TCA programming if the orientation is NONE, should a similar check be present
here to prevent enabling the SuperSpeed data path for a phantom connection?

> +}

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

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