RE: [PATCH] usb: dwc3: Add dwc3 lock for blocking interrupt storming

[정재훈]

> -----Original Message-----
> From: Thinh Nguyen [mailto:Thinh.Nguyen@xxxxxxxxxxxx]
> Sent: Friday, March 11, 2022 12:55 PM
> To: 정재훈; 'Felipe Balbi'; 'Greg Kroah-Hartman'
> Cc: 'open list:USB XHCI DRIVER'; 'open list'; 'Seungchull Suh'; 'Daehwan
> Jung'; cpgs@xxxxxxxxxxx; cpgsproxy5@xxxxxxxxxxx
> Subject: Re: [PATCH] usb: dwc3: Add dwc3 lock for blocking interrupt
> storming
> 
> Thinh Nguyen wrote:
> > 정재훈 wrote:
> >>> -----Original Message-----
> >>> From: Thinh Nguyen [mailto:Thinh.Nguyen@xxxxxxxxxxxx]
> >>> Sent: Friday, March 11, 2022 10:57 AM
> >>> To: 정재훈; Thinh Nguyen; 'Felipe Balbi'; 'Greg Kroah-Hartman'
> >>> Cc: 'open list:USB XHCI DRIVER'; 'open list'; 'Seungchull Suh';
> >>> 'Daehwan Jung'; cpgs@xxxxxxxxxxx; cpgsproxy5@xxxxxxxxxxx
> >>> Subject: Re: [PATCH] usb: dwc3: Add dwc3 lock for blocking interrupt
> >>> storming
> >>>
> >>> 정재훈 wrote:
> >>>> Hi.
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Thinh Nguyen [mailto:Thinh.Nguyen@xxxxxxxxxxxx]
> >>>>> Sent: Thursday, March 10, 2022 11:14 AM
> >>>>> To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman
> >>>>> Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; Daehwan
> >>>>> Jung
> >>>>> Subject: Re: [PATCH] usb: dwc3: Add dwc3 lock for blocking
> >>>>> interrupt storming
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> JaeHun Jung wrote:
> >>>>>> Interrupt Storming occurred with a very low probability of
> occurrence.
> >>>>>> The occurrence of the problem is estimated to be caused by a race
> >>>>>> condition between the top half and bottom half of the interrupt
> >>>>>> service
> >>>>> routine.
> >>>>>> It was confirmed that variables have values that cannot be held
> >>>>>> when ISR occurs through normal H / W irq.
> >>>>>> =================================================================
> >>>>>> === = (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF88DE6A0380 (
> >>>>>> 	(void *) buf = 0xFFFFFFC01594E000,
> >>>>>> 	(void *) cache = 0xFFFFFF88DDC14080,
> >>>>>> 	(unsigned int) length = 4096,
> >>>>>> 	(unsigned int) lpos = 0,
> >>>>>> 	(unsigned int) count = 0, <<
> >>>>>> 	(unsigned int) flags = 1, <<
> >>>>>> =================================================================
> >>>>>> === = "evt->count=0" and "evt->flags=DWC3_EVENT_PENDING" cannot
> >>>>>> be set at the same time.
> >>>>>>
> >>>>>> We estimate that a race condition occurred between
> >>>>>> dwc3_interrupt() and dwc3_process_event_buf() called by
> >>>>>> dwc3_gadget_process_pending_events().
> >>>>>> So I try to block the race condition through spin_lock.
> >>>>>
> >>>>> This looks like it needs a memory barrier. Would this work for you?
> >>>> Maybe it could be. But "evt->count = 0;" is updated on
> >>> dwc3_process_event_buf().
> >>>> So, I think spin_lock is more clear routine for this issue.
> >>>>
> >>>
> >>> Not really. If problem is due to the evt->flags not updated in time,
> >>> then the solution should be using the memory barrier. The spin_lock
> >>> would obfuscate the issue. And we should avoid using spin_lock in the
> top-half.
> >>
> >> This issue was occurred by watchdog. The interrupt occurred in units of
> 4 to 5us and cannot be released until the bottom is executed.
> >> If it is a problem with the memory barrier, the value should be updated
> after a few clocks and the TOP should run normally. Isn't it?
> >
> > Can you guarantee that a value is stored after X amount of time, every
> time?
Yes, I think it's guaranteed. The system was working with other cores for 20 seconds.

> >
> >> And Could you explain me why we should avoid using spin_lock in the
> top-half.
> >>
> >
> > The top-half and bottom-half are serialized. While the bottom-half
> > handler is running, the interrupt should be masked. If the top-half
> > got called in the middle of the bottom-half handler, something else is
> > wrong. There should not be a race that requires a spin_lock for this
> > particular critical section.

> >
> > The problem you're seeing is pointing toward a memory barrier issue.
> >
> > Also you noted that there's an "interrupt storm", which doesn't
> > indicate to me that it's due to PCIe legacy interrupt de-assertion
> > delay response either.
> >
> > Can you test it out and we can take a look further?
> >
> We want to avoid spin_lock because the top-half shouldn't stall for too
> long, affecting performance. This can happen if some async call from the
> upperlayer driver's holding the lock.
I also do not think that the serialization of the top and bottom of the ISR has been broken.

I think that dwc3_interrupt() called by dwc3_gadget_process_pending_events() influenced the serialization with the bottom. At the time of the problem, 20 seconds ago, dwc3_runtime_resume()->dwc3_gadget_process_pending_events() was called, and the problem began at that time.

I think so that it can make deadlock when using spin_lock in top, too.
Thank you for your feedback. I'll consider another way.

> 
> Thinh