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
