pm: fix runtime powermanagement's /sys interface

drivers/base/power/sysfs.c line 34

Quoted 8 lines

static ssize_t state_show(struct device * dev, struct device_attribute *attr, char * buf) {

drivers/base/power/sysfs.c +4 -1

-       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?

Hi!

Quoted 9 lines

Quoted 8 lines

static ssize_t state_show(struct device * dev, struct device_attribute *attr, char * buf) {

drivers/base/power/sysfs.c +4 -1

-       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");
 }

Quoted 2 lines

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 :-).

One day, when we find device that needs it, we may want to add more states. I don't know about such device currently.

Quoted 11 lines · Pavel Machek

Hi!

Quoted 9 lines

Quoted 8 lines

static ssize_t state_show(struct device * dev, struct device_attribute *attr, char * buf) {

drivers/base/power/sysfs.c +4 -1

-       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");
 }

Quoted 4 lines

Quoted 2 lines

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.

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.

Quoted 2 lines

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

Quoted 6 lines

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.

Quoted 11 lines · Pavel Machek

Quoted 6 lines

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".

Quoted 11 lines · Pavel Machek

Quoted 6 lines

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".

Quoted 4 lines

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.

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 2 lines · Patrick Mochel

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?

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 1 line · Pavel Machek

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 11 lines

Quoted 6 lines

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

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

Quoted 2 lines

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.

Quoted 7 lines · Pavel Machek

Quoted 2 lines

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...).

Quoted 2 lines

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?

Quoted 5 lines · Pavel Machek

Quoted 2 lines

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...

Quoted 5 lines · Pavel Machek

Quoted 2 lines

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...

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?

Quoted 6 lines · Pavel Machek

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.

Quoted 9 lines · Dominik Brodowski

Quoted 6 lines · Pavel Machek

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.

I don't understand at this point.

Current code takes value from user, and passes it down to driver, without any checking. If user writes 666 into the file, it will happily pass down 666 to the driver. Driver does not expect 666 in pm_message_t.event. It may oops, BUG_ON() or anything like that.

Shall I change

#define PM_EVENT_SUSPEND 2

to

#define PM_EVENT_SUSPEND 1324

to get my point across? This is kernel-specific value, it should not be exported to userland.

Quoted 26 lines · Pavel Machek

Quoted 9 lines · Dominik Brodowski

Quoted 6 lines · Pavel Machek

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.

I don't understand at this point.

Current code takes value from user, and passes it down to driver, without any checking. If user writes 666 into the file, it will happily pass down 666 to the driver. Driver does not expect 666 in pm_message_t.event. It may oops, BUG_ON() or anything like that.

Shall I change

#define PM_EVENT_SUSPEND 2

to

#define PM_EVENT_SUSPEND 1324

Quoted 2 lines

to get my point across? This is kernel-specific value, it should not be exported to userland.

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.

Quoted 4 lines · Patrick Mochel

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.

Quoted 8 lines · Pavel Machek

Quoted 4 lines · Patrick Mochel

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.

Quoted 21 lines · Patrick Mochel

Quoted 8 lines · Pavel Machek

Quoted 4 lines · Patrick Mochel

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.

Quoted 3 lines

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.

Already we have PCI exporting "pm_possible_states" and "pm_state" while PCMCIA exports "suspend". How many other different schemes are going to crop up? How much bus-specific information will have to be built into a user utility?

If possible states are represented as arrays of pointers to strings, then the PM core can easily supply the sysfs interface. If Patrick's patch were re-written so that the sysfs interface were moved into the PM core, leaving only the PCI-specific portions in the PCI drivers, I would be much happier. This would also mean that Dominik's patch could be replaced by something a good deal smaller.

And it wouldn't hurt to add some mechanism for indicating which of the possible states is the generic "suspend" state (usually D3 for PCI devices, but not necessarily).

Quoted 17 lines · Alan Stern

On Thu, 5 Jan 2006, Patrick Mochel wrote:
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.

Already we have PCI exporting "pm_possible_states" and "pm_state" while PCMCIA exports "suspend". How many other different schemes are going to crop up? How much bus-specific information will have to be built into a user utility?

If possible states are represented as arrays of pointers to strings, then the PM core can easily supply the sysfs interface. If Patrick's patch were re-written so that the sysfs interface were moved into the PM core, leaving only the PCI-specific portions in the PCI drivers, I would be much happier. This would also mean that Dominik's patch could be replaced by something a good deal smaller.

Quoted 3 lines

And it wouldn't hurt to add some mechanism for indicating which of the possible states is the generic "suspend" state (usually D3 for PCI devices, but not necessarily).

I think we should start with string-based interface, with just two states ("on" and "off"). That is easily extensible into future, and suits current PCMCIA nicely. It also allows us to experiment with PCI power management... I can cook up a patch, but it will be simple reintroduction of .../power file under different name.

Quoted 22 lines · Pavel Machek

Quoted 17 lines · Alan Stern

On Thu, 5 Jan 2006, Patrick Mochel wrote:
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.

Already we have PCI exporting "pm_possible_states" and "pm_state" while PCMCIA exports "suspend". How many other different schemes are going to crop up? How much bus-specific information will have to be built into a user utility?

If possible states are represented as arrays of pointers to strings, then the PM core can easily supply the sysfs interface. If Patrick's patch were re-written so that the sysfs interface were moved into the PM core, leaving only the PCI-specific portions in the PCI drivers, I would be much happier. This would also mean that Dominik's patch could be replaced by something a good deal smaller.

Quoted 3 lines

And it wouldn't hurt to add some mechanism for indicating which of the possible states is the generic "suspend" state (usually D3 for PCI devices, but not necessarily).

Quoted 5 lines

I think we should start with string-based interface, with just two states ("on" and "off"). That is easily extensible into future, and suits current PCMCIA nicely. It also allows us to experiment with PCI power management... I can cook up a patch, but it will be simple reintroduction of .../power file under different name.

The driver core can provide some infustructure, but let's leave the states up to the drivers. Afterall, some drivers might only be interested in "on" during runtime. Also, drivers might support some sort of partial-off but not "off".

And no, this does not allow us to experiment with PCI power management.

Also there's nothing "runtime" about the PCMCIA PM API. It's much more like calling ->remove() as it disabled the device all together.

I'm more
interested in saving power without crippling functionality.

Quoted 4 lines · Adam Belay

The driver core can provide some infustructure, but let's leave the states up to the drivers. Afterall, some drivers might only be interested in "on" during runtime. Also, drivers might support some sort of partial-off but not "off".

Then they map "off" to "as much off as I can". Big deal.

Quoted 3 lines

And no, this does not allow us to experiment with PCI power management.

Well, suse was already doing experiments with 2.6.14 sysfs...

Quoted 3 lines

Also there's nothing "runtime" about the PCMCIA PM API. It's much more like calling ->remove() as it disabled the device all together.

It looks enough runtime to me.

Quoted 3 lines

I'm more
interested in saving power without crippling functionality.

You are on wrong mailing list, then. Seriously. If you can save power without affecting functionality, then just doing, you don't need any userland interface.

Quoted 4 lines

Quoted 2 lines

Also there's nothing "runtime" about the PCMCIA PM API. It's much more like calling ->remove() as it disabled the device all together.

It looks enough runtime to me.

As was already discussed, we don't want to modify every userspace application to check if the device it needs is "on" (resumed) or "off" (suspended). It's just two painful with third party apps etc. Therefore, the kernel needs to handle this more seemlessly. In my

Quoted 1 line

This trend is extremely alarming!!

It scares me a bit, too.

I think we should start with string-based interface, with just two states ("on" and "off"). That is easily extensible into future, and suits current PCMCIA nicely. It also allows us to experiment with PCI power management... I can cook up a patch, but it will be simple reintroduction of .../power file under different name.

Quoted 18 lines · Patrick Mochel

Quoted 8 lines · Pavel Machek

Quoted 4 lines · Patrick Mochel

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).

Quoted 7 lines

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 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.

Quoted 8 lines · Pavel Machek

Quoted 4 lines · Patrick Mochel

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

drivers/pci/pm.c new file +4 -0

+static ssize_t pm_possible_states_show(struct device * d,
+				       struct device_attribute * a,
+				       char * buffer)
+{

Quoted 9 lines · Patrick Mochel

drivers/pci/pm.c new file +9 -0

+static ssize_t pm_possible_states_show(struct device * d,
+				       struct device_attribute * a,
+				       char * buffer)
+{
+	struct pci_dev * dev = to_pci_dev(d);
+	char * s = buffer;
+
+	s += sprintf(s, "d0");
+	if (dev->poss_states[PCI_D1])

drivers/pci/pm.c line 21

Quoted 5 lines

drivers/pci/pm.c new file +5 -0

+		s += sprintf(s, " d1");
+	if (dev->poss_states[PCI_D2])
+		s += sprintf(s, " d2");
+	if (dev->poss_states[PCI_D3hot])
+		s += sprintf(s, " d3");

Could we use same states as rest of code, i.e. "D2" instead of "d2" and "D3hot" instead of "d3"?

drivers/pci/pm.c new file +7 -0

+	s += sprintf(s, "d0");
+	if (dev->poss_states[PCI_D1])
+		s += sprintf(s, " d1");
+	if (dev->poss_states[PCI_D2])
+		s += sprintf(s, " d2");
+	if (dev->poss_states[PCI_D3hot])
+		s += sprintf(s, " d3");

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).

Hi,

Quoted 4 lines · Patrick Mochel

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).

Your patch doesn't handle the PM dependencies, unfortunately...

Thanks,
Dominik

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.

Quoted 4 lines · Patrick Mochel

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.

Then I'll try to push this into 2.6.16 later on; also I still need to fix up userspace accordingly...

BTW, ACK on the removal of power/state from me... PCMCIA shouldn't hold this removal up.

Dominik

drivers/pcmcia/ds.c +0 -0

index 0252582..a4f3258 100644
@@ -920,6 +920,37 @@ pcmcia_device_stringattr(prod_id2, prod_
 pcmcia_device_stringattr(prod_id3, prod_id[2]);
 pcmcia_device_stringattr(prod_id4, prod_id[3]);
 

Quoted 5 lines · Pavel Machek

Quoted 2 lines

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...

Quoted 3 lines

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.

Quoted 4 lines · Greg KH

Quoted 3 lines

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...

Quoted 2 lines

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?

Quoted 5 lines · Dominik Brodowski

Quoted 4 lines · Greg KH

Quoted 3 lines

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...

Quoted 7 lines

Quoted 2 lines

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.

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?]

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.

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

[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.]

Quoted 3 lines · Pavel Machek

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.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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 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".

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).

Quoted 3 lines · Patrick Mochel

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

(2) is quite a lot of work

(3) ...with very little benefit, until drivers support >2 states

I want to fix invalid values being passed down to drivers, not rewrite half of driver model.

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.

Quoted 2 lines

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.

Quoted 1 line

I don't understand what you're saying. If I have a driver that Iwant to

                                         ~~~~~~~~~~~~~~~~~~

Quoted 3 lines

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.

Quoted 4 lines

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).

...which seems like one more reason to only export "on" and "off" in radeon case. We don't want userspace to write "D3" to radeon, then wondering why it failed.

Passing "on"/"off" down to radeon lets the driver decide what power state it should enter.

Quoted 1 line · Pavel Machek

On Čt 05-01-06 14:15:39, Patrick Mochel wrote:

Quoted 5 lines

Quoted 3 lines

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.

Quoted 1 line

(2) is quite a lot of work

In the long run, it's not.

Quoted 1 line

(3) ...with very little benefit, until drivers support >2 states

Without it, you are preventing drivers from being able to support > 2 states.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 1 line

I don't understand what you're saying. If I have a driver that Iwant to

                                         ~~~~~~~~~~~~~~~~~~

Quoted 3 lines

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".

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 9 lines

Quoted 2 lines

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... :(

On Út 27-12-05 20:22:04, Patrick Mochel wrote:
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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 13 lines · Pavel Machek

On Út 27-12-05 20:22:04, Patrick Mochel wrote:
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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 1 line

I asked for an example.

Look at the ACPI spec, it has several examples...

1.) most sound cards have more than two states. (once again latency over power savings trade offs)

2.) many PCI devices with wake support use different D-levels depending on wake settings

4.) IDE hard drives and other storage media have "sleep", "suspend", etc.

5.) SATA controllers have more states than just "on" and "off". Also these states are independent of the PCI d-states.

6.) many video cards implement D1 and D2 as you've already seen. This is often more a matter of "we only know how to restore from such and such states"

7.) Many processors support of wealth of different power states

Quoted 6 lines

Quoted 1 line

I asked for an example.

Look at the ACPI spec, it has several examples...

1.) most sound cards have more than two states. (once again latency over power savings trade offs)

What is the latency in typical "most sound card" case?

Quoted 2 lines

4.) IDE hard drives and other storage media have "sleep", "suspend", etc.

Yep; but spindown takes 5 seconds, so if you need to reset ide bus or not to get it back is driver detail. Plus notice how power consuption in sleep and suspend is almost same; motor not running is big deal there. Ouch and hdparm already handles these.

Quoted 4 lines

6.) many video cards implement D1 and D2 as you've already seen. This is often more a matter of "we only know how to restore from such and such states"

Excatly, so "on"/"off" is enough for them.

Quoted 5 lines

Quoted 3 lines

1.) most sound cards have more than two states. (once again latency over power savings trade offs)

What is the latency in typical "most sound card" case?

ACPI requires within 100ms for D1 and D2. D3 can be longer.

Quoted 4 lines

Quoted 3 lines

4.) IDE hard drives and other storage media have "sleep", "suspend", etc.

Quoted 4 lines

Yep; but spindown takes 5 seconds, so if you need to reset ide bus or not to get it back is driver detail. Plus notice how power consuption in sleep and suspend is almost same; motor not running is big deal there. Ouch and hdparm already handles these.

5 seconds is more of an upper bound. My laptop hardrive can do spindown and spinup in around 1 second. hdparm doesn't handle the deeper suspend case (it can enter but not recover). Kernel level support is required.

Quoted 5 lines

Quoted 3 lines

6.) many video cards implement D1 and D2 as you've already seen. This is often more a matter of "we only know how to restore from such and such states"

Excatly, so "on"/"off" is enough for them.

Not really. The ACPI spec suggests latency requirements for each state. If we had better drivers, we might enter a lower latency video card state when the screen is blanked that's not quite an "off" state.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

1.) most sound cards have more than two states. (once again latency over power savings trade offs)

What is the latency in typical "most sound card" case?

ACPI requires within 100ms for D1 and D2. D3 can be longer.

Spec is useless here. 1usec is <100msec. Do you know any soundcard that takes >10msec from D3?