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. > 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. > 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_. > 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. 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. On the other hand it should, because the users expect that to happen (yes, they do). > 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. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
