On Mon, 22 Nov 2010, Rafael J. Wysocki wrote:
> > > I didn't like this change before and I still don't like it. Quite
> > > frankly, I'm
> > > not sure I can convince Linus to pull it. :-)
> > >
> > > Why don't we simply execute the callback under the spinlock in the
> > > IRQ safe case?
> >
> > Because it wouldn't work. The job of the runtime_idle callback is to
> > call pm_runtime_suspend when needed. But if the callback runs under
> > the spinlock then pm_runtime_suspend would hang when it tries to grab
> > the lock.
>
> Yes, in the _idle case. I actually should have put my comment under
> the change in rpm_callback(), which is what I really meant.
But the new rpm_callback() _does_ simply execute the callback under the
spinlock in the irq-safe case. So I don't understand what you mean
here.
> Moreover, I'm not sure if we need an "IRQ safe" version of _idle. Why do
> we need it, exactly?
Because pm_runtime_put_sync() calls rpm_idle(). If there were no
irq-safe version of rpm_idle() then drivers wouldn't be able to call
pm_runtime_put_sync() from within an interrupt handler.
> The problem I have with this change is that switching interrupts off really is
> a part of the locking operation, so using spin_unlock() after
> spin_lock_irq...()
> is kind of like releasing the lock partially, which I don't think is valid
> (even if we're going to reacquire the lock immediately).
On the contrary; spin_unlock() after spin_lock_irq() doesn't partially
release the lock -- it releases the lock _entirely_! :-)
Besides which, the existing code already releases the spinlock before
making callbacks, so there should be no reason to worry about that
issue. The new code either:
releases the spinlock but keeps interrupts disabled (in
rpm_idle), or
doesn't release the spinlock (in rpm_callback).
Either way, I should think you'd find the new code _less_ objectionable
than the existing code, not _more_ objectionable.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
