Hi All, On 2019-01-30 09:19, Wang, Yu1 wrote: > >> "Wang, Yu1" <[email protected]> writes: >>>>> so it's better move the synchronize_irq() after the >>>>> spin_unlock_irqrestore(). >>>>> static int dwc3_suspend_common(struct dwc3 *dwc) >>>>> { >>>>> unsigned long flags; >>>>> >>>>> switch (dwc->dr_mode) { >>>>> case USB_DR_MODE_PERIPHERAL: >>>>> case USB_DR_MODE_OTG: >>>>> spin_lock_irqsave(&dwc->lock, flags); >>>>> dwc3_gadget_suspend(dwc); >>>>> spin_unlock_irqrestore(&dwc->lock, flags); >>>>> synchronize_irq() >>>> indeed, I missed that when I first reviewed the patch. Care to send a fix? >>> With this new change, the dwc3 irq thread will be sync after dwc3 >>> stopped, is this irq thread can be handled correctly for this case? >> how about this? >> >> modified drivers/usb/dwc3/gadget.c >> @@ -3373,6 +3373,8 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >> } >> >> int dwc3_gadget_suspend(struct dwc3 *dwc) >> + __releases(dwc->lock) >> + __acquires(dwc->lock) >> { >> if (!dwc->gadget_driver) >> return 0; >> @@ -3381,7 +3383,9 @@ int dwc3_gadget_suspend(struct dwc3 *dwc) >> dwc3_disconnect_gadget(dwc); >> __dwc3_gadget_stop(dwc); >> >> + spin_unlock_irq(dwc->lock); >> synchronize_irq(dwc->irq_gadget); >> + spin_lock_irq(dwc->lock); >> >> return 0; >> } >> >> Not the most elegant solution. I'm open to other suggestions. > Is there any conflict if dwc3_thread_interrupt and below three functions > execute by parallel? > > dwc3_gadget_run_stop(dwc, false, false); > dwc3_disconnect_gadget(dwc); > __dwc3_gadget_stop(dwc); > > That is means the dwc3_thread_interrupt is trying to handle pending > events but another core is trying to do dwc3 stop/suspend.
Isn't that serialized by dwc->lock? It quite late in release cycle and the issue is still not fixed. Maybe it would make sense to revert that change and prepare a proper fix for the next release? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
