Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Serrao <quic_eserrao@quicinc.com>]

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