Re: [PATCH v7 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] phy: Kconfig missing USB_ROLE_SWITCH dependency causes built-in link failure
- [High] phy: DT backward compatibility regression for over-current polarity
- [Medium] phy: ignored regmap_read return values cause uninitialized variable usage and info leak
- [Medium] phy: regression in PHY reset fallback when clock polling fails
- [Medium] PM/clock: PHY permanently holds HUSBH0_GATE preventing USB suspend
--

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

    phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support
    
    PHY0 and PHY1 use the same power-on/reset sequence in USBPMISCR, with
    PHY1 control bits shifted 16 positions relative to PHY0.  A separate
    driver for PHY1 would duplicate this logic, so the existing driver is
    extended to manage both ports.

> diff --git a/drivers/phy/nuvoton/phy-ma35d1-usb2.c b/drivers/phy/nuvoton/phy-ma35d1-usb2.c
> index 0b9976fb15c17..25bd0a7a1314b 100644
> --- a/drivers/phy/nuvoton/phy-ma35d1-usb2.c
> +++ b/drivers/phy/nuvoton/phy-ma35d1-usb2.c
[ ... ]
> -static int ma35_usb_phy_power_on(struct phy *phy)
> +static int ma35_usb_phy_init(struct phy *phy)
>  {
> -	struct ma35_usb_phy *p_phy = phy_get_drvdata(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;
>  
> -	ret = clk_prepare_enable(p_phy->clk);
> -	if (ret < 0) {
> -		dev_err(p_phy->dev, "Failed to enable PHY clock: %d\n", ret);
> -		return ret;
> +	regmap_read(p->sysreg, MA35_SYS_USBPMISCR, &val);
> +
> +	if (!(val & USBPMISCR_PHY_SUSPEND(n))) {

[Severity: Medium]
Is this code branching on uninitialized stack memory?

If regmap_read() fails, the return value is ignored and val is left
uninitialized, leading to non-deterministic PHY state handling.

[Severity: Medium]
Could this strict condition remove the fallback reset sequence?

The original driver would reset the PHY if the clock failed to stabilize during
polling. By encapsulating the reset strictly inside this block, it skips the
reset entirely if the PHY is already running.

If the subsequent clock polling times out, the driver immediately returns an
error. Can this cause a regression where the PHY fails initialization permanently
if it was left running but unstable by the bootloader or a prior kexec?

> +		regmap_update_bits(p->sysreg, MA35_SYS_USBPMISCR,
> +				   USBPMISCR_PHY_CTL_MASK(n),
> +				   USBPMISCR_PHY_POR(n) | USBPMISCR_PHY_SUSPEND(n));
> +		udelay(20);
> +
> +		regmap_update_bits(p->sysreg, MA35_SYS_USBPMISCR,
> +				   USBPMISCR_PHY_CTL_MASK(n),
> +				   USBPMISCR_PHY_SUSPEND(n));
>  	}
[ ... ]
> -static int ma35_usb_phy_power_off(struct phy *phy)
> -{
> -	struct ma35_usb_phy *p_phy = phy_get_drvdata(phy);
> +static const struct phy_ops ma35_usb_phy_ops = {
> +	.init		= ma35_usb_phy_init,
> +	.owner		= THIS_MODULE,
> +};

[Severity: Medium]
Will the removal of dynamic clock toggling in .power_on and .power_off cause
a power regression?

Since the clock is now acquired permanently via devm_clk_get_enabled() during
probe, the USB host clock gate (HUSBH0_GATE) remains open permanently,
preventing it from closing and consuming unnecessary power when the USB host
controller (EHCI0/OHCI0) suspends.

> +
> +static int ma35_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +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]
Can this leak uninitialized stack memory to userspace?

If regmap_read() fails, the return value is ignored and val remains
uninitialized. This uninitialized data determines the returned USB role,
inadvertently leaking 1 bit of kernel stack state to userspace via the sysfs
role attribute.

> +
> +static int ma35_role_switch_init(struct platform_device *pdev,
> +				 struct ma35_usb_phy *p)
> +{
> +	struct usb_role_switch_desc sw_desc = {0};
> +
> +	sw_desc.set = ma35_role_sw_set;
> +	sw_desc.get = ma35_role_sw_get;
> +	sw_desc.allow_userspace_control = true;
> +	sw_desc.driver_data = p;
> +	sw_desc.fwnode = dev_fwnode(&pdev->dev);
> +
> +	p->role_sw = usb_role_switch_register(&pdev->dev, &sw_desc);

[Severity: High]
Would this introduce a built-in link failure if the driver is built-in
(CONFIG_PHY_MA35_USB=y) and the role switch framework is modular
(CONFIG_USB_ROLE_SWITCH=m)?

In this configuration, IS_ENABLED(CONFIG_USB_ROLE_SWITCH) evaluates to true,
so include/linux/usb/role.h declares the external functions instead of providing
inline stubs, causing an undefined reference to usb_role_switch_register.

Should drivers/phy/nuvoton/Kconfig be updated to include
depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH for PHY_MA35_USB to prevent
this invalid Kconfig combination?

> +	if (IS_ERR(p->role_sw)) {
[ ... ]
>  static int ma35_usb_phy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *provider;
> -	struct ma35_usb_phy *p_phy;
> -	struct phy *phy;
> +	struct ma35_usb_phy *p;
> +	struct clk *clk;
> +	int n, ret;
> +	u32 code;
[ ... ]
> +	regmap_update_bits(p->sysreg, MA35_SYS_MISCFCR0, MISCFCR0_UHOVRCURH,
> +			   of_property_read_bool(pdev->dev.of_node,
> +						 "nuvoton,oc-active-high") ?
> +			   MISCFCR0_UHOVRCURH : 0);

[Severity: High]
Does this unconditional write break device tree backward compatibility?

If the optional property nuvoton,oc-active-high is absent, this explicitly
writes 0 to the bit, forcing the over-current polarity to active-low. The
original driver relied on the bootloader's configuration.

Will this cause a regression in over-current detection on existing boards that
use active-high hardware configured by the bootloader but lack this new
property in their older device tree?

> +
> +	for (n = 0; n < MA35_PHY_NUM; n++) {

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