lkml.org 
[lkml]   [2012]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH][Alternative][RFC] PM / Runtime: Introduce driver runtime PM work routine
Date
On Sunday, August 12, 2012, Alan Stern wrote:
> On Thu, 9 Aug 2012, Rafael J. Wysocki wrote:
>
> > There are some known concerns about this approach.
> >
> > First of all, the patch at
> >
> > https://patchwork.kernel.org/patch/1299781/
> >
> > increases the size of struct device by the size of a pointer, which may seem to
> > be a bit excessive to somebody, although I personally don't think it's a big
> > problem. We don't use _that_ many struct device objects for it to matter much.
> >
> > Second, which is more important to me, it seems that for a given device func()
> > will always be the same pointer and it will be used by the device's driver
> > only. In that case, most likely, it will be possible to determine the
> > address of func() at the time of driver initialization, so the setting and
> > clearing of power.func and passing the address of func() as an argument every
> > time __pm_runtime_get_and_call() is run may turn out to be an unnecessary
> > overhead. Thus it may be more efficient to use a function pointer in struct
> > device_driver (it can't be located in struct dev_pm_ops, because some drivers
> > don't use it at all, like USB drivers, and it wouldn't be useful for subsystems
> > and PM domains) to store the address of func() permanently.
> >
> > For the above reasons, the appended patch implements an alternative approach,
> > which is to modify the way pm_runtime_get() works so that, when the device is
> > not active, it will queue a resume request for the device _along_ _with_ the
> > execution of a driver routine provided through a new function pointer
> > .pm_runtime_work(). There also is pm_runtime_get_nowork() that won't do that
> > and works in the same way as the "old" pm_runtime_get().
> >
> > Of course, the disadvantage of the new patch is that it makes the change
> > in struct device_driver, but perhaps it's not a big deal.
> >
> > I wonder what you think.
>
> I have some concerns about this patch.
>
> Firstly, the patch doesn't do anything in the case where the device is
> already at full power.

This is intentional, because I'm not sure that the code to be run
if pm_runtime_get() returns 1 should always be pm_runtime_work().

For example, the driver may want to acquire a lock before running
pm_runtime_get() and execute that code under the lock.

> Should we invoke the callback routine
> synchronously? This loses the advantage of a workqueue's "pristine"
> environment, but on the other hand it is much more efficient.

I'm not sure if it is always going to be more efficient.

> (And we're talking about hot pathways, where efficiency matters.) The
> alternative, of course, is to have the driver invoke the callback
> directly if pm_runtime_get() returns 1.

Sure. If every user of .pm_runtime_work() ends up calling it when
pm_runtime_get() returns 1, then there will be a point to modify the
core to do that instead. However, I'm not sure if that's going to be the
case at the moment.

> Secondly, is it necessary for __pm_runtime_barrier() to wait for the
> callback routine?

I believe so. At least that's what is documented about __pm_runtime_barrier().

> More generally, does __pm_runtime_barrier() really
> do what it's supposed to do? What happens if it runs at the same time
> as another thread is executing the pm_runtime_put(parent) call at the
> end of rpm_resume(), or the rpm_resume(parent, 0) in the middle?

So these are two different situations. When pm_runtime_put(parent) is
executed, the device has been resumed and no runtime PM code _for_ _the_
_device_ is running (although the trace_rpm_return_int() is at a wrong
place in my opinion). The second one is more interesting, but it really
is equivalent to calling pm_runtime_resume() (in a different thread)
after __pm_runtime_barrier() has run.

> Thirdly, I would reorganize the new code added to pm_runtime_work();
> see below.
>
>
> > @@ -533,6 +550,13 @@ static int rpm_resume(struct device *dev
> > goto out;
> > }
> >
> > + if ((rpmflags & RPM_ASYNC) && (rpmflags & RPM_RUN_WORK)) {
> > + dev->power.run_driver_work = true;
> > + rpm_queue_up_resume(dev);
> > + retval = 0;
> > + goto out;
> > + }
> > +
>
> The section of code just before the start of this hunk exits the
> routine if the device is already active. Do you want to put this new
> section in the preceding "if" block?

Yes, I do. This is to ensure that the execution of pm_runtime_work() will
be scheduled if RPM_RUN_WORK is set.

> Also, it feels odd to have this code here when there is another section
> lower down that also tests for RPM_ASYNC and does almost the same
> thing. It suggests that this new section isn't in the right place.

Yes, it does. However, the code between the two questions contains some checks
that I want to skip if RPM_RUN_WORK is set (otherwis, the execution of
pm_runtime_work() may not be scheduled at all).

> For instance, consider what happens in the "no_callbacks" case where
> the parent is already active.

The no_callbacks case is actually interesting, because I think that the
function should return 1 in that case. Otherwise, the caller of
pm_runtime_get() may think that it has to wait for the device to resume,
which isn't the case. So, this seems to need fixing now.

Moreover, if power.no_callbacks is set, the RPM_SUSPENDING and RPM_RESUMING
status values are impossible, as far as I can say, so the entire "no callbacks"
section should be moved right after the check against RPM_ACTIVE. The same
appears to apply to the analogous "no callbacks" check in rpm_suspend() (i.e.
it should be done earlier).

After those changes I'd put "my" check against RPM_RUN_WORK after the
"no callbacks" check, but before the "RPM_SUSPENDING or RPM_RESUMING" one.

> > @@ -715,11 +736,29 @@ static void pm_runtime_work(struct work_
> > rpm_suspend(dev, RPM_NOWAIT | RPM_AUTO);
> > break;
> > case RPM_REQ_RESUME:
> > - rpm_resume(dev, RPM_NOWAIT);
> > + if (dev->power.run_driver_work && dev->driver->pm_runtime_work)
> > + driver_work = dev->driver->pm_runtime_work;
> > +
> > + dev->power.run_driver_work = false;
> > + rpm_resume(dev, driver_work ? 0 : RPM_NOWAIT);
> > break;
> > }
> >
> > out:
> > + if (driver_work) {
> > + pm_runtime_get_noresume(dev);
> > + dev->power.work_in_progress = true;
> > + spin_unlock_irq(&dev->power.lock);
> > +
> > + driver_work(dev);
> > +
> > + spin_lock_irq(&dev->power.lock);
> > + dev->power.work_in_progress = false;
> > + wake_up_all(&dev->power.wait_queue);
> > + pm_runtime_put_noidle(dev);
> > + rpm_idle(dev, RPM_NOWAIT);
> > + }
> > +
>
> It seems very illgical to do all the callback stuff here, after the
> "switch" statement. IMO it would make more sense to put it all
> together, more or less as follows:
>
> case RPM_REQ_RESUME:
> if (dev->power.run_driver_work && dev->driver->pm_runtime_work) {
> driver_work = dev->driver->pm_runtime_work;
> dev->power.run_driver_work = false;
> dev->power.work_in_progress = true;
> pm_runtime_get_noresume(dev);
> rpm_resume(dev, 0);
> spin_unlock_irq(&dev->power.lock);
>
> driver_work(dev);
>
> spin_lock_irq(&dev->power.lock);
> dev->power.work_in_progress = false;
> wake_up_all(&dev->power.wait_queue);
> pm_runtime_put_noidle(dev);
> rpm_idle(dev, RPM_NOWAIT);
> } else {
> rpm_resume(dev, RPM_NOWAIT);
> }
> break;

OK

> Notice also that it's important to do the _get_noresume _before_
> calling rpm_resume(). Otherwise the device might get suspended again
> before the callback gets a chance to run.

You're right. I forgot about dropping the lock in order to call
pm_runtime_put(parent).

Thanks,
Rafael


\
 
 \ /
  Last update: 2012-08-13 01:02    [W:0.706 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site