Re: [PATCH v4 1/7] usb: dwc3: qcom: Keep wakeup IRQ enable/disable balanced on failed resume

[Faisal Hassan <faisal.hassan@oss.qualcomm.com>]

On 30-10-2026 02:54 pm, Peter Chen wrote:
> On 26-10-29 00:29:31, Faisal Hassan wrote:
>> dwc3_qcom_resume() disables the wakeup interrupts before enabling the
>> clocks. If clk_bulk_prepare_enable() fails, it returns with them
>> disabled.
> 
> What's the relationship between clk_bulk_prepare_enable() fails and this patch?
> 
Hi Peter,

The wakeup interrupts were disarmed before the clock enable. If that
failed, is_suspended stayed set, the next suspend returned early, and
the next resume disarmed them again. disable_irq_nosync() and
disable_irq_wake() nest, so each failed resume left the disable depth
one level deeper. This is the rarer case, so I will reword the commit
message to lead with the runtime suspend case below.

>> The next suspend returns early on is_suspended without
>> re-enabling them, and the next resume disables them again, so every
>> failed resume leaves the disable depth one level deeper and host-mode
>> wakeup is lost for good.
>>
>> Re-enabling them on the error path is not safe either: the
>> level-triggered ss_phy interrupt can stay asserted, and the handler
>> can't make progress while the controller is not resumed, so it would
>> fire continuously.
>>
>> Track whether the wakeup interrupts are armed and make enabling and
>> disabling them idempotent. A failed resume now leaves them disarmed and
>> balanced, and the next suspend arms them again. Disarm them in resume
>> regardless of the wakeup setting, so interrupts armed by a runtime
>> suspend are also disarmed when system resume runs with device wakeup
>> disabled.
>>
>> Signed-off-by: Faisal Hassan <faisal.hassan@oss.qualcomm.com>
>> ---
>>   drivers/usb/dwc3/dwc3-qcom.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index d44f093ac243..d4fda4dd81f6 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -81,6 +81,7 @@ struct dwc3_qcom {
>>   	enum usb_dr_mode	mode;
>>   	bool			is_suspended;
>>   	bool			pm_suspended;
>> +	bool			wakeup_irqs_armed;
>>   	struct icc_path		*icc_path_ddr;
>>   	struct icc_path		*icc_path_apps;
>>   
>> @@ -323,16 +324,26 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>>   {
>>   	int i;
>>   
>> +	if (!qcom->wakeup_irqs_armed)
>> +		return;
>> +
>>   	for (i = 0; i < qcom->num_ports; i++)
>>   		dwc3_qcom_disable_port_interrupts(&qcom->ports[i]);
>> +
>> +	qcom->wakeup_irqs_armed = false;
>>   }
>>   
>>   static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
>>   {
>>   	int i;
>>   
>> +	if (qcom->wakeup_irqs_armed)
>> +		return;
>> +
>>   	for (i = 0; i < qcom->num_ports; i++)
>>   		dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
>> +
>> +	qcom->wakeup_irqs_armed = true;
>>   }
>>   
>>   static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>> @@ -377,8 +388,13 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>>   	if (!qcom->is_suspended)
>>   		return 0;
>>   
>> -	if (dwc3_qcom_is_host(qcom) && wakeup)
>> -		dwc3_qcom_disable_interrupts(qcom);
>> +	/*
>> +	 * Wakeup interrupts may have been armed by an earlier runtime suspend
>> +	 * even if wakeup is now disabled,
> 
> How wakeup interrupt is triggered even the wake interrupt is disabled? Is the
> hardware default value makes it enabled? If it is, we need to disable all
> wakeup interrupt at probe, otherwise, there are un-expected wakeup interrupts.
> 
> Peter
> 

They are not. They are requested with IRQF_NO_AUTOEN, so they stay
disabled until the first suspend and nothing needs disabling at probe.
The issue is that runtime suspend always calls
dwc3_qcom_suspend(qcom, true), which arms them in host mode regardless
of device_may_wakeup(). The old resume only disarmed them when wakeup
was enabled, so a system resume with device wakeup disabled left them
armed. The comment read as if the interrupts trigger while disabled,
so I will reword it as:

     /*
      * Runtime suspend arms the wakeup interrupts regardless of the
      * device wakeup setting, so always disarm them here. On failure
      * they stay disarmed, as a level-triggered wakeup line could
      * otherwise keep firing while the controller can't be resumed.
      */

I will update the subject and commit message for v5 to cover this case.

Thanks,
Faisal
>> so always disarm them. On failure
>> +	 * they stay disarmed, as a level-triggered wakeup line could otherwise
>> +	 * keep firing while the controller can't be resumed.
>> +	 */
>> +	dwc3_qcom_disable_interrupts(qcom);
>>   
>>   	ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
>>   	if (ret < 0)
>> -- 
>> 2.34.1
>>
>>
>