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

[Joey Lu <a0987203069@gmail.com>]

On 10/8/2026 2:26 PM, sashiko-bot@kernel.org wrote:
> 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.
Leaving this as is.
>
> [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?
PHY reset fallback: valid, v7 dropped the old reset-on-unstable-clock
behaviour. In v8 I'll restore it: if SUSPEND is already set, poll the ready
bits first and return on success; on timeout (or if SUSPEND was clear) 
run the
POR assert / udelay(20) / POR deassert sequence, then poll again and 
fail only
if the second poll times out. This covers a PHY 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.
HUSBH0_GATE: valid. devm_clk_get_enabled() keeps the gate open for the
driver's lifetime. In v8 I'll use devm_clk_get() in probe and restore
.power_on/.power_off doing clk_prepare_enable()/clk_disable_unprepare(). 
Both
ports share the one clock; the PHY core calls these per port and the clk
framework refcounts, so the gate closes once neither port is powered. init
will also enable the clock around its register accesses and polling.
>> +
>> +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.
Leaving this as is.
>> +
>> +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?
Kconfig: valid, thanks. In v8 I'll add
"depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH" to PHY_MA35_USB in
drivers/phy/nuvoton/Kconfig. With the role switch disabled, the existing
-ENODEV handling in ma35_role_switch_init() still lets probe succeed.
>> +	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?
OC polarity: the unconditional write is intentional. The binding in
patch 1 defines "property absent" as active-low, and writing the bit in both
cases makes the result independent of what the bootloader left in MISCFCR0.
This bit only affects the EHCI/OHCI host ports' VBUS switch and is 
irrelevant
to the gadget (DWC2) use the old driver supported, so existing gadget-only
users are not affected. The old driver never touched MISCFCR0, and no 
mainline
DT uses this PHY with host ports, so no mainline board depends on a
bootloader-programmed polarity; nuvoton,oc-active-high is new in this 
series.
Out-of-tree boards with an active-high switch need to add the property. I'll
state this explicitly in the commit message and binding description.

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

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