Re: [PATCH 2/2] phy: nuvoton: Add MA35D1 USB2 OTG PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver implements a dummy USB role switch that falsely reports success without modifying hardware state.
- [High] Race condition exposing the PHY provider before initialization completes leads to a Use-After-Free.
- [High] PHY initialization fails due to polling clock stability bits while the PHY clock is disabled.
- [Medium] Ignoring the return value of `regmap_read()` results in the use of uninitialized stack memory.
--

commit 9e57cc2572b36a8f75f7ca9bc25a14c367c18b1a
Author: Joey Lu <a0987203069@gmail.com>
Subject: phy: nuvoton: Add MA35D1 USB2 OTG PHY driver

Add a PHY driver for the USB 2.0 PHYs in the Nuvoton MA35D1 SoC. It 
implements Power-On Reset sequence, optional resistor calibration trim, 
and a USB role switch.

> diff --git a/drivers/phy/nuvoton/phy-ma35d1-otg.c b/drivers/phy/nuvoton/phy-ma35d1-otg.c
> new file mode 100644
> index 0000000000000..53bc6ddf755e6
> --- /dev/null
> +++ b/drivers/phy/nuvoton/phy-ma35d1-otg.c
[ ... ]
> +static int ma35_otg_phy_init(struct phy *phy)
> +{
> +	struct ma35_otg_phy *p = phy_get_drvdata(phy);
> +	unsigned int n = p->phy_idx;
> +	u32 ready_mask = USBPMISCR_PHY_HOST_READY(n);
> +	unsigned int val;
> +	int ret;
> +
> +	regmap_read(p->sysreg, MA35_SYS_USBPMISCR, &val);
> +	if ((val & ready_mask) == ready_mask)
> +		return 0;

[Severity: Medium]
Does ignoring the return value of regmap_read() risk evaluating uninitialized
stack memory here?

If the read fails, val remains uninitialized, and using it in the bitwise
operation could cause non-deterministic behavior, such as incorrectly skipping
the PHY power-on reset sequence.

> +
> +	regmap_update_bits(p->sysreg, MA35_SYS_USBPMISCR,
> +			   USBPMISCR_PHY_CTL_MASK(n),
> +			   USBPMISCR_PHY_POR(n) | USBPMISCR_PHY_SUSPEND(n));
> +	msleep(20);
> +
> +	regmap_update_bits(p->sysreg, MA35_SYS_USBPMISCR,
> +			   USBPMISCR_PHY_CTL_MASK(n),
> +			   USBPMISCR_PHY_SUSPEND(n));
> +
> +	ret = regmap_read_poll_timeout(p->sysreg, MA35_SYS_USBPMISCR, val,
> +				       (val & ready_mask) == ready_mask,
> +				       10, 1000);

[Severity: High]
Is it possible for this polling to time out because the PHY clock isn't
enabled yet?

The polling expects the PHY clock to be stable, but the PHY clock (p->clk) is
only enabled later in ma35_otg_phy_power_on(). Because the Linux PHY framework
guarantees that phy_init() is called before phy_power_on(), the PHY reference
clock remains disabled during this stability polling.

> +	if (ret) {
> +		dev_err(p->dev, "USB PHY%u clock not stable (USBPMISCR=0x%08x)\n",
> +			n, val);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static enum usb_role ma35_otg_read_id(struct ma35_otg_phy *p)
> +{
> +	unsigned int val;
> +
> +	regmap_read(p->sysreg, MA35_SYS_PWRONOTP, &val);
> +	return (val & PWRONOTP_USBP0ID) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> +}

[Severity: Medium]
Can this return an arbitrary USB role if regmap_read() fails?

If the read fails, the uninitialized stack variable val is evaluated, leading
to a potentially incorrect role being reported.

> +
> +static int ma35_otg_role_sw_set(struct usb_role_switch *sw,
> +				enum usb_role role)
> +{
> +	struct ma35_otg_phy *p = usb_role_switch_get_drvdata(sw);
> +
> +	p->cur_role = role;
> +
> +	return 0;
> +}

[Severity: High]
Since the hardware mux is entirely automatic based on the physical USB ID pin,
should this .set callback return an error instead of falsely reporting success?

Writing to p->cur_role doesn't configure any hardware register, and the .get
callback correctly queries the hardware directly. Returning 0 here defeats the
subsystem's -EOPNOTSUPP rejection for read-only switches, as writes from
userspace will appear to succeed but will not change the actual hardware state.

> +
> +static enum usb_role ma35_otg_role_sw_get(struct usb_role_switch *sw)
> +{
> +	struct ma35_otg_phy *p = usb_role_switch_get_drvdata(sw);
> +
> +	return ma35_otg_read_id(p);
> +}
> +
> +static int ma35_otg_role_switch_init(struct platform_device *pdev,
> +				     struct ma35_otg_phy *p)
> +{
> +	struct usb_role_switch_desc sw_desc = { };
> +
> +	p->cur_role = ma35_otg_read_id(p);
> +
> +	sw_desc.set = ma35_otg_role_sw_set;
> +	sw_desc.get = ma35_otg_role_sw_get;
> +	sw_desc.allow_userspace_control = true;

[Severity: High]
Is it appropriate to set allow_userspace_control to true given that the switch
strictly reflects an automatic hardware state?

> +	sw_desc.driver_data = p;
> +	sw_desc.fwnode = dev_fwnode(&pdev->dev);
[ ... ]
> +static int ma35_otg_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	provider = devm_of_phy_provider_register(&pdev->dev,
> +						 of_phy_simple_xlate);
> +	if (IS_ERR(provider))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(provider),
> +				     "Failed to register PHY provider\n");
> +
> +	if (p->phy_idx == 0) {
> +		ret = ma35_otg_role_switch_init(pdev, p);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Could registering the PHY provider before role switch initialization risk a
use-after-free?

If devm_of_phy_provider_register() exposes the PHY globally, a consumer on
another CPU could look up the PHY and call phy_init(), acquiring a reference.

If ma35_otg_role_switch_init() subsequently fails, probe returns an error,
triggering the devm cleanup which frees the struct ma35_otg_phy *p memory.
Because the consumer already holds a valid reference, subsequent calls to
PHY APIs will dereference the freed pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604101220.1092822-1-a0987203069@gmail.com?part=2

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