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

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

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.

[ ... ]

+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