Re: [PATCH] PM: runtime: add might_sleep to PM runtime functions

[Rafael J. Wysocki]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/