Re: [PATCH v17 5/7] usb: dwc3: rockchip: add USBDP PHY reset quirk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: dwc3: NULL pointer dereference on missing generic PHY
- [High] usb: dwc3: rockchip: Runtime PM usage count underflow on PHY reset
- [High] build: missing stubs for rk_udphy_*_reset_handler causes linker error
- [Low] usb: dwc3: unused struct dwc3_rk_phy_nb
--

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

usb: dwc3: rockchip: add USBDP PHY reset quirk

This patch introduces a Rockchip USBDP-specific quirk to assert PHYSOFTRST
before disabling the external PHY and deassert it once the PHY returns. This
prevents the DWC3 IP from corrupting the PHY's internal state when the mode
changes on RK3588 and RK3576 platforms.

> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 3c120ea9746d9..c86c1ac293bfa 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -193,6 +193,7 @@ config USB_DWC3_OCTEON
>  config USB_DWC3_ROCKCHIP
>  	tristate "Rockchip DWC3 Platform Driver"
>  	depends on ARCH_ROCKCHIP || COMPILE_TEST
> +	depends on PHY_ROCKCHIP_USBDP || !PHY_ROCKCHIP_USBDP

[Severity: High]
If CONFIG_PHY_ROCKCHIP_USBDP is disabled, `!PHY_ROCKCHIP_USBDP` evaluates to y,
which permits USB_DWC3_ROCKCHIP to be built. Since
include/linux/phy/rockchip-usbdp.h does not provide static inline dummy stubs
for the rk_udphy reset handlers, will this cause a linker error due to missing
symbols?

>  	depends on OF
>  	default USB_DWC3
>  	help
> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 4129aefd6a8b0..1f32150caa090 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
> @@ -2,11 +2,144 @@
>  /* Copyright (c) 2026, Collabora Ltd. */
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/phy/rockchip-usbdp.h>
>  #include <linux/pm_runtime.h>
>  #include "glue.h"
> +#include "io.h"
> +
> +struct dwc3_rockchip;
> +
> +/**
> + * struct dwc3_rk_phy_nb - wrapper for PHY notifier block
> + * @nb: notifier block
> + * @dwc: back-pointer to the DWC3 controller
> + * @port_index: USB3 port index this notifier is registered for
> + */
> +struct dwc3_rk_phy_nb {
> +	struct notifier_block	nb;
> +	struct dwc3_rockchip	*dwc_rk;
> +	u8			port_index;
> +};

[Severity: Low]
Is this struct dwc3_rk_phy_nb declaration still needed? It appears to be
unused anywhere in the driver, possibly left over from an earlier version
that used notifier blocks.

> +
>  struct dwc3_rockchip {
>  	struct dwc3		dwc;
> +	u8			phy_reset_active;
> +};
> +
> +static int dwc3_rockchip_phy_pre_reset(struct dwc3_rockchip *dwc_rk, int port)
> +{
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	unsigned long flags;
> +	u32 reg;
> +	int ret;
> +
> +	/*
> +	 * 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 ret;

[Severity: High]
If runtime PM is disabled for the device (e.g., during suspend or removal),
pm_runtime_get_if_active() returns -EINVAL and does not increment the usage
count.

Because `if (!ret)` evaluates to false for -EINVAL, the driver proceeds with
the reset. However, the post-reset and unregister paths unconditionally drop
the usage count via pm_runtime_put_autosuspend(), which could lead to a
mismatched decrement and trigger a "Runtime PM usage count underflow!" warning.

[ ... ]
> +static int dwc3_rockchip_phy_post_reset(struct dwc3_rockchip *dwc_rk, int port)
> +{
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	if (!(dwc_rk->phy_reset_active & BIT(port))) {
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		return 0;
> +	}
> +
> +	dwc_rk->phy_reset_active &= ~BIT(port);
> +
> +	/* Restore normal operation */
> +	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 unconditionally drop the PM usage count even if the initial
pm_runtime_get_if_active() call returned -EINVAL without incrementing it?

[ ... ]
> +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++) {
> +		struct phy *phy = dwc->usb3_generic_phy[i];
> +
> +		if (!strcmp(dev_driver_string(phy->dev.parent), "rockchip-usbdp-phy"))

[Severity: High]
Is it possible for `phy` to be NULL here?

If a USB3 port is not configured with a PHY in the device tree, the DWC3
core driver leaves `dwc->usb3_generic_phy[i]` as NULL. Evaluating
`phy->dev.parent` without a valid pointer check would cause an unconditional
NULL pointer dereference during unbind or teardown.

> +			rk_udphy_unregister_reset_handler(dwc->usb3_generic_phy[i]);
> +	}
> +
> +	/* 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);
> +	}
> +	dwc_rk->phy_reset_active = 0;
> +}
> +
> +static int dwc3_rk_phy_register_reset_handler(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++) {
> +		struct phy *phy = dwc->usb3_generic_phy[i];
> +
> +		if (!strcmp(dev_driver_string(phy->dev.parent), "rockchip-usbdp-phy"))

[Severity: High]
Could this also dereference a NULL pointer during probe if a USB3 port lacks
a defined PHY?

> +			rk_udphy_register_reset_handler(phy, dwc3_rockchip_phy_reset, dwc_rk, i);

[Severity: High]
Does this direct call to the PHY driver API require an IS_ENABLED() check?

Since the Kconfig permits USB_DWC3_ROCKCHIP to be built even when
PHY_ROCKCHIP_USBDP is disabled, this could cause a build failure during
linking if the PHY driver lacks dummy stubs for these functions.

> +	}
> +
> +	return devm_add_action_or_reset(dwc->dev, dwc3_rk_phy_unregister_notifiers, dwc_rk);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-b4-rockchip-dwc3-rockchip-glue-v17-0-876f96893e2f@collabora.com?part=5