[lkml]   [2009]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [RFC] Run-time PM framework (was: Re: [patch update] PM: Introduce core framework for run-time PM of I/O devices (rev. 6))
    On Thu, 2 Jul 2009, Rafael J. Wysocki wrote:

    > > > _and_ to ensure that these callbacks will be executed when it makes sense.
    > >
    > > Thus if the situation changes before the callback can be made, so that
    > > it no longer makes sense, the framework should cancel the callback.
    > Yes, but there's one thing to consider. Suppose a remote wake-up causes a
    > resume request to be queued up and pm_runtime_resume() is called synchronously
    > exactly at the time the request's work function is started. There are two
    > attempts to resume in progress, but only one of them can call
    > ->runtime_resume(), so what's the other one supposed to do? The asynchronous
    > one can just return error code, but the the caller of the synchronous
    > pm_runtime_resume() must know whether or not the resume was successful.
    > So, perhaps, if the synchronous resume happens to lose the race, it should
    > wait for the other one to complete, check the device's status and return 0 if
    > it's active? That wouldn't cause the workqueue thread to wait.

    I didn't address this explicitly in the previous message, but yes.
    This is no different from the way your current version works.

    Similarly, if a synchronous resume call occurs while a suspend is in
    progress, it should wait until the suspend finishes and then carry out
    a resume.

    > > We can summarize these rules as follows:
    > >
    > > Never allow more than one callback at a time, except that
    > > runtime_suspend may be invoked while runtime_idle is running.
    > Caution here. If ->runtime_idle() runs ->runtime_suspend() and immediately
    > after that resume is requested by remote wake-up, ->runtime_resume() may also
    > be run while ->runtime_idle() is still running.

    Yes, I didn't think of that case. We have to allow either of the other
    two to be invoked while runtime_idle is running. But we can rule out
    calling runtime_idle recursively.

    > OTOH, we need to know when ->runtime_idle() has completed, because we have to
    > ensure it won't still be running after run-time PM has been disabled for the
    > device.
    > IMO, we need two flags, one indicating that either ->runtime_suspend(), or
    > ->runtime_resume() is being executed (they are mutually exclusive) and the
    > the other one indicating that ->runtime_idle() is being executed. For the
    > purpose of further discussion below I'll call them RPM_IDLE_RUNNING and

    The RPM_IN_TRANSITION flag is unnecessary. It would always be equal to
    (status == RPM_SUSPENDING || status == RPM_RESUMING).

    > With this notation, the above rule may be translated as:
    > Don't run any of the callbacks if RPM_IN_TRANSITION is set. Don't run
    > ->runtime_idle() if RPM_IDLE_RUNNING is set.
    > Which implies that RPM_IDLE_RUNNING cannot be set when RPM_IN_TRANSITION is
    > set, but it is valid to set RPM_IN_TRANSITION when RPM_IDLE_RUNNING is set.

    That is equivalent to my conclusion above.

    > There are two possible "final" states, so I'd use one flag to indicate the
    > current status. Let's call it RPM_SUSPENDED for now (which means that the
    > device is suspended when it's set and active otherwise) and I think we can make
    > the rule that this flag is only changed after successful execution of
    > ->runtime_suspend() or ->runtime_resume().
    > Whether the device is suspending or resuming follows from the values of

    You can use two single-bit flags (SUSPEND and IN_TRANSITION) or a
    single two-bit state value (ACTIVE, SUSPENDING, SUSPENDED, RESUMING).
    It doesn't make much difference which you choose.

    > > Should the counters also be checked when the request is submitted?
    > > And should the same go for pm_schedule_suspend? These are nontrivial
    > > questions; good arguments can be made both ways.
    > That's the difficult part. :-)
    > First, I think a delayed suspend should be treated in a special way, because
    > it's not really a request to suspend. Namely, as long as the timer hasn't
    > triggered yet, nothing happens and there's nothing against the rules above.
    > A request to suspend is queued up after the timer has triggered and the timer
    > function is where the rules come into play. IOW, it consists of two
    > operations, setting up a timer and queuing up a request to suspend when the
    > timer triggers. IMO the first of them can be done at any time, while the other
    > one may be affected by the rules.

    I don't agree. For example, suppose the device has an active child
    when the driver says: Suspend it in 30 seconds. If the child is then
    removed after only 10 seconds, does it make sense to go ahead with
    suspending the parent 20 seconds later? No -- if the parent is going
    to be suspended, the decision as to when should be made at the time the
    child is removed, not beforehand.

    (Even more concretely, suppose there is a 30-second inactivity timeout
    for autosuspend. Removing the child counts as activity and so should
    restart the timer.)

    To put it another way, suppose you accept a delayed request under
    inappropriate conditions. If the conditions don't change, the whole
    thing was a waste of effort. And if the conditions do change, then the
    whole delayed request should be reconsidered anyhow. So why accept it?

    > It implies that we should really introduce a timer and a timer function that
    > will queue up suspend requests, instead of using struct delayed_work.

    Yes, this was part of my proposal.

    > Second, I think it may be a good idea to use the usage counter to block further
    > requests while submitting a resume request.
    > Namely, suppose that pm_request_resume() increments usage_count and returns 0,
    > if the resume was not necessary and the caller can do the I/O by itself, or
    > error code, which means that it was necessary to queue up a resume request.
    > If 0 is returned, the caller is supposed to do the I/O and call
    > pm_runtime_put() when done. Otherwise it just quits and ->runtime_resume() is
    > supposed to take care of the I/O, in which case the request's work function
    > should call pm_runtime_put() when done. [If it was impossible to queue up a
    > request, error code is returned, but the usage counter is decremented by
    > pm_request_resume(), so that the caller need not handle that special case,
    > hopefully rare.]

    Trying to keep track of reasons for incrementing and decrementing
    usage_count is very difficult to do in the core. What happens if
    pm_request_resume increments the count but then the driver calls
    pm_runtime_get, pm_runtime_resume, pm_runtime_put all before the work
    routine can run?

    It's better to make the driver responsible for maintaining the counter
    value. Forcing the driver to do pm_runtime_get, pm_request_resume is
    better than having the core automatically change the counter.

    > This implies that it may be a good idea to check usage_count when submitting
    > idle notification and suspend requests (where in case of suspend a request is
    > submitted by the timer function, when the timer has already triggered, so
    > there's no need to check the counter while setting up the timer).
    > The counter of unsuspended children may change after a request has been
    > submitted and before its work function has a chance to run, so I don't see much
    > point checking it when submitting requests.

    As I said above, if the counters don't change then the submission was
    unnecessary, and if they do change then the submission should be
    reconsidered. Therefore they _should_ be checked in submissions.

    > So, if the above idea is adopted, idle notification and suspend requests
    > won't be queued up when a resume request is pending (there's the question what
    > the timer function attempting to queue up a suspend request is supposed to do
    > in such a case) and in the other cases we can use the following rules:
    > Any pending request takes precedence over a new idle notification request.

    For pending resume requests this rule is unnecessary; it's invalid to
    submit an idle notification request while a resume request is pending
    (since resume requests can be pending only in the RPM_SUSPENDING and
    RPM_SUSPENDED states while idle notification requests are accepted only

    For pending suspends, I think we should allow synchronous idle
    notifications while the suspend is pending. The runtime_idle callback
    might then start its own suspend before the workqueue can get around to
    it. You're right about async idle requests though; that was the
    exception I noted below.

    > If a new request is not an idle notification request, it takes precedence
    > over the pending one, so it cancels it with the help of cancel_work().
    > [In the latter case, if a suspend request is canceled, we may want to set up the
    > timer for another one.] For that, we're going to need a single flag, say
    > RPM_PENDING, which is set whenever a request is queued up.

    That's what I called work_pending in my proposal.

    > > The error codes you have been using seem okay to me, in general.
    > >
    > > However, some of those requests would violate the rules in a trivial
    > > way. For these we might return a positive value rather than a negative
    > > error code. For example, calling pm_runtime_resume while the device is
    > > already active shouldn't be considered an error. But it can't be
    > > considered a complete success either, because it won't invoke the
    > > runtime_resume method.
    > That need not matter from the caller's point of view, though. In the case of
    > pm_runtime_resume() the caller will probably be mostly interested whether or
    > not it can do I/O after the function has returned.

    Yes. But the driver might depend on something happening inside the
    runtime_resume method, so it would need to know if a successful
    pm_runtime_resume wasn't going to invoke the callback.

    > > To be determined: How runtime PM will interact with system sleep.
    > Yes. My first idea was to disable run-time PM before entering a system sleep
    > state, but that would involve canceling all of the pending requests.

    Or simply freezing the workqueue.

    > > About all I can add is the "New requests override previous requests"
    > > policy. This would apply to all the non-synchronous requests, whether
    > > they are delayed or added directly to the workqueue. If a new request
    > > (synchronous or not) is received before the old one has started to run,
    > > the old one will be cancelled. This holds even if the new request is
    > > redundant, like a resume request received while the device is active.
    > >
    > > There is one exception to this rule: An idle_notify request does not
    > > cancel a delayed or queued suspend request.
    > I'm not sure if such a rigid rule will be really useful.

    A rigid rule is easier to understand and apply than one with a large
    number of special cases. However, in the statement of the rule above,
    I forgot to mention that this applies only if the new request is valid,
    i.e., if it's not forbidden by the current status or the counter

    > Also, as I said above, I think we shouldn't regard setting up the suspend
    > timer as queuing up a request, but as a totally separate operation.

    Well, there can't be any pending resume requests when the suspend timer
    is set up, so we have to consider only pending idle notifications or
    pending suspends. I agree, we would want to allow an idle notification
    to remain pending when the suspend timer is set up. As for pending
    suspends, we _should_ allow the new request to override the old one.
    This will come up whenever the timeout value is changed.

    Alan Stern

     \ /
      Last update: 2009-07-02 17:59    [W:0.045 / U:68.132 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site