Re: [PATCH v14 32/38] usb: dwc3: rockchip: fix USB-C reconnect in gadget mode

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Checking !dwc->gadget_driver in dwc3_rockchip_vbus_handler breaks the UDC core's VBUS state tracking, defeating the patch's intended PM resume logic.
- [High] TOCTOU race in dwc3_rockchip_vbus_handler leading to Use-After-Free of gadget and UDC structures.

Pre-existing issues:
- [High] Using asynchronous pm_runtime_put_autosuspend() in the device removal path introduces race conditions.
- [High] Mishandling of -EINVAL from pm_runtime_get_if_active() causes PM usage count underflow and unsafe MMIO access without clocks.
--

--- Patch [32]: [PATCH v14 32/38] usb: dwc3: rockchip: fix USB-C reconnect in gadget mode ---
commit 9dbfd69b985e240b0787670a034f6b9c37a180d0
Author: Sebastian Reichel <[email protected]>

usb: dwc3: rockchip: fix USB-C reconnect in gadget mode

When USB-C is configured in gadget mode and the cable is unplugged the
USB controller is suspended. This patch triggers a runtime PM resume
when the role is changed to fix an issue where the controller stays
suspended and the port status remains not-attached after reconnecting.

> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 9e91e5f7e53d6..246d7dcafc68f 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
> @@ -25,8 +25,17 @@ struct dwc3_rockchip {
>  	struct dwc3		dwc;
>  	struct dwc3_rk_phy_nb	usb3_phy_nb[DWC3_USB3_MAX_PORTS];
>  	u8			phy_reset_active;
> +	enum usb_role		role;
>  };
>  
> +static void dwc3_rockchip_vbus_handler(struct dwc3 *dwc, bool present)
> +{
> +	if (!dwc->gadget || !dwc->gadget_driver)
> +		return;

[Severity: High]
Does this early return break the UDC core's VBUS state tracking when a gadget
driver is not yet bound?

If the system boots with the cable unplugged and no gadget driver is loaded,
this returns early and drops the unplug event. The UDC core's udc->vbus would
remain true.

When the gadget driver is later loaded, this true state would cause a
premature pullup that wakes the device but then lets it autosuspend. When the
cable is actually plugged in later, the UDC core might see the state hasn't
changed and skip the pullup, meaning the device never receives the PM resume
needed to detect the connection.

[Severity: High]
Can this lead to a use-after-free of the gadget and UDC structures?

This function is called locklessly from the asynchronous PHY notifier
dwc3_usb3_phy_notify(). If a concurrent role switch occurs (like DEVICE to
HOST), dwc3_gadget_exit() could be freeing the gadget structure under
dwc->mutex:

dwc3_rockchip_vbus_handler()
    if (!dwc->gadget || !dwc->gadget_driver)

Concurrent dwc3_gadget_exit()
    usb_put_gadget() -> kfree(gadget)

Would this result in the notifier dereferencing a freed gadget pointer when
it calls usb_udc_vbus_handler() below?

> +
> +	usb_udc_vbus_handler(dwc->gadget, present);
> +}
> +

[ ... ]

> @@ -57,6 +66,8 @@ static int dwc3_usb3_phy_notify(struct notifier_block *nb,
>  		if (!ret)
>  			return NOTIFY_OK;

[Severity: High]
This is a pre-existing issue, but does this check properly handle the
-EINVAL return from pm_runtime_get_if_active()?

pm_runtime_get_if_active() returns -EINVAL when runtime PM is disabled.
By checking if (!ret), the -EINVAL case falls through and allows the code
to proceed. This can cause unsafe MMIO access without clocks.

Additionally, in the PHY_NOTIFY_POST_RESET case, pm_runtime_put_autosuspend()
is called unconditionally:

dwc3_usb3_phy_notify() {
    ...
    case PHY_NOTIFY_POST_RESET:
        ...
        pm_runtime_put_autosuspend(dwc->dev);
        break;
}

Since pm_runtime_put_autosuspend() calls rpm_drop_usage_count(), this would
decrement the usage count and cause an underflow if the corresponding get
failed with -EINVAL.

> +		dwc3_rockchip_vbus_handler(dwc, false);
> +
>  		/*
>  		 * Assert USB3 PHY soft reset within DWC3 before the external

[ ... ]

> @@ -130,7 +144,16 @@ static int dwc3_rk_phy_register_notifiers(struct dwc3 *dwc)
>  	return devm_add_action_or_reset(dwc->dev, dwc3_rk_phy_unregister_notifiers, dwc_rk);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the cleanup function
dwc3_rk_phy_unregister_notifiers() safely handle device removal?

The cleanup function executes during devres teardown:

dwc3_rk_phy_unregister_notifiers() {
    ...
    for (i = 0; i < dwc->num_usb3_ports; i++) {
        if (dwc_rk->phy_reset_active & BIT(i))
            pm_runtime_put_autosuspend(dwc->dev);
    }
}

Since this runs during device removal, the asynchronous suspend could fire
after device memory is freed or during bus teardown. Should this use
pm_runtime_put_sync() instead to ensure hardware is in the correct state
before removal completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=32

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy