2006-02-18 02:04:40

by Patrick Mochel

[permalink] [raw]
Subject: [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


Fix the per-device state file to respect the actual state that
is reported by the device, or written to the file.

Using the struct pm_message::state field:

On read, report the actual state the device is in by looking at the
struct pm_message::state field (instead of just "0" or "2"). If the
device is suspended, but the state is "0", then report PM_STATE_OFF
(defined as INT_MAX).

On write, assume that the value passed in is the state that the
user wants the device to enter. Set the struct pm_message::state
field to that value.
- When (state != 0), assume a suspend request, and always set the
event field to PM_EVENT_SUSPEND
- When (state == 0), assume a resume request.

[ Previously, only values "0", "2", and "3" were accepted, and the
actual value passed was silently dropped. ]

Update the comments so they match the functionality.

Signed-off-by: Patrick Mochel <[email protected]>

---

drivers/base/power/sysfs.c | 66 ++++++++++++++++++++++++++------------------
1 files changed, 39 insertions(+), 27 deletions(-)

applies-to: c5da8670faf610adc21e38c9d2da5330e9232a37
0f3eb786f769badd9044117c2976f829d66dec73
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 40d7242..308baff 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -7,50 +7,62 @@
#include "power.h"


+#define PM_STATE_OFF INT_MAX
+
/**
* state - Control current power state of device
*
* show() returns the current power state of the device. '0' indicates
* the device is on. Other values (1-3) indicate the device is in a low
* power state.
- *
- * store() sets the current power state, which is an integer value
- * between 0-3. If the device is on ('0'), and the value written is
- * greater than 0, then the device is placed directly into the low-power
- * state (via its driver's ->suspend() method).
- * If the device is currently in a low-power state, and the value is 0,
- * the device is powered back on (via the ->resume() method).
- * If the device is in a low-power state, and a different low-power state
- * is requested, the device is first resumed, then suspended into the new
- * low-power state.
+ * A value of INT_MAX (0x0fffffff) indicates that the device is off - it is
+ * suspended, but the actual state is unknown (for devices and buses that only
+ * support "on" and "off").
+ *
+ * store() takes an integer value and changes the current power state of the
+ * device. If this value is 0, the device is resumed - a PM message with event
+ * type PM_EVENT_ON, and a state of 0 is passed to dpm_runtime_resume().
+ * Otherwise, the device is suspended. A PM message with event type
+ * PM_EVENT_SUSPEND is passed to the device, with the state being the value
+ * written to this file.
+ *
+ * The meaning of the integer value states are bus and/or device specific, with
+ * the exception of 0 - that always means "on".
+ * The core will not do any further interpretation of the values. It is up to
+ * the bus and/or device drivers to both document and check the values that are
+ * read and written from this file.
+ * For buses and devices that only support "on" and "off", any non-zero number
+ * can be used to indicate "off".
*/

static ssize_t state_show(struct device * dev, struct device_attribute *attr, char * buf)
{
- if (dev->power.power_state.event)
- return sprintf(buf, "2\n");
- else
- return sprintf(buf, "0\n");
+ u32 state = 0;
+
+ if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
+ state = dev->power.power_state.state;
+ if (!state)
+ state = PM_STATE_OFF;
+ }
+ return sprintf(buf, "%x\n", dev->power.power_state.state);
}

static ssize_t state_store(struct device * dev, struct device_attribute *attr, const char * buf, size_t n)
{
- pm_message_t state;
- int error = -EINVAL;
+ pm_message_t msg = {
+ .event = PM_EVENT_SUSPEND,
+ };
+ char * rest;
+ int error = 0;

- state.event = PM_EVENT_SUSPEND;
- /* Older apps expected to write "3" here - confused with PCI D3 */
- if ((n == 1) && !strcmp(buf, "3"))
- error = dpm_runtime_suspend(dev, state);
-
- if ((n == 1) && !strcmp(buf, "2"))
- error = dpm_runtime_suspend(dev, state);
+ msg.state = simple_strtoul(buf, &rest, 0);
+ if (*rest)
+ return -EINVAL;

- if ((n == 1) && !strcmp(buf, "0")) {
+ if (msg.state)
+ error = dpm_runtime_suspend(dev, msg);
+ else
dpm_runtime_resume(dev);
- error = 0;
- }
-
return error ? error : n;
}

---
0.99.9.GIT


2006-02-18 16:01:03

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

Hi!

> Fix the per-device state file to respect the actual state that
> is reported by the device, or written to the file.

Can we let "state" file die? You actually suggested that at one point.

I do not think passing states in u32 is good idea. New interface that passes
state as string would probably be better.

Pavel

--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms

2006-02-19 23:59:38

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Sat, 18 Feb 2006, Pavel Machek wrote:

> Hi!
>
> > Fix the per-device state file to respect the actual state that
> > is reported by the device, or written to the file.
>
> Can we let "state" file die? You actually suggested that at one point.
>
> I do not think passing states in u32 is good idea. New interface that passes
> state as string would probably be better.

Yup, in the future that will be better. For now, let's work with what we
got and fix 2.6.16 to be compatible with previous versions..

Thanks,


Pat

2006-02-20 00:09:21

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

On Ne 19-02-06 15:59:25, Patrick Mochel wrote:
>
> On Sat, 18 Feb 2006, Pavel Machek wrote:
>
> > Hi!
> >
> > > Fix the per-device state file to respect the actual state that
> > > is reported by the device, or written to the file.
> >
> > Can we let "state" file die? You actually suggested that at one point.
> >
> > I do not think passing states in u32 is good idea. New interface that passes
> > state as string would probably be better.
>
> Yup, in the future that will be better. For now, let's work with what we
> got and fix 2.6.16 to be compatible with previous versions..

It already is. It accepts "0" and "2" and "3". That's all values that
used to work. Other values used to trigger BUG() in pci.c (and we do
not want to re-introduce _that_ behaviour, right?).

If you add u32 into pm_message_t, it will be impossible to remove in
future.
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...

2006-02-20 00:17:08

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Mon, 20 Feb 2006, Pavel Machek wrote:

> On Ne 19-02-06 15:59:25, Patrick Mochel wrote:
> >
> > On Sat, 18 Feb 2006, Pavel Machek wrote:
> >
> > > Hi!
> > >
> > > > Fix the per-device state file to respect the actual state that
> > > > is reported by the device, or written to the file.
> > >
> > > Can we let "state" file die? You actually suggested that at one point.
> > >
> > > I do not think passing states in u32 is good idea. New interface that passes
> > > state as string would probably be better.
> >
> > Yup, in the future that will be better. For now, let's work with what we
> > got and fix 2.6.16 to be compatible with previous versions..
>
> It already is. It accepts "0" and "2" and "3". That's all values that
> used to work.

The core should not dictate the valid range of values. The bus drivers
should decide, since they are their states. "1" also used to work.

> Other values used to trigger BUG() in pci.c (and we do not want to
> re-introduce _that_ behaviour, right?).

I don't follow - the BUG() call was introduced recently for reasons
unknown. The interface had never triggered that previously.

> If you add u32 into pm_message_t, it will be impossible to remove in
> future.

I don't follow this argument either.

I really fail to see what your fundamental objection is. This restores
compatability, makes the core simpler, and adds the ability to use the
additional states, should drivers choose to implement them; all for
relatively little code. It seems a like a good thing to me..

Thanks,


Pat

2006-02-20 00:21:08

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

On Ne 19-02-06 16:17:01, Patrick Mochel wrote:
>
> On Mon, 20 Feb 2006, Pavel Machek wrote:
>
> > On Ne 19-02-06 15:59:25, Patrick Mochel wrote:
> > >
> > > On Sat, 18 Feb 2006, Pavel Machek wrote:
> > >
> > > > Hi!
> > > >
> > > > > Fix the per-device state file to respect the actual state that
> > > > > is reported by the device, or written to the file.
> > > >
> > > > Can we let "state" file die? You actually suggested that at one point.
> > > >
> > > > I do not think passing states in u32 is good idea. New interface that passes
> > > > state as string would probably be better.
> > >
> > > Yup, in the future that will be better. For now, let's work with what we
> > > got and fix 2.6.16 to be compatible with previous versions..
> >
> > It already is. It accepts "0" and "2" and "3". That's all values that
> > used to work.
>
> The core should not dictate the valid range of values. The bus drivers
> should decide, since they are their states. "1" also used to work.

We are talking about hotfix. Maybe "1" used to work year ago, but not
in recent history. 0/2/3 seems to do for a hotfix.

> > If you add u32 into pm_message_t, it will be impossible to remove in
> > future.
>
> I don't follow this argument either.
>
> I really fail to see what your fundamental objection is. This restores
> compatability, makes the core simpler, and adds the ability to use the
> additional states, should drivers choose to implement them; all for
> relatively little code. It seems a like a good thing to me..

Compatibility is already restored.

Introducing additional states should be done in right way, something
we can keep long-term.
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...

2006-02-20 00:36:46

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Mon, 20 Feb 2006, Pavel Machek wrote:

> On Ne 19-02-06 16:17:01, Patrick Mochel wrote:

> > I really fail to see what your fundamental objection is. This restores
> > compatability, makes the core simpler, and adds the ability to use the
> > additional states, should drivers choose to implement them; all for
> > relatively little code. It seems a like a good thing to me..
>
> Compatibility is already restored.

No, the interface is currently broken. The driver core does not dictate
what values are valid or invalid. It is meant to be used by the bus
drivers to help provide a simiar interface as each other, and to reduce
duplicate code.

It should not be filtering the value. Period. That is a policy decision,
and what is worse, is that it is a static, hard-coded policy decision. The
decision to only accept "0", "2", or "3" is a decision that should be made
on a bus-by-bus basis.

> Introducing additional states should be done in right way, something
> we can keep long-term.

Yup, and it starts with the core not deciding what values are wrong or
right.

Thanks,


Pat

2006-02-20 00:41:56

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

On Ne 19-02-06 16:36:34, Patrick Mochel wrote:
>
> On Mon, 20 Feb 2006, Pavel Machek wrote:
>
> > On Ne 19-02-06 16:17:01, Patrick Mochel wrote:
>
> > > I really fail to see what your fundamental objection is. This restores
> > > compatability, makes the core simpler, and adds the ability to use the
> > > additional states, should drivers choose to implement them; all for
> > > relatively little code. It seems a like a good thing to me..
> >
> > Compatibility is already restored.
>
> No, the interface is currently broken. The driver core does not
> dictate

There were two different interfaces, one accepted 0 and 2, everything
else was invalid, and second accepted 0, 1, 2, 3.

If you enter D2 on echo 2, you are breaking compatibility with 2.6.15
or something like that.

Please let this interface die and create new one. (And notice that
this is exactly same advice you gave me month ago).
Pavel

--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...

2006-02-20 00:46:47

by Greg KH

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

On Sun, Feb 19, 2006 at 03:59:25PM -0800, Patrick Mochel wrote:
>
> On Sat, 18 Feb 2006, Pavel Machek wrote:
>
> > Hi!
> >
> > > Fix the per-device state file to respect the actual state that
> > > is reported by the device, or written to the file.
> >
> > Can we let "state" file die? You actually suggested that at one point.
> >
> > I do not think passing states in u32 is good idea. New interface that passes
> > state as string would probably be better.
>
> Yup, in the future that will be better. For now, let's work with what we
> got and fix 2.6.16 to be compatible with previous versions..

It's _way_ too late in the 2.6.16 cycle for this series of patches, if
that is what you are proposing.

thanks,

greg k-h

2006-02-20 17:55:16

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Mon, 20 Feb 2006, Pavel Machek wrote:

> On Ne 19-02-06 16:36:34, Patrick Mochel wrote:
> >
> > On Mon, 20 Feb 2006, Pavel Machek wrote:
> >
> > > On Ne 19-02-06 16:17:01, Patrick Mochel wrote:
> >
> > > > I really fail to see what your fundamental objection is. This restores
> > > > compatability, makes the core simpler, and adds the ability to use the
> > > > additional states, should drivers choose to implement them; all for
> > > > relatively little code. It seems a like a good thing to me..
> > >
> > > Compatibility is already restored.
> >
> > No, the interface is currently broken. The driver core does not
> > dictate
>
> There were two different interfaces, one accepted 0 and 2, everything
> else was invalid, and second accepted 0, 1, 2, 3.
>
> If you enter D2 on echo 2, you are breaking compatibility with 2.6.15
> or something like that.

I don't see how this is true. If a process writes "2" to a PCI device's
state file, what else are sane things to do?

You dropped the fundamental point, and I don't understand why you disagree
with it - the driver core should not be dictating policy to the downstream
drivers. It is currently doing this by filtering the power state that is
passed in via the "state" file.

> Please let this interface die and create new one. (And notice that
> this is exactly same advice you gave me month ago).

And I was sort of wrong. I think an ideal interface would allow the bus
drivers to export the states that they support, and would allow a state
name of any format to be written to that file.

One way to do that is to have each bus driver export its equivalent of a
"state" file for each device. But, a better solution is to leverage the
infrastructure that is already in place and export things through the
existing "state" file.

These patches are steps along that path. They prevent the core from
filtering the state value passed in. It's still an integer value, but it
allows any possible state to be used.

We could just as easily leave it as a string, and have the buses parse the
value to their expectations. Eventually we might want to do something like
that, or have them export a list of string states that they support.
Regardless, that is all in the future, and is completely orthogonal to
letting a device support more than just "on" or "off".

Thanks,


Pat

2006-02-20 17:58:37

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Sun, 19 Feb 2006, Greg KH wrote:

> On Sun, Feb 19, 2006 at 03:59:25PM -0800, Patrick Mochel wrote:
> >
> > On Sat, 18 Feb 2006, Pavel Machek wrote:
> >
> > > Hi!
> > >
> > > > Fix the per-device state file to respect the actual state that
> > > > is reported by the device, or written to the file.
> > >
> > > Can we let "state" file die? You actually suggested that at one point.
> > >
> > > I do not think passing states in u32 is good idea. New interface that passes
> > > state as string would probably be better.
> >
> > Yup, in the future that will be better. For now, let's work with what we
> > got and fix 2.6.16 to be compatible with previous versions..
>
> It's _way_ too late in the 2.6.16 cycle for this series of patches, if
> that is what you are proposing.

Would you mind commmenting on why, as well as your opinion on the validity
of the patches themselves?

This static, hardcoded policy was introduced into the core ~2 weeks ago,
and it doesn't seem like it belongs there at all. This seems like the
easiest way to fixing it, but I'm open to alternative suggestions..

Thanks,


Pat

2006-02-20 17:59:10

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

Hi!

> > > > Compatibility is already restored.
> > >
> > > No, the interface is currently broken. The driver core does not
> > > dictate
> >
> > There were two different interfaces, one accepted 0 and 2, everything
> > else was invalid, and second accepted 0, 1, 2, 3.
> >
> > If you enter D2 on echo 2, you are breaking compatibility with 2.6.15
> > or something like that.
>
> I don't see how this is true. If a process writes "2" to a PCI device's
> state file, what else are sane things to do?

In some kernel version (2.6.15, iirc), device entered D3 if you wrote
"2" to state file, and there are programs out there that depend on
it. And there are some older programs that write "3" and expect D3. So
this interface really needs to die.

> You dropped the fundamental point, and I don't understand why you disagree
> with it - the driver core should not be dictating policy to the downstream
> drivers. It is currently doing this by filtering the power state that is
> passed in via the "state" file.

That's best we can do to stay compatible. Please introduce new file,
and make states string-based.
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...

2006-02-20 18:08:48

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Mon, 20 Feb 2006, Pavel Machek wrote:

> Hi!
>
> > > > > Compatibility is already restored.
> > > >
> > > > No, the interface is currently broken. The driver core does not
> > > > dictate
> > >
> > > There were two different interfaces, one accepted 0 and 2, everything
> > > else was invalid, and second accepted 0, 1, 2, 3.
> > >
> > > If you enter D2 on echo 2, you are breaking compatibility with 2.6.15
> > > or something like that.
> >
> > I don't see how this is true. If a process writes "2" to a PCI device's
> > state file, what else are sane things to do?
>
> In some kernel version (2.6.15, iirc), device entered D3 if you wrote
> "2" to state file, and there are programs out there that depend on
> it.

Like what?

> > You dropped the fundamental point, and I don't understand why you disagree
> > with it - the driver core should not be dictating policy to the downstream
> > drivers. It is currently doing this by filtering the power state that is
> > passed in via the "state" file.
>
> That's best we can do to stay compatible. Please introduce new file,
> and make states string-based.

You are still overlooking the point - the core should not be filtering the
values. It currently is, but it's trivial to fix. What is your issue with
that?

Thanks,


Pat

2006-02-20 18:11:09

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

On Po 20-02-06 10:08:36, Patrick Mochel wrote:
>
> On Mon, 20 Feb 2006, Pavel Machek wrote:
>
> > Hi!
> >
> > > > > > Compatibility is already restored.
> > > > >
> > > > > No, the interface is currently broken. The driver core does not
> > > > > dictate
> > > >
> > > > There were two different interfaces, one accepted 0 and 2, everything
> > > > else was invalid, and second accepted 0, 1, 2, 3.
> > > >
> > > > If you enter D2 on echo 2, you are breaking compatibility with 2.6.15
> > > > or something like that.
> > >
> > > I don't see how this is true. If a process writes "2" to a PCI device's
> > > state file, what else are sane things to do?
> >
> > In some kernel version (2.6.15, iirc), device entered D3 if you wrote
> > "2" to state file, and there are programs out there that depend on
> > it.
>
> Like what?

Search archives. Some PCMCIA, IIRC.

> > > You dropped the fundamental point, and I don't understand why you disagree
> > > with it - the driver core should not be dictating policy to the downstream
> > > drivers. It is currently doing this by filtering the power state that is
> > > passed in via the "state" file.
> >
> > That's best we can do to stay compatible. Please introduce new file,
> > and make states string-based.
>
> You are still overlooking the point - the core should not be filtering the
> values. It currently is, but it's trivial to fix. What is your issue with
> that?

That you are breaking compatibility in middle of 2.6 series. And very
close to 2.6.16 at that.
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...

2006-02-20 22:05:11

by Greg KH

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

On Mon, Feb 20, 2006 at 09:58:27AM -0800, Patrick Mochel wrote:
>
> On Sun, 19 Feb 2006, Greg KH wrote:
>
> > On Sun, Feb 19, 2006 at 03:59:25PM -0800, Patrick Mochel wrote:
> > >
> > > On Sat, 18 Feb 2006, Pavel Machek wrote:
> > >
> > > > Hi!
> > > >
> > > > > Fix the per-device state file to respect the actual state that
> > > > > is reported by the device, or written to the file.
> > > >
> > > > Can we let "state" file die? You actually suggested that at one point.
> > > >
> > > > I do not think passing states in u32 is good idea. New interface that passes
> > > > state as string would probably be better.
> > >
> > > Yup, in the future that will be better. For now, let's work with what we
> > > got and fix 2.6.16 to be compatible with previous versions..
> >
> > It's _way_ too late in the 2.6.16 cycle for this series of patches, if
> > that is what you are proposing.
>
> Would you mind commmenting on why, as well as your opinion on the validity
> of the patches themselves?
>
> This static, hardcoded policy was introduced into the core ~2 weeks ago,
> and it doesn't seem like it belongs there at all.

That patch was accepted as it fixed a oops. It also went in for
2.6.16-rc2, which is much earlier than 2.6.16-rc4, and it had been in
the -mm tree for quite a while for people to test it out and verify that
it didn't break anything. I didn't hear any complaints about it, so
that is why it went in.

In contrast, this patch series creates a new api and doesn't necessarily
fix any reported bugs. It also has not had the time to be tested in the
-mm tree, and there is quite a lot of disagreement about the patches on
the lists. All of that combinded makes it not acceptable for so late in
the -rc cycle (remember, -rc4 means only serious bug fixes.)

> This seems like the easiest way to fixing it, but I'm open to
> alternative suggestions..

Care to resend the series based on all of the comments you have
addressed so far? I'll be glad to review it then.

thanks,

greg k-h

2006-02-21 01:09:31

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Mon, 20 Feb 2006, Greg KH wrote:

> On Mon, Feb 20, 2006 at 09:58:27AM -0800, Patrick Mochel wrote:

> > Would you mind commmenting on why, as well as your opinion on the validity
> > of the patches themselves?
> >
> > This static, hardcoded policy was introduced into the core ~2 weeks ago,
> > and it doesn't seem like it belongs there at all.
>
> That patch was accepted as it fixed a oops. It also went in for
> 2.6.16-rc2, which is much earlier than 2.6.16-rc4, and it had been in
> the -mm tree for quite a while for people to test it out and verify that
> it didn't break anything. I didn't hear any complaints about it, so
> that is why it went in.
>
> In contrast, this patch series creates a new api and doesn't necessarily
> fix any reported bugs. It also has not had the time to be tested in the
> -mm tree, and there is quite a lot of disagreement about the patches on
> the lists. All of that combinded makes it not acceptable for so late in
> the -rc cycle (remember, -rc4 means only serious bug fixes.)

Thanks.

However, there are a couple of things to note:

- These patches don't create a new API; they fix the semantics of an
existing API by restoring them to its originally designed semantics.

- The BUG() still exists and is relatively easily triggerable (by calling
pci_choose_state() with the wrong value). The fact that the BUG() was
allowed into the kernel is surprising - the mantra for a long time has
been that no new BUG()s should be added. This one is easily made nicer
(see patch 4/4 in the next series), so I don't see why it wasn't
targeted before..

- There is a bug, reported by me, and with patches to fix the behavior.
What better solution is there than that?

For context, I am experimenting with the power consumption of devices
and systems in various power states. Not many devices support states
other than D3, but some do, and it seems like a completely valid choice
option to use those states, if I choose to do so.

There is currently no other nice way to do so. And, I'm sure that most
will agree that modifying this sysfs interface is a lot nicer than
manually tickling PCI config space from a userspace process..

> > This seems like the easiest way to fixing it, but I'm open to
> > alternative suggestions..
>
> Care to resend the series based on all of the comments you have
> addressed so far? I'll be glad to review it then.

Done and done.

Thanks,


Pat

2006-02-21 10:57:47

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

On Po 20-02-06 17:09:26, Patrick Mochel wrote:
>
> On Mon, 20 Feb 2006, Greg KH wrote:
>
> > On Mon, Feb 20, 2006 at 09:58:27AM -0800, Patrick Mochel wrote:
>
> > > Would you mind commmenting on why, as well as your opinion on the validity
> > > of the patches themselves?
> > >
> > > This static, hardcoded policy was introduced into the core ~2 weeks ago,
> > > and it doesn't seem like it belongs there at all.
> >
> > That patch was accepted as it fixed a oops. It also went in for
> > 2.6.16-rc2, which is much earlier than 2.6.16-rc4, and it had been in
> > the -mm tree for quite a while for people to test it out and verify that
> > it didn't break anything. I didn't hear any complaints about it, so
> > that is why it went in.
> >
> > In contrast, this patch series creates a new api and doesn't necessarily
> > fix any reported bugs. It also has not had the time to be tested in the
> > -mm tree, and there is quite a lot of disagreement about the patches on
> > the lists. All of that combinded makes it not acceptable for so late in
> > the -rc cycle (remember, -rc4 means only serious bug fixes.)
>
> Thanks.
>
> However, there are a couple of things to note:
>
> - These patches don't create a new API; they fix the semantics of an
> existing API by restoring them to its originally designed semantics.

They may reintroduce "original" semantics, but they'll break
applications needing 2.6.15 semantic (where 2 meant D3hot).

> - The BUG() still exists and is relatively easily triggerable (by calling
> pci_choose_state() with the wrong value). The fact that the BUG() was
> allowed into the kernel is surprising - the mantra for a long time has
> been that no new BUG()s should be added. This one is easily made nicer
> (see patch 4/4 in the next series), so I don't see why it wasn't
> targeted before..

I don't know what you are talking about here. "No new BUGs"?! It is
bad to have bug triggerable from userspace, but that was fixed.

Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...

2006-02-21 11:12:38

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface




On Tue, 21 Feb 2006, Pavel Machek wrote:

> On Po 20-02-06 17:09:26, Patrick Mochel wrote:
> >
> > On Mon, 20 Feb 2006, Greg KH wrote:
> >
> > > On Mon, Feb 20, 2006 at 09:58:27AM -0800, Patrick Mochel wrote:
> >
> > > > Would you mind commmenting on why, as well as your opinion on the validity
> > > > of the patches themselves?
> > > >
> > > > This static, hardcoded policy was introduced into the core ~2 weeks ago,
> > > > and it doesn't seem like it belongs there at all.
> > >
> > > That patch was accepted as it fixed a oops. It also went in for
> > > 2.6.16-rc2, which is much earlier than 2.6.16-rc4, and it had been in
> > > the -mm tree for quite a while for people to test it out and verify that
> > > it didn't break anything. I didn't hear any complaints about it, so
> > > that is why it went in.
> > >
> > > In contrast, this patch series creates a new api and doesn't necessarily
> > > fix any reported bugs. It also has not had the time to be tested in the
> > > -mm tree, and there is quite a lot of disagreement about the patches on
> > > the lists. All of that combinded makes it not acceptable for so late in
> > > the -rc cycle (remember, -rc4 means only serious bug fixes.)
> >
> > Thanks.
> >
> > However, there are a couple of things to note:
> >
> > - These patches don't create a new API; they fix the semantics of an
> > existing API by restoring them to its originally designed semantics.
>
> They may reintroduce "original" semantics, but they'll break
> applications needing 2.6.15 semantic (where 2 meant D3hot).

Like what?

> > - The BUG() still exists and is relatively easily triggerable (by calling
> > pci_choose_state() with the wrong value). The fact that the BUG() was
> > allowed into the kernel is surprising - the mantra for a long time has
> > been that no new BUG()s should be added. This one is easily made nicer
> > (see patch 4/4 in the next series), so I don't see why it wasn't
> > targeted before..
>
> I don't know what you are talking about here. "No new BUGs"?! It is
> bad to have bug triggerable from userspace, but that was fixed.

http://article.gmane.org/gmane.linux.usb.devel/5411/match=no+new+bug

Thanks,


Pat

2006-02-21 11:25:56

by Pavel Machek

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface

Hi!

> > > However, there are a couple of things to note:
> > >
> > > - These patches don't create a new API; they fix the semantics of an
> > > existing API by restoring them to its originally designed semantics.
> >
> > They may reintroduce "original" semantics, but they'll break
> > applications needing 2.6.15 semantic (where 2 meant D3hot).
>
> Like what?

Like:

http://article.gmane.org/gmane.linux.kernel/364195/match=pavel+2+sys+power+state+pcmcia
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...

2006-02-27 19:06:29

by Patrick Mochel

[permalink] [raw]
Subject: Re: [linux-pm] [PATCH 3/5] [pm] Respect the actual device power states in sysfs interface


On Tue, 21 Feb 2006, Pavel Machek wrote:

> Hi!
>
> > > > However, there are a couple of things to note:
> > > >
> > > > - These patches don't create a new API; they fix the semantics of an
> > > > existing API by restoring them to its originally designed semantics.
> > >
> > > They may reintroduce "original" semantics, but they'll break
> > > applications needing 2.6.15 semantic (where 2 meant D3hot).
> >
> > Like what?
>
> Like:
>
> http://article.gmane.org/gmane.linux.kernel/364195/match=pavel+2+sys+power+state+pcmcia

Thanks, but recall that that is PCMCIA. The point of the patch was to pass
the value to the bus drivers, so that they could decide what were valid
values. In the case of PCMCIA, they had adapted to "2", so they could
still receive "2", if the user process wrote that..

Thanks,


Patrick