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 [off-list ref]

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.

[ ... ]

[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;

[ ... ]

[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