Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson <ulf.hansson@linaro.org>]

On 9 November 2017 at 17:31, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Nov 9, 2017 at 3:41 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Ulf,
>>
>> On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson <ulf.hansson@linaro.org> 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