Alan,
many thanks for your extensive feedback!
On Mon, Mar 22, 2010 at 11:38:44AM -0400, Alan Stern wrote:
> On Sun, 21 Mar 2010, Dominik Brodowski wrote:
> > (2) Whenever two functions local to drivers/pcmcia/ds.c --
> > runtime_suspend() or runtime_resume() -- are called,
> > we need to force a synchronous call to pcmcia_dev_suspend(),
> > or to pcmcia_dev_resume(). Currently, we do this by taking
> > device_lock() and calling the function manually.
>
> That seems like the right thing to do. (Except that I don't know what
> your device_lock() routine is for.)
Actually, that's the generic device_lock(), which replaced down(&dev->sem)
... which is used here to not let a reset or a runtime suspend/resume race
with a system sleep event.
> > (3) However, it'd be much nicer to be able to use the new runtime
> > PM interface.
>
> I don't understand. Aren't runtime_suspend() and runtime_resume(), as
> mentioned above, already part of your runtime PM interface? Or at
> least, intended to be part of your runtime PM interface?
Well, they are part of the PCMCIA-local runtime PM interface -- it is based
around the concept that an user may, whenever she wants, can set a PCMCIA
device (or the whole card) to the "suspend" state.
> > As the PCMCIA bus can always suspend,
>
> Is that really literally true? Don't you run into trouble if you try
> to suspend while a data transfer is taking place? And if you've got a
> whole series of transfers going on, does it really make sense to
> suspend after each individual transfer, only to resume immediately when
> the next transfer comes along?
>
> How long should a pcmcia device have to remain idle before suspending
> it will actually end up saving energy?
>
> How does your driver determine when the device becomes idle (and just
> as important, stops being idle)?
Well, all of this is done by the user. I agree that it might make sense to
take all these issues into consideration -- e.g. by get/put commands used by
the ATA layer etc.
> > To enable it, we should add
> >
> > pm_runtime_set_active(&p_dev->dev);
> > pm_runtime_enable(&p_dev->dev);
> >
> > in pcmcia_device_add().
>
> Yes.
>
> > (4) What is then to be done to modify runtime_suspend() / runtime_resume() ?
> > Is it:
> >
> > + static int runtime_suspend(struct device *dev)
> > + {
> > + pm_runtime_put_noidle(dev);
> > + return pm_runtime_suspend(dev);
> > + }
>
> This makes no sense at all. pm_runtime_suspend() calls your
> runtime_suspend() function, not the other way around.
Well, that's not what this "runtime_suspend()" does -- it is the wrapper for
userspace-issued suspend commands.
> > (5) Another problem I'm seeing with the put/get approach: there are three
> > different callpaths where runtime_suspend() / runtime_resume() might
> > be called.[*] However, each one decreases the counter, which might get < 0.
>
> The counter should never become negative; if it does, there's a bug in
> your driver. Every decrement should match a corresponding increment,
> just like with refcounts. And the device should never be
> runtime-suspended unless the counter is 0.
what's the initial value, actually? Is it 0 or 1?
> One of the background assumptions behind the runtime PM
> framework is that users don't tell the system when to suspend devices.
Ah, that's the fundamental issue. I agree that the PCMCIA subsystem does
something unusual here (relating to PCMCIA cards or PCMCIA devices) --
but I would prefer not to break how the system currently works.
> Instead, the system automatically suspends devices when it realizes
> they aren't being used.
That's what we could use for PCMCIA sockets.
> > --- a/drivers/pcmcia/Kconfig
> > +++ b/drivers/pcmcia/Kconfig
> > @@ -20,6 +20,7 @@ if PCCARD
> > config PCMCIA
> > tristate "16-bit PCMCIA support"
> > select CRC32
> > + select PM_RUNTIME
>
> This seems very wrong. Why do you need it? Can't the pcmcia subsystem
> work correctly without runtime PM support? Hasn't it been working
> correctly that way all along?
Oh yes, it did. However, continuing support for the PCMCIA-style "runtime
PM" might need to depend on PM_RUNTIME.
Will do write (and possibly test) some patches first, then get back to you
-- many thanks for your feedback. Really appreciate it!
Best,
Dominik
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
