Re: [PATCH v14 31/38] usb: dwc3: rockchip: support PHY reset notifications

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The PHY notification handler unconditionally drops a PM usage reference if `pm_runtime_get_if_active()` returns `-EINVAL`, causing a usage counter underflow and potentially dropping references owned by other parts of the driver.
- [High] The PHY notifier unregistration is deferred to the devres teardown phase, creating a race condition where an asynchronous PHY reset event accesses unclocked and reset DWC3 hardware, causing a kernel panic.
--

--- Patch [31]: [PATCH v14 31/38] usb: dwc3: rockchip: support PHY reset notifications ---
commit f458b4c213899f50f1d40c52f7c5aaeb38fdb675
Author: Sebastian Reichel <[email protected]>

usb: dwc3: rockchip: support PHY reset notifications

On recent Rockchip platforms (at least RK3588 & RK3576), DWC3 IP is used
with a USBDP PHY providing USB3 and DP. This PHY needs to be reset when
the mode changes, which may happen when plugging in different USB-C
devices. This adds handling for the new PHY framework reset notifications.

> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 1df33625b69f8..9e91e5f7e53d6 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
[ ... ]
> +static int dwc3_usb3_phy_notify(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
[ ... ]
> +	switch (action) {
> +	case PHY_NOTIFY_PRE_RESET:
> +		/*
> +		 * If already suspended, the resume path will reinit GUSB3PIPECTL
> +		 * via dwc3_core_init(). A forced resume is not possible as that
> +		 * would call phy_init() resulting in a deadlock. Due to the
> +		 * phy_init() in the resume path there is also no need to block
> +		 * async RPM resume on our side, since the PHY synchronizes it
> +		 * for us.
> +		 *
> +		 * pm_runtime_get_if_active() returns 0 when suspended (skip),
> +		 * 1 when active (ref held), or -EINVAL when PM is disabled
> +		 * (device always active). In the -EINVAL case PM ref counting
> +		 * is a no-op, so the unconditional put in POST_RESET is safe.
> +		 */
> +		ret = pm_runtime_get_if_active(dwc->dev);
> +		if (!ret)
> +			return NOTIFY_OK;

[Severity: High]
Does this code risk a PM usage counter underflow if PM is disabled?

When pm_runtime_get_if_active() returns -EINVAL, the !ret check evaluates
to false, allowing execution to proceed without incrementing the usage counter.

[ ... ]
> +	case PHY_NOTIFY_POST_RESET:
[ ... ]
> +		/*
> +		 * Deassert PHY soft reset to reconnect the PIPE interface
> +		 * after PHY reinitialization.
> +		 */
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> +		reg &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +		pm_runtime_put_autosuspend(dwc->dev);

[Severity: High]
Is it safe to unconditionally call pm_runtime_put_autosuspend() here?

The comment in PHY_NOTIFY_PRE_RESET states that PM ref counting is a no-op
in the -EINVAL case, but pm_runtime_put_autosuspend() will decrement the
usage counter regardless. This could cause an unbalanced put and drop
references owned by other parts of the driver.

> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static void dwc3_rk_phy_unregister_notifiers(void *data)
> +{
> +	struct dwc3_rockchip *dwc_rk = data;
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	int i;
> +
> +	for (i = 0; i < dwc->num_usb3_ports; i++)
> +		phy_unregister_notifier(dwc->usb3_generic_phy[i],
> +					&dwc_rk->usb3_phy_nb[i].nb);
> +
> +	/* Release any PM references from in-flight resets */
> +	for (i = 0; i < dwc->num_usb3_ports; i++) {
> +		if (dwc_rk->phy_reset_active & BIT(i))
> +			pm_runtime_put_autosuspend(dwc->dev);

[Severity: High]
Can this also cause a PM usage counter underflow?

If an in-flight reset was initiated while pm_runtime_get_if_active()
returned -EINVAL, we didn't acquire a reference. Calling
pm_runtime_put_autosuspend() here would result in the same unbalanced put.

> +	}
> +	dwc_rk->phy_reset_active = 0;
> +}
> +
> +static int dwc3_rk_phy_register_notifiers(struct dwc3 *dwc)
> +{
> +	struct dwc3_rockchip *dwc_rk = container_of(dwc, struct dwc3_rockchip, dwc);
> +	int i;
> +
> +	for (i = 0; i < dwc->num_usb3_ports; i++) {
> +		dwc_rk->usb3_phy_nb[i].nb.notifier_call = dwc3_usb3_phy_notify;
> +		dwc_rk->usb3_phy_nb[i].dwc_rk = dwc_rk;
> +		dwc_rk->usb3_phy_nb[i].port_index = i;
> +		phy_register_notifier(dwc->usb3_generic_phy[i],
> +				      &dwc_rk->usb3_phy_nb[i].nb);
> +	}
> +
> +	return devm_add_action_or_reset(dwc->dev, dwc3_rk_phy_unregister_notifiers, dwc_rk);

[Severity: High]
Can deferring notifier unregistration to the devres phase cause a kernel
panic during driver unbind?

When the driver is unbound, the DWC3 core is typically shut down (clocks
disabled and hardware reset asserted) before the devres teardown phase runs.

If an external PHY reset event arrives in this window, dwc3_usb3_phy_notify()
would execute and attempt to read DWC3_GUSB3PIPECTL from unclocked and reset
hardware, potentially causing a Synchronous External Abort.

> +}

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

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