Re: [PATCH v2] usb: chipidea: otg: use autosuspend in ci_otg_work

["Peter Chen (CIX)" <peter.chen@kernel.org>]

On 26-01-09 11:57:17, PETER Mario wrote:
> On 1/8/26 17:12, Alan Stern wrote:
> 
> > On Thu, Jan 08, 2026 at 03:34:58PM +0000, Mario Peter wrote:
> >> Switch to autosuspend mechanism in ci_otg_work() to prevent race
> >> conditions during rapid device plug/unplug cycles.
> >>
> >> The current implementation uses pm_runtime_put_sync() which triggers
> >> immediate runtime suspend. This creates a race condition when a USB
> >> device is unplugged and quickly replugged. The controller may suspend
> >> before the new device detection completes, causing detection to fail.
> > You should be aware that this change alone won't prevent race
> > conditions.  They might not occur during rapid plug/unplug cycles, but
> > they can still occur.
> >
> > Consider what would happen if the device is unplugged and then plugged
> > back in a few seconds later, exactly when the autosuspend mechanism
> > kicks in and starts suspending the controller.
> 
> I tested this scenario specifically with my automated test setup,
> including plug/unplug cycles timed around the autosuspend delay of 2
> seconds to provoke exactly the issue you describe. I was not able to
> reproduce the detection failure, which does not mean the race condition
> doesn't exist - only that it may be harder to trigger or dependent on
> other timing factors.
> 
> > The real way to fix this race is by ensuring that device detection will
> > occur in all cases, whether the controller is at full power, suspended,
> > or in the process of switching between the two.
> 
> I agree this would be the proper fix. Do you have any proposals or ideas
> on how this potential race could be addressed?
> 

Hi Mario,

Since you have enabled runtime PM for chipidea core, I assmue your SoC
has designed wakeup logic for USB controller or you have non-USB external
events (eg GPIO) for hot plug, which one for your platform?

- If your platform has VBUS wakeup logic at SoC, you may enable wakeup
at your glue layer, it is later than chipidea core's runtime suspend.
If VBUS change occurs before controller goes to suspend, you get normal
vbus change interrupt, else, you get an VBUS wakeup interrupt.

- If you use external event for hot plug, then this event will not lost
because it is not related to controller's suspend, unless this event
itself has debounce requirement.

Peter


> >
> >> Replace pm_runtime_put_sync() with pm_runtime_put_sync_autosuspend()
> >> as recommended by Section 9 of Documentation/power/runtime_pm.rst.
> >> This updates the device's last_busy timestamp and delays the suspend
> >> until after the autosuspend_delay period, allowing pending device
> >> detection to complete. As documented, this approach prevents
> >> "bouncing too rapidly between low-power and full-power states".
> >>
> >> The synchronous variant pm_runtime_put_sync_autosuspend() is used
> >> (rather than the asynchronous __pm_runtime_put_autosuspend()) to match
> >> the semantics of the original pm_runtime_put_sync() call.
> >>
> >> Signed-off-by: Mario Peter <mario.peter@leica-geosystems.com>
> >> ---
> >>
> >> Testing was performed using a scripted sequence with a relay to simulate
> >> USB plug/unplug operations at various timing intervals, verifying that
> >> devices are consistently detected after replugging.
> >>
> >> v1: submitted (https://lore.kernel.org/linux-usb/zuzsjr6isq6i5izw3rkyo45opyikiqjmy5xk7flpmlgmqb6mgh@rpbdvi3s4u54/)
> >> v2: dropped redundant pm_runtime_mark_last_busy() call
> >>
> >>   drivers/usb/chipidea/otg.c | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> >> index 647e98f4e351..d19c27f44424 100644
> >> --- a/drivers/usb/chipidea/otg.c
> >> +++ b/drivers/usb/chipidea/otg.c
> >> @@ -230,7 +230,7 @@ static void ci_otg_work(struct work_struct *work)
> >>                ci_handle_vbus_change(ci);
> >>        }
> >>
> >> -     pm_runtime_put_sync(ci->dev);
> >> +     pm_runtime_put_sync_autosuspend(ci->dev);
> >>
> >>        enable_irq(ci->irq);
> >>   }
> >>
> >> base-commit: f0b9d8eb98dfee8d00419aa07543bdc2c1a44fb1
> >> --
> >> 2.43.0
> 
> 

-- 

Best regards,
Peter