Re: [PATCH] Fix /sys/device/.../power/state regression

[David Brownell <david-b@pacbell.net>]

On Friday 26 January 2007 3:15 pm, Matthew Garrett wrote:
> On Fri, Jan 26, 2007 at 01:56:41PM -0800, David Brownell wrote:
> 
> > 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...

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

There are broadly two things happening in a driver suspend()
method:

 -  One of them *always* happens on rmmod:  stopping all driver
    activity.  That eliminates the dynamic power usage, which as
    a rule is what consumes most of the power.

 -  And issuing a pci_set_power_state() call.  That eliminates
    some more power usage, but as a rule it's not as much.

Plus some drivers will also enable the device as system wakeup
source.  That behaves poorly on PC Linux (ACPI doesn't handle it
well yet), but on more power-aware Linux systems it's important
as a way to let the system stay in low power states most of the
time, without sacrificing system response to external actions.

If you measure and find that setting the power state matters,
making those drivers do that on rmmod should be easy.  (And IMO
it would be worth trying to make PCI use those states by default
for driverless devices.  Different issue though.)


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


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

If you want sympathy for an application that took the time during which
that mechanism was getting phased out, and then decided to phase *IN* a
new use ... sorry, I'm not constitutionally able to give sympathy.  I can
maybe offer sympathy that you didn't know it was being phased out (since
the decision to do that ISTR predated the "feature removal" schedule, with
its additional publicity), but not much more than that.

- Dave