On 9 November 2017 at 17:31, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > 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. Agreed, so using it to enable/disable wakeups during suspend is a bad idea. I will re-spin my series, addressing your review comments. Kind regards Uffe
