Re: [PATCH v2 4/4] phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The custom `ma35_usb_phy_xlate` callback accesses `args->args[0]` without verifying `args->args_count`, leading to a potential read of uninitialized stack memory.
- [High] The driver breaks Device Tree backward compatibility by unconditionally relying on the parent's `of_node` for the syscon regmap, removing the legacy 'nuvoton,sys' phandle fallback.
- [High] The driver fails to explicitly clear the 'MISCFCR0_UHOVRCURH' bit if the 'nuvoton,oc-active-high' property is absent, leading to persistent incorrect hardware state across soft reboots.
- [High] The driver incorrectly skips PHY clock stabilization checks if the `SUSPEND` bit is already set, potentially leading to hardware hangs.
- [Medium] The variable 'val' in `ma35_role_sw_get` is used uninitialized if the `regmap_read` call fails.
--

commit ff3f6d83cfb95f48a64d4c533549a7465a41d150
Author: Joey Lu <a0987203069@gmail.com>

phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support

This commit extends the driver to support both PHY ports and integrates OTG
role switching. It introduces a custom xlate function, unifies the init
callback, removes redundant clock management, and registers a USB role switch.

> --- a/drivers/phy/nuvoton/phy-ma35d1-usb2.c
> +++ b/drivers/phy/nuvoton/phy-ma35d1-usb2.c
[ ... ]
> +static int ma35_usb_phy_init(struct phy *phy)
> +{
> +	struct ma35_phy_port *port = phy_get_drvdata(phy);
> +	struct ma35_usb_phy *p = container_of(port - port->idx,
> +					      struct ma35_usb_phy, port[0]);
> +	unsigned int n = port->idx;
> +	unsigned int val;
> +	int ret;
> +
> +	regmap_read(p->sysreg, MA35_SYS_USBPMISCR, &val);
> +
> +	if (val & USBPMISCR_PHY_SUSPEND(n))
> +		return 0;

[Severity: High]
Does returning early here skip necessary PHY clock stabilization checks?

The previous implementation specifically polled the clock stable bit
even when the suspend bit was already set. By returning immediately,
the USB controller might start accessing the PHY before its newly ungated
clock has fully stabilized, potentially resulting in bus hangs.

[ ... ]
> +static enum usb_role ma35_role_sw_get(struct usb_role_switch *sw)
> +{
> +	struct ma35_usb_phy *p = usb_role_switch_get_drvdata(sw);
> +	u32 val;
> +
> +	regmap_read(p->sysreg, MA35_SYS_PWRONOTP, &val);
> +
> +	return (val & PWRONOTP_USBP0ID) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> +}

[Severity: Medium]
Is it possible for val to be evaluated uninitialized here?

If regmap_read() encounters an error, it returns a non-zero error code
and leaves val unmodified. This could cause the function to return an
incorrect USB role based on stack garbage.

[ ... ]
> +static struct phy *ma35_usb_phy_xlate(struct device *dev,
> +				      const struct of_phandle_args *args)
> +{
> +	struct ma35_usb_phy *p = dev_get_drvdata(dev);
> +
> +	if (args->args[0] >= MA35_PHY_NUM)
> +		return ERR_PTR(-EINVAL);

[Severity: High]
Does this code safely handle device trees where the number of phy cells is zero?

If args_count is zero, args->args would be uninitialized stack memory,
which could result in returning an incorrect PHY port or out-of-bounds
access. Should there be a check for args->args_count >= 1?

[ ... ]
> +static int ma35_usb_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	p->sysreg = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(p->sysreg))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(p->sysreg),
> +				     "failed to get parent SYS regmap\n");

[Severity: High]
Does this break Device Tree backward compatibility?

The old implementation relied on a "nuvoton,sys" phandle fallback which has
been removed here. Systems using older device tree binaries might fail to
probe the USB PHY on kernel upgrades if the node isn't a direct child of
the syscon.

[ ... ]
> +	if (of_property_read_bool(pdev->dev.of_node, "nuvoton,oc-active-high"))
> +		regmap_update_bits(p->sysreg, MA35_SYS_MISCFCR0,
> +				   MISCFCR0_UHOVRCURH, MISCFCR0_UHOVRCURH);

[Severity: High]
If the "nuvoton,oc-active-high" property is absent, should this code explicitly
clear the MISCFCR0_UHOVRCURH bit?

Without an else branch to clear the bit, a soft reboot could incorrectly
inherit an active-high state set by a bootloader or previous kernel.

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