Re: [PATCH v2 1/7] usb: dwc3: qcom: re-enable wakeup interrupts on failed resume

[sashiko-bot@kernel.org]

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

New issues:
- [High] Re-enabling wakeup interrupts on a failed resume path causes an unrecoverable interrupt storm.

Pre-existing issues:
- [High] Using a read-modify-write operation (`dwc3_qcom_setbits`) to clear a Write-1-to-Clear (W1C) interrupt status register unintentionally clears all other pending events.
--

commit 04b120125f0ae65d0e301d68751f219b8e9f86e0
Author: Faisal Hassan <faisal.hassan@oss.qualcomm.com>

usb: dwc3: qcom: re-enable wakeup interrupts on failed resume

This commit modifies dwc3_qcom_resume() to handle failures from
clk_bulk_prepare_enable(). On failure, it jumps to a new enable_irq label
that re-enables wakeup interrupts if they were disabled earlier.

>     dwc3_qcom_resume() disables wakeup interrupts before re-enabling
>     clocks, then re-enables them again once resume completes. If
>     clk_bulk_prepare_enable() fails partway through, the function
>     returns early without re-enabling the interrupts that were just
>     disabled, leaving host-mode wakeup interrupts masked after a
>     failed resume.
>     
>     Jump to a common cleanup path that re-enables the interrupts
>     before returning the error, mirroring the success path.

[Severity: High]
Does the success path actually re-enable the interrupts? Looking at
dwc3_qcom_resume(), the success path just returns 0 without re-enabling them,
as wakeup interrupts are meant to be active only while the device is suspended.

> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ac68b4218b56b..49698a31b2f41 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -382,7 +382,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>  
>  	ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
>  	if (ret < 0)
> -		return ret;
> +		goto enable_irq;

[Severity: High]
If we jump to enable_irq here, we bypass clearing the hardware events in
pwr_evnt_irq_stat_reg (which happens later in this function). Since clearing
the register requires the clocks to be enabled, jumping to the error path means
the pending hardware event is never cleared.

Will re-enabling the interrupt line below without clearing this hardware event
cause the interrupt to remain asserted? Since qcom_dwc3_resume_irq() returns
IRQ_HANDLED, this could cause the kernel IRQ subsystem to unmask the line,
resulting in an immediate interrupt storm that hangs the CPU.

[ ... ]

> @@ -398,6 +398,16 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>  	qcom->is_suspended = false;
>  
>  	return 0;
> +
> +enable_irq:
> +	/*
> +	 * Re-enable interrupts if they were disabled earlier, so a failed
> +	 * resume doesn't leave wakeup interrupts masked.
> +	 */
> +	if (dwc3_qcom_is_host(qcom) && wakeup)
> +		dwc3_qcom_enable_interrupts(qcom);

[Severity: High]
This is a pre-existing issue, but earlier in dwc3_qcom_resume() the code clears
L2-related events using dwc3_qcom_setbits():

    for (i = 0; i < qcom->num_ports; i++) {
        dwc3_qcom_setbits(qcom->qscratch_base,
                  pwr_evnt_irq_stat_reg[i],
                  PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
    }

Since pwr_evnt_irq_stat_reg is a Write-1-to-Clear (W1C) register, doesn't using
a read-modify-write operation like dwc3_qcom_setbits() read all currently
pending interrupts as '1's and write them back as '1's?

Could this unintentionally acknowledge and clear every other unrelated pending
event in the register, causing missed hardware events and stuck states?

> +
> +	return ret;
>  }

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