Re: Problems with remote-wakeup settings

[Alan Stern]

On Fri, 5 Mar 2010, Rafael J. Wysocki wrote:

> On Friday 05 March 2010, Alan Stern wrote:
> > The way we implement remote wakeup has got some problems.
> > 
> > Here's a brief summary of the way it currently works.  Each dev->power
> > structure contains two bitflags: can_wakeup and should_wakeup.
> > 
> > 	The can_wakeup flag indicates whether the device is _capable_
> > 	of issuing wakeup requests; it is supposed to be set by the
> > 	driver.
> 
> No, it is not.  Perhaps that was the original idea, but we don't use it this
> way, at least not for PCI devices.  It actually is set by the subsystem (the
> PCI bus type in this particular case) depending on whether or not the device
> is capable of waking up the system.
> 
> The reason is that for PCI the information needed to decide belongs to the
> PCI core rather than to the driver.

Wherever I wrote "driver", interpret it as meaning "driver or
subsystem".  You're right, in many or most cases can_wakeup is set by
the subsystem and not the driver.

> > 	The should_wakeup flag indicates whether the device should be
> > 	_allowed_ to issue wakeup requests.  This is a policy decision
> > 	which generally should be left up to userspace, through the
> > 	/sys/devices/.../power/wakeup sysfs attribute.
> 
> That is supposed to work more-or-less this way, although there's the exception
> of some network adapters that enable WoL by default _if_ the device can wake up
> (ie. its can_wakeup flag is set by the PCI core).  For the network adapters
> there's an alternative way to set this via ethtool and it should be in
> agreement with the sysfs setting.

Agreed, ethtool and sysfs should affect the same flags.

> > There are various routines defined in include/linux/pm_wakeup.h for
> > manipulating these flags.  In particular, device_may_wakeup() tells
> > whether or not the device's wakeup facility should be enabled by the
> > driver's suspend routine when a system sleep starts.  It returns true
> > only when both flags are set.
> 
> Right.
> 
> > (Wakeup settings during runtime suspend are a different matter; I'm not
> > going to discuss them here.)
> 
> OK
> 
> > The first problem is that device_initialize() calls 
> > device_init_wakeup(), setting both flags to 0.  This is simply a 
> > mistake.  For one thing, the bits should have been initialized to 0 
> > when the device structure was kzalloc'ed.  For another, it means that 
> > drivers can't usefully set the can_wakeup flag before doing either 
> > device_initialize() or device_register().
> 
> For PCI devices can_wakeup is set by the PCI core and drivers shouldn't really
> modify it, because it is the source of information about the device's
> capability to wake up _for_ _them_.

So the problem is that subsystems can't usefully set the can_wakeup 
flag before doing either device_initialize() or device_register().  
This can be fixed easily by removing the call in device_initialize().

> > The next problem has to do with the sysfs interface.  The power/wakeup 
> > attribute file is implemented to contain either "enabled" or "disabled" 
> > (according to the should_wakeup flag) if can_wakeup is set, and to be 
> > empty if can_wakeup is clear.  Userspace can write "enabled" or 
> > "disabled" to the file to set or clear the should_wakeup flag.
> > 
> > This is bad, because it means the should_wakeup setting is invisible 
> > when can_wakeup is off.  Userspace certainly will want to affect the 
> > should_wakeup flag at times when can_wakeup is off (for example, before 
> > the device has finished binding to its driver).
> 
> Well, the network adapters mentioned above are a bit of a problem here,
> because they'd have to be reworked to look at should_wakeup before enabling
> WoL by default.

IMO they _should_ test device_may_wakeup() before setting WoL.  That's 
its whole purpose.

And also IMO, enabling WoL by default is very questionable.  But that's 
a separate matter.

> Apart from that, changing the interface as proposed if fine by me.
> 
> 
> > The third problem concerns the initial or default should_wakeup
> > setting.  There isn't any coherent thought about what value the flag 
> > should have before userspace takes control of it.
> > 
> > In many cases setting a default value isn't feasible.  Only the driver
> > is in a position to know whether or not the device should be allowed to
> > issue wakeup requests, but by the time the driver binds to the device,
> > the power/wakeup attribute has already been exposed through sysfs.  
> > Any change the driver makes might therefore overwrite the user's
> > preference.  Hence drivers should not be allowed to change the
> > should_wakeup flag.  Unfortunately many drivers do exactly that, as
> > can be seen by searching for "device_set_wakeup_enable" or
> > "device_init_wakeup".
> > 
> > In principle, the decision about setting should_wakeup ought to be left
> > entirely up to userspace.
> 
> I agree in general, but there's the question whether or not the sysfs setting
> should be tied to the WoL setting done via ethtool.  On the one hand it
> shouldn't, because we have no means to update the driver's settings (ie. WoL)
> if the sysfs attribute is set.

I don't understand.  Do you mean there's no way to update the
_device's_ WoL setting when the sysfs attribute is changed?

The device's WoL setting matters only at suspend time.  So the network
driver's suspend routine ought to test device_may_wakeup() to see
whether or not WoL should be enabled.  Maybe this can be centralized 
somewhere in the network stack.

>  On the other hand it should, because the users
> expect that to happen (yes, they do).

And so do I!

> > In practice, userspace doesn't do a very good job of carrying out this
> > decision.  Programs like udev or hal ought to impose useful settings, but
> > as far as I know, they do not.
> > 
> > And then what about systems that don't have udev?  Presumably embedded
> > systems can be trusted to set up the wakeup flags appropriately for
> > their own needs.  Is there a significant number of other systems
> > without udev?  And even if there isn't, the necessary changes to udev
> > would take a while to percolate out.
> > 
> > So we have a situation where the kernel is defining policy which should
> > be set by userspace, and doing so in a way which would often override
> > the user's settings.  Clearly this isn't good.  The only sane approach
> > I can think of is for the kernel never to change should_wakeup --
> > always leave it set to 0 until userspace changes it.  (There could be a
> > few exceptions for devices which everyone always expects to be
> > wakeup-enabled, such as power buttons and keyboards.)  But then of
> > course we would have the problem that in today's distributions,
> > userspace never does change it.
> > 
> > The lack of a proper design for all this has already started to cause 
> > problems.  There are bug reports about systems failing to suspend 
> > because some device, enabled for remote wakeup when it shouldn't be,
> > sends a wakeup request as soon as it gets suspended.  One example of 
> > such a bug report is
> > 
> > 	https://bugs.launchpad.net/ubuntu/+source/linux/+bug/515109
> > 
> > What do people think?
> 
> Hard to say what's the best approach here in general.
> 
> If we unset should_wakeup by default for all devices, the users of some drivers
> (mostly network ones) expecting wakeup to be enabled by default will report
> regressions.  If we don't unset it for all devices, we'll need to do that
> individually for the drivers where there are known bad devices.
> 
> I guess it's better if drivers don't set should_wakeup if unsure, but of course
> that's impossible to enforce.

That's the real question.  Ideally, drivers won't touch should_wakeup.  
How do we get there from here?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/