Re: [PATCH] usb: dwc3: Potential fix of possible dwc3 interrupt storm

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Thu, Aug 08, 2024, Selvarasu Ganesan wrote:
> 
> On 8/8/2024 6:45 AM, Thinh Nguyen wrote:
> > On Wed, Aug 07, 2024, Selvarasu Ganesan wrote:
> >> On 8/7/2024 6:08 AM, Thinh Nguyen wrote:
> >>> On Fri, Jul 19, 2024, Selvarasu Ganesan wrote:
> >>>> In certain scenarios, there is a chance that the CPU may not be
> >>>> scheduled the bottom half of dwc3 interrupt. This is because the CPU
> >>>> may hang up where any work queue lockup has happened for the same CPU
> >>>> that is trying to schedule the dwc3 thread interrupt. In this scenario,
> >>>> the USB can enter runtime suspend as the bus may idle for a longer time
> >>>> , or user can reconnect the USB cable. Then, the dwc3 event interrupt
> >>>> can be enabled when runtime resume is happening with regardless of the
> >>>> previous event status. This can lead to a dwc3 IRQ storm due to the
> >>>> return from the interrupt handler by checking only the evt->flags as
> >>>> DWC3_EVENT_PENDING, where the same flag was set as DWC3_EVENT_PENDING
> >>>> in previous work queue lockup.
> >>>> Let's consider the following sequences in this scenario,
> >>>>
> >>>> Call trace of dwc3 IRQ after workqueue lockup scenario
> >>>> ======================================================
> >>>> IRQ #1:
> >>>> ->dwc3_interrupt()
> >>>>     ->dwc3_check_event_buf()
> >>>>           ->if (evt->flags & DWC3_EVENT_PENDING)
> >>>>                        return IRQ_HANDLED;
> >>>>           ->evt->flags |= DWC3_EVENT_PENDING;
> >>>>           ->/* Disable interrupt by setting DWC3_GEVNTSIZ_INTMASK  in
> >>>>                                                           DWC3_GEVNTSIZ
> >>>>           ->return IRQ_WAKE_THREAD; // No workqueue scheduled for dwc3
> >>>>                                        thread_fu due to workqueue lockup
> >>>>                                        even after return IRQ_WAKE_THREAD
> >>>>                                        from top-half.
> >>>>
> >>>> Thread #2:
> >>>> ->dwc3_runtime_resume()
> >>>>    ->dwc3_resume_common()
> >>>>      ->dwc3_gadget_resume()
> >>>>         ->dwc3_gadget_soft_connect()
> >>>>           ->dwc3_event_buffers_setup()
> >>>>              ->/*Enable interrupt by clearing  DWC3_GEVNTSIZ_INTMASK in
> >>>>                                                           DWC3_GEVNTSIZ*/
> >>>>
> >>>> Start IRQ Storming after enable dwc3 event in resume path
> >>>> =========================================================
> >>>> CPU0: IRQ
> >>>> dwc3_interrupt()
> >>>>    dwc3_check_event_buf()
> >>>>           if (evt->flags & DWC3_EVENT_PENDING)
> >>>>            return IRQ_HANDLED;
> >>>>
> >>>> CPU0: IRQ
> >>>> dwc3_interrupt()
> >>>>    dwc3_check_event_buf()
> >>>>           if (evt->flags & DWC3_EVENT_PENDING)
> >>>>            return IRQ_HANDLED;
> >>>> ..
> >>>> ..
> >>>>
> >>>> To fix this issue by avoiding enabling of the dwc3 event interrupt in
> >>>> the runtime resume path if dwc3 event processing is in progress.
> >>>>
> >>>> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> >>>> ---
> >>>>    drivers/usb/dwc3/core.c | 8 ++++++--
> >>>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >>>> index cb82557678dd..610792a70805 100644
> >>>> --- a/drivers/usb/dwc3/core.c
> >>>> +++ b/drivers/usb/dwc3/core.c
> >>>> @@ -549,8 +549,12 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc)
> >>>>    			lower_32_bits(evt->dma));
> >>>>    	dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(0),
> >>>>    			upper_32_bits(evt->dma));
> >>>> -	dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> >>>> -			DWC3_GEVNTSIZ_SIZE(evt->length));
> >>>> +
> >>>> +	/* Skip enable dwc3 event interrupt if event is processing in middle */
> >>>> +	if (!(evt->flags & DWC3_EVENT_PENDING))
> >>>> +		dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> >>>> +				DWC3_GEVNTSIZ_SIZE(evt->length));
> >>>> +
> >>>>    	dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 0);
> >>>>    
> >>>>    	return 0;
> >>>> -- 
> >>>> 2.17.1
> >>>>
> >>> We're not waking up from a hibernation. So after a soft-reset and
> >>> resume, the events that weren't processed are stale. They should be
> >>> processed prior to entering suspend or be discarded before resume.
> >>>
> >>> The synchronize_irq() during suspend() was not sufficient to prevent
> >>> this? What are we missing here.
> >>>
> >>> Thanks,
> >>> Thinh
> >> I don’t think the triggering of interrupt would not be stopped even if
> >> do soft reset. It's because of event count is may be valid .
> > Ok. I think I see what you're referring to when you say "event is
> > processing in the middle" now.
> >
> > What you want to check is probably this in dwc3_event_buffers_setup().
> > Please confirm:
> >
> > if (dwc->pending_events)
> > 	dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> > 			DWC3_GEVNTSIZ_INTMASK | DWC3_GEVNTSIZ_SIZE(evt->length));
> > else
> > 	dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), DWC3_GEVNTSIZ_SIZE(evt->length));
> 
> Yes, we are expecting the same. But, we must verify the status of 
> evt->flags, which will indicate whether the event is currently 
> processing in middle or not. The below code is for the reference.
> 
> if (!(evt->flags & DWC3_EVENT_PENDING))
> 	dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> 			 DWC3_GEVNTSIZ_SIZE(evt->length));
> else
> 	dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> 			DWC3_GEVNTSIZ_INTMASK | DWC3_GEVNTSIZ_SIZE(evt->length));

So, this happens while pending_events is set right? I need to review
this runtime suspend flow next week. Something doesn't look right. When
there's a suspend/resume runtime or not, there's a soft disconnect. We
shouldn't be processing any event prior to going into suspend. Also, we
shouldn't be doing soft-disconnect while connected and in operation
unless we specifically tell it to.

> 
> >> Let consider the scenarios where SW is not acknowledge the event count
> >> after getting a interrupt with disable GEVNTSIZ_INTMASK =0. It will
> >> triggering the spurious interrupts until enable GEVNTSIZ_INTMASK=1 or
> >> acknowledge the event count by SW. This is happening here because of We
> >> just return from interrupt handler by checking if evt->flags as
> >> DWC3_EVENT_PENDING. Clearing of DWC3_EVENT_PENDING flag is done in
> >> dwc3_thread_interrupt. In some scenario it's not happening due to cpu
> >> hangup or work queue lockup.
> > This can be mitigated by adjusting the imod_interval (interrupt
> > moderation). Have you tried that?
> 
> 
> Yes we tried to play around the changing of imod interval value but no 
> improvements.

Ok.

Thanks,
Thinh