Messages in this thread | | | Date | Fri, 14 Jul 2023 13:34:36 -0700 | Subject | Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend | From | Elson Serrao <> |
| |
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
| |