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

["Rafael J. Wysocki" <rafael@kernel.org>]

On Thu, Nov 9, 2017 at 12:59 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 9 November 2017 at 10:02, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> Hi Ulf,
>>>
>>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> The generic problem this series is trying to solve, is that for some bus types
>>>>>> and PM domains, it's not sufficient to only check the return value from
>>>>>> device_may_wakeup(), to fully understand how to treat the device during system
>>>>>> suspend.
>>>>>>
>>>>>> One particular case that suffers from this, is the generic PM domain (aka genpd)
>>>>>> and that is taken care of in the final change in this series.
>>>>>>
>>>>>> The special case this series address, is to enable drivers to instruct bus types
>>>>>> and PM domains, that the device need to stay powered in case wakeup signals
>>>>>> is enabled for it.
>>>
>>>>>> Geert Uytterhoeven, has been working on some related problems for some Renesas
>>>>>> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
>>>>>> devices/drivers, which are used together with genpd. My intent is that this
>>>>>> series enables a solution for those problems.
>>>>>>
>>>>>> [1]
>>>>>> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html
>>>>>
>>>>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>>>>> it's the right solution for the Renesas SoCs.  I can just set the recently
>>>>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>>>>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>>>>> the clock/power domain must be kept enabled if they're a wakeup source, or
>>>>> part of the wakeup path.
>>>>
>>>> Right, that would work! However, to me, I don't think it's fine grained enough.
>>>>
>>>> Let's take the Ethernet device/driver using WoL as an example, similar
>>>> to your cases.
>>>>
>>>> First, let's assume device_may_wakeup() returns true, meaning that the
>>>> Ethernet device is wakeup capable and that userspace has requested
>>>> wakeup to be enabled.
>>>>
>>>> Then we have three scenarios to consider when the Ethernet driver
>>>> becomes suspended (typically when its ->suspend() callback gets
>>>> invoked).
>>>> 1) The Ethernet interface is down.
>>>> 2) The Ethernet interface is up, but no connection established.
>>>> 3) The Ethernet interface is up, connection established.
>>>>
>>>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>>>> that we can't distinguish between any of the the scenarios above, but
>>>> instead always keep the Ethernet device powered on and thus the PM
>>>> domain also.
>>>>
>>>> In the more fine grained solution, we can change the Ethernet driver
>>>> to consider under what scenario it's being suspended. For 1) and 2),
>>>> there is no need to keep the Ethernet device being powered, but
>>>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>>>> flag.
>>>
>>> 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?
>
> Well, it means what it says.
>
> If you call device_set_wakeup_enable() from a driver, user space will
> see a change in sysfs and may be confused by that and that's why doing
> so is not recommended.
>
> Not that people listen to recommendations too much, though. :-)

But setting up WoL via ethtool from user space is an exception,
because user space actually *does* expect to see a change in sysfs in
this particular case.

It's basically two different ways to change the same setting and both
should result in the same behavior, ideally.

Thanks,
Rafael