On Thursday, September 30, 2010, Alan Stern wrote:
> On Thu, 30 Sep 2010, Rafael J. Wysocki wrote:
>
> > > --- usb-2.6.orig/include/linux/pm.h
> > > +++ usb-2.6/include/linux/pm.h
> > > @@ -485,6 +485,7 @@ struct dev_pm_info {
> > > unsigned int run_wake:1;
> > > unsigned int runtime_auto:1;
> > > unsigned int no_callbacks:1;
> > > + unsigned int callbacks_in_irq:1;
> >
> > I kind of don't like this name. What about irq_safe ?
>
> Sure, that's a better name.
>
> > > --- usb-2.6.orig/include/linux/pm_runtime.h
> > > +++ usb-2.6/include/linux/pm_runtime.h
> > > @@ -21,6 +21,7 @@
> > > #define RPM_GET_PUT 0x04 /* Increment/decrement the
> > > usage_count */
> > > #define RPM_AUTO 0x08 /* Use autosuspend_delay */
> > > +#define RPM_IRQ 0x10 /* Don't enable interrupts */
> > >
> > > #ifdef CONFIG_PM_RUNTIME
> > >
> > > @@ -40,6 +41,7 @@ extern int pm_generic_runtime_idle(struc
> > > extern int pm_generic_runtime_suspend(struct device *dev);
> > > extern int pm_generic_runtime_resume(struct device *dev);
> > > extern void pm_runtime_no_callbacks(struct device *dev);
> > > +extern void pm_runtime_callbacks_in_irq(struct device *dev);
> >
> > Perhaps this one can be called pm_runtime_irq_safe() in analogy to the struct
> > member?
>
> Yes.
>
> > Hmm. This looks rather complicated. I don't really feel good about this
> > change. It seems to me that it might be better to actually implement
> > the _irq() helpers from scratch. I think I'll try to do that.
>
> That might work out better, but there will be a substantial amount of
> code duplication. Try it and see how it turns out.
I think you're right, but I'm not sure yet.
> > Overall, it looks like there's some overlap between RPM_IRQ and
> > power.callbacks_in_irq, where the latter may influence the behavior of the
> > helpers even if RPM_IRQ is unset.
>
> Exactly. That was intentional.
>
> > I think we should return error code if RPM_IRQ is used, but
> > power.callbacks_in_irq is not set.
>
> The patch does that.
>
> > If RPM_IRQ is not set, but
> > power.callbacks_in_irq is set, we should fall back to the normal behavior
> > (ie. do not avoid sleeping).
>
> By "do not avoid sleeping", do you mean that
>
> (1) the helper functions should be allowed to sleep, or
>
> (2) the callbacks should be invoked with interrupts enabled?
>
> The patch already does (1). By contrast, (2) isn't safe. It means
> there is a risk of having one thread do a busy-wait with interrupts
> disabled, waiting for another thread that can sleep.
In my opinion the _irq operations should really be one-shot, without
any looping, waking up parents etc. I mean, if the parent is not RPM_ACTIVE,
the _irq resume should immediately fail and analogously for the _irq
suspend. And so on. As simple as reasonably possible.
> > That's why I think it's better to change the name of power.callbacks_in_irq
> > to power.irq_safe.
> >
> > Also I think we should refuse to set power.callbacks_in_irq for a device
> > whose partent (1) doesn't have power.callbacks_in_irq and (2) doesn't
> > have power.ignore_children set , and (3) doesn't have power.disable_depth > 0.
> > Then, once we've set power.callbacks_in_irq for a device, we should take
> > this into consideration when trying to change the parent's settings.
>
> That's not a bad idea, but I don't know if it's necessary in practice.
> With the current patch, if you violate this condition you will simply
> encounter an error when the PM core refuses to resume a sleeping
> parent. Maybe we should allow violations and the resulting refusals
> but print a warning message in the system log.
Yes, see above. I think printing a message in case a suspended parent
prevented an _irq resume from happening, for example, is a good idea.
> > There probably is more subtelties like this, but I can't see them at the moment.
>
> There are some subtle aspects related to the interplay between idle and
> the other callbacks. In principle, the idle and suspend callbacks
> doesn't need to have interrupts disabled -- we don't care if interrupt
> handlers can't do synchronous suspends; they only need synchronous
> resumes. But I wanted to keep things simple, so I treated all the
> callbacks the same.
OK
> Going further, I'm still a little dubious about the whole point of
> having idle notifications at all. I don't see why the PM core can't
> notify a subsystem that a device is idle just by calling the
> runtime_suspend routine.
Well, calling _idle is like saying "I think this device may be idle, please
consider suspending it", while calling _suspend is like saying "suspend
this device unless you can't". I think that's enough of a difference to
have separate _idle.
> The routine doesn't have to suspend if it
> doesn't want to, and it can always schedule a delayed suspend or an
> autosuspend. Maybe you know of a use case where having explicit idle
> notifications really does help, but I can't think of any.
A subsystem or a driver may want to schedule the suspend with a timeout
if _idle is called instead of just suspending synchronously. The r8169 driver
actually does something similar (although a bit more complicated).
> One more thing: I'm not sure we really need the "notify" variable in
> rpm_suspend. When a suspend callback fails, it means the device is in
> use. Whatever is using the device should then be responsible for
> sending its own idle notification when it is finished; we shouldn't
> need to send a notification as soon as the suspend callback fails.
OK, I'm fine with removing it or calling rpm_idle(dev, RPM_ASYNC)
instead of rpm_idle(dev, 0) in there.
Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
