Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On Út 27-12-05 20:22:04, Patrick Mochel wrote:
> 
> 
> On Tue, 27 Dec 2005, Pavel Machek wrote:
> 
> > Hi!
> >
> > > >  static ssize_t state_show(struct device * dev, struct device_attribute *attr, char * buf)
> > > >  {
> > > > -       return sprintf(buf, "%u\n", dev->power.power_state.event);
> > > > +       if (dev->power.power_state.event)
> > > > +               return sprintf(buf, "suspend\n");
> > > > +       else
> > > > +               return sprintf(buf, "on\n");
> > > >  }
> > >
> > > Are you sure that having only 2 options (suspend/on) is enough at the
> > > core level? I could envision having more levels, like "poweroff", etc?
> >
> > Note that interface is 0/2, currently, so this is improvement :-).
> 
> Heh, not really. You're not really solving any problems, only giving the
> illusion that you've actually fixed something.

Except perhaps userspace passing invalid values down to drivers in
pm_message_t.event?

> As I mentioned in the thread (currently happening, BTW) on the linux-pm
> list, what you want to do is accept a string that reflects an actual state
> that the device supports. For PCI devices that support low-power states,
> this would be "D1", "D2", "D3", etc. For USB devices, which only support
> an "on" and "suspended" state, the values that this patch parses would
> actually work.

We want _common_ values, anyway. So, we do not want "D0", "D1", "D2",
"D3hot" in PCI cases. We probably want "on", "D1", "D2", "suspend",
and I'm not sure about those "D1" and "D2" parts. Userspace should not
have to know about details, it will mostly use "on"/"suspend" anyway.

> > One day, when we find device that needs it, we may want to add more
> > states. I don't know about such device currently.
> 
> There are many devices already do - there are PCI, PCI-X, PCI Express,
> ACPI devices, etc that do. But, you simply cannot create a single
> decent

I asked for an example.
								Pavel
-- 
Thanks, Sharp!

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On St 04-01-06 17:06:09, Alan Stern wrote:
> On Wed, 4 Jan 2006, Pavel Machek wrote:
> 
> > > As I mentioned in the thread (currently happening, BTW) on the linux-pm
> > > list, what you want to do is accept a string that reflects an actual state
> > > that the device supports. For PCI devices that support low-power states,
> > > this would be "D1", "D2", "D3", etc. For USB devices, which only support
> > > an "on" and "suspended" state, the values that this patch parses would
> > > actually work.
> > 
> > We want _common_ values, anyway. So, we do not want "D0", "D1", "D2",
> > "D3hot" in PCI cases. We probably want "on", "D1", "D2", "suspend",
> > and I'm not sure about those "D1" and "D2" parts. Userspace should not
> > have to know about details, it will mostly use "on"/"suspend" anyway.
> 
> It would be good to make the details available so that they are there when
> needed.  For instance, we might export "D0", "on", "D1", "D2", "D3", and
> "suspend", treating "on" as a synonym for "D0" and "suspend" as a synonym
> for "D3".

Why to make it this complex?

I do not think there's any confusion possible. "on" always corresponds
to "D0", and "suspend" is "D3". Anyone who knows what "D2" means,
should know that, too...
							Pavel
-- 
Thanks, Sharp!

Re: [patch] pm: fix runtime powermanagement's /sys interface

[Scott E. Preece]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]


My inclination would be to have the sysfs interface know generic terms,
with the implementation mapping them to device-specific terms. It ought
to be possible to build portable tools that don't have to know about
device-specific states and have the device interfaces (in sysfs) do the
necessary translation.

However, I also think there is value in having the sysfs interface
recognize the device-specific values as well, so that device-specific
tools can also be written (offering the option of taking advantage of
special capabilities of a particular device).

scott

| On St 04-01-06 17:06:09, Alan Stern wrote:
| > On Wed, 4 Jan 2006, Pavel Machek wrote:
| > 
| > > > As I mentioned in the thread (currently happening, BTW) on the linux-pm
| > > > list, what you want to do is accept a string that reflects an actual state
| > > > that the device supports. For PCI devices that support low-power states,
| > > > this would be "D1", "D2", "D3", etc. For USB devices, which only support
| > > > an "on" and "suspended" state, the values that this patch parses would
| > > > actually work.
| > > 
| > > We want _common_ values, anyway. So, we do not want "D0", "D1", "D2",
| > > "D3hot" in PCI cases. We probably want "on", "D1", "D2", "suspend",
| > > and I'm not sure about those "D1" and "D2" parts. Userspace should not
| > > have to know about details, it will mostly use "on"/"suspend" anyway.
| > 
| > It would be good to make the details available so that they are there when
| > needed.  For instance, we might export "D0", "on", "D1", "D2", "D3", and
| > "suspend", treating "on" as a synonym for "D0" and "suspend" as a synonym
| > for "D3".
| 
| Why to make it this complex?
| 
| I do not think there's any confusion possible. "on" always corresponds
| to "D0", and "suspend" is "D3". Anyone who knows what "D2" means,
| should know that, too...
| 							Pavel
| -- 
| Thanks, Sharp!
| 
| --===============85566774732936779==
| ----------
| _______________________________________________
| linux-pm mailing list
| linux-pm@lists.osdl.org
| https://lists.osdl.org/mailman/listinfo/linux-pm
| 
| --===============85566774732936779==--

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Alan Stern]

On Thu, 5 Jan 2006, Scott E. Preece wrote:

> My inclination would be to have the sysfs interface know generic terms,
> with the implementation mapping them to device-specific terms. It ought
> to be possible to build portable tools that don't have to know about
> device-specific states and have the device interfaces (in sysfs) do the
> necessary translation.
> 
> However, I also think there is value in having the sysfs interface
> recognize the device-specific values as well, so that device-specific
> tools can also be written (offering the option of taking advantage of
> special capabilities of a particular device).

Yes, that was part of my point.

> | > It would be good to make the details available so that they are there when
> | > needed.  For instance, we might export "D0", "on", "D1", "D2", "D3", and
> | > "suspend", treating "on" as a synonym for "D0" and "suspend" as a synonym
> | > for "D3".
> | 
> | Why to make it this complex?
> | 
> | I do not think there's any confusion possible. "on" always corresponds
> | to "D0", and "suspend" is "D3". Anyone who knows what "D2" means,
> | should know that, too...

Not necessarily.  For instance, a particular driver might want to map 
"suspend" to D1 instead of to D3.

Given that "on" and "suspend" are generic names and not actual states (at 
least, not for PCI devices and presumably not for others as well), I think 
it makes sense to treat them specially.

And it's not all that complex.  Certainly no more complex than forcing
userspace tools to use {"on", "D1, "D2", "suspend"} instead of the
much-more-logical {"D0", "D1", "D2", "D3"}.

Alan Stern

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On Čt 05-01-06 11:41:34, Alan Stern wrote:
> On Thu, 5 Jan 2006, Scott E. Preece wrote:
> 
> > My inclination would be to have the sysfs interface know generic terms,
> > with the implementation mapping them to device-specific terms. It ought
> > to be possible to build portable tools that don't have to know about
> > device-specific states and have the device interfaces (in sysfs) do the
> > necessary translation.
> > 
> > However, I also think there is value in having the sysfs interface
> > recognize the device-specific values as well, so that device-specific
> > tools can also be written (offering the option of taking advantage of
> > special capabilities of a particular device).
> 
> Yes, that was part of my point.
> 
> > | > It would be good to make the details available so that they are there when
> > | > needed.  For instance, we might export "D0", "on", "D1", "D2", "D3", and
> > | > "suspend", treating "on" as a synonym for "D0" and "suspend" as a synonym
> > | > for "D3".
> > | 
> > | Why to make it this complex?
> > | 
> > | I do not think there's any confusion possible. "on" always corresponds
> > | to "D0", and "suspend" is "D3". Anyone who knows what "D2" means,
> > | should know that, too...
> 
> Not necessarily.  For instance, a particular driver might want to map 
> "suspend" to D1 instead of to D3.

Ok, lets change that to "on" and "off". That way, hopefully all the
drivers will agree that "off" means as low Dstate as possible.
 
> Given that "on" and "suspend" are generic names and not actual states (at 
> least, not for PCI devices and presumably not for others as well), I think 
> it makes sense to treat them specially.
> 
> And it's not all that complex.  Certainly no more complex than forcing
> userspace tools to use {"on", "D1, "D2", "suspend"} instead of the
> much-more-logical {"D0", "D1", "D2", "D3"}.

It is not much more logical. First, noone really needs D1 and
D2. Plus, people want to turn their devices on and off, and don't want
and should not have to care about details like D1.
							Pavel
-- 
Thanks, Sharp!

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Alan Stern]

On Thu, 5 Jan 2006, Pavel Machek wrote:

> > > | Why to make it this complex?
> > > | 
> > > | I do not think there's any confusion possible. "on" always corresponds
> > > | to "D0", and "suspend" is "D3". Anyone who knows what "D2" means,
> > > | should know that, too...
> > 
> > Not necessarily.  For instance, a particular driver might want to map 
> > "suspend" to D1 instead of to D3.
> 
> Ok, lets change that to "on" and "off". That way, hopefully all the
> drivers will agree that "off" means as low Dstate as possible.

Nothing wrong with that.  We could let "off" be another synonym.  Although 
I don't see the need for it, really.

> > Given that "on" and "suspend" are generic names and not actual states (at 
> > least, not for PCI devices and presumably not for others as well), I think 
> > it makes sense to treat them specially.
> > 
> > And it's not all that complex.  Certainly no more complex than forcing
> > userspace tools to use {"on", "D1, "D2", "suspend"} instead of the
> > much-more-logical {"D0", "D1", "D2", "D3"}.
> 
> It is not much more logical. First, noone really needs D1 and
> D2. Plus, people want to turn their devices on and off, and don't want
> and should not have to care about details like D1.

Who are you to say what people really need?  What about people who want to 
test their PCI device and see if it behaves properly in D1 or D2?  How are 
they going to do that if you don't let them put it in that state?

What about people with platform-specific non-PCI devices that have a whole
bunch of different internal power states?  Why force them to use only two
of those states?

The kernel isn't supposed to prevent people from doing perfectly legal
things.  The kernel should provide mechanisms to help people do what they
want.

Besides, as long as the sysfs interface accepts "on" and "suspend" (or 
"off"), what harm does it do to accept the device-specific names as 
well?

Alan Stern

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Patrick Mochel]

On Wed, 4 Jan 2006, Pavel Machek wrote:

> On Út 27-12-05 20:22:04, Patrick Mochel wrote:

> > Heh, not really. You're not really solving any problems, only giving the
> > illusion that you've actually fixed something.
>
> Except perhaps userspace passing invalid values down to drivers in
> pm_message_t.event?

It is up to the layer parsing the value to filter out bad values.

> > As I mentioned in the thread (currently happening, BTW) on the linux-pm
> > list, what you want to do is accept a string that reflects an actual state
> > that the device supports. For PCI devices that support low-power states,
> > this would be "D1", "D2", "D3", etc. For USB devices, which only support
> > an "on" and "suspended" state, the values that this patch parses would
> > actually work.
>
> We want _common_ values, anyway. So, we do not want "D0", "D1", "D2",
> "D3hot" in PCI cases. We probably want "on", "D1", "D2", "suspend",
> and I'm not sure about those "D1" and "D2" parts. Userspace should not
> have to know about details, it will mostly use "on"/"suspend" anyway.

D0 - D3 are common for all PCI devices. "on" and "suspend" are not device
states. They are conceptual representations of device states.

I understand where you are coming from. Most users will only care that a
particular device is "on" or "off". That is fine. They will click through
a gui that turns off a device and never think any more about it.

However, we are not developing an interface for end-users. We're
developing an interface that the guis may use. And, along with the guis,
there are also people that care about everything in between "on" and
"suspend".

If we export exactly the device states that a device supports, then we can
leave it up to the layers taking user input to translate between the real
device states and the user conceptions ("on", "suspend", etc), instead of
doing the translation ourselves.

That way, the kernel PM layers (as part of the bus drivers), can easily
and simply support every device that a device may have. What happens when
device manufacturers start coming out with D4, D5, etc (like they have
with the CPU C? States)? "suspend" takes on a completely different
meaning. With a simple (and accurate) kernel interface, this will never be
an issue.

> > > One day, when we find device that needs it, we may want to add more
> > > states. I don't know about such device currently.
> >
> > There are many devices already do - there are PCI, PCI-X, PCI Express,
> > ACPI devices, etc that do. But, you simply cannot create a single
> > decent
>
> I asked for an example.

Hey, calm down.

See for yourself:

for i in `lspci | cut -d ' ' -f 1 `
do
        pm_str=`sudo lspci -s $i -vv | grep -A 1 "Power Management" | grep
Flags | cut -d ' ' -f 4-5`;

        if [ ! -z "$pm_str" ]
        then
                printf "Device: %s   ==>  Power States: %s\n" $i "$pm_str"
        fi
done


I have a firewire controller in a desktop system, and a ATI Radeon in a
T42 that support D1 and D2..

Thanks,


	Patrick

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On Čt 05-01-06 16:37:52, Alan Stern wrote:
> > > And it's not all that complex.  Certainly no more complex than forcing
> > > userspace tools to use {"on", "D1, "D2", "suspend"} instead of the
> > > much-more-logical {"D0", "D1", "D2", "D3"}.
> > 
> > It is not much more logical. First, noone really needs D1 and
> > D2. Plus, people want to turn their devices on and off, and don't want
> > and should not have to care about details like D1.
> 
> Who are you to say what people really need?  What about people who want to 
> test their PCI device and see if it behaves properly in D1 or D2?  How are 
> they going to do that if you don't let them put it in that state?

> What about people with platform-specific non-PCI devices that have a whole
> bunch of different internal power states?  Why force them to use only two
> of those states?

Its okay with me to add more states _when they are needed_. Just now,
many drivers do not even handle system suspend/resume correctly.

> The kernel isn't supposed to prevent people from doing perfectly legal
> things.  The kernel should provide mechanisms to help people do what they
> want.

We are not adding random crap to kernel just because "someone may need
it". And yes, having aliases counts as "random crap". Perfectly legal
but totally useless things count as a random crap, too.

Bring example hardware that needs more than two states, implement
driver support for that, and then we can talk about adding more than
two states into core code.

								Pavel
-- 
Thanks, Sharp!

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Patrick Mochel]

On Thu, 5 Jan 2006, Scott E. Preece wrote:

> --===============26103097005026354==
>
>
> My inclination would be to have the sysfs interface know generic terms,
> with the implementation mapping them to device-specific terms. It ought
> to be possible to build portable tools that don't have to know about
> device-specific states and have the device interfaces (in sysfs) do the
> necessary translation.

Userspace should do the translation. You should give the user the ability
to specify simple, meaningful states, like "on" and "off". But, it should
be the tools itself that are mapping those requests to valid input for the
sysfs files.

Why force the translation into the kernel, and provide more opportunities
for error in parsing the sysfs files? Do it in userspace, and you can
afford much more flexibility and portability.

Thanks,


	Patrick

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Alan Stern]

On Thu, 5 Jan 2006, Patrick Mochel wrote:

> > It would be good to make the details available so that they are there when
> > needed.  For instance, we might export "D0", "on", "D1", "D2", "D3", and
> > "suspend", treating "on" as a synonym for "D0" and "suspend" as a synonym
> > for "D3".
> 
> Do it in userspace; the kernel doesn't need to know about "on" or
> "suspend". It should just validate and forward requests to enter specific
> states.

The problem is that the set of devices, drivers, and states is not 
bounded.  A single userspace tool might not know about all the 
device-specific states some weird driver supports.  The tool should still 
be able to ask the kernel to suspend the device without needing to know 
exactly which device state corresponds to "suspended".


On Thu, 5 Jan 2006, Pavel Machek wrote:

> Its okay with me to add more states _when they are needed_. Just now,
> many drivers do not even handle system suspend/resume correctly.

> We are not adding random crap to kernel just because "someone may need
> it". And yes, having aliases counts as "random crap". Perfectly legal 
> but totally useless things count as a random crap, too.
> 
> Bring example hardware that needs more than two states, implement
> driver support for that, and then we can talk about adding more than
> two states into core code.

Embedded devices are a great example.  Consider putting Linux on a 
portable phone.  The individual components can have many different power 
states, depending on which clock and power lines are enabled.  "on" and 
"suspend" won't be sufficient to handle the vendor's needs.

Alan Stern

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Dominik Brodowski]

On Thu, Jan 05, 2006 at 10:55:29PM +0100, Pavel Machek wrote:
> > I have a firewire controller in a desktop system, and a ATI Radeon in a
> > T42 that support D1 and D2..
> 
> Ok, now we have a concrete example. So Radeon supports D1. But putting
> radeon into D1 means you probably want to blank your screen and turn
> the backlight off; that takes *long* time anyway. So you can simply
> put your radeon into D3 and save a bit more power.

Using your logic, you never want to put your CPU into C2 power-saving state
instead of C3 or C4. Which is ridiculous. There are technical reasons why
you want to put devices into different power-saving states. E.g. wakeup
latency, ability to receive wakeup signals, snooping and so on.

In addition, your patch breaks pcmcia / pcmciautils which already uses
numbers (which I already had to change from "3" to "2" before...).

	Dominik

RE: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Preece Scott-PREECE]

User space has no particular reason to know which state of a particular
device corresponds to the logical state "on" or "off" or whatever other
states might be needed. Once you've defined the set of standard, generic
states, then the device driver writer can figure out which device state
matches the requirements for a given generic state.

While I wouldn't hate putting this in a system-level configuration file,
I really think it's device-specific stuff that should be built-in.

scott

-----Original Message-----
From: Patrick Mochel [mailto:mochel@digitalimplant.org] 
Sent: Thursday, January 05, 2006 3:48 PM
To: Preece Scott-PREECE
Cc: pavel@suse.cz; ; ; 
Subject: Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys
interface


On Thu, 5 Jan 2006, Scott E. Preece wrote:

> --===============26103097005026354==
>
>
> My inclination would be to have the sysfs interface know generic 
> terms, with the implementation mapping them to device-specific terms. 
> It ought to be possible to build portable tools that don't have to 
> know about device-specific states and have the device interfaces (in 
> sysfs) do the necessary translation.

Userspace should do the translation. You should give the user the
ability to specify simple, meaningful states, like "on" and "off". But,
it should be the tools itself that are mapping those requests to valid
input for the sysfs files.

Why force the translation into the kernel, and provide more
opportunities for error in parsing the sysfs files? Do it in userspace,
and you can afford much more flexibility and portability.

Thanks,


	Patrick

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Patrick Mochel]

On Thu, 5 Jan 2006, Pavel Machek wrote:

> Do you have the patch to filter bad values? I submitted one. You
> rejected it, because it does not support D1. Never mind that original
> code does not support D1, either. [Should I retransmit the patch?]

No, I offered guidance in one of the first emails. The code that exports a
'power' file for every single device from the driver model core should be
removed.

It should be replaced with a file exported by the bus driver that exports
the actual states that the device supports. The parsing can easily happen
at this point, because the bus knows what a good value is.

> If you suggest to just add check for == 0 or == 2... I think I can do
> that, but that's going to break userspace, anyway (it passes _3_
> there) and have no reasonable path to sane interface.

The userspace interface is broken. We can keep it for compatability
reasons, but there needs to be a new interface.

> There may be more than "D1" and "D2" between "D0" and "D3". There may
> be two different ways to put particular device into D1 sleep. This
> means buses and devices contributing states, and all this is
> complexity noone really wants.

It is not unnecessary, though. If the buses are the ones exporting the
states, then the number of states and the names of the states can be
accurate.

> > If we export exactly the device states that a device supports, then
> >we can
>
> Exporting multiple states is quite a lot of code, and it needs driver
> changes. There's no clear benefit.

I don't understand what you're saying. If I have a driver that I want to
make support another power state and I'm willing to write that code, then
there is a clear benefit to having the infrastructure for it to "just
work".

> > I have a firewire controller in a desktop system, and a ATI Radeon in a
> > T42 that support D1 and D2..
>
> Ok, now we have a concrete example. So Radeon supports D1. But putting
> radeon into D1 means you probably want to blank your screen and turn
> the backlight off; that takes *long* time anyway. So you can simply
> put your radeon into D3 and save a bit more power.
>
> So yes, Radeon supports D1, but we probably do not want to use it.

I'm sorry, but that's not your call to make.

If the device supports it, and if the driver supports it (or if it is made
to support it), then we don't want to modify the kernel (the core driver
model code, no less) just to add the ability to pass one more state down
to the driver, when we could get it for free now (by having the PCI bus
export the PCI device states it sees the device support via the config
space).

If you want a more concrete example, consider the possibility where it may
be possible to reinitialize the device from D1 or D2, but not from D3. For
the radeon, this is true in some cases (if I understand Ben H correctly).
There is also the case where screen blanking is either free or
non-existent (when the GPU is used for computation).

> [You may still want to do D1 on radeon for
> debugging/testing/something. Fine, but we are trying to build
> power-management infrastructure, not debugging one.]

I see those things as means to the same end. We want a good debugging
interface so people will actually try these things and use them. Plus,
testing is a big piece and going to get bigger - Many people and companies
want to know how much battery life they can squeeze out of a laptop, and
that evaluation involves experiementing with different configurations of
devices being in various states of suspend.

Thanks,


	Patrick

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On Čt 05-01-06 23:13:34, Dominik Brodowski wrote:
> On Thu, Jan 05, 2006 at 10:55:29PM +0100, Pavel Machek wrote:
> > > I have a firewire controller in a desktop system, and a ATI Radeon in a
> > > T42 that support D1 and D2..
> > 
> > Ok, now we have a concrete example. So Radeon supports D1. But putting
> > radeon into D1 means you probably want to blank your screen and turn
> > the backlight off; that takes *long* time anyway. So you can simply
> > put your radeon into D3 and save a bit more power.
> 
> Using your logic, you never want to put your CPU into C2 power-saving state
> instead of C3 or C4. Which is ridiculous. There are technical reasons why
> you want to put devices into different power-saving states. E.g. wakeup
> latency, ability to receive wakeup signals, snooping and so on.

Well, yes. Two years before we supported hlt, it would be stupid to
try to support multiple hlt states. We do not support hlt for any
devices today.

> In addition, your patch breaks pcmcia / pcmciautils which already uses
> numbers (which I already had to change from "3" to "2" before...).

pcmcia actually uses this? Ouch. Do you just read the power file, or
do you write to it, too?
								Pavel

-- 
Thanks, Sharp!

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Dominik Brodowski]

On Thu, Jan 05, 2006 at 11:23:38PM +0100, Pavel Machek wrote:
> > In addition, your patch breaks pcmcia / pcmciautils which already uses
> > numbers (which I already had to change from "3" to "2" before...).
> 
> pcmcia actually uses this? Ouch. Do you just read the power file, or
> do you write to it, too?

Reading and writing. Replacement for "cardctl suspend" and "cardctl resume".


static int pccardctl_power_one(unsigned long socket_no, unsigned int device,
                               unsigned int power)
{
        int ret;
        char file[SYSFS_PATH_MAX];
        struct sysfs_attribute *attr;

        snprintf(file, SYSFS_PATH_MAX,
                 "/sys/bus/pcmcia/devices/%lu.%u/power/state",
                 socket_no, device);

        attr = sysfs_open_attribute(file);
        if (!attr)
                return -ENODEV;

        ret = sysfs_write_attribute(attr, power ? "2" : "0", 1);

        sysfs_close_attribute(attr);

        return (ret);
}


NB: it will break one day, one way or another, when gregkh makes the
/sys/class -> /sys/devices conversion. However, I'd want to try not to break
the new pcmciautils userspace too often...

	Dominik

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On Čt 05-01-06 23:27:05, Dominik Brodowski wrote:
> On Thu, Jan 05, 2006 at 11:23:38PM +0100, Pavel Machek wrote:
> > > In addition, your patch breaks pcmcia / pcmciautils which already uses
> > > numbers (which I already had to change from "3" to "2" before...).
> > 
> > pcmcia actually uses this? Ouch. Do you just read the power file, or
> > do you write to it, too?
> 
> Reading and writing. Replacement for "cardctl suspend" and "cardctl resume".
> 
> 
> static int pccardctl_power_one(unsigned long socket_no, unsigned int device,
>                                unsigned int power)
> {
>         int ret;
>         char file[SYSFS_PATH_MAX];
>         struct sysfs_attribute *attr;
> 
>         snprintf(file, SYSFS_PATH_MAX,
>                  "/sys/bus/pcmcia/devices/%lu.%u/power/state",
>                  socket_no, device);
> 
>         attr = sysfs_open_attribute(file);
>         if (!attr)
>                 return -ENODEV;
> 
>         ret = sysfs_write_attribute(attr, power ? "2" : "0", 1);
> 
>         sysfs_close_attribute(attr);
> 
>         return (ret);
> }
> 
> 
> NB: it will break one day, one way or another, when gregkh makes the
> /sys/class -> /sys/devices conversion. However, I'd want to try not to break
> the new pcmciautils userspace too often...

Do you know how soon that day is?

I'd really like to change it into strings so that interface will not
have to change when/if someone will introduce multiple power states.

I could accept both "0" and "on" on input, but have to output single
value.
								Pavel 

-- 
Thanks, Sharp!

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Dominik Brodowski]

On Fri, Jan 06, 2006 at 12:08:49AM +0100, Pavel Machek wrote:
> Ok, so lets at least add value-checking to .../power file, and prevent
> userspace see changes to PM_EVENT_SUSPEND value. 2 and 0 are now
> "arbitrary cookies". I'd like to use "on" and "off", but pcmcia
> apparently depends on "2" and "0", so...
> 
> Any objections?

Sorry, but yes -- the same as before, minus the PCMCIA breakage.

	Dominik

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On Čt 05-01-06 15:54:15, Patrick Mochel wrote:
> 
> On Thu, 5 Jan 2006, Pavel Machek wrote:
> 
> > On Čt 05-01-06 14:15:39, Patrick Mochel wrote:
> 
> > > It should be replaced with a file exported by the bus driver that exports
> > > the actual states that the device supports. The parsing can easily happen
> > > at this point, because the bus knows what a good value is.
> >
> > (1) would change core<->driver interface
> 
> It's broken anyway for runtime power management.

Please explain. As far as I can see, it is fairly simple, but good
enough. pm_message_t.flags indicating it is runtime suspend would be
nice, but I do not think it is broken.

> > (2) is quite a lot of work
> 
> In the long run, it's not.

Nobody fixed it in a year, so apparently it is a lot of work.

> > (3) ...with very little benefit, until drivers support >2 states
> 
> Without it, you are preventing drivers from being able to support > 2
> states.

0 drivers support > 2 states. So it is indeed very little benefit.

> > If you want to rewrite driver model for >2 states, great, but that is
> > going to take at least a year AFAICS, so please let me at least fix
> > the bugs in meantime.
> 
> It's a band-aid; it is not a long-term solution.

But band-aid is apparently neccessary unless you want drivers to see
invalid values.

> > > The userspace interface is broken. We can keep it for compatability
> > > reasons, but there needs to be a new interface.
> >
> > I assumed we could fix the interface without actually introducing >2
> > states support. That can be done in reasonable ammount of code.
> 
> The interface is irreparably broken. You can't fix it with an infinite
> number of band aids.

Without "band aids", you'll get BUG()s all over the kernel.

> > > I don't understand what you're saying. If I have a driver that Iwant to
> >                                          ~~~~~~~~~~~~~~~~~~
> > > make support another power state and I'm willing to write that code, then
> > > there is a clear benefit to having the infrastructure for it to "just
> > > work".
> >
> > I do not see such drivers around me, that's all. It seems fair to me
> > that first driver author wanting that is the one who introduces >2
> > states support to generic infrastructure.
> 
> Just because you personally have not seen such things does not mean they
> do not exist.

Just because you claim they exist does not mean they exist.

> > Passing "on"/"off" down to radeon lets the driver decide what power
> > state it should enter.
> 
> Driver implements power state policy? Sounds like that policy would find a
> much more comfortable home in userspace.

Userspace can't know that driver does not support D3 on this
particular hardware version because of hardware problems... or simply
because it is not yet implemented.
								Pavel
-- 
Thanks, Sharp!

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Tom Marshall]

> > I have a firewire controller in a desktop system, and a ATI Radeon in a
> > T42 that support D1 and D2..
> 
> Ok, now we have a concrete example. So Radeon supports D1. But putting
> radeon into D1 means you probably want to blank your screen and turn
> the backlight off; that takes *long* time anyway. So you can simply
> put your radeon into D3 and save a bit more power.
> 
> So yes, Radeon supports D1, but we probably do not want to use it.

I understand this is a theoretical argument.  However, in reality, a
significant number of T42 owners get less than 12 hours of battery life in
S3 suspend because the Radeon chip sucks a huge amount of power with the
current code.  We would be eternally grateful if someone could figure out
why only some models are affected and, more importantly, submit a patch that
will reliably enter D2 (or D3 if it is supported?) for all Radeon 7500
chips.  I've found a couple patches that were submitted to this list but,
for whatever reason, nobody seems to have found a solution that is
acceptable yet.  I've been manually patching my kernels with code to enter
D2 for the last year or so, and from the volume of google results, it looks
like quite a lot of other folks are doing the same... :(

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek]

On Pá 06-01-06 06:34:14, Tom Marshall wrote:
> > > I have a firewire controller in a desktop system, and a ATI Radeon in a
> > > T42 that support D1 and D2..
> > 
> > Ok, now we have a concrete example. So Radeon supports D1. But putting
> > radeon into D1 means you probably want to blank your screen and turn
> > the backlight off; that takes *long* time anyway. So you can simply
> > put your radeon into D3 and save a bit more power.
> > 
> > So yes, Radeon supports D1, but we probably do not want to use it.
> 
> I understand this is a theoretical argument.  However, in reality, a
> significant number of T42 owners get less than 12 hours of battery life in
> S3 suspend because the Radeon chip sucks a huge amount of power with the
> current code.  We would be eternally grateful if someone could figure out
> why only some models are affected and, more importantly, submit a patch that
> will reliably enter D2 (or D3 if it is supported?) for all Radeon 7500
> chips.  I've found a couple patches that were submitted to this list but,
> for whatever reason, nobody seems to have found a solution that is
> acceptable yet.  I've been manually patching my kernels with code to enter
> D2 for the last year or so, and from the volume of google results, it looks
> like quite a lot of other folks are doing the same... :(

We were talking runtime pm here.
									Pavel

-- 
Thanks, Sharp!

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Adam Belay]

On Fri, Jan 06, 2006 at 10:42:24AM -0500, Alan Stern wrote:
> On Thu, 5 Jan 2006, Patrick Mochel wrote:
> 
> > On Fri, 6 Jan 2006, Pavel Machek wrote:
> > 
> > > On 05-01-06 16:04:07, Patrick Mochel wrote:
> > 
> > > > A better point, and one that would actually be useful, would be to remove
> > > > the file altogether. Let Dominik export a power file, with complete
> > > > control over the values, for each pcmcia device. Then you never have to
> > > > worry about breaking PCMCIA again.
> > >
> > > Fine with me.
> > 
> > ACK, you beat me to it.
> > 
> > And, appended is a patch to export PM controls for PCI devices. The file
> > "pm_possible_states" exports the states a device supports, and "pm_state"
> > exports the current state (and provides the interface for entering a
> > state).
> > 
> > Eventually, some drivers will want to fix up those values so that it can
> > mask of states that it doesn't support, as well as offer possible device-
> > specific states.
> > 
> > What's interesting is that with this patch, I can see that two more
> > devices on my system support D1 and D2 -- the cardbus controllers, which
> > are actually bridges whose PM capabilities aren't exported via lspci.
> 
> This trend is extremely alarming!!
> 
> It's a very bad idea to make bus drivers export and manage the syfs power 
> interface.  It means that lots of code gets repeated and different buses 
> do things differently.

In my opinion, the vast and often fundamentally different power
management specifications contribute greatly to the problem of
coordinated operating system controlled power management.  ACPI has
defined D0 - D3, and frankly, on x86 platforms, limiting the core interface
to those four states can be very functional.  Of course this isn't
pratical for the Linux PM layer because there several other important
platforms.  With that in mind, any generic representation of power
states has a tendency to be either overly complex or unacceptably limiting.

Considering these factors, I think allowing each bus to define its own
power management states and capabilities is a sensible option.  However,
I'm not convinced that it is necessary for these bus specific interfaces
to provide direct control of a device's power management states in most
situations.  That's not to say that some platforms won't need this
functionality but rather that PCI, USB, ACPI, and many others may not
want to provide userspace control of these low-level details.

As an alternative, it might be possible to allow each driver to export a
list of runtime power management states.  These states might revolve around
high level device class definitions rather than bus and platform interfaces.
The mechanism of reading and controlling these states could be similar to
the one you previously proposed for bus-level states.

As an example, a typical ethernet driver might export "on" and "off".
It doesn't matter if the ethernet device is PCI, ACPI, USB, etc.  The
key matter is that, for the "net" device class, most drivers will want 
to providee "on" and "off" as they correspond to "up" and "down". For the
PCI case, "off" will mean the highest (most off) D-state capable of
supporting the user's current wake settings.  This might be D2 if link
detect is enabled or D3 if it is disabled.  The actual PCI state can
be changed by the driver at any time, but the driver level state
dictates the drivers current intentions ("off" meaning save as much
power as would be possible while satisfying constraints).

Most sound card drivers will probably have more complex states.
They might be "on", "sleep", and "off".  "sleep" could be invoked as a
low latency state when the input and output lines have been quiet for
a certain uesr specified timeout period.  "off" could be be much higher
latency (some output might be lost i.e. skipping) and only invoked when
the audio interface has been closed from the userspace end.  Once again,
these states are not required to have a direct relation to bus level states.
A PCI sound card might remain in "D0" during the "sleep" state but turn off
many sub-components of the card and still save some power.

I think runtime power management is really all about what functionality
the drivers are willing provide.  If we focus on presenting bus-level power
management capabilities under a unified interface, then at best we are
ignoring the various subtleties of each specification (even ACPI and PCI
have minor differences), and at worst we're preventing drivers from revealing
the states that are actually important.  In other words, power management
can also be seen as a behavior, not just a power level.  Afterall, even
devices without bus level PM suport can save power just by doing things like
stopping DMA.  Even "virtual" devices can be seen as power-managable.

In short I'm suggesting the following:

1.) Every bus and device has its own unique PM mechanisms and specifications.
Representing this in a single unified model of any sort is nearly impossible.
Therefore, it may be best to allow each bus to define its own PM
infustructure and sysfs files (perhaps in a way similar to Pat's recent
patch).

2.) Device drivers on the other hand exist at a more abstract level and,
as a result, we have greater flexability and more options.  Therefore, I
think this is an excellent place to define power states and driver core PM
infustructure.

3.) System suspend and runtime power management are not even close to
similar.  Trying to use the same ->suspend and ->resume API is
ridiculious because it prevents intermediate power states and doesn't
properly perpare devices and device classes for a runtime environment.
Therefore, I'm in favor of a seperate interface tailored specifically for
runtime power management.

4.) If we're going to make any meaningful progress, we need to also
focus on device classes and class orriented power policy.  For example,
the "net" device class should provide infustructure and helper functions
for runtime power management of that flavor.  This might include some
generic "net" PM sysfs files.

Thanks,
Adam

Re: [linux-pm] [patch] pm: fix runtime powermanagement's /sys interface

[Dominik Brodowski]

Hi,

On Fri, Jan 06, 2006 at 05:25:16PM +0100, Kay Sievers wrote:
> On Fri, Jan 06, 2006 at 04:03:34PM +0100, Dominik Brodowski wrote:
> > On Thu, Jan 05, 2006 at 04:38:06PM -0800, Greg KH wrote:
> > > > NB: it will break one day, one way or another, when gregkh makes the
> > > > /sys/class -> /sys/devices conversion. However, I'd want to try not to break
> > > > the new pcmciautils userspace too often...
> > > 
> > > Why would the conversion that I'm working on break this userspace code?
> > > You are only using the device directory, which will not change at all.
> > 
> > Actually, pcmciautils uses both paths starting with
> > /sys/bus/pcmcia/devices/ and /sys/class/pcmcia_socket/pcmcia_socket%d/ --
> > and I was expecting that the latter path won't be available somewhen in
> > future?
> 
> That will just be resolved by a symlink. The access path to the device will
> stay the same, but the location of the "real" object will move to /devices.
> All classification will point into the unified /devices tree but keep
> the pathes as we have today. /sys/class will contain only symlinks in
> the end.

OK, thanks for this clarification!

	Dominik