Re: [PATCH 4/5] usb: dwc3: rockchip: support PHY reset notifications

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: dwc3: rockchip: mishandled -EINVAL causes PM usage count underflow
- [High] usb: dwc3: rockchip: devres registration of PHY notifier causes use-after-teardown
- [Medium] usb: dwc3: rockchip: async PM put during device removal violates PM guidelines
--

commit 6d282938d61239be811da28f2efdb481215351ef
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

usb: dwc3: rockchip: support PHY reset notifications

This commit adds handling for PHY reset notifications in the Rockchip DWC3
driver. It asserts a soft reset before the external PHY resets to prevent
internal state corruption. Once the PHY returns, the soft reset is
deasserted.

> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 62f2a03b08a28..7bdd6e2eb22d1 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)
> +{
> +	struct dwc3_rk_phy_nb *pnb = container_of(nb, struct dwc3_rk_phy_nb, nb);
> +	struct dwc3_rockchip *dwc_rk = pnb->dwc_rk;
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	int port = pnb->port_index;
> +	unsigned long flags;
> +	u32 reg;
> +	int ret;
> +
> +	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 check handle the -EINVAL return value correctly?

When runtime PM is disabled (e.g., during driver teardown, system suspend,
or if CONFIG_PM is disabled), pm_runtime_get_if_active() returns -EINVAL.
Since -EINVAL is non-zero, this check fails and the driver proceeds, falsely
assuming it acquired a PM reference.

In PHY_NOTIFY_POST_RESET, the driver unconditionally calls
pm_runtime_put_autosuspend(). If the RPM_GET_PUT flag is passed, the usage
count is dropped immediately via rpm_drop_usage_count() regardless of the PM
disabled state. Since the usage count was never incremented, could this
cause a usage count underflow warning and potentially corrupt the refcount
if racing with another PM operation?

> +
> +		/*
> +		 * Assert USB3 PHY soft reset within DWC3 before the external
> +		 * PHY resets. This disconnects the PIPE interface, preventing
> +		 * the DWC3 from interfering with PHY reinitialization and
> +		 * avoiding LCPLL lock failures.
> +		 */

[ ... ]

> +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: Medium]
Is it safe to use an asynchronous PM put during device removal here?

When the driver is unbound, devres cleanup executes this function. If there
is an in-flight PHY reset, it attempts to release the PM reference using
pm_runtime_put_autosuspend().

The PM subsystem guidelines explicitly state that asynchronous puts must not
be used when device removal follows immediately, as the pending async
idle/suspend work can be cancelled by pm_runtime_disable(). In this case,
pm_runtime_disable() has already been called during dwc3_core_remove(), so
does this async put merely drop the usage count and completely fail to
queue the suspend work, leaving the device powered on?

> +	}
> +	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]
Could this devres registration cause a use-after-teardown if a PHY reset
notification arrives asynchronously?

Devres cleanup actions execute after the driver's .remove() callback
completes. Here, dwc3_rockchip_remove() calls dwc3_core_remove(), which
tears down the hardware, disables clocks, and disables PM.

If a PHY reset notification arrives after .remove() completes but before
this devres action runs, dwc3_usb3_phy_notify() will execute. Due to the
mishandled -EINVAL from pm_runtime_get_if_active(), the code may attempt
to read DWC3 registers with dwc3_readl().

Does accessing these hardware registers with clocks disabled lead to a
synchronous external abort? Should the notifiers be unregistered
synchronously before hardware teardown begins instead?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-b4-rockchip-dwc3-rockchip-glue-v1-0-763bb546824e@collabora.com?part=4

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