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