[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <be57511d-2005-a1f5-d5a5-809e71029aec@quicinc.com>
Date: Fri, 14 Jul 2023 13:34:36 -0700
From: Elson Serrao <quic_eserrao@...cinc.com>
To: Roger Quadros <rogerq@...nel.org>, <Thinh.Nguyen@...opsys.com>,
<stern@...land.harvard.edu>
CC: <gregkh@...uxfoundation.org>, <linux-kernel@...r.kernel.org>,
<linux-usb@...r.kernel.org>, <quic_wcheng@...cinc.com>,
<quic_jackp@...cinc.com>
Subject: Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus
suspend
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
Powered by blists - more mailing lists