On Sunday, July 24, 2011, Colin Cross wrote: > On Sat, Jul 23, 2011 at 3:57 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote: > > On Saturday, July 23, 2011, Colin Cross wrote: > >> The list of functions that can be called in atomic context is > >> non-intuitive (pm_runtime_put_sync can not, but > >> pm_runtime_put_sync_suspend can, if pm_runtime_irq_safe has > >> been called?). > > > > However, this behavior is documented. > > > > Also, if you have a clean use case for calling rpm_idle() with interrupts > > off, it can be modified to work in analogy with rpm_suspend() in that respect. > > Yes, Kevin posted that patch in response to a bug that would never > have existed with this patch. Even with Kevin's change, this patch > still detects drivers that are missing pm_runtime_irq_safe(). > > >> The code is actively misleading - the entry > >> points all start with spin_lock_irqsave, suggesting they > >> are safe to call in atomic context, but may later > >> enable interrupts. > > > > May I say it is this way for a reason? > > I'll reword that > > >> Add might_sleep_if to all the __pm_runtime_* entry points > >> to enforce correct usage. > > > > I'm not sure how this makes things better. > > I spent hours tracking down a bug that was caused by > pm_runtime_put_sync enabling interrupts when entering idle, which was > causing the timer interrupt to be serviced before the cpu entered > idle, and the cpu to idle forever until a non-timer interrupt > occurred. The bug would never have been introduced with this patch. > When I ran with this patch, it immediately caught 3 other cases of > incorrect usage in atomic context, any of which could cause deadlocks: > spin_lock_irqsave(driver lock) > pm_runtime_put_sync > spin_lock_irqsave(dev lock) > spin_unlock_irq(dev_lock) - enables interrupts > driver irq > spin_lock(driver lock) > > One of the bugs was put_sync instead of put_sync_suspend, which would > not be a problem after Kevin's patch, but the other two were missing > pm_runtime_irq_safe. > > Not every developer who calls a pm_runtime function is going to read > the documentation, and this patch will catch the common incorrect > usage the first time it is run. > > I'll update this patch on top of Kevin's. > > >> Also add pm_runtime_put_sync_autosuspend to the list of > >> functions that can be called in atomic context. > > > > OK > > > > In addition to that rpm_idle() is missing the __releases __acquires > > annotations. > > Do you want that added to this patch? It seems like that fits better > into Kevin's patch, or a third patch. OK, I'll do a separate patch adding those. Thanks, Rafael _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
