Re: [PATCH v16 4/6] usb: dwc3: rockchip: support PHY reset notifications
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-24 17:38:35
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc3: rockchip: PM usage counter underflow due to unbalanced put
- [High] usb: dwc3: rockchip: kernel panic on unclocked register access during removal
--
commit d23961727db1cf3094808a06fa4f08ec3b13900c
Author: Sebastian Reichel [off-list ref]
usb: dwc3: rockchip: support PHY reset notifications
On recent Rockchip platforms (at least RK3588 & RK3576), DWC3 IP is used
with a USBDP PHY providing USB3 and DP. This PHY needs to be reset when
the mode changes, which may happen when plugging in different USB-C
devices.
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c index 4129aefd6a8b0..17b6c2ca9e192 100644 --- a/drivers/usb/dwc3/dwc3-rockchip.c +++ b/drivers/usb/dwc3/dwc3-rockchip.c
[ ... ]
quoted hunk ↗ jump to hunk
+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 error handling correctly account for -EINVAL? When PM is disabled (such as during driver removal), pm_runtime_get_if_active() returns -EINVAL. Since the if (!ret) check only evaluates to true for 0, the code falls through on -EINVAL without holding a PM reference.
quoted hunk ↗ jump to hunk
+ /* + * 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_rk->phy_reset_active |= BIT(port); + reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
[Severity: High] Can accessing hardware registers here cause a kernel panic during removal? Because dwc3_rk_phy_register_notifiers() uses devm_add_action_or_reset(), the notifier is unregistered during devres cleanup, which happens after dwc3_rockchip_remove() returns. The dwc3_rockchip_remove() path calls dwc3_core_remove(), which disables PM and shuts down hardware clocks via dwc3_core_exit(). If a PHY reset notification arrives during this window, pm_runtime_get_if_active() returns -EINVAL, the code falls through to this point, and we attempt to read registers via dwc3_readl() when clocks are disabled. Accessing unclocked registers on ARM/Rockchip architectures causes a synchronous external abort. Should the driver explicitly unregister the hardware-accessing PHY notifiers before tearing down the DWC3 core?
quoted hunk ↗ jump to hunk
+ 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_rk->phy_reset_active & BIT(port))) { + spin_unlock_irqrestore(&dwc->lock, flags); + return NOTIFY_OK; + } + + dwc_rk->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] Does this unconditional put cause a PM usage counter underflow if the initial get returned -EINVAL? As seen in __pm_runtime_suspend(), the usage counter is decremented via rpm_drop_usage_count() before checking the PM enable status. If the counter was > 0 (held by another component), dropping it prematurely could cause the device to auto-suspend while in use.
quoted hunk ↗ jump to hunk
+ break; + } + + return NOTIFY_OK; +} + +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: High] Similarly, can this unconditional cleanup put cause corruption if the initial get failed with -EINVAL?
quoted hunk ↗ jump to hunk
+ } + dwc_rk->phy_reset_active = 0; +}
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260924-b4-rockchip-dwc3-rockchip-glue-v16-0-126a2e9133c3@collabora.com?part=4 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy