On Thu, Nov 9, 2017 at 3:41 PM, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > Hi Ulf, > > On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> [...] >> >>>>> The Ethernet driver can still call device_set_wakeup_enable(... , false) >>>>> to control this. If WoL is disabled by the user (or deemed not usable, see >>>>> below), it already does so. >>>> >>>> I don't think that API is intended to be used like that and I wonder >>>> if it even works as expected. >>>> >>>> Quoting the doc: >>>> "If device wakeup mechanisms are enabled or disabled directly by >>>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do >>>> during a system sleep transition. Device drivers, however, are not expected to >>>> call :c:func:`device_set_wakeup_enable()` directly in any case." >>>> >>>> Rafael, can you comment on this? >>> >>> There are ca. 100 callers in drivers. >> >> Yeah, but those doesn't normally use it to toggle the setting, but >> instead only to set a default value during ->probe() or whatever >> initialization code that runs. >> >> I think that makes a big difference, doesn't it? > > The few Ethernet drivers I looked at change the state in their > ethtool_ops.set_wol() callback, not during probe. > This is to be configured from userspace using ethtool. Which is the case I was talking about. Since changing the WoL setting via ethtool is expected to cause the sysfs knob to reflect its status, using device_set_wakeup_enable() in there is not actually confusing. The same applies to setting the default from ->probe(). It will be confusing in all of the other cases, though. Thanks, Rafael
