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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux