Re: [PATCH v9 32/38] usb: dwc3: core: support PHY reset notifications

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The PHY reset notifier checks `pm_runtime_suspended()` without securing a PM reference, which can cause a kernel crash if the DWC3 controller suspends concurrently.
- [High] Unsynchronized modification of the `phy_reset_in_progress` bitfield causes a data race that can corrupt adjacent bitfields in `struct dwc3`, including PM state flags like `suspended`.
- [High] The PHY reset notifier incorrectly treats a per-port PHY reset as a global event, applying soft reset to all USB3 ports and using a single global state flag, breaking multi-port controllers.
- [High] The notifier performs read-modify-write operations on `DWC3_GUSB3PIPECTL` without acquiring the DWC3 spinlock, leading to register state corruption.
--

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

usb: dwc3: core: support PHY reset notifications

This adds handling for the new PHY framework reset notifications to assert
PHYSOFTRST within the DWC3 controller before the external PHY resets. This
prevents the DWC3 from interfering with PHY reinitialization and avoids
LCPLL lock failures on Rockchip platforms.

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
[ ... ]
> @@ -1430,6 +1430,7 @@ struct dwc3 {
>  	unsigned		wakeup_configured:1;
>  	unsigned		suspended:1;
>  	unsigned		susphy_state:1;
> +	unsigned		phy_reset_in_progress:1;

[Severity: High]
Can modifying this locklessly corrupt the adjacent bitfield flags?

Since phy_reset_in_progress shares a memory word with suspended and
susphy_state, a concurrent modification to those PM state flags could be
overwritten during the non-atomic read-modify-write of this bitfield.

>  
>  	u16			imod_interval;

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
[ ... ]
> @@ -886,10 +886,86 @@ static int dwc3_phy_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +static int dwc3_usb3_phy_notify(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct dwc3 *dwc = container_of(nb, struct dwc3_phy_nb, nb)->dwc;
> +	int i;
> +
> +	switch (action) {
> +	case PHY_NOTIFY_PRE_RESET:
> +		/*
> +		 * If the controller is already suspended (e.g. runtime PM),
> +		 * there is no internal state to clean up.
> +		 */
> +		if (pm_runtime_suspended(dwc->dev))
> +			return NOTIFY_OK;

[Severity: High]
Does this check protect against the device suspending immediately after?

If pm_runtime_suspended() returns false, the controller could still
asynchronously suspend before PHY_NOTIFY_POST_RESET occurs.

Should this acquire a reference via pm_runtime_get_if_active() to prevent
the device clocks from being gated while the reset is in progress?

> +
> +		dwc->phy_reset_in_progress = true;
> +
> +		/*
> +		 * 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.
> +		 */
> +		for (i = 0; i < dwc->num_usb3_ports; i++) {

[Severity: High]
Does this unintentionally disrupt all ports on a multi-port controller?

The notifier block identifies the specific dwc3_phy_nb (and implicitly the
port) that triggered the event, but this loops over all num_usb3_ports and
asserts soft reset on all of them.

Overlapping resets on different ports might also clobber the shared
dwc->phy_reset_in_progress flag, dropping connections on ports that did
not trigger the notification.

> +			u32 reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(i));
> +
> +			reg |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +			dwc3_writel(dwc, DWC3_GUSB3PIPECTL(i), reg);

[Severity: High]
Can this lockless read-modify-write corrupt DWC3_GUSB3PIPECTL?

This modifies the register without holding dwc->lock.

If this races with PM transitions running dwc3_enable_susphy(), which also
modifies this register, could one of the register updates be lost?

> +		}
> +		break;
> +
> +	case PHY_NOTIFY_POST_RESET:
> +		if (!dwc->phy_reset_in_progress)
> +			return NOTIFY_OK;
> +
> +		dwc->phy_reset_in_progress = false;
> +
> +		/*
> +		 * Deassert PHY soft reset and reconfigure the PIPE interface
> +		 * settings after PHY reinitialization.
> +		 */
> +		for (i = 0; i < dwc->num_usb3_ports; i++) {
> +			u32 reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(i));

[Severity: High]
If the device suspended asynchronously between PRE_RESET and POST_RESET,
could accessing this hardware register cause a synchronous external abort?

Since phy_reset_in_progress is true, this path executes unconditionally
even if the controller's clocks are now disabled.

> +
> +			reg &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +			dwc3_writel(dwc, DWC3_GUSB3PIPECTL(i), reg);
> +		}
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-rockchip-usbdp-cleanup-v9-0-e31efbb62d2e@collabora.com?part=32