Changes to sysfs PM layer break userspace

Commit 047bda36150d11422b2c7bacca1df324c909c0b3 broke userspace. Previously, /sys/bus/pci/devices/foo/power/state could have values echoed into it for triggering suspend/resume calls in the driver. The breakage is handily mentioned in the comment:

"Devices with bus.suspend_late(), or bus.resume_early() methods fail this operation; those methods couldn't be called."

but there's no mention of what previously working code is supposed to do now. That's the second time in the past year or so that this interface has been broken - can we have it working again, please, especially as there doesn't appear to be an alternative yet?

which userspace is using this btw?

Quoted 1 line · Arjan van de Ven

which userspace is using this btw?

Ubuntu uses it to disable wireless hardware under certain circumstances. I believe that Suse's powernowd uses it to power down wired ethernet hardware when it's not in use.

humm shouldn't the driver do this when the interface is brought down? sounds like you're playing with fire to do this behind the drivers' back....

Quoted 3 lines · Arjan van de Ven

humm shouldn't the driver do this when the interface is brought down? sounds like you're playing with fire to do this behind the drivers' back....

I'm not sure. Suspending the chip means you lose things like link beat detection, so it's not something you necessarily want to automatically tie to something like interface status.

Some chips support more
fine-grained power management, so we could do something more sensible in that case - but right now, there doesn't seem to be a lot of driver support for it.

Quoted 3 lines · Matthew Garrett

I'm not sure. Suspending the chip means you lose things like link beat detection, so it's not something you necessarily want to automatically tie to something like interface status.

right now the "spec" for Linux network drivers assumes that you put the NIC into D3 on down, except for cases where Wake-on-Lan is enabled etc.

Quoted 4 lines

Some chips support more
fine-grained power management, so we could do something more sensible in that case - but right now, there doesn't seem to be a lot of driver support for it.

sounds like that's the right approach at least .. not talking to the PCI hardware directly from userspace...

I can see the point of having more than just "UP" and "DOWN" as interface states; "UP", "DOWN" and "OFF" for example...

Quoted 5 lines · Arjan van de Ven

Quoted 3 lines · Matthew Garrett

I'm not sure. Suspending the chip means you lose things like link beat detection, so it's not something you necessarily want to automatically tie to something like interface status.

Quoted 2 lines

right now the "spec" for Linux network drivers assumes that you put the NIC into D3 on down, except for cases where Wake-on-Lan is enabled etc.

Really? I can't find any drivers that seem to do this. The only calls to pci_set_power_state seem to be in the suspend, resume, init and exit routines.

Quoted 7 lines · Matthew Garrett

Quoted 4 lines · Arjan van de Ven

Quoted 1 line · Matthew Garrett

I'm not sure. Suspending the chip means you lose things like link beat detection, so it's not something you necessarily want to automatically tie to something like interface status.

right now the "spec" for Linux network drivers assumes that you put the NIC into D3 on down, except for cases where Wake-on-Lan is enabled etc.

Really? I can't find any drivers that seem to do this. The only calls to pci_set_power_state seem to be in the suspend, resume, init and exit routines.

your grep missed tg3.c for example, which has a wrapper around the power state code and goes to D3hot on downing of the interface. (just the first one I looked at as a "reference driver", others probably do the same thing)

Quoted 2 lines · Arjan van de Ven

right now the "spec" for Linux network drivers assumes that you put the NIC into D3 on down, except for cases where Wake-on-Lan is enabled etc.

Really? I can't find any drivers that seem to do this. The only calls to pci_set_power_state seem to be in the suspend, resume, init and exit routines.

Quoted 7 lines

Quoted 4 lines

Some chips support more
fine-grained power management, so we could do something more sensible in that case - but right now, there doesn't seem to be a lot of driver support for it.

sounds like that's the right approach at least .. not talking to the PCI hardware directly from userspace...

I'd certainly agree that that's the right thing to do, but userspace has a habit of using whatever functionality /is/ available to get to a given end. The semantics of the device/power/state file were never made terribly clear, and it did have the desired effect of suspending the device. Removing it without providing warning or a transition pathway is a pain.

Quoted 2 lines

I can see the point of having more than just "UP" and "DOWN" as interface states; "UP", "DOWN" and "OFF" for example...

Agreed.

Documentation/feature-removal-schedule.txt has warned about this since August, and the PM list has discussed how broken that model is numerous times over the past several years. (I'm pretty sure that discussion has leaked out to LKML on occasion.) It shouldn't be news today.

Quoted 4 lines · David Brownell

Documentation/feature-removal-schedule.txt has warned about this since August, and the PM list has discussed how broken that model is numerous times over the past several years. (I'm pretty sure that discussion has leaked out to LKML on occasion.) It shouldn't be news today.

1) feature-removal-schedule.txt says that it'll be removed in July 2007. This isn't July 2007.

2) The functionality was disabled in 2.6.19. The addition to feature-removal-schedule.txt was in, uh, 2.6.19.

3) "The whole _point_ of a kernel is to act as a abstraction layer and resource management between user programs and hardware/outside world. That's why kernels _exist_. Breaking user-land API's is thus by definition something totally idiotic.

If you need to break something, you create a new interface, and try to translate between the two, and maybe you deprecate the old one so that it can be removed once it's not in use any more. If you can't see that this is how a kernel should work, you're missing the point of having a kernel in the first place."

Linus, http://lkml.org/lkml/2006/10/4/327

Quoted 2 lines · Matthew Garrett

1) feature-removal-schedule.txt says that it'll be removed in July 2007. This isn't July 2007.

Which is why the functionality is still there.

Quoted 2 lines

2) The functionality was disabled in 2.6.19. The addition to feature-removal-schedule.txt was in, uh, 2.6.19.

Please respond to the technical explanation I provided, and stop referring to the functionality ** which is still there and works ** as being disabled.

I can't help it if that schedule.txt patch took until 2.6.19 to get upstream; ISTR it was available before 2.6.18 shipped. Maybe patches to that file should be accelerated, even into the stable series.

One of the missing steps in Linus' formulation there is that not all interfaces are equivalent in terms of support guarantee. Bugs are interfaces, for example, and sometimes folk wrongly depend on them when they persist for a long time (like, cough, this one).

In contrast, the /sys/devices/.../power/state API has never had many users beyond developers trying to test their drivers (without taking the whole system into a low power state, which probably didn't work in any case), and has *always* been problematic. And the change you object to doesn't "break" anything fundamental, either. Everything still works.

On Tue, Dec 19, 2006 at 07:59:42PM -0800, David Brownell wrote:

The existence of the power/state interface wasn't a bug - it was a deliberate decision to add it. It's the only reason the dpm_runtime_suspend() interface exists.

It's perfectly reasonable to
refer to it as a flawed interface, or perhaps even a buggy one. But in itself, it's clearly not a bug.

Quoted 6 lines

In contrast, the /sys/devices/.../power/state API has never had many users beyond developers trying to test their drivers (without taking the whole system into a low power state, which probably didn't work in any case), and has *always* been problematic. And the change you object to doesn't "break" anything fundamental, either. Everything still works.

It's used on every Ubuntu and Suse system,

Quoted 4 lines · Matthew Garrett

On Tue, Dec 19, 2006 at 07:59:42PM -0800, David Brownell wrote:
It's perfectly reasonable to
refer to it as a flawed interface, or perhaps even a buggy one. But in itself, it's clearly not a bug.

This class of bug is also called a "design bug" or sometimes "mistake".

Quoted 7 lines

Quoted 6 lines

In contrast, the /sys/devices/.../power/state API has never had many users beyond developers trying to test their drivers (without taking the whole system into a low power state, which probably didn't work in any case), and has *always* been problematic. And the change you object to doesn't "break" anything fundamental, either. Everything still works.

Quoted 1 line

It's used on every Ubuntu and Suse system,

Odd how the relevant Suse developers didn't mention any issues with those files going away, any of the times problems with them were discussed on the PM list. Also, I have a Suse system that doesn't use those files for anything ... maybe only newer release use it.

I would be very interested to see any newer SuSE programs using that interface. Just point them out to me and I'll quickly fix them.

And yes, as a SuSE developer (and one of the people in charge of the SuSE kernels), I have no problem with these files just going away. Because, as David keeps repeating, they are broken and wrong.

Seriously. How many pieces of userspace-visible functionality have recently been removed without there being any sort of alternative?

Quoted 2 lines

Seriously. How many pieces of userspace-visible functionality have recently been removed without there being any sort of alternative?

There IS an alternative, you're using it for networking:

You *down the interface*.

If there's a NIC that doesn't support that let us (or preferably netdev) know and it'll get fixed quickly I'm sure.

The situation is more complicated for wireless. Userspace expects to be able to get scan results from the card even if the interface is down.

In
that case, I'm pretty sure we need a third state rather than just "up" or "down".

about your driver list;
do you have an idea of what the top 5 relevant ones would be? I'd be surprised if the top 5 together had less than 95% market share, so if we fix those we'd be mostly done already.

Quoted 2 lines

The situation is more complicated for wireless. Userspace expects to be able to get scan results from the card even if the interface is down.

if it's down userspace cannot currently expect this (if it does it's broken), just as it currently can't expect link notifications when the interface is down. It needs to have the interface up for this. (but point taken for a 3rd state)

so what do you want from this 3rd state? rough guess based on what I think the desktop wants (so please correct/append)

In the third state you
* don't expect to get or send "regular" packets
* don't have a dhcp lease or anything like that
* you do expect to get link change notification [1]
* you do expect to be able to scan for access points [2]

open questions
* what if you get a WOL event?

[1] What kind of latency would be allowed? Would an implementation be allowed to power up the phy say once per minute or once per 5 minutes to see if there is link? The implementation could do this progressively; first poll every X seconds, then after an hour, every minute etc.

[2] would it be permissible to temporarily power up the device on scan? Eg how frequently does the desktop expect to poll for scanning, and what kind of latency would be tolerable?

Yeah, I guess that's a problem. From a user perspective, the functionality is only really useful if the latency is very small. I think where possible we'd want to power down the chip while keeping the phy up, but it would be nice to know how much power that would actually cost us.

Quoted 6 lines

Yeah, I guess that's a problem. From a user perspective, the functionality is only really useful if the latency is very small. I think where possible we'd want to power down the chip while keeping the phy up, but it would be nice to know how much power that would actually cost us.

I'm no expert but afaik the PHY is the power hungry part, the rest is peanuts. So if we can get the PHY to sleep most of the time that would be great.

There are two different problems:

1) Behavior seems to be different depending on device driver author. We should document the expected semantics better.

IMHO:
When device is down, it should:
a) use as few resources as possible:
- not grab memory for buffers
- not assign IRQ unless it could get one
- turn off all power consumption possible
b) allow setting parameters like speed/duplex/autonegotiation, ring buffers, ... with ethtool, and remember the state
c) not accept data coming in, and drop packets queued

Quoted 5 lines

There are two different problems:

1) Behavior seems to be different depending on device driver author. We should document the expected semantics better.

Quoted 9 lines

IMHO:
When device is down, it should:
a) use as few resources as possible:
- not grab memory for buffers
- not assign IRQ unless it could get one
- turn off all power consumption possible
b) allow setting parameters like speed/duplex/autonegotiation, ring buffers, ... with ethtool, and remember the state
c) not accept data coming in, and drop packets queued

What implications does c have for something like tcpdump?

rick jones

IMHO:
When device is down, it should:
a) use as few resources as possible:
- not grab memory for buffers
- not assign IRQ unless it could get one
- turn off all power consumption possible
b) allow setting parameters like speed/duplex/autonegotiation, ring buffers, ... with ethtool, and remember the state
c) not accept data coming in, and drop packets queued

Stephen Hemminger <[email protected]> :
[...]

Quoted 9 lines

IMHO:
When device is down, it should:
a) use as few resources as possible:
- not grab memory for buffers
- not assign IRQ unless it could get one
- turn off all power consumption possible
b) allow setting parameters like speed/duplex/autonegotiation, ring buffers, ... with ethtool, and remember the state
c) not accept data coming in, and drop packets queued

<nit>
Imho speed/duplex/autoneg is not the business of the device: they belong to the phy and it's up to it to decide if its state allows to set the requested parameters or not.
</nit>

We need to allow ethtool setting to be done before device has been brought up and started autonegotiation. The current MII library doesn't really support it.

When device is down, it should:
a) use as few resources as possible:
- not grab memory for buffers
- not assign IRQ unless it could get one
- turn off all power consumption possible
b) allow setting parameters like speed/duplex/autonegotiation, ring buffers, ... with ethtool, and remember the state

2) Network device infrastructure should make it easier for devices:
bring interface down on suspend and bring it up after resume (if it was running when suspended). This would allow many devices to have no suspend/resume hook; except those that have some better power control over hardware.

Veering off at something of a tangent - how much of this should be true for wireless devices? Softmac seems to be unhappy about setting the essid unless the card is up, which breaks various assumptions...

Beyond that, I think your descriptions of up and down states make sense for userspace. As Arjan suggests, there's then the intermediate state of "disable as much as possible while still providing scanning and link detection".

Softmac isn't the only wireless code that likes to be configured after going up first. Configuring after the card goes up has generally been more reliable, though that should not be necessary and is a bug IMHO.

In order to scan, we need to have the radio on and we need to be able to send and receive. What are you gonna turn off?

Quoted 1 line

In order to scan, we need to have the radio on and we need to be able to send and receive. What are you gonna turn off?

The obvious route would be to power the card down, but come back up every two minutes to perform a scan, or if userspace explicitly requests one. Would this cause problems in some cases?

I don't think that supporting scanning when the interface is supposed to be disabled is sensible. If you want to scan, you are simply sending and receiving frames, it's no different from having the interface up and sending/receiving data frames.

From a usability point of view, it's helpful to power the card down as

There are additional implementation problems: scanning requires 2 different ioctl calls: siwscan, then several giwscan. If you want the driver to effectively temporarily bring the interface up when userspace requests a scan but the interface was down, then how does the driver know when to bring it down again?

Quoted 1 line

There are additional implementation problems: scanning requires 2 different ioctl calls: siwscan, then several giwscan. If you want the driver to effectively temporarily bring the interface up when userspace requests a scan but the interface was down, then how does the driver know when to bring it down again?

Hm. Does the spec not set any upper bound on how long it might take for APs to respond? I'm afraid that my 802.11 knowledge is pretty slim.

Picking a number out of thin air would be one answer, but clearly less than ideal. This may be a case of us not being able to satisfy everyone, and so just having to force the user to choose between low power or wireless scanning.

Quoted 2 lines · Matthew Garrett

Hm. Does the spec not set any upper bound on how long it might take for APs to respond? I'm afraid that my 802.11 knowledge is pretty slim.

I'm not sure, but thats not entirely relevant either. The time it takes for the AP to respond is not related to the delay between userspace sending the siwscan and giwscan ioctls (unless you're thinking of userspace being too quick, but GIWSCAN already returns -EINPROGRESS when appropriate so this is detectable)

I think it's reasonable to keep the interface down, but then when the user does want to connect, bring the interface up, scan, present scan results. Scanning is quick, there would be minimal wait needed here.

Quoted 9 lines · Daniel Drake

Quoted 2 lines · Matthew Garrett

Hm. Does the spec not set any upper bound on how long it might take for APs to respond? I'm afraid that my 802.11 knowledge is pretty slim.

I'm not sure, but thats not entirely relevant either. The time it takes for the AP to respond is not related to the delay between userspace sending the siwscan and giwscan ioctls (unless you're thinking of userspace being too quick, but GIWSCAN already returns -EINPROGRESS when appropriate so this is detectable)

Ah - I've mostly been looking at the ipw* drivers, where giwscan just seems to return information cached by the ieee80211 layer. A quick scan suggests that most cards behave like this, but prism54 seems to refer to the hardware. I can see why that would cause problems.

Quoted 4 lines

I think it's reasonable to keep the interface down, but then when the user does want to connect, bring the interface up, scan, present scan results. Scanning is quick, there would be minimal wait needed here.

Yeah, that's true.

Quoted 6 lines · Matthew Garrett

Quoted 5 lines

There are additional implementation problems: scanning requires 2 different ioctl calls: siwscan, then several giwscan. If you want the driver to effectively temporarily bring the interface up when userspace requests a scan but the interface was down, then how does the driver know when to bring it down again?

Quoted 2 lines

Hm. Does the spec not set any upper bound on how long it might take for APs to respond? I'm afraid that my 802.11 knowledge is pretty slim.

I'm not sure, but thats not entirely relevant either. The time it takes for the AP to respond is not related to the delay between userspace sending the siwscan and giwscan ioctls (unless you're thinking of userspace being too quick, but GIWSCAN already returns -EINPROGRESS when appropriate so this is detectable)

Quoted 4 lines

Picking a number out of thin air would be one answer, but clearly less than ideal. This may be a case of us not being able to satisfy everyone, and so just having to force the user to choose between low power or wireless scanning.

I think it's reasonable to keep the interface down, but then when the user does want to connect, bring the interface up, scan, present scan results. Scanning is quick, there would be minimal wait needed here.

Alternatively, if you do want to prepare scan results in the background every 2 minutes, use a sequence something like:

- bring interface up
- siwscan
- giwscan [...]
- bring interface down
- repeat after 2 mins

If this kind of thing was implemented at the driver level, in most cases it would be identical to doing the above anyway.

Daniel
-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Michael Wu

Softmac isn't the only wireless code that likes to be configured after going up first. Configuring after the card goes up has generally been more reliable, though that should not be necessary and is a bug IMHO.

Ok, that's nice to know.

Quoted 2 lines

In order to scan, we need to have the radio on and we need to be able to send and receive. What are you gonna turn off?

The obvious route would be to power the card down, but come back up every two minutes to perform a scan, or if userspace explicitly requests one. Would this cause problems in some cases?

Simple == good. Down == down. Lets just agree on that and save ourselves a lot of pain.

Veering off at something of a tangent - how much of this should be true for wireless devices? Softmac seems to be unhappy about setting the essid unless the card is up, which breaks various assumptions...

Quoted 6 lines

Yeah, I guess that's a problem. From a user perspective, the functionality is only really useful if the latency is very small. I think where possible we'd want to power down the chip while keeping the phy up, but it would be nice to know how much power that would actually cost us.

I'm no expert but afaik the PHY is the power hungry part, the rest is peanuts. So if we can get the PHY to sleep most of the time that would be great.

Is there some reason why we can't have the OS just do the D3 transition for all drivers that register support? I mean, this power management using D states is actually driver *independent* and at least way back in the day was supposed to be implemented for "OS power management"

In terms of what I've seen on vaguely modern hardware, I'd guess at e1000 and sky2 as the top ones. b44 is still common in cheaper hardware, with via-rhine appearing at the very low end. I'll try to grep through our hardware database results to get a stronger idea about percentages.

Quoted 4 lines

about your driver list;
do you have an idea of what the top 5 relevant ones would be? I'd be surprised if the top 5 together had less than 95% market share, so if we fix those we'd be mostly done already.

In terms of what I've seen on vaguely modern hardware, I'd guess at e1000 and sky2 as the top ones. b44 is still common in cheaper hardware,

[1] What kind of latency would be allowed? Would an implementation be allowed to power up the phy say once per minute or once per 5 minutes to see if there is link? The implementation could do this progressively; first poll every X seconds, then after an hour, every minute etc.

Quoted 4 lines · Arjan van de Ven

[1] What kind of latency would be allowed? Would an implementation be allowed to power up the phy say once per minute or once per 5 minutes to see if there is link? The implementation could do this progressively; first poll every X seconds, then after an hour, every minute etc.

I suspect that the hard maximum latency is the time needed by the user to start the network himself, be it opening a root xterm and doing the appropriate invocation or pulling up and clicking where appropriate in a GUI. That's probably around 5 seconds. Over that, and they won't even notice there is an autodetection running.

But still, 5 seconds is probably too much too, because it's going to look like it's unreliable. The user has to see something happen within half-a-second or so, otherwise he's going to start doing it by hand. The "see" part is distribution/desktop-dependant and not the kernel problem, but the top chrono happens when the rj45 is plugged in.

.

5 seconds is unfair and unrealistic though. The *hardware* negotiation before link is seen can easily take upto 45 seconds already. That's a network topology/hardware issue (spanning tree fun) that software or even the hardware in your PC can do nothing about.

this means that the "power up time" needs to be at least 45 seconds, if it's then down 5 seconds inbetween... that's not real power savings.

Quoted 4 lines · Arjan van de Ven

5 seconds is unfair and unrealistic though. The *hardware* negotiation before link is seen can easily take upto 45 seconds already. That's a network topology/hardware issue (spanning tree fun) that software or even the hardware in your PC can do nothing about.

It's about ergonomics, not technical capabilities or fairness.

Quoted 4 lines

this means that the "power up time" needs to be at least 45 seconds, if it's then down 5 seconds inbetween... that's not real power savings.

Then that means you can't have usable autodetection and power savings at the same time.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"AvdV" == Arjan van de Ven <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"AvdV" == Arjan van de Ven <[email protected]> writes:

AvdV> even if you have NO power savings you still don't meet your AvdV> criteria. That's basic ethernet for you....

AvdV> That's what I was trying to say; your criteria is unrealistic AvdV> regardless of what the kernel does, ethernet already dictates 30 AvdV> to 45 seconds there.

Can you get to such high numbers without STP?

5 seconds is unfair and unrealistic though. The *hardware* negotiation before link is seen can easily take upto 45 seconds already. That's a network topology/hardware issue (spanning tree fun) that software or even the hardware in your PC can do nothing about.

The situation is more complicated for wireless. Userspace expects to be able to get scan results from the card even if the interface is down.

In that case, I'm pretty sure we need a third state rather than just "up" or "down".

Quoted 2 lines · Matthew Garrett

The situation is more complicated for wireless. Userspace expects to be able to get scan results from the card even if the interface is down.

User space should get an error when trying to get scan results from the interface that is down. Some drivers are broken and don't do this but when they're fixed there is no problem here.

Quoted 2 lines

In that case, I'm pretty sure we need a third state rather than just "up" or "down".

We have that third state, it's IFF_DORMANT. Not supported yet by any wireless driver/stack, unfortunately.

Thanks,

Jiri

Entirely correct. If the card is DOWN, the radio should be off (both TX & RX) and it should be in max power save mode. If userspace expects to be able to get the card to do _anything_ when it's down, that's just 110% wrong. You can't get link events for many wired cards when they are down, so I fail to see where userspace could expect to do anything with a wireless card when it's down too.

Also, how does rfkill fit into this? rfkill implies killing TX, but do we have the granularity to still receive while the transmit paths are powered down?

Because it works on the common hardware? If there's documentation about what userspace can legitimately expect, then I'm happy to defer to that. But in the absence of any indication as to what functionality users can depend on, deciding that existing functionality is a bug is, well, impolite.

No, it's absolutely a bug. It just so happens that some drivers incorrectly allowed it.

On Wed, Dec 20, 2006 at 08:57:05PM -0500, Michael Wu wrote:

(allowing scanning while the interface is down)

Quoted 2 lines

No, it's absolutely a bug. It just so happens that some drivers incorrectly allowed it.

Ok. Would it be reasonable to add warnings to any devices that allow it?

Quoted 6 lines · Dan Williams

Entirely correct. If the card is DOWN, the radio should be off (both TX & RX) and it should be in max power save mode. If userspace expects to be able to get the card to do _anything_ when it's down, that's just 110% wrong. You can't get link events for many wired cards when they are down, so I fail to see where userspace could expect to do anything with a wireless card when it's down too.

Because it works on the common hardware? If there's documentation about what userspace can legitimately expect, then I'm happy to defer to that. But in the absence of any indication as to what functionality users can depend on, deciding that existing functionality is a bug is, well, impolite.

Quoted 3 lines

Also, how does rfkill fit into this? rfkill implies killing TX, but do we have the granularity to still receive while the transmit paths are powered down?

Is rfkill not just primarily an interface for us getting events when the radio changes state? Every time I read up on it I get a little more confused - some time I really need to make sense of it...

a) tied to the wireless hardware, switch kills hardware directly
b) tied to wireless hardware, but driver handles the kill request
c) just another key, a separate key driver handles the event and asks the wireless driver to kill the card

It's also complicated because some switches are supposed to rfkill both an 802.11 module _and_ a bluetooth module at the same time, or I guess some laptops may even have one rfkill switch for each wireless device. Furthermore, some people want to 'softkill' the hardware via software without pushing the key, which is a subset of (b) or (c) above.

Quoted 10 lines · Dan Williams

a) tied to the wireless hardware, switch kills hardware directly
b) tied to wireless hardware, but driver handles the kill request
c) just another key, a separate key driver handles the event and asks the wireless driver to kill the card

It's also complicated because some switches are supposed to rfkill both an 802.11 module _and_ a bluetooth module at the same time, or I guess some laptops may even have one rfkill switch for each wireless device. Furthermore, some people want to 'softkill' the hardware via software without pushing the key, which is a subset of (b) or (c) above.

If we define interface down as meaning that the device is powered down and the radio switched off, then (b) and (c) would presumably just need to ensure that the interface is downed. (a) is a slightly more special case - if the switch disables the radio, I guess we then want the driver to down the interface as well.

In the (a) case, drivers should presumably refuse to bring the interface up if the radio is disabled?

Quoted 5 lines

If we define interface down as meaning that the device is powered down and the radio switched off, then (b) and (c) would presumably just need to ensure that the interface is downed. (a) is a slightly more special case - if the switch disables the radio, I guess we then want the driver to down the interface as well.

Correct.

Quoted 2 lines

In the (a) case, drivers should presumably refuse to bring the interface up if the radio is disabled?

Right; the driver simply can't do anything about it, because the switch is hardwired to the card and either the card's firmware takes care of it, or the chipset takes care of it. The driver has no say whatsoever in the state of the card's radio for this case. I tend to think this case is on it's way out in the same way that fullmac cards are falling out of favor (ie, do everything in software and save $$$), but they are around and we need to support them.

In this case, down really does mean down too. The driver cannot honor requests to set SSID, frequency, etc, because it's simply not possible at that time.

Quoted 19 lines · Dan Williams

Quoted 5 lines

If we define interface down as meaning that the device is powered down and the radio switched off, then (b) and (c) would presumably just need to ensure that the interface is downed. (a) is a slightly more special case - if the switch disables the radio, I guess we then want the driver to down the interface as well.

Correct.

Quoted 2 lines

In the (a) case, drivers should presumably refuse to bring the interface up if the radio is disabled?

Right; the driver simply can't do anything about it, because the switch is hardwired to the card and either the card's firmware takes care of it, or the chipset takes care of it. The driver has no say whatsoever in the state of the card's radio for this case. I tend to think this case is on it's way out in the same way that fullmac cards are falling out of favor (ie, do everything in software and save $$$), but they are around and we need to support them.

Quoted 3 lines

In this case, down really does mean down too. The driver cannot honor requests to set SSID, frequency, etc, because it's simply not possible at that time.

What do you mean with this exactly?
Should the user not be able to set these values, or should the driver not be able to activate them?

I think it is correct when the driver does not activate them, but I think the user should be able to configure them, have them stored inside cfg80211/the driver, and have them activated/used when uping the interface, or when the rfkill switch has been deactivated. Otherwise it will get impossible to boot with rfkill disabled, toggle the switch later on and have everything working.

And another side to this:
if a disabled rfkill switch downs the interface (opposed to just disabling it but staying "ifconfig up") - what happens to the ip config of this interface? What reconfigures the needed routes when a re-enabled rfkill switch reactivates the interface? Will manual route add and ifconfig statements be impossible and we'll get forced to use some crappy distri-scripts and daemons for it?

And third point just coming to my mind:
how is changing the mac address of the card supposed to work? Chaning it through ifconfig only works when the interface is downed, so the newly wanted mac address has to be saved somewhere before the interface is reenabled and reinitialized on the next "ifconfig up".
(And I think it is an absolute requirement that NO packet with the old/default mac address may be sent into the air whatsoever)

It's also complicated because some switches are supposed to rfkill both an 802.11 module _and_ a bluetooth module at the same time, or I guess some laptops may even have one rfkill switch for each wireless device.

Quoted 3 lines · Dan Williams

It's also complicated because some switches are supposed to rfkill both an 802.11 module _and_ a bluetooth module at the same time, or I guess some laptops may even have one rfkill switch for each wireless device.

On my Dell D820, it's bios-selectable if the switch is enabled, or if it controls just the 802.11 card, or 802.11 and bluetooth, or just bluetooth, or 802.11 and mobile broadband, or ...

This way lies madness. :)

(Oddest part - said bios config screen offers the choices for bluetooth and mobile broadband even though the hardware config doesn't include it. ;)

As far as I can tell, the following network devices don't put the hardware into D3 on interface down:

defxx

Quoted 3 lines

The existence of the power/state interface wasn't a bug - it was a deliberate decision to add it. It's the only reason the dpm_runtime_suspend() interface exists.

Quoted 7 lines

Quoted 6 lines

In contrast, the /sys/devices/.../power/state API has never had many users beyond developers trying to test their drivers (without taking the whole system into a low power state, which probably didn't work in any case), and has *always* been problematic. And the change you object to doesn't "break" anything fundamental, either. Everything still works.

Quoted 1 line

It's used on every Ubuntu and Suse system,

Odd how the relevant Suse developers didn't mention any issues with those files going away, any of the times problems with them were discussed on the PM list. Also, I have a Suse system that doesn't use those files for anything ... maybe only newer release use it.

Actually, if we noticed power/state during PM framework review, it would have been killed. It is just way too ugly.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

In contrast, the /sys/devices/.../power/state API has never had many users beyond developers trying to test their drivers ...

It's used on every Ubuntu and Suse system,

Quoted 4 lines

Odd how the relevant Suse developers didn't mention any issues with those files going away, any of the times problems with them were discussed on the PM list. Also, I have a Suse system that doesn't use those files for anything ... maybe only newer release use it.

Not on *every* suse system. power/state is known to oops kernels, so it is only enabled when user explicitely asks for 'dangerous aggresive experimental power saving' or something like that.

Seems to me anyone really desperate to put PCI devices into a low power mode, without driver support at the "ifdown" level, would be able just "rmmod driver; setpci".

Quoted 3 lines

Seems to me anyone really desperate to put PCI devices into a low power mode, without driver support at the "ifdown" level, would be able just "rmmod driver; setpci".

Incorrect for very obvious reasons - there may be two devices driven by the same driver one up and one down.

Quoted 3 lines

Seems to me anyone really desperate to put PCI devices into a low power mode, without driver support at the "ifdown" level, would be able just "rmmod driver; setpci".

Incorrect for very obvious reasons - there may be two devices driven by the same driver one up and one down.

Documentation/feature-removal-schedule.txt has warned about this since August

Quoted 2 lines · On Tue, 19 Dec 2006 13:34:49 -0800 David Brownell <[email protected]> wrote:

Documentation/feature-removal-schedule.txt has warned about this since August

Nobody reads that.

Please, wherever possible, put a nice printk("this is going away") in the code when planning these things.

Quoted 7 lines · On Tue, 19 Dec 2006 18:15:24 -0800 Andrew Morton wrote:

Quoted 2 lines · On Tue, 19 Dec 2006 13:34:49 -0800 David Brownell <[email protected]> wrote:

Documentation/feature-removal-schedule.txt has warned about this since August

Nobody reads that.

Ugh, I read it.

Quoted 2 lines

Please, wherever possible, put a nice printk("this is going away") in the code when planning these things.

Can notices go in both places, or is in the source code (printk) now the preferred way?

I think that we can point people to Doc/feature-removal-schedule.txt easier (and more effectively) than we can source code (or noisy kernel logs).

Quoted 2 lines · On Tue, 19 Dec 2006 13:34:49 -0800 David Brownell <[email protected]> wrote:

Documentation/feature-removal-schedule.txt has warned about this since August

Nobody reads that.

Please, wherever possible, put a nice printk("this is going away") in the code when planning these things.

Quoted 10 lines · Andrew Morton

Quoted 2 lines · On Tue, 19 Dec 2006 13:34:49 -0800 David Brownell <[email protected]> wrote:

Documentation/feature-removal-schedule.txt has warned about this since August

Nobody reads that.

Please, wherever possible, put a nice printk("this is going away") in the code when planning these things.

Signed-off-by David Brownell <[email protected]>

Index: g26/drivers/base/power/sysfs.c
===================================================================

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

@@ -42,9 +42,17 @@ static ssize_t state_show(struct device 
 
 static ssize_t state_store(struct device * dev, struct device_attribute *attr, const char * buf, size_t n)
 {
+	static int warned;
 	pm_message_t state;
 	int error = -EINVAL;
 

Quoted 5 lines

drivers/base/power/sysfs.c +5 -0

+	if (!warned) {
+		printk(KERN_WARNING
+			"*** WARNING *** sysfs devices/.../power/state files "
+			"are only for testing, and will be removed\n");
+		warned = error;

drivers/base/power/sysfs.c static ssize_t state_show(struct device · line 58

Quoted 5 lines

drivers/base/power/sysfs.c +2 -0

+	}
+
 	/* disallow incomplete suspend sequences */
 	if (dev->bus && (dev->bus->suspend_late || dev->bus->resume_early))
 		return error;

Well that's not much use. It tells people "hey, we broke it". They already knew that.

What we should do is to revert 047bda36150d11422b2c7bacca1df324c909c0b3 and

Quoted 5 lines · Andrew Morton

Quoted 5 lines

drivers/base/power/sysfs.c +5 -0

+	if (!warned) {
+		printk(KERN_WARNING
+			"*** WARNING *** sysfs devices/.../power/state files "
+			"are only for testing, and will be removed\n");
+		warned = error;

Quoted 8 lines

drivers/base/power/sysfs.c static ssize_t state_show(struct device · line 58

Quoted 5 lines

drivers/base/power/sysfs.c +2 -0

+	}
+
 	/* disallow incomplete suspend sequences */
 	if (dev->bus && (dev->bus->suspend_late || dev->bus->resume_early))
 		return error;

Well that's not much use. It tells people "hey, we broke it". They already knew that.

No, it only does what you asked for: warning people that they're using something that's going away. It says nothing about "broke".

Quoted 2 lines

What we should do is to revert 047bda36150d11422b2c7bacca1df324c909c0b3 and

Bad answer

... see my original reply in this thread. If "the answer" is
to involve making PCI devices work again, better solutions include reverting the patch I mentioned (adding the suspend_late/resume_early support to PCI) or a version of what Matthew has produced (poking through bus layers so that test can be made to fail when the bus supports those methods but the specific device's driver doesn't use them).

Quoted 7 lines

... see my original reply in this thread. If "the answer" is
to involve making PCI devices work again, better solutions include reverting the patch I mentioned (adding the suspend_late/resume_early support to PCI) or a version of what Matthew has produced (poking through bus layers so that test can be made to fail when the bus supports those methods but the specific device's driver doesn't use them).

We appear to have a choice of three options. But I see no fix in Greg's tree. Please let's not just accidentally forget to do this.

Quoted 5 lines

Quoted 3 lines

What we should do is to revert 047bda36150d11422b2c7bacca1df324c909c0b3 and

Bad answer

Is better than breaking stuff.

Quoted 1 line

which userspace is using this btw?

Ubuntu uses it to disable wireless hardware under certain circumstances. I believe that Suse's powernowd uses it to power down wired ethernet hardware when it's not in use.

Hi!

Quoted 5 lines

Quoted 1 line

which userspace is using this btw?

Ubuntu uses it to disable wireless hardware under certain circumstances. I believe that Suse's powernowd uses it to power down wired ethernet hardware when it's not in use.

I flamed seife for this. It was always broken for 20%-or-so of hardware. It is _not_ simple to fix.

Commit 047bda36150d11422b2c7bacca1df324c909c0b3 broke userspace.

Previously, /sys/bus/pci/devices/foo/power/state could have values echoed into it for triggering suspend/resume calls in the driver.

The
breakage is handily mentioned in the comment:

"Devices with bus.suspend_late(), or bus.resume_early() methods fail this operation; those methods couldn't be called."

but there's no mention of what previously working code is supposed to do now.

Quoted 3 lines

Ubuntu uses it to disable wireless hardware under certain circumstances. I believe that Suse's powernowd uses it to power down wired ethernet hardware when it's not in use.

That's the second time in the past year or so that this interface has been broken - can we have it working again, please, especially as there doesn't appear to be an alternative yet?

Stop trying to use broken and deprecated APIs; and realize that "previously working" meant you just hadn't tripped over the serious bugs yet.

Drivers can and should know how to do that sort of stuff themselves, so the power savings are reliable and consistent no matter what user space tools are (or aren't) used.

Drivers know how to get power savings a lot better than any userspace code ever could ... with the exception of hints like "ifdown eth0" letting the driver know that right now is a good time to power down almost everything, since it's not going to be used until "ifup".

As a generic mechanism, that interface has *ALWAYS* been "broken by design"; I'd call it unfixable. It's deprecated, and scheduled to vanish; see Documentation/feature-removal-schedule.txt ...

Quoted 3 lines · David Brownell

As a generic mechanism, that interface has *ALWAYS* been "broken by design"; I'd call it unfixable. It's deprecated, and scheduled to vanish; see Documentation/feature-removal-schedule.txt ...

The fact that something is scheduled to be removed in July 2007 does *not* mean it's acceptable to break it in 2006. We need to find a way to fix this functionality in the meantime.

Quoted 3 lines · Matthew Garrett

The fact that something is scheduled to be removed in July 2007 does *not* mean it's acceptable to break it in 2006. We need to find a way to fix this functionality in the meantime.

The disconnect here is analagous to: I tell you the alleged perpetual motion machine never worked, and can't ever work; and you push back and say that you need a perpetual motion machine that works, NOW please, because you need something that pushes those widgets around. (There are better ways to push widgets than side effects of a broken machine...)

Given that your examples are network adapters, you should really look more at why "ifdown eth0" (etc) having drivers put the device into a low power state (like PCI D3hot, or maybe D2) wouldn't work in any particular case. If you actually have such cases, then maybe those specific drivers need to drive new power management interfaces.

That's a workable approach to resolving the underlying problem in the long term. In the short term, notice the system still works correctly if you don't try writing those files.

I'd not be keen on reverting Linus' patch [1] myself, even though few drivers have started to use that mechanism yet; that would be a step backwards, and would perpetuate users of that broken sysfs file.

Quoted 5 lines · David Brownell

Quoted 3 lines · Matthew Garrett

The fact that something is scheduled to be removed in July 2007 does *not* mean it's acceptable to break it in 2006. We need to find a way to fix this functionality in the meantime.

Quoted 5 lines

The disconnect here is analagous to: I tell you the alleged perpetual motion machine never worked, and can't ever work; and you push back and say that you need a perpetual motion machine that works, NOW please, because you need something that pushes those widgets around. (There are better ways to push widgets than side effects of a broken machine...)

But it *did* work.

Quoted 3 lines

I'd not be keen on reverting Linus' patch [1] myself, even though few drivers have started to use that mechanism yet; that would be a step backwards, and would perpetuate users of that broken sysfs file.

I'm sorry, which bit of "Don't break userspace API without adequate prior warning and with a workable replacement" is difficult to understand?

Quoted 3 lines · Matthew Garrett

I'm sorry, which bit of "Don't break userspace API without adequate prior warning and with a workable replacement" is difficult to understand?

What part of "it was already broken" do YOU not understand? The whole notion is unsustainable. It doesn't work cross-platform, or for multiple bus types. It confuses system-wide suspend mechanisms with runtime mechanisms. It breaks guaranteed parent/child ordering of suspend/resume calls. (And more...)

Let us know when you get tired of whining and want to move on to getting a real solution to the set of problems here. I've pointed out that reverting Linus' patch would be one option to get your short term issue rsolved ... that would remove a capability from PCI drivers, but you could then use that deprecated mechanism. I've also pointed out that you could start working towards a real long term solution.

Do you have an alternate solution?

Quoted 1 line

Do you have an alternate solution?

How about something like this? Entirely untested, but I think it shows the basic idea.

drivers/base/power/sysfs.c static ssize_t state_store(struct device * dev, struct device_attribute *attr, c · line 51

Quoted 6 lines

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

 	/* disallow incomplete suspend sequences */
-	if (dev->bus && (dev->bus->suspend_late || dev->bus->resume_early))
+	if (dev->bus && dev->bus->pm_has_noirq_stage 
+	    && dev->bus->pm_has_noirq_stage(dev))
 		return error;

I'm suspecting these two patches won't be merged, but this fragment has two bugs. One is the whitespace bug already mentioned.

The other is that
the original test must still be used if that bus primitve doesn't exist.

And in a different vein, I'm a bit surprised that the update to the feature-removal-schedule.txt file is a separate patch, but:

Review

Quoted 3 lines

+       bus->pm_has_noirq_stage()

(quoted diff) +1 -1

-When:  July 2007
+When:  Once alternative functionality has been implemented

The "When" shouldn't change.

Quoted 8 lines · David Brownell

drivers/base/power/sysfs.c static ssize_t state_store(struct device * dev, struct device_attribute *attr, c · line 51

Quoted 6 lines

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

 	/* disallow incomplete suspend sequences */
-	if (dev->bus && (dev->bus->suspend_late || dev->bus->resume_early))
+	if (dev->bus && dev->bus->pm_has_noirq_stage 
+	    && dev->bus->pm_has_noirq_stage(dev))
 		return error;

I'm suspecting these two patches won't be merged,

Quoted 2 lines

but this fragment has
two bugs. One is the whitespace bug already mentioned.

I'm a bit curious about the whitespace issue - CodingStyle doesn't seem to discuss what to do with if statements that end up longer than 80 characters, which is (I think) what you're talking about?

Quoted 3 lines

The other is that
the original test must still be used if that bus primitve doesn't exist.

I dislike that.

We're asking to suspend an individual device - whether
the bus supports devices that need to suspend with interrupts disabled is irrelevent, it's the device that we care about. We should just make it necessary for every bus to support this method until the interface is removed.

Quoted 5 lines

Review

Quoted 3 lines

+       bus->pm_has_noirq_stage()

(quoted diff) +1 -1

-When:  July 2007
+When:  Once alternative functionality has been implemented

The "When" shouldn't change.

We shouldn't remove interfaces that userland uses until there's been a replacement for long enough that userland can switch over.

Quoted 1 line · Matthew Garrett

I dislike that.

Tough noogies, as they say. In a tradeoff between correctness and your personal taste (or even mine, sigh!), the normal tradeoff is in favor of correctness.

Quoted 5 lines

We're asking to suspend an individual device - whether
the bus supports devices that need to suspend with interrupts disabled is irrelevent, it's the device that we care about. We should just make it necessary for every bus to support this method until the interface is removed.

But you _didn't_ do anything to "make it necessary". Which means that your patch *WILL* cause bugs whenever a driver uses those calls, and courtesy of your patch userspace tries to suspend that device ...

Quoted 2 lines

We shouldn't remove interfaces that userland uses until there's been a replacement for long enough that userland can switch over.

Userland can stop using this **TODAY** and just "ifdown", so that argument seems weak. For all your examples, the userland interface is already available.

Quoted 6 lines · David Brownell

Quoted 1 line · Matthew Garrett

I dislike that.

Tough noogies, as they say. In a tradeoff between correctness and your personal taste (or even mine, sigh!), the normal tradeoff is in favor of correctness.

But it's not correct - the test prohibits suspending devices even if it would be safe to do so.

Quoted 9 lines

Quoted 5 lines

We're asking to suspend an individual device - whether
the bus supports devices that need to suspend with interrupts disabled is irrelevent, it's the device that we care about. We should just make it necessary for every bus to support this method until the interface is removed.

But you _didn't_ do anything to "make it necessary". Which means that your patch *WILL* cause bugs whenever a driver uses those calls, and courtesy of your patch userspace tries to suspend that device ...

New patch attached.

In 2.6.19, support for splitting driver suspend and resume callbacks into interrupt and non-interrupt contexts was added. Unfortunately, this broke /sys/device/.../power/state support for all devices. In the long run, this should be obsoleted by power management support in the individual drivers - however, in the case of network drivers (for example), currently only three drivers implement any sort of useful run-time power management.

This patch allows the bus driver to check whether a specific driver requires the split. If not, the 2.6.18 functionality is restored. It also alters feature-removals.txt to note that the deprecated functionality should not be removed until a replacement actually exists.

Quoted 8 lines · On Thu, 25 Jan 2007 05:00:09 +0000 Matthew Garrett <[email protected]> wrote:

In 2.6.19, support for splitting driver suspend and resume callbacks into interrupt and non-interrupt contexts was added. Unfortunately, this broke /sys/device/.../power/state support for all devices. In the long run, this should be obsoleted by power management support in the individual drivers - however, in the case of network drivers (for example), currently only three drivers implement any sort of useful run-time power management.

Quoted 4 lines

This patch allows the bus driver to check whether a specific driver requires the split. If not, the 2.6.18 functionality is restored. It also alters feature-removals.txt to note that the deprecated functionality should not be removed until a replacement actually exists.

That sounds like material for 2.6.20 as well as for 2.6.19.x.

No, it's not stable material, as drivers would have to be modified to support it, and that is adding new stuff. See my other comment about why this was changed because it was broken...

Quoted 4 lines

This patch allows the bus driver to check whether a specific driver requires the split. If not, the 2.6.18 functionality is restored. It also alters feature-removals.txt to note that the deprecated functionality should not be removed until a replacement actually exists.

That sounds like material for 2.6.20 as well as for 2.6.19.x.

In 2.6.19, support for splitting driver suspend and resume callbacks into interrupt and non-interrupt contexts was added. Unfortunately, this broke /sys/device/.../power/state support for all devices. In the long run, this should be obsoleted by power management support in the individual drivers - however, in the case of network drivers (for example), currently only three drivers implement any sort of useful run-time power management.

This patch allows the bus driver to check whether a specific driver requires the split. If not, the 2.6.18 functionality is restored. It also alters feature-removals.txt to note that the deprecated functionality should not be removed until a replacement actually exists.

Quoted 4 lines · Matthew Garrett

This patch allows the bus driver to check whether a specific driver requires the split. If not, the 2.6.18 functionality is restored. It also alters feature-removals.txt to note that the deprecated functionality should not be removed until a replacement actually exists.

Ick, no.

As stated before, it was broken in the past, and no userspace tools use it because of that. So it was disabled.

Or am I misstating that long thread? David, your thoughts?

Quoted 1 line

Or am I misstating that long thread? David, your thoughts?

My recollection of that thread was that *NOBODY* claimed that a replacement of that (broken-by-design) mechanism would ever exist.

However, there was some interesting discussion of just how to fix the wireless adapters. There appeared to be consensus that the right solution involved "ifdown wlan0" (or whatever) ought to power down that adapter, in the way Matthew wanted.

I thought the resolution was that fixing a few of those drivers should solve the problem Matthew needed resolved, and that in the meanwhile "rmmod drivername" should suffice. There also seemed to be agreement that power management for wireless devices needed more work; there might need to be a state between "down/off" and "configured and able to talk IP".

I thought the resolution was that fixing a few of those drivers should solve the problem Matthew needed resolved, and that in the meanwhile "rmmod drivername" should suffice. There also seemed to be agreement that power management for wireless devices needed more work; there might need to be a state between "down/off" and "configured and able to talk IP".

Quoted 6 lines · David Brownell

I thought the resolution was that fixing a few of those drivers should solve the problem Matthew needed resolved, and that in the meanwhile "rmmod drivername" should suffice. There also seemed to be agreement that power management for wireless devices needed more work; there might need to be a state between "down/off" and "configured and able to talk IP".

It's certainly the case that fixing those drivers would result in a better long-term situation - however, nobody currently seems to have any interest in doing so...

I'm not sure what you mean by using rmmod instead. Most drivers don't explicitly set the power state when unbinding, and I can't see anywhere in the generic PCI code that will do it.

As I've said before, I think it's unreasonable to cripple interfaces for (mostly) aesthetic reasons without ensuring that equivalent functionality already exists.

This patch restores useful functionality
without breaking the extra sanity checks that you've added. I appreciate that it's not an interface that you want to support in the long term (well, even the short term...),

Quoted 9 lines · Matthew Garrett

Quoted 6 lines · David Brownell

I thought the resolution was that fixing a few of those drivers should solve the problem Matthew needed resolved, and that in the meanwhile "rmmod drivername" should suffice. There also seemed to be agreement that power management for wireless devices needed more work; there might need to be a state between "down/off" and "configured and able to talk IP".

Quoted 3 lines

It's certainly the case that fixing those drivers would result in a better long-term situation - however, nobody currently seems to have any interest in doing so...

And the way these things work, unfortunately, is that merging your patch would ensure nobody ever gets such interest. Removing that "state" file (and its bogus infrastructure) has already taken a few years too long.

Quoted 4 lines

As I've said before, I think it's unreasonable to cripple interfaces for (mostly) aesthetic reasons without ensuring that equivalent functionality already exists.

I don't recall anyone raising aesthetic concerns. And bug-equivalence has never been a goal of Linux.

Quoted 5 lines

This patch restores useful functionality
without breaking the extra sanity checks that you've added. I appreciate that it's not an interface that you want to support in the long term (well, even the short term...),

You imply that it _was_ once supported, which is not true. Like any other bug (in this case "design bug"), it was there and could be abused. And like some other bugs, fixing it can trigger complaints from (ab)users.

Quoted 7 lines

Quoted 3 lines

It's certainly the case that fixing those drivers would result in a better long-term situation - however, nobody currently seems to have any interest in doing so...

And the way these things work, unfortunately, is that merging your patch would ensure nobody ever gets such interest. Removing that "state" file (and its bogus infrastructure) has already taken a few years too long.

No, we shouldn't just break stuff for our users in the hope that said breakage will force some other developer to come in and fix things later.

We should revert the breakage-causing patch, with the expectation that its submitter will ensure that all prerequisites are in place before it is reapplied.

Quoted 7 lines

Quoted 3 lines

As I've said before, I think it's unreasonable to cripple interfaces for (mostly) aesthetic reasons without ensuring that equivalent functionality already exists.

I don't recall anyone raising aesthetic concerns. And bug-equivalence has never been a goal of Linux.

Not breaking things for end-users is a goal. Prime directive, indeed.

Quoted 8 lines

Quoted 4 lines

This patch restores useful functionality
without breaking the extra sanity checks that you've added. I appreciate that it's not an interface that you want to support in the long term (well, even the short term...),

You imply that it _was_ once supported, which is not true. Like any other bug (in this case "design bug"), it was there and could be abused. And like some other bugs, fixing it can trigger complaints from (ab)users.

Could someone please explain in easy-to-understand terms what the real-world impact of this bug is upon our users? How many are affected, and under what circumstances, and with what effects?

From what I can see, a small subset of Ubuntu users will notice that

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

I thought the resolution was that fixing a few of those drivers should solve the problem Matthew needed resolved, and that in the meanwhile "rmmod drivername" should suffice. There also seemed to be agreement that power management for wireless devices needed more work; there might need to be a state between "down/off" and "configured and able to talk IP".

It's certainly the case that fixing those drivers would result in a better long-term situation - however, nobody currently seems to have any interest in doing so...

Quoted 4 lines

And the way these things work, unfortunately, is that merging your patch would ensure nobody ever gets such interest. Removing that "state" file (and its bogus infrastructure) has already taken a few years too long.

No, we shouldn't just break stuff for our users in the hope that said breakage will force some other developer to come in and fix things later.

We should revert the breakage-causing patch, with the expectation that its submitter will ensure that all prerequisites are in place before it is reapplied.

Quoted 3 lines

You imply that it _was_ once supported, which is not true. Like any other bug (in this case "design bug"), it was there and could be abused. And like some other bugs, fixing it can trigger complaints from (ab)users.

Could someone please explain in easy-to-understand terms what the real-world impact of this bug is upon our users? How many are affected, and under what circumstances, and with what effects?

Change breaking that was 'introduce suspend early to fix suspend on mac mini', by Linus, IIRC. So no, it is not easy to revert this one.

Quoted 2 lines

Change breaking that was 'introduce suspend early to fix suspend on mac mini', by Linus, IIRC. So no, it is not easy to revert this one.

But it's easy to fix it. Either drivers need suspend routines that

called without interrupts enabled, or they don't. The current situation is that the interface is broken regardless of which is the case - the situation with the patch is that the interface only stops working for drivers that need the suspend routine to be called with disabled interrupts.

Quoted 3 lines · Matthew Garrett

It's certainly the case that fixing those drivers would result in a better long-term situation - however, nobody currently seems to have any interest in doing so...

And the way these things work, unfortunately, is that merging your patch would ensure nobody ever gets such interest. Removing that "state" file (and its bogus infrastructure) has already taken a few years too long.

Quoted 4 lines

This patch restores useful functionality
without breaking the extra sanity checks that you've added. I appreciate that it's not an interface that you want to support in the long term (well, even the short term...),

You imply that it _was_ once supported, which is not true. Like any other bug (in this case "design bug"), it was there and could be abused. And like some other bugs, fixing it can trigger complaints from (ab)users.

It's been several years now that this interface has been well recognized as trouble. Years in which all _other_ users went and did the Right Thing:
they stopped using it, or never started.

Quoted 5 lines · David Brownell

Quoted 3 lines · Matthew Garrett

It's certainly the case that fixing those drivers would result in a better long-term situation - however, nobody currently seems to have any interest in doing so...

Quoted 3 lines

And the way these things work, unfortunately, is that merging your patch would ensure nobody ever gets such interest. Removing that "state" file (and its bogus infrastructure) has already taken a few years too long.

I'd argue that the onus is on those who wish to remove the interface to ensure that equivalent functionality exists first.

Now this is what throws me somewhat. Last May, you argued strongly in favour of keeping the interface:

"Which IMO makes removing this a Bad Thing. It needs to have some kind of replacement in place before the "magic numbers" go away."

(http://lists.osdl.org/pipermail/linux-pm/2006-May/002376.html)

Quoted 10 lines · Matthew Garrett

Quoted 8 lines · David Brownell

Quoted 3 lines · Matthew Garrett

It's certainly the case that fixing those drivers would result in a better long-term situation - however, nobody currently seems to have any interest in doing so...

And the way these things work, unfortunately, is that merging your patch would ensure nobody ever gets such interest. Removing that "state" file (and its bogus infrastructure) has already taken a few years too long.

Quoted 2 lines

I'd argue that the onus is on those who wish to remove the interface to ensure that equivalent functionality exists first.

Are you now arguing that "rmmod $DRIVER" doesn't suffice for what you were wanting to do? If so, why? What's the delta in power usage?

In 2.6.19, support for splitting driver suspend and resume callbacks into interrupt and non-interrupt contexts was added. Unfortunately, this broke /sys/device/.../power/state support for all devices. In the long run, this should be obsoleted by power management support in the individual drivers - however, in the case of network drivers (for example), currently only three drivers implement any sort of useful run-time power management.

Hi!

Quoted 1 line

In 2.6.19, support for splitting driver suspend and resume callbacks into interrupt and non-interrupt contexts was added. Unfortunately, this broke /sys/device/.../power/state support for all devices. In the long run, this should be obsoleted by power management support in the individual drivers - however, in the case of network drivers (for example), currently only three drivers implement any sort of useful run-time power management.

Well... solution seems to be 'implement useful pm for more drivers' not 'discourage people from doing so by re-enabling broken interface'.

Documentation/feature-removal-schedule.txt be removed from this file. · line 13

Quoted 9 lines

Documentation/feature-removal-schedule.txt +2 -1

@@ -9,7 +9,8 @@ be removed from this file.
 What:	/sys/devices/.../power/state
 	dev->power.power_state
 	dpm_runtime_{suspend,resume)()
-When:	July 2007
+	bus->pm_has_noirq_stage()
+When:	Once alternative functionality has been implemented

.../power/state never worked properly. You have been warned and it is going to be removed. It oopses kernels... while 'only' providing power savings. If you are interested in power savings, please help doing them right.

I have been using this interface for a _long_ time (before june 2005, cf
http://marc.theaimsgroup.com/?l=linux-usb-devel&m=111869558800526&w=2)
and for different things than power saving: I can turn off my (usb) optical mouse when watching a movie or going to bed.

Is there any hardware independent alternative to turn off USB devices?

Am Dienstag, 30. Januar 2007 19:43 schrieb Eric Piel:

Quoted 3 lines

I have been using this interface for a _long_ time (before june 2005, cf http://marc.theaimsgroup.com/?l=linux-usb-devel&m=111869558800526&w=2) and for different things than power saving: I can turn off my (usb) optical mouse when watching a movie or going to bed.

Is there any hardware independent alternative to turn off USB devices?

Would you test a patch for autosuspension of hid devices?

Quoted 1 line

Is there any hardware independent alternative to turn off USB devices?

Would you test a patch for autosuspension of hid devices?

Quoted 3 lines

Quoted 2 lines

Is there any hardware independent alternative to turn off USB devices?

Would you test a patch for autosuspension of hid devices?

Hi Oliver,

do you have such patch already? I would also like to see it; if it looks OK, I will push it to -mm through my tree for wider testing.

Quoted 3 lines

That's a workable approach to resolving the underlying problem in the long term. In the short term, notice the system still works correctly if you don't try writing those files.

Well, except I'm now burning an extra couple of watts of power. I consider that pretty broken.

Quoted 3 lines

I'd not be keen on reverting Linus' patch [1] myself, even though few drivers have started to use that mechanism yet; that would be a step backwards, and would perpetuate users of that broken sysfs file.

I'm sorry, which bit of "Don't break userspace API without adequate prior warning and with a workable replacement" is difficult to understand?