Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 1 Aug 2012, Rafael J. Wysocki wrote:

> What I really thought about was to do _get_noresume(), then check if the
> device is active and if not, queue up a work item (or another delayed
> execution in process context) that will do pm_runtime_resume() and
> then access the hardware.
> 
> Why do I think it's better than plain pm_runtime_get()?  Because the code
> path calling pm_runtime_resume() will know that it is safe to access the
> hardware after it has returned (unless it returns an error code, but
> that's exceptional).
> 
> In contrast, with pm_runtime_get() there is no way to predict when the
> device is going to be resumed, so if the device happens to be suspended
> when pm_runtime_get() returns, the driver kind of has to poll it until
> it becomes active, or use a wait queue woken up from the resume callback,
> or do all of the processing in the resume callback itself (like in the
> example you mentioned).  I'm not sure if the expectation that all driver
> writers will be able to implement any of these options correctly is a realistic
> one.

I don't know about that -- the logic involved in doing the processing 
within the resume callback isn't terribly complicated.  At least, not 
much more complicated than the logic involved in setting up a custom 
work routine as you suggest.

Anyway with the existing code, driver writers are free to choose 
whichever approach they prefer.

> > Check out the sample driver code in section 9 of 
> > Documentation/power/runtime_pm.txt, especially the foo_read_or_write() 
> > routine.
> 
> Well, that shouldn't need the is_suspended flag at all, methinks, and the
> reason it does need it is because it uses pm_runtime_get().

Not so.  Consider your scheme.  When starting an I/O request, you call
pm_runtime_get_noresume() and then check to see if the device is
active, say by calling pm_runtime_suspended().  Suppose at that moment
the suspend callback has just finished and has released the private
spinlock.  The device's status is still RPM_SUSPENDING, so
pm_runtime_suspended() returns 0 and you try to carry out the I/O.

To fix this problem you have to synchronize the status checking with
the suspend/resume operations.  This means the status changes have to
occur under the protection of the private lock, which means a private
flag is needed.

>  Moreover,
> processing requests in the resume callback is not something I'd recommend
> to anyone, because that's going to happen when the status of the device
> is RPM_RESUMING, we're holding a reference on the parent (if there's one) etc.

I don't see any problem with that.  The parent's child_count will be 
incremented while the requests are being processed regardless.  And if 
you've been waiting for the device to resume in order to carry out some 
processing, within the resume callback is the logical place to do the 
work.  It avoids the overhead of a second context switch.

> So, it looks like I don't really agree with the example. :-)

Feel free to add your scheme as a second example in the document.  :-)
But please don't remove the first example, unless you can find 
something actually wrong with it.

> > While you're changing names around, consider also adding a "_runtime"  
> > somewhere to:
> > 
> > 	pm_children_suspended,
> > 	pm_schedule_suspend,
> > 	pm_request_idle,
> > 	pm_request_resume,
> > 	pm_request_autosuspend.
> > 
> > For example, we could have pm_runtime_idle_async instead of 
> > pm_request_idle.
> 
> Well, these are not as misleading as pm_runtime_get(), at least in principle.

No, but it would be good to be more consistent about our naming.  
Making sure all the function names contain "_runtime" would help.

I suppose we could keep pm_runtime_get_sync as is, and just change
pm_runtime_get to pm_runtime_get_async (and likewise for _put).  That
could reduce the confusion during the changeover.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux