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

New issues:
- [High] Missing hardware state update for the USB3 TCA switch when Type-C orientation changes.
- [High] google_usb3_phy_power_on() times out on subsequent calls due to cleared Power-on-Reset interrupt status.
- [High] Asymmetric hardware teardown sequence in google_usb3_phy_exit().
- [Medium] Mixing scope-based cleanup (guard) with goto-based error handling in google_usb3_phy_init().
- [Medium] Missing cleanup of pmgt_ref_clk_req_n hardware state on error paths in google_usb3_phy_init().
- [Medium] The usb3_core memory region is mapped as a mandatory resource but is never used.

Pre-existing issues:
- [High] TOCTOU race with pm_runtime_suspended() leading to unsafe MMIO access.
--

commit ab00032aabbed532e8e0ec1ad61b86eb2f271b6d
Author: RD Babiera <rdbabiera@google.com>

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

This commit adds USB3 PHY support to the Google Tensor G5 USB PHY driver.
It introduces functionality for the usb3_core and usb3_tca registers, clocks,
and resets, and implements the USB3 phy_ops for init, exit, and power_on.

> 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
[ ... ]
> @@ -65,15 +135,79 @@ static void set_vbus_valid(struct google_usb_phy *gphy)
[ ... ]
> +static int program_tca_locked(struct google_usb_phy *gphy)
> +	   __must_hold(&gphy->phy_mutex)
> +{
[ ... ]
> +}
>  
>  static int google_usb_set_orientation(struct typec_switch_dev *sw,

[Severity: High]
Does this missing update cause USB3 signal routing failures on orientation
changes?

When the Type-C orientation changes, google_usb_set_orientation() updates
the software state and calls set_vbus_valid(gphy). While this was sufficient
for USB2, it doesn't seem to reprogram the newly added USB3 TCA hardware
multiplexer.

If the orientation flips while the USB3 PHY is active, will the hardware
state miss the update because program_tca_locked() is not invoked, leaving
the TCA_TCPC_CONNECTOR_ORIENTATION bit stale?

[Severity: High]
This is a pre-existing issue, but does the lockless check of
pm_runtime_suspended() create a TOCTOU race condition?

In google_usb_set_orientation():

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

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

    set_vbus_valid(gphy);

Because no PM runtime reference is acquired (like pm_runtime_get_sync()),
can the device be suspended by the PM core immediately after the check
returns false? This would cause the subsequent register access in
set_vbus_valid() to trigger a synchronous external abort on suspended
hardware.

> @@ -161,6 +295,103 @@ static const struct phy_ops google_usb2_phy_ops = {
[ ... ]
> +static int google_usb3_phy_init(struct phy *_phy)
> +{
[ ... ]
> +	guard(mutex)(&gphy->phy_mutex);
[ ... ]
> +	set_pmgt_ref_clk_req_n(gphy, true);
> +
> +	ret = clk_bulk_prepare_enable(inst->num_clks, inst->clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_bulk_deassert(inst->num_rsts, inst->rsts);
> +	if (ret)
> +		goto disable_clocks;
> +
> +	ret = readl_poll_timeout(gphy->usb3_tca_base + TCA_PSTATE_0_OFFSET,
> +				 reg, !(reg & TCA_PSTATE_0_UPCS_LANE0_PHYSTATUS),
> +				 GPHY_TCA_DELAY_US, GPHY_TCA_TIMEOUT_US);
> +	if (ret) {
> +		dev_err(gphy->dev, "wait for lane0 phystatus timed out");
> +		goto assert_resets;
> +	}
> +
> +	return 0;
> +
> +assert_resets:
> +	reset_control_bulk_assert(inst->num_rsts, inst->rsts);
> +disable_clocks:
> +	clk_bulk_disable_unprepare(inst->num_clks, inst->clks);
> +	return ret;
> +}

[Severity: Medium]
Is it safe to mix goto-based error handling with scope-based cleanup here?

According to the cleanup subsystem documentation, usage of goto and cleanup
helpers like guard(mutex) should never be mixed in the same function. Can
this create confusing execution order and resource ownership?

[Severity: Medium]
Are we missing a step to revert the hardware clock request on the error path?

Earlier in this function, the reference clock request is enabled via
set_pmgt_ref_clk_req_n(). If clk_bulk_prepare_enable() or
reset_control_bulk_deassert() fail, the error paths disable the clocks
and assert resets, but they don't seem to call set_pmgt_ref_clk_req_n()
to disable the clock request. Will this leave the hardware clock request
permanently asserted on failure?

> +static int google_usb3_phy_exit(struct phy *_phy)
> +{
[ ... ]
> +	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);
> +
> +	return 0;
> +}

[Severity: High]
Is this hardware teardown sequence in the correct order?

In google_usb3_phy_init(), the sequence is:
1) Request reference clock
2) Enable bus clocks
3) Deassert resets

Here in the exit function, the reference clock request is released first,
then resets are asserted, and bus clocks are disabled last. Could releasing
the reference clock while the PHY is still active and out of reset lead to
bus faults and system lockups?

> +static int google_usb3_phy_power_on(struct phy *_phy)
> +{
[ ... ]
> +	ret = wait_tca_xa_ack(gphy);
> +	if (ret) {
> +		dev_err(gphy->dev, "PoR->NC transition timeout");
> +		return ret;
> +	}
> +
> +	ret = program_tca_locked(gphy);
> +
> +	return ret;
> +}

[Severity: High]
Will this unconditionally time out on subsequent power-on calls?

This function waits for a PoR->NC transition by checking the
TCA_INTR_STS_XA_ACT_EVT bit. However, this interrupt bit is explicitly
cleared at the end of program_tca_locked().

If the PHY framework calls phy_power_on() sequentially (for example,
during PM runtime resume) without a preceding phy_init() to trigger a
physical reset, the bit will remain 0. Does this cause wait_tca_xa_ack()
to inevitably time out after 2.5 seconds?

> @@ -212,39 +490,45 @@ static int google_usb_phy_probe(struct platform_device *pdev)
[ ... ]
> +	gphy->usb3_core_base = devm_platform_ioremap_resource_byname(pdev,
> +								     "usb3_core");
> +	if (IS_ERR(gphy->usb3_core_base))
> +		return dev_err_probe(dev, PTR_ERR(gphy->usb3_core_base),
> +				    "invalid usb3 core\n");
> +

[Severity: Medium]
Is the usb3_core memory region actually used anywhere in the driver?

This maps the usb3_core resource and aborts the probe if it is missing,
but the resulting gphy->usb3_core_base pointer doesn't appear to be
referenced later. Could this introduce an unnecessary strict dependency
that breaks the probe on older Device Trees lacking this resource?

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

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