Message-Id: <200906221749.50784.rjw@sisk.pl>
Date:	Mon, 22 Jun 2009 17:49:49 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Oliver Neukum <oliver@...kum.org>,
	Magnus Damm <magnus.damm@...il.com>,
	linux-pm@...ts.linux-foundation.org,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>, Greg KH <gregkh@...e.de>
Subject: Re: [patch update 3] PM: Introduce core framework for run-time PM of I/O devices

On Monday 22 June 2009, Alan Stern wrote:
> On Sun, 21 Jun 2009, Rafael J. Wysocki wrote:
> 
> > > Sorry, what I meant was that in each case the counter should be
> > > {inc,dec}remented if a new request had to be queued.  If one was
> > > already queued then the counter should be left alone.
> > > 
> > > The reason behind this is that a bunch of pm_request_suspend calls
> > > which all end up referring to the same workqueue item will result in a
> > > single async call to the runtime_suspend method.
> > 
> > Yes, that's why only the first one results in queuing up a request.
> > 
> > There is a problem with that if the later calls are supposed to use shorter
> > delays, but I have no real idea to handle this cleanly.
> 
> Nor do I.  When the time-of-last-use and delay fields are implemented, 
> this should never arise.

OK, so I'd like to leave it as is for now with the assumption that it's going
to be solved in future.

> > > Therefore they should cause a single decrement of the counter.  Likewise for
> > > pm_request_resume.
> > 
> > Hmm.  Why exactly do you think it's necessary to decrease the usage counter
> > in suspend functions?  You can't suspend a device more than once and you have
> > to resume it at the first request anyway.
> > 
> > I think it makes sense to increase the usage counter on every attempt to
> > resume, even if the device is not woken up as a result, because that means the
> > caller wants the device not to be suspended until the counter is decreased.
> > This way, even if the device is already active, multiple callers can prevent it
> > from suspending by calling pm_request_resume_get() or pm_runtime_resume_get()
> > and then dropping the references.
> 
> Again, this boils down to how drivers decide to use the async 
> interface.  I can see justifications for both pm_request_resume_get 
> (which would always increment the counter) and pm_request_resume (which 
> would increment the counter only if a work item had to be queued).

OK, so this means we should provide both at the core level and let the drivers
decide which one to use.

I think in both cases the caller would be responsible for decrementing the
counter?

> And of course, synchronous pm_runtime_resume should always increment the 
> counter.

Sure.

> > Now, we can also make pm_request_suspend() and pm_runtime_suspend() drop
> > the usage counter (if it's greater than zero), but that implies a usage model
> > in which a resume function called when I/O is started should be balanced with a
> > suspend function called after the I/O has been finished.
> > 
> > However, I'd prefer a usage model in which ->runtime_idle() is called when the
> > I/O is finished and the usage counter is zero and it decides whether to call a
> > suspend function.
> > 
> > So, perhaps I should make resume functions increase the usage counter
> > unconditionally and introduce pm_runtime_idle() to be called when the I/O is
> > done?  That is, pm_runtime_idle() will decrement the usage counter, check if
> > it's zero and call ->runtime_idle() when that's the case (well, this is what
> > pm_runtime_put_notify() does right now, but maybe the name is wrong).
> 
> Maybe it should just be called pm_runtime_put.  There could be a
> separate pm_runtime_idle that doesn't decrement the counter but invokes
> the callback if the counter is already 0.  (This could be useful after
> a runtime_resume method returned -EBUSY.)

OK

> > Also, there should be a function to use when it's only necessary to drop the
> > usage counter, without calling ->runtime_idle() (for example, if another code
> > path is supposed to call a suspend function directly).
> 
> I don't see any reason for that.  It says: "The device isn't in use any
> more, but even though we support autosuspend we aren't going to try to
> suspend it now."  What's the point?  And as for the other code path, if
> the device is already suspended when it calls the suspend function
> directly, there's no harm done.

OK

Best,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists