정재훈 wrote: > > >> -----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. Ok. This doesn't sound right. On suspend, we can just mask the interrupt, and unmask after resume. There's no need to even track dwc->pending_events flag either. While the event count from GEVNTCOUNT is non-zero, the controller will generate interrupts if unmasked. dwc3 is currently not handling hibernation, and we're not restoring anything on resume. > > 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. > Thanks, Thinh
