Re: [PATCH v13 29/35] usb: dwc3: core: support PHY reset notifications

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unhandled `-EINVAL` from `pm_runtime_get_if_active()` leads to unsafe unclocked hardware access and guaranteed PM usage count underflow.
- [Medium] `PHY_NOTIFY_PRE_RESET` skips asserting `PHYSOFTRST` while DWC3 is runtime suspending, undermining the patch's purpose.
--

--- Patch [29]: [PATCH v13 29/35] usb: dwc3: core: support PHY reset notifications ---

commit 6e8dbcf8839d758e54f1776486d2e59f6acec9ca
Author: Sebastian Reichel [off-list ref]

usb: dwc3: core: support PHY reset notifications

This commit adds handling for PHY framework reset notifications to the DWC3
core. It asserts the PHYSOFTRST bit before the external PHY resets and
deasserts it afterward. This prevents the external USBDP PHY from entering
a corrupted state resulting in LCPLL lock failures.

[ ... ]

[Severity: High]
If pm_runtime_get_if_active() returns -EINVAL (e.g. when PM is disabled),
this check only looks for 0 and allows execution to proceed without having
incremented the usage count.

Since the device might not actually be active or clocked when PM is disabled,
could this lead to unclocked hardware access on DWC3_GUSB3PIPECTL and a kernel
panic?

Also, the comment mentions that PM ref counting is a no-op for -EINVAL,
making the unconditional put in POST_RESET safe. However,
pm_runtime_put_autosuspend() unconditionally decrements the atomic usage
counter via rpm_drop_usage_count(). Won't this cause a usage count underflow?

[Severity: Medium]
If the device state is RPM_SUSPENDING, pm_runtime_get_if_active() returns 0.
Could an external PHY reset occur concurrently while dwc3_suspend_common() is
executing but before it disables clocks?

If so, this returns NOTIFY_OK and bypasses PHYSOFTRST assertion while the
DWC3 IP is still actively driving the PIPE interface, potentially causing the
exact LCPLL lock failure this patch is meant to fix.

+
+		/*
+		 * 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.
+		 */
+		spin_lock_irqsave(&dwc->lock, flags);
+		dwc->phy_reset_active |= BIT(port);
+		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
+		reg |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
+		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
+		spin_unlock_irqrestore(&dwc->lock, flags);
+		break;
+
+	case PHY_NOTIFY_POST_RESET:
+		spin_lock_irqsave(&dwc->lock, flags);
+		if (!(dwc->phy_reset_active & BIT(port))) {
+			spin_unlock_irqrestore(&dwc->lock, flags);
+			return NOTIFY_OK;
+		}
+
+		dwc->phy_reset_active &= ~BIT(port);
+
+		/*
+		 * 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]
If pm_runtime_get_if_active() returned -EINVAL earlier, won't this
unconditional put drop the PM reference and lead to a usage count underflow?

+		break;
+	}
+
+	return NOTIFY_OK;
+}

[ ... ]

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

[Severity: High]
Similarly, if a reset is in-flight when PM is disabled (meaning no reference
was acquired), won't this also cause a usage count underflow?

+	}
+	dwc->phy_reset_active = 0;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com?part=29

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