Re: [PATCH v14 33/38] phy: rockchip: usbdp: Add phy reset notification support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unbalanced PM runtime reference decrement in the DWC3 PHY reset notifier due to mishandling of `-EINVAL`.
- [High] The PHY reset notification handler in the DWC3 Rockchip driver accesses `dwc->gadget` without adequate synchronization, leading to a potential Use-After-Free during a concurrent USB role switch.
- [Medium] The return value of `phy_notify_reset(..., PHY_NOTIFY_PRE_RESET)` is ignored in `rk_udphy_power_on()`, potentially forcing a hardware reset even if a consumer rejects the request.
--

--- Patch [33]: [PATCH v14 33/38] phy: rockchip: usbdp: Add phy reset notification support ---
commit 101376e3124c8975c12127c9a52db0a5300092c4
Author: Sebastian Reichel <[email protected]>
Subject: phy: rockchip: usbdp: Add phy reset notification support

To resolve issues with running into permanent cmn ana lcpll lock timeout
errors after a few device replugs, this commit adds support for reset
notifications. These notifications will be handled by the DWC3 driver to
gracefully handle the PHY being disabled. This avoids corrupting the
controller's internal state.

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 59b836a3796ce..21f282216aef7 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1005,24 +1005,39 @@ static int rk_udphy_power_on(struct rk_udphy *udphy, u8 mode)
>  	}
>  
>  	if (udphy->status == UDPHY_MODE_NONE) {
> +		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_PRE_RESET);
> +

[Severity: Medium]
Is it intentional to ignore the return value of phy_notify_reset() here?

In rk_udphy_update_power_state() the return value is checked and propagated,
but here the reset proceeds even if a consumer rejects the request. Can this
force a hardware reset when the consumer is unready, risking state corruption?


[Severity: High]
Does triggering this notification expose a PM runtime usage counter underflow
in the DWC3 driver?

If the PHY mode change triggers a PRE_RESET notification while runtime PM is
disabled for the device:

drivers/usb/dwc3/dwc3-rockchip.c:dwc3_usb3_phy_notify() {
    ...
    ret = pm_runtime_get_if_active(dwc->dev);
    if (!ret)
        return NOTIFY_OK;
    ...
}

The pm_runtime_get_if_active() call returns -EINVAL if PM is disabled. Because
this is non-zero, the driver bypasses the !ret check without incrementing the
usage count.

However, during POST_RESET, it unconditionally calls:

    pm_runtime_put_autosuspend(dwc->dev);

Will this internally force a decrement of the usage count and cause an
underflow?


[Severity: High]
Does triggering this notification also expose a potential use-after-free in the
DWC3 driver during a concurrent USB role switch?

When the POST_RESET notification is handled, dwc3_usb3_phy_notify() drops
the lock and calls dwc3_rockchip_vbus_handler():

drivers/usb/dwc3/dwc3-rockchip.c:dwc3_usb3_phy_notify() {
    ...
    spin_unlock_irqrestore(&dwc->lock, flags);
    dwc3_rockchip_vbus_handler(dwc, dwc_rk->role == USB_ROLE_DEVICE);
    ...
}

Inside dwc3_rockchip_vbus_handler():

    if (!dwc->gadget || !dwc->gadget_driver)
        return;

    usb_udc_vbus_handler(dwc->gadget, present);

Since this runs asynchronously, a concurrent USB role switch to host mode
could trigger dwc3_gadget_exit(), which unregisters and frees dwc->gadget.

Is there a time-of-check to time-of-use window between the NULL check and the
dereference of dwc->gadget?

> +		rk_udphy_u3_port_disable(udphy, true);
> +		udelay(10);
> +
>  		ret = rk_udphy_setup(udphy);
> -		if (ret)
> +		if (ret) {
> +			phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
>  			return ret;
> +		}
>  
>  		if (!udphy->hs && udphy->mode & UDPHY_MODE_USB)
>  			rk_udphy_u3_port_disable(udphy, false);
>  		udphy->phy_needs_reinit = false;
> +
> +		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
>  	} else if (udphy->phy_needs_reinit) {
> +		phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_PRE_RESET);
> +

[Severity: Medium]
Similarly here, the return value of phy_notify_reset() for PHY_NOTIFY_PRE_RESET
is ignored before proceeding with the hardware reset. Should it be checked?

> +		rk_udphy_u3_port_disable(udphy, true);
> +		udelay(10);
> +
>  		ret = rk_udphy_init(udphy);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=33

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy