lkml.org 
[lkml]   [2023]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend
From


On 7/14/2023 5:23 AM, Roger Quadros wrote:
>
>

>>>>>>    }
>>>>>>      static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>>>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>>>>>    {
>>>>>>        if (dwc->pending_events) {
>>>>>>            dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>>>>> +        pm_runtime_put(dwc->dev);
>>>>>
>>>>> Why the put here?
>>>>>
>>>>
>>>> To balance the get() called when setting the pending_events flag in dwc3_check_event_buf()
>>>>
>>>>      if (pm_runtime_suspended(dwc->dev)) {
>>>>          pm_runtime_get(dwc->dev);
>>>>          disable_irq_nosync(dwc->irq_gadget);
>>>>          dwc->pending_events = true;
>>>>          return IRQ_HANDLED;
>>>>      }
>>>>
>>>
>>> No this wrong. We want the device to be active from now on.
>>>
>>> runtime suspended->interrupt->pm_runtime_get->runtime_resume->process_pending_events->USB gadget resumed
>>>
>>> Only on next USB suspend you want to do the pm_runtime_put like you are doing it
>>> in dwc3_gadget_suspend_interrupt() by pm_request_autosuspend()
>>>
>>
>> That would break/block dwc3 runtime suspend during DISCONNECT case in below scenario
>>
>> runtime suspended->interrupt->pm_runtime_get (runtime usage count is 1)->runtime_resume->process_pending_events->USB gadget resumed -> USB disconnect (autosuspend blocked due to runtime usage count being 1 due to unbalanced get() ).
>>
>> The idea here is to balance the get() that was requested for processing the pending events, after processing those events. (like how we balance get() of ep_queue through put() in ep_dequeue)
>>
>> Also pm_request_autosuspend() doesnt decrement the usage count, it only requests for autosuspend.
>
> Ah, indeed.
>
>>
>> But I think better approach in terms of ordering is below
>
> ok, but should dwc->pending_events be set before calling pm_runtime_get() in dwc3_check_event_buf()?

Yes that would be a better approach (just in case if there is any race
between dwc3_check_event_buf() and the resume() path).

> Can we add a comment there that the get will be balanced out in dwc3_gadget_process_pending_events()?

Sure.
>
>>
>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>  {
>>      if (dwc->pending_events) {
>>          dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>> +        /*
>> +         * We have only stored the pending events as part
>> +         * of dwc3_interrupt() above, but those events are
>> +         * not yet handled. So explicitly invoke the
>> +         * interrupt handler for handling those events.
>> +         */
>> +        dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>          dwc->pending_events = false;
>>          enable_irq(dwc->irq_gadget);
>> +        pm_runtime_put(dwc->dev);
>
> We could do the put right after dwc3_thread_interrupt().

Ack.

>
>>
>>      }
>>  }
>
> I think this fix should be an independent patch
> as this fixes an issue that existed prior to this series?
> also need to -cc stable?
>
Agree. Both dwc3_thread_interrupt() and pm_runtime_put() above are
addressing an issue that existed prior. I will submit a separate patch
for this modification.

Thanks
Elson

\
 
 \ /
  Last update: 2023-07-16 20:51    [W:1.365 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site