On Wednesday 05 August 2009, Alan Stern wrote:
> On Wed, 5 Aug 2009, Rafael J. Wysocki wrote:
>
> > > > + spin_unlock_irq(&dev->power.lock);
> > > > +
> > > > + if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
> > > > + dev->bus->pm->runtime_idle(dev);
> > > > +
> > > > + spin_lock_irq(&dev->power.lock);
> > >
> > > Small optimization: Put the spin_{un}lock_irq stuff inside the "if"
> > > statement, so it doesn't happen if the test fails.
> >
> > Well, I don't think so. We need to take the lock here unconditionally,
> > because the caller is going to unlock it.
>
> No, I meant do this:
>
> if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
> spin_unlock_irq(&dev->power.lock);
> dev->bus->pm->runtime_idle(dev);
> spin_lock_irq(&dev->power.lock);
> }
Ah, OK. That makes sense.
> By the way, I don't know if anyone still pays attention to sparse-type
> annotations. If you do,
Well, I guess I should. :-)
> you should add "__releases(&dev->power.lock)"
> and "__acquires(&dev->power.lock)" annotations to functions like this,
> which release the lock without first acquiring it, and then acquire
> the lock without releasing before returning.
>
> > > The same thing can be done in other places.
> >
> > I'm not really sure it can.
>
> The other places aren't quite the same as this. I'll leave it to your
> discretion. :-)
>
>
> > > > +int __pm_runtime_set_status(struct device *dev, unsigned int status)
> > > > +{
> > > > + struct device *parent = dev->parent;
> > > > + unsigned long flags;
> > > > + int error = 0;
> > > > +
> > > > + if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
> > > > + return -EINVAL;
> > >
> > > This should go inside the spinlocked area.
> >
> > Why? 'status' is a function argument, it doesn't need to be protected from
> > concurrent modification.
>
> Oops, my mistake. Never mind...
>
>
> > > > +int pm_runtime_disable(struct device *dev)
> > > > +{
> > > ...
> > > > + if (dev->power.disable_depth++ > 0)
> > > > + goto out;
> > > > +
> > > > + if (dev->power.runtime_failure)
> > > > + goto out;
> > >
> > > I don't see why this is needed.
> >
> > If dev->power.runtime_failure, there's no need to do anything more.
>
> Don't you still want to deactivate the timer and kill any pending
> requests? True, they won't be able to do anything until the failure
> state is cleared, but even so...
OK, I'll drop that if ().
> > > > +void pm_runtime_remove(struct device *dev)
> > > > +{
> > > > + pm_runtime_disable(dev);
> > > > +
> > > > + if (dev->power.runtime_status == RPM_ACTIVE) {
> > > > + struct device *parent = dev->parent;
> > > > +
> > > > + /*
> > > > + * Change the status back to 'suspended' to match the initial
> > > > + * status.
> > > > + */
> > > > + pm_runtime_set_suspended(dev);
> > > > + if (parent && !parent->power.ignore_children)
> > > > + pm_request_idle(parent);
> > >
> > > Shouldn't these last two lines be part of __pm_runtime_set_status()?
> >
> > No. It is valid to call __pm_runtime_set_status() when runtime PM is disabled
> > for the device and I don't think we should kick the parent in such cases.
>
> Ah, an interesting point. Suppose the device is in RPM_ACTIVE, and
> then someone calls pm_runtime_disable followed by
> pm_runtime_set_suspended. Then the device's status would change to
> RPM_SUSPENDED and the parent's count of active children would be
> decremented, perhaps to 0. If the count does go to 0, why wouldn't you
> want to send out an idle notification for the parent?
OK, having reconsidered that, I think you're right, it should go into
__pm_runtime_set_status().
> > > > @@ -757,11 +771,15 @@ static int dpm_prepare(pm_message_t stat
> > > > dev->power.status = DPM_PREPARING;
> > > > mutex_unlock(&dpm_list_mtx);
> > > >
> > > > - error = device_prepare(dev, state);
> > > > + if (pm_runtime_disable(dev) && device_may_wakeup(dev))
> > > > + error = -EBUSY;
> > >
> > > What's the reason for the -EBUSY error?
> >
> > If this is a wake-up device and pm_runtime_disable(dev) returned 1 (it can only
> > return 1 or 0), which means there was a resume request pending for the device,
> > suspend fails with -EBUSY (wake-up event during suspend).
>
> I see. That's a little obscure; a comment would help. Even something
> as simple as:
>
> error = -EBUSY; /* wake-up during suspend */
OK
> > > > @@ -202,7 +203,9 @@ int driver_probe_device(struct device_dr
> > > > pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
> > > > drv->bus->name, __func__, dev_name(dev), drv->name);
> > > >
> > > > + pm_runtime_get_noresume(dev);
> > > > ret = really_probe(dev, drv);
> > > > + pm_runtime_put_noidle(dev);
> > >
> > > This is bad because it won't wait if there's a runtime-PM call in
> > > progress. Also, we shouldn't use put_noidle because it might subvert
> > > the driver's attempt to autosuspend.
> >
> > I'm not sure how that's possible, but whatever.
>
> Suppose the probe routine does:
>
> pm_runtime_get_sync(dev);
> /* do some work */
> pm_runtime_put_sync(dev);
>
> There is a clear expectation that an idle notification will eventually
> be sent. But if the probe is surrounded by
>
> pm_runtime_get_noresume(dev);
> ... probe ...
> pm_runtime_put_noidle(dev);
>
> then there won't be any idle notifications.
Ah, OK. So the point is that we should always idle-notify after .probe(),
because that's what .probe() would most probably want to do. I guess that
makes sense.
> > > > +2. Device Run-time PM Callbacks
> > >
> > > > +In particular, it is recommended that ->runtime_suspend() return -EBUSY if
> > > > +device_may_wakeup() returns 'false' for the device.
> > >
> > > What's the point of this? I don't understand -- we don't want to
> > > discourage people from suspending devices with wakeup enabled.
> >
> > device_may_wakeup(dev) == false means wake-up is disabled for dev, so
> > suspending it might not be a good idea.
>
> Okay. This needs to be rephrased. For example,
>
> In particular, if the driver requires remote wakeup capability
> for proper functioning and device_may_wakeup() returns 'false'
> for the device, then ->runtime_suspend() should return -EBUSY.
>
> The point is that plenty of drivers can work perfectly well without
> remote wakeup, so they have no reason to check device_may_wakeup().
OK
Thanks a lot again, I'll do my best to address the comments in the next version
of the patch.
Best,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
