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

[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