On 9 November 2017 at 10:02, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > Hi Ulf, > > On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: >>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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? > > In addition, keeping WoL enabled for cases 1 and 2 may be desirable > (e.g allow wake-up if a cable is plugged in during system suspend and > a magic packet is received afterwards), depending on the hardware. > But the driver can already control that through device_set_wakeup_enable(). > Ehh, that sounds weird. :-) If the Ethernet interface is down, how would such packet be received? Kind regards Uffe
