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-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
