Re: [PATCH][Alternative][RFC] PM / Runtime: Introduce driver runtime PM work routine

["Rafael J. Wysocki" <rjw@sisk.pl>]

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