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

[Peter Chen]

On 26-10-30 16:54:55, Faisal Hassan wrote:
> 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.
> 

Okay, so here you may need another patch for error handling improvement.

> > >   }
> > >   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.

Get it.

At your second patch, it already knows the context between runtime
and system suspend, you could disable wakeup enable if the device_may_wakeup
return false, that could avoid introducing more judgements, and have this
patch after your 2nd patch.

> 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,
Peter Chen