Hi All,
On 2019-01-30 09:19, Wang, Yu1 wrote:
>
>> "Wang, Yu1" <yu1.wang@xxxxxxxxx> 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
