| Message ID | 1510154134-1248-1-git-send-email-ulf.hansson@linaro.org |
|---|---|
| Headers | show |
| Series | PM / core: Invent a WAKEUP_POWERED driver flag | expand |
Hi Ulf, 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. Thanks for your patches! They look good to me, hence my Reviewed-by. > 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. Not using GENPD_FLAG_ACTIVE_WAKEUP means I would have to add the WAKEUP_POWERED flag to every single driver that can either be a wakeup source itself, or be part of the wakeup path. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ulf, > > 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. > > Thanks for your patches! > They look good to me, hence my Reviewed-by. Hi Geert, Thanks for reviewing, much appreciated! > >> 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. > > Not using GENPD_FLAG_ACTIVE_WAKEUP means I would have to add the > WAKEUP_POWERED flag to every single driver that can either be a wakeup > source itself, or be part of the wakeup path. Right. First, is that really that many? Second, nothing prevent us from doing the migration of each driver in step by step. Kind regards Uffe
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. 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(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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? > > 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
Hi Ulf, 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: >> 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? There are ca. 100 callers in drivers. >> 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? It depends on your meaning of "up". My interpretation is that "up" means ready to handle packets between physical media and the Linux networking stack. So even when "down", the actual Ethernet controller may still be able to receive a magic packet if WoL is enabled. The magic packet is really a magic packet not intended to be transmitted to the networking stack, but merely serves as a wakeup signal. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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. :-) >> >> 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? In PCI NICs, if wakeup power is provided, the NIC can detect activity on the port and generate a PCI PME even if the I/F is down otherwise. Thanks, Rafael
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
[...] >>> 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? > >>> 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? > > It depends on your meaning of "up". My interpretation is that "up" means > ready to handle packets between physical media and the Linux networking stack. > > So even when "down", the actual Ethernet controller may still be able to > receive a magic packet if WoL is enabled. The magic packet is really a > magic packet not intended to be transmitted to the networking stack, but > merely serves as a wakeup signal. I see! So, in the end this seems like a combination of what the HW supports and what the user policy is set to. Out of curiosity, can you tell how those Renesas Ethernet devices works in this regards? Anyway, thanks for clarifying! Kind regards Uffe
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. >>>> 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? >> >> It depends on your meaning of "up". My interpretation is that "up" means >> ready to handle packets between physical media and the Linux networking stack. >> >> So even when "down", the actual Ethernet controller may still be able to >> receive a magic packet if WoL is enabled. The magic packet is really a >> magic packet not intended to be transmitted to the networking stack, but >> merely serves as a wakeup signal. > > I see! So, in the end this seems like a combination of what the HW > supports and what the user policy is set to. > > Out of curiosity, can you tell how those Renesas Ethernet devices > works in this regards? I don't know, I was just playing the devil's advocate ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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. Thanks, Rafael
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
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 Ulf Hansson (3): PM / core: Re-factor some code dealing with parents in __device_suspend() PM / core: Add WAKEUP_POWERED driver flag PM / Domains: Take WAKEUP_POWERED driver flag into account Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ drivers/base/power/domain.c | 14 ++++++++++---- drivers/base/power/main.c | 33 +++++++++++++++++++++------------ include/linux/pm.h | 5 +++++ 4 files changed, 48 insertions(+), 16 deletions(-) -- 2.7.4