Re: [PATCH v2 7/7] usb: dwc3: qcom: Support firmware-managed resource states for power management

[sashiko-bot@kernel.org]

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

New issues:
- [High] System suspend fails to release the 'power' domain if the device is already runtime suspended, causing a power leak.
- [High] Early return in dwc3_qcom_domains_put() on -EAGAIN leaks the 'power' domain vote and corrupts PM state.
- [High] Double pm_runtime_disable() in remove causes a permanent PM state corruption.
- [Medium] Probe error paths bypass the newly added reset_control_assert() hardening.
- [Medium] PM domain runtime usage count leak in the remove() error path.

Pre-existing issues:
- [High] Skipping dwc3_core_remove() on resume failure leaves child devices active, causing a Use-After-Free.
- [High] Hardware registers are accessed while the power domain is disabled, causing a synchronous external abort.
--

commit 322ab32f12f7861ac5e4f76813db99fd65efad09
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>

usb: dwc3: qcom: Support firmware-managed resource states for power management

This commit adds support for firmware-managed resource handling in the
Qualcomm DWC3 USB controller driver via SCMI-backed power domains ("power"
and "bus"). It coordinates runtime and system suspend state through domain
voting and attempts to harden the driver's probe error-unwind and remove
paths.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index e58a9ca23b009..7139eadbfb1e3 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c

[ ... ]

> +static int dwc3_qcom_domains_put(struct dwc3_qcom *qcom, bool bus_only)
> +{
> +	struct device *power_dev = qcom->pd_list->pd_devs[0];
> +	struct device *bus_dev = qcom->pd_list->pd_devs[1];
> +	int ret;
> +
> +	ret = pm_runtime_put_sync(bus_dev);
> +	if (ret < 0) {
> +		dev_err(qcom->dev, "failed to disable bus domain: %d\n", ret);
> +		return ret;
> +	}

[Severity: High]
When pm_runtime_put_sync() is called for the bus domain, could it return
-EAGAIN if the device is kept active by another thread? If it does, this
early return treats it as a failure, skipping the power domain put and
propagating the error back to the PM core. The PM core aborts the suspend
and marks the device as active, but the bus domain usage counter was
already decremented. If the bus physically suspends later, does this lead
to accessing unclocked hardware and causing a synchronous external abort?

[ ... ]

> @@ -386,11 +521,23 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
>  		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>  			dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
>  	}
> -	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>  
> -	ret = dwc3_qcom_interconnect_disable(qcom);
> -	if (ret)
> -		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
> +	if (!qcom->fw_managed) {
> +		clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> +
> +		ret = dwc3_qcom_interconnect_disable(qcom);
> +		if (ret)
> +			dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
> +	} else {
> +		/*
> +		 * Runtime suspend only needs to drop the bus domain; power
> +		 * stays voted on so register accesses remain possible for
> +		 * whatever briefly resumes it. System suspend drops both.
> +		 */
> +		ret = dwc3_qcom_domains_put(qcom, PMSG_IS_AUTO(msg));
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/*
>  	 * The role is stable during suspend as role switching is done from a
> @@ -450,13 +597,25 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
>  	if (dwc3_qcom_is_host(qcom) && wakeup) {
>  		for (i = 0; i < qcom->num_ports; i++)
>  			qcom->ports[i].usb2_speed = dwc3_qcom_read_usb2_speed(qcom, i);
>  		dwc3_qcom_enable_interrupts(qcom);
>  	}

[Severity: High]
This is a pre-existing issue, but by the time we reach this point during
system suspend, dwc3_qcom_domains_put() has already disabled both the bus
and power domains. If the device is a wakeup source, this code accesses
hardware registers by calling dwc3_qcom_read_usb2_speed() and
dwc3_qcom_enable_interrupts(). Because the power domain (GDSC) is off,
will accessing these registers cause a hardware exception or bus fault?

[ ... ]

>  static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
>  {
>  	int ret;
>  	int i;
>  	bool wakeup;
>  
>  	if (!qcom->is_suspended)
>  		return 0;

[Severity: High]
If the device is already runtime suspended, qcom->is_suspended is true and
this returns early during system suspend. However, during runtime suspend,
only the bus domain vote is dropped and the power domain vote is kept. By
returning early here, does system suspend fail to release the power domain,
causing a permanent power leak of the PM domain usage counter?

[ ... ]

>  	/*
>  	 * For runtime resume, always assume wakeup was enabled.
>  	 * For system resume, check device wakeup capability.
>  	 */
>  	if (PMSG_IS_AUTO(msg))
>  		wakeup = true;
>  	else
>  		wakeup = device_may_wakeup(qcom->dev);
>  
>  	if (dwc3_qcom_is_host(qcom) && wakeup)
>  		dwc3_qcom_disable_interrupts(qcom);

[Severity: High]
This is a pre-existing issue, but this code accesses hardware registers
by calling dwc3_qcom_disable_interrupts() before the power and bus domains
are enabled by dwc3_qcom_domains_get() below. Does accessing these registers
while the power domain is disabled cause a synchronous external abort?

> -	ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> -	if (ret < 0)
> -		goto enable_irq;
> +	if (!qcom->fw_managed) {
> +		ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> +		if (ret < 0)
> +			goto enable_irq;
>  
> -	ret = dwc3_qcom_interconnect_enable(qcom);
> -	if (ret)
> -		dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
> +		ret = dwc3_qcom_interconnect_enable(qcom);
> +		if (ret)
> +			dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
> +	} else {
> +		/*
> +		 * Runtime resume only needs to re-vote bus (power was left
> +		 * on across runtime suspend). System resume votes both.
> +		 */
> +		ret = dwc3_qcom_domains_get(qcom, PMSG_IS_AUTO(msg));

[Severity: High]
If the device was runtime suspended prior to system suspend,
dwc3_qcom_suspend() returned early and skipped dropping the power domain
vote. During system resume, dwc3_qcom_resume() does not return early, and
this unconditionally calls dwc3_qcom_domains_get() for both domains. Doesn't
this increment the power domain usage counter a second time, leading to a
permanent resource leak where the power domain never turns off?

[ ... ]

> @@ -741,9 +905,31 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  		goto err_remove_swnode;
>  	}
>  
> -	ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> -	if (ret < 0)
> -		goto err_remove_swnode;
> +	if (!qcom->fw_managed) {
> +		ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
> +		if (ret < 0)
> +			goto err_remove_swnode;

[Severity: Medium]
If early probe functions like clk_bulk_prepare_enable() fail, this jumps
directly to err_remove_swnode. Since err_remove_swnode is located after
the newly added reset_control_assert() block, doesn't this bypass the
intended hardware cleanup and leave the hardware controller out of reset?

[ ... ]

> @@ -829,15 +1034,63 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>  {
>  	struct dwc3 *dwc = platform_get_drvdata(pdev);
>  	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> +	bool pm_resumed = false;
> +	bool domains_suspended = true;
> +	int ret;
>  
> -	if (pm_runtime_resume_and_get(qcom->dev) < 0)
> -		return;
> +	ret = pm_runtime_resume_and_get(qcom->dev);
> +	if (ret < 0)
> +		dev_warn(qcom->dev, "Failed to resume. Perform critical cleanups only.\n");
> +	else
> +		pm_resumed = true;
>  
> -	dwc3_core_remove(&qcom->dwc);
> -	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> -	dwc3_qcom_interconnect_exit(qcom);
> +	/*
> +	 * Disable runtime PM to prevent any automatic PM operations during removal.
> +	 * This prevents race conditions where interrupt handlers or other subsystems
> +	 * might trigger runtime PM operations while the device is being torn down.
> +	 */
> +	pm_runtime_disable(qcom->dev);

[Severity: High]
This adds an initial pm_runtime_disable() on the shared device. However,
when dwc3_core_remove() is called immediately after, the core unconditionally
calls pm_runtime_disable() on the exact same device. Doesn't this double
disable increment the disable_depth twice, permanently corrupting the PM
state and preventing the device from ever runtime suspending or resuming
again after a driver unbind/rebind cycle?

> +	if (pm_resumed) {
> +		/* Only perform operations that require device to be active */
> +		dwc3_core_remove(&qcom->dwc);

[Severity: High]
This isn't a bug introduced by this patch, but if pm_runtime_resume_and_get()
fails, pm_resumed evaluates to false and dwc3_core_remove() is skipped.
Because the qcom structure is devres-managed, the memory backing it (including
the dwc3 struct) is unconditionally freed upon return. Does skipping core
removal leave the USB gadget, xHCI platform device, and notifiers active,
causing a use-after-free when these subsystems attempt to access the freed
memory?

> +		if (!qcom->fw_managed) {
> +			dwc3_qcom_interconnect_exit(qcom);
> +			clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> +		} else {
> +			ret = dwc3_qcom_domains_put(qcom, false /* bus_only */);

[Severity: Medium]
If the device fails to resume, pm_resumed is false and this skips calling
dwc3_qcom_domains_put(). However, a few lines down, dwc3_qcom_domain_detach()
is called unconditionally. Does this destroy the virtual domain devices while
the underlying genpd provider still holds an active PM vote from probe,
stranding the domain in the powered-on state indefinitely?

> +			if (ret) {
> +				dev_err(qcom->dev, "Failed to disable power domains during remove: %d\n",
> +					ret);
> +				domains_suspended = false;
> +			}
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153827.3463313-1-faisal.hassan@oss.qualcomm.com?part=7