| Message ID | 1510154134-1248-3-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PM / core: Invent a WAKEUP_POWERED driver flag | expand |
On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > 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. > > In particular, sometimes the device may need to stay in full power state, > to have wakeup signals enabled for it. Therefore, define and document a > WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains > exactly about that. > > During __device_suspend() in the PM core, let's make sure to also propagate > the setting of the flag to the parent device, when wakeup signals are > enabled and unless the parent has the "ignore_children" flag set. This > makes it also consistent with how the existing "wakeup_path" flag is being > assigned. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> 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 Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > 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. > > In particular, sometimes the device may need to stay in full power state, > to have wakeup signals enabled for it. Therefore, define and document a > WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains > exactly about that. > > During __device_suspend() in the PM core, let's make sure to also propagate > the setting of the flag to the parent device, when wakeup signals are > enabled and unless the parent has the "ignore_children" flag set. This > makes it also consistent with how the existing "wakeup_path" flag is being > assigned. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ > drivers/base/power/main.c | 6 +++++- > include/linux/pm.h | 5 +++++ > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst > index 53c1b0b..1ca2d0f 100644 > --- a/Documentation/driver-api/pm/devices.rst > +++ b/Documentation/driver-api/pm/devices.rst > @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` > might identify GPIO signals hooked up to a switch or other external hardware, > and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. > > +Moreover, in case wakeup signals are enabled for a device, some bus types and > +PM domains may manage the device slightly differently during system suspend. For > +example, sometimes the device needs to stay in full power state, to have wakeup > +signals enabled for it. In cases when the wakeup settings are mostly managed by > +the driver, it may not be sufficient for bus types and PM domains to only check > +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions > +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in > +:c:member:`power.driver_flags`, by passing the flag to > +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM > +domains to leave the device in full power state, when wakeup signals are enabled > +for it. IMO this is a bit unclear. First off, how does the driver know that the device has to be in full power for wakeup to work? Second, this requirement sort of implies that the device cannot go into a low-power state during runtime suspend too, so it basically remains stays at full power even when runtime-suspended. Does it then mean that the middle layer is expected to simply avoid changing the power state of the device when enabled to wake up the system, or is there more to that? In the former case, it may be better to rename the flag to something along the lines of "don't change power state if wakeup enabled". > + > If any of these callbacks returns an error, the system won't enter the desired > low-power state. Instead, the PM core will unwind its actions by resuming all > the devices that were suspended. > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 8089e72..f64f945 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device *dev) > spin_lock_irq(&parent->power.lock); > > parent->power.direct_complete = false; > - if (dev->power.wakeup_path && !parent->power.ignore_children) > + if (dev->power.wakeup_path && !parent->power.ignore_children) { > parent->power.wakeup_path = true; > > + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED)) > + parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED; > + } > + > spin_unlock_irq(&parent->power.lock); > } > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 65d3911..34c2404 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -559,6 +559,7 @@ struct pm_subsys_data { > * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device. > * SMART_PREPARE: Check the return value of the driver's ->prepare callback. > * SMART_SUSPEND: No need to resume the device from runtime suspend. > + * WAKEUP_POWERED: Keep the device powered if it has wakeup enabled. > * > * Setting SMART_PREPARE instructs bus types and PM domains which may want > * system suspend/resume callbacks to be skipped for the device to return 0 from > @@ -572,10 +573,14 @@ struct pm_subsys_data { > * necessary from the driver's perspective. It also may cause them to skip > * invocations of the ->suspend_late and ->suspend_noirq callbacks provided by > * the driver if they decide to leave the device in runtime suspend. > + * > + * Setting WAKEUP_POWERED instructs bus types and PM domains that the device > + * needs to remain powered in system suspend, in case wakeup is enabled for it. > */ > #define DPM_FLAG_NEVER_SKIP BIT(0) > #define DPM_FLAG_SMART_PREPARE BIT(1) > #define DPM_FLAG_SMART_SUSPEND BIT(2) > +#define DPM_FLAG_WAKEUP_POWERED BIT(3) I'd prefer this to be BIT(4). > > struct dev_pm_info { > pm_message_t power_state; > -- > 2.7.4 > Thanks, Rafael
On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > 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. > > In particular, sometimes the device may need to stay in full power state, > to have wakeup signals enabled for it. Therefore, define and document a > WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains > exactly about that. > > During __device_suspend() in the PM core, let's make sure to also propagate > the setting of the flag to the parent device, when wakeup signals are > enabled and unless the parent has the "ignore_children" flag set. This > makes it also consistent with how the existing "wakeup_path" flag is being > assigned. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ > drivers/base/power/main.c | 6 +++++- > include/linux/pm.h | 5 +++++ > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst > index 53c1b0b..1ca2d0f 100644 > --- a/Documentation/driver-api/pm/devices.rst > +++ b/Documentation/driver-api/pm/devices.rst > @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` > might identify GPIO signals hooked up to a switch or other external hardware, > and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. > > +Moreover, in case wakeup signals are enabled for a device, some bus types and > +PM domains may manage the device slightly differently during system suspend. For > +example, sometimes the device needs to stay in full power state, to have wakeup > +signals enabled for it. In cases when the wakeup settings are mostly managed by > +the driver, it may not be sufficient for bus types and PM domains to only check > +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions > +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in > +:c:member:`power.driver_flags`, by passing the flag to > +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM > +domains to leave the device in full power state, when wakeup signals are enabled > +for it. > + > If any of these callbacks returns an error, the system won't enter the desired > low-power state. Instead, the PM core will unwind its actions by resuming all > the devices that were suspended. > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 8089e72..f64f945 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device *dev) > spin_lock_irq(&parent->power.lock); > > parent->power.direct_complete = false; > - if (dev->power.wakeup_path && !parent->power.ignore_children) > + if (dev->power.wakeup_path && !parent->power.ignore_children) { > parent->power.wakeup_path = true; > > + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED)) > + parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED; No, sorry. The flag cannot be propagated this way, because that effectively overrides the choices made by the parent driver and up. Besides, wakeup_path already had a similar purpose. What has happened to that? Thanks, Rafael
On 9 November 2017 at 01:24, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> 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. >> >> In particular, sometimes the device may need to stay in full power state, >> to have wakeup signals enabled for it. Therefore, define and document a >> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains >> exactly about that. >> >> During __device_suspend() in the PM core, let's make sure to also propagate >> the setting of the flag to the parent device, when wakeup signals are >> enabled and unless the parent has the "ignore_children" flag set. This >> makes it also consistent with how the existing "wakeup_path" flag is being >> assigned. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ >> drivers/base/power/main.c | 6 +++++- >> include/linux/pm.h | 5 +++++ >> 3 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst >> index 53c1b0b..1ca2d0f 100644 >> --- a/Documentation/driver-api/pm/devices.rst >> +++ b/Documentation/driver-api/pm/devices.rst >> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` >> might identify GPIO signals hooked up to a switch or other external hardware, >> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. >> >> +Moreover, in case wakeup signals are enabled for a device, some bus types and >> +PM domains may manage the device slightly differently during system suspend. For >> +example, sometimes the device needs to stay in full power state, to have wakeup >> +signals enabled for it. In cases when the wakeup settings are mostly managed by >> +the driver, it may not be sufficient for bus types and PM domains to only check >> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions >> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in >> +:c:member:`power.driver_flags`, by passing the flag to >> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM >> +domains to leave the device in full power state, when wakeup signals are enabled >> +for it. > > IMO this is a bit unclear. > > First off, how does the driver know that the device has to be in full > power for wakeup to work? Because the device is accessed as part of dealing with the wakeup. > > Second, this requirement sort of implies that the device cannot go > into a low-power state during runtime suspend too, so it basically > remains stays at full power even when runtime-suspended. No, not really, because that depends on the current situation. An Ethernet device can surely go into a low power state, at runtime suspend, when the interface is down, for example. > > Does it then mean that the middle layer is expected to simply avoid > changing the power state of the device when enabled to wake up the > system, or is there more to that? In the former case, it may be > better to rename the flag to something along the lines of "don't > change power state if wakeup enabled". Yes, correct. I can try to clarify that in the description and unless you have a suggestion for a better name of the flag, I try to come up with something for that too. >> #define DPM_FLAG_NEVER_SKIP BIT(0) >> #define DPM_FLAG_SMART_PREPARE BIT(1) >> #define DPM_FLAG_SMART_SUSPEND BIT(2) >> +#define DPM_FLAG_WAKEUP_POWERED BIT(3) > > I'd prefer this to be BIT(4). OK. I guess you can always also amend my patch, depending on in what order you merge things. :-) [...] Kind regards Uffe
On 9 November 2017 at 01:41, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> 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. >> >> In particular, sometimes the device may need to stay in full power state, >> to have wakeup signals enabled for it. Therefore, define and document a >> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains >> exactly about that. >> >> During __device_suspend() in the PM core, let's make sure to also propagate >> the setting of the flag to the parent device, when wakeup signals are >> enabled and unless the parent has the "ignore_children" flag set. This >> makes it also consistent with how the existing "wakeup_path" flag is being >> assigned. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ >> drivers/base/power/main.c | 6 +++++- >> include/linux/pm.h | 5 +++++ >> 3 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst >> index 53c1b0b..1ca2d0f 100644 >> --- a/Documentation/driver-api/pm/devices.rst >> +++ b/Documentation/driver-api/pm/devices.rst >> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` >> might identify GPIO signals hooked up to a switch or other external hardware, >> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. >> >> +Moreover, in case wakeup signals are enabled for a device, some bus types and >> +PM domains may manage the device slightly differently during system suspend. For >> +example, sometimes the device needs to stay in full power state, to have wakeup >> +signals enabled for it. In cases when the wakeup settings are mostly managed by >> +the driver, it may not be sufficient for bus types and PM domains to only check >> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions >> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in >> +:c:member:`power.driver_flags`, by passing the flag to >> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM >> +domains to leave the device in full power state, when wakeup signals are enabled >> +for it. >> + >> If any of these callbacks returns an error, the system won't enter the desired >> low-power state. Instead, the PM core will unwind its actions by resuming all >> the devices that were suspended. >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c >> index 8089e72..f64f945 100644 >> --- a/drivers/base/power/main.c >> +++ b/drivers/base/power/main.c >> @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device *dev) >> spin_lock_irq(&parent->power.lock); >> >> parent->power.direct_complete = false; >> - if (dev->power.wakeup_path && !parent->power.ignore_children) >> + if (dev->power.wakeup_path && !parent->power.ignore_children) { >> parent->power.wakeup_path = true; >> >> + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED)) >> + parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED; > > No, sorry. > > The flag cannot be propagated this way, because that effectively > overrides the choices made by the parent driver and up. Yes, but that is the hole point. If a child device needs to stay powered as to deal with wakeup, so is required by the parent. > > Besides, wakeup_path already had a similar purpose. What has happened to that? Yes, but wakeup_path is only telling half of what is needed. Because even if wakeup_path becomes set for a parent device, doesn't mean that it must stay in full power during system suspend to serve wakeups for a child. That's why I think the DPM_FLAG_WAKEUP_POWERED flag needs to be propagated also. Kind regards Uffe
On Thu, Nov 9, 2017 at 9:44 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 9 November 2017 at 01:24, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> 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. >>> >>> In particular, sometimes the device may need to stay in full power state, >>> to have wakeup signals enabled for it. Therefore, define and document a >>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains >>> exactly about that. >>> >>> During __device_suspend() in the PM core, let's make sure to also propagate >>> the setting of the flag to the parent device, when wakeup signals are >>> enabled and unless the parent has the "ignore_children" flag set. This >>> makes it also consistent with how the existing "wakeup_path" flag is being >>> assigned. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ >>> drivers/base/power/main.c | 6 +++++- >>> include/linux/pm.h | 5 +++++ >>> 3 files changed, 22 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst >>> index 53c1b0b..1ca2d0f 100644 >>> --- a/Documentation/driver-api/pm/devices.rst >>> +++ b/Documentation/driver-api/pm/devices.rst >>> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` >>> might identify GPIO signals hooked up to a switch or other external hardware, >>> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. >>> >>> +Moreover, in case wakeup signals are enabled for a device, some bus types and >>> +PM domains may manage the device slightly differently during system suspend. For >>> +example, sometimes the device needs to stay in full power state, to have wakeup >>> +signals enabled for it. In cases when the wakeup settings are mostly managed by >>> +the driver, it may not be sufficient for bus types and PM domains to only check >>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions >>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in >>> +:c:member:`power.driver_flags`, by passing the flag to >>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM >>> +domains to leave the device in full power state, when wakeup signals are enabled >>> +for it. >> >> IMO this is a bit unclear. >> >> First off, how does the driver know that the device has to be in full >> power for wakeup to work? > > Because the device is accessed as part of dealing with the wakeup. Yes, it is, In the working state of the system. In the system wakeup case it may not be. Essentially, what happens then is that driver-provided interrupt handlers don't run as a rule and system wakeup is triggered by the low-level handler at the IRQ chip level. Next, the PM callbacks invoked for the device are expected to clean up the wakeup status etc. Of course, power still is necessary for that to work, but it may be not be in-band. That may be either in-band power used for normal operations and provided through the interconnect used by the device or it may be special wakeup power provided out-of-band. Also the wakeup signal itself may be an in-band device interrupt (like the ones used for signaling IO events during normal operation) or it may be a special wakeup signal (like PCI PME) in which case, from the driver's perspective, the wakeup signaling is out-of-band. Usually, the driver doesn't know how this is set up for the particular device in the particular platform and hence my question. :-) The case at hand seems to be when the wakeup power is in-band or the wakeup signal is an in-band interrupt (in which case power needs to be provided to the interconnect at least). If they both are out-of-band, the middle layer should know that, because as a rule it will be involved in setting up both of them. Otherwise, in principle, it should assume that in-band power needs to be provided for wakeup to work and avoid turning things off if wakeup is enabled. >> >> Second, this requirement sort of implies that the device cannot go >> into a low-power state during runtime suspend too, so it basically >> remains stays at full power even when runtime-suspended. > > No, not really, because that depends on the current situation. > > An Ethernet device can surely go into a low power state, at runtime > suspend, when the interface is down, for example. But then it is not expected to generate wakeup signals I suppose. [BTW, I wonder how it detects when the cable is connected again to it. I know what happens in PCI NICs, but that clearly is not the case here.] Well, anyway, it looks like the case when the device is runtime-suspended right before system suspend and it is going to stay suspended is not interesting here, because the state will be retained whatever it is then. An interesting case seems to be when the device is not runtime-suspended when system suspend triggers. >> >> Does it then mean that the middle layer is expected to simply avoid >> changing the power state of the device when enabled to wake up the >> system, or is there more to that? In the former case, it may be >> better to rename the flag to something along the lines of "don't >> change power state if wakeup enabled". > > Yes, correct. > > I can try to clarify that in the description and unless you have a > suggestion for a better name of the flag, I try to come up with > something for that too. I was thinking about something like DPM_FLAG_IN_BAND_WAKEUP or similar. But please note that there are cases in which the middle layer has information on what power states to put devices into for wakeup to work and IMO that should take precedence over the flag as a rule. >>> #define DPM_FLAG_NEVER_SKIP BIT(0) >>> #define DPM_FLAG_SMART_PREPARE BIT(1) >>> #define DPM_FLAG_SMART_SUSPEND BIT(2) >>> +#define DPM_FLAG_WAKEUP_POWERED BIT(3) >> >> I'd prefer this to be BIT(4). > > OK. > > I guess you can always also amend my patch, depending on in what order > you merge things. :-) Right. Thanks, Rafael
On Thu, Nov 9, 2017 at 9:53 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 9 November 2017 at 01:41, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> 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. >>> >>> In particular, sometimes the device may need to stay in full power state, >>> to have wakeup signals enabled for it. Therefore, define and document a >>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains >>> exactly about that. >>> >>> During __device_suspend() in the PM core, let's make sure to also propagate >>> the setting of the flag to the parent device, when wakeup signals are >>> enabled and unless the parent has the "ignore_children" flag set. This >>> makes it also consistent with how the existing "wakeup_path" flag is being >>> assigned. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ >>> drivers/base/power/main.c | 6 +++++- >>> include/linux/pm.h | 5 +++++ >>> 3 files changed, 22 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst >>> index 53c1b0b..1ca2d0f 100644 >>> --- a/Documentation/driver-api/pm/devices.rst >>> +++ b/Documentation/driver-api/pm/devices.rst >>> @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` >>> might identify GPIO signals hooked up to a switch or other external hardware, >>> and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. >>> >>> +Moreover, in case wakeup signals are enabled for a device, some bus types and >>> +PM domains may manage the device slightly differently during system suspend. For >>> +example, sometimes the device needs to stay in full power state, to have wakeup >>> +signals enabled for it. In cases when the wakeup settings are mostly managed by >>> +the driver, it may not be sufficient for bus types and PM domains to only check >>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions >>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in >>> +:c:member:`power.driver_flags`, by passing the flag to >>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM >>> +domains to leave the device in full power state, when wakeup signals are enabled >>> +for it. >>> + >>> If any of these callbacks returns an error, the system won't enter the desired >>> low-power state. Instead, the PM core will unwind its actions by resuming all >>> the devices that were suspended. >>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c >>> index 8089e72..f64f945 100644 >>> --- a/drivers/base/power/main.c >>> +++ b/drivers/base/power/main.c >>> @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device *dev) >>> spin_lock_irq(&parent->power.lock); >>> >>> parent->power.direct_complete = false; >>> - if (dev->power.wakeup_path && !parent->power.ignore_children) >>> + if (dev->power.wakeup_path && !parent->power.ignore_children) { >>> parent->power.wakeup_path = true; >>> >>> + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED)) >>> + parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED; >> >> No, sorry. >> >> The flag cannot be propagated this way, because that effectively >> overrides the choices made by the parent driver and up. > > Yes, but that is the hole point. > > If a child device needs to stay powered as to deal with wakeup, so is > required by the parent. So you need a flag and a status bit. The flag says what the driver wants (and what it wants for a particular device) and the status bit reflects the current situation (taking dependencies into account). Say a device has two children, A and B, and both of them have the new flag set. If either A or B is configured for system wakeup, power should not be removed from the parent. However, if neither A nor B is configured for system wakeup, power can be removed from the parent on suspend unless the parent driver itself has set the new flag. Thus setting the new flag by the child drivers alone doesn't imply the specific handling of the parent unless additional conditions occur. That can be represented by a status bit that will be set or unset on every suspend individually taking the current configuration into account every time. >> >> Besides, wakeup_path already had a similar purpose. What has happened to that? > > Yes, but wakeup_path is only telling half of what is needed. > > Because even if wakeup_path becomes set for a parent device, doesn't > mean that it must stay in full power during system suspend to serve > wakeups for a child. That's why I think the DPM_FLAG_WAKEUP_POWERED > flag needs to be propagated also. OK, so one more status bit is needed, then. Thanks, Rafael
[...] >>> >>> First off, how does the driver know that the device has to be in full >>> power for wakeup to work? >> >> Because the device is accessed as part of dealing with the wakeup. > > Yes, it is, In the working state of the system. In the system wakeup > case it may not be. > > Essentially, what happens then is that driver-provided interrupt > handlers don't run as a rule and system wakeup is triggered by the > low-level handler at the IRQ chip level. Next, the PM callbacks > invoked for the device are expected to clean up the wakeup status etc. > > Of course, power still is necessary for that to work, but it may be > not be in-band. That may be either in-band power used for normal > operations and provided through the interconnect used by the device or > it may be special wakeup power provided out-of-band. > > Also the wakeup signal itself may be an in-band device interrupt (like > the ones used for signaling IO events during normal operation) or it > may be a special wakeup signal (like PCI PME) in which case, from the > driver's perspective, the wakeup signaling is out-of-band. > > Usually, the driver doesn't know how this is set up for the particular > device in the particular platform and hence my question. :-) > > The case at hand seems to be when the wakeup power is in-band or the > wakeup signal is an in-band interrupt (in which case power needs to be > provided to the interconnect at least). Correct! > > If they both are out-of-band, the middle layer should know that, > because as a rule it will be involved in setting up both of them. > Otherwise, in principle, it should assume that in-band power needs to > be provided for wakeup to work and avoid turning things off if wakeup > is enabled. Agree! > >>> >>> Second, this requirement sort of implies that the device cannot go >>> into a low-power state during runtime suspend too, so it basically >>> remains stays at full power even when runtime-suspended. >> >> No, not really, because that depends on the current situation. >> >> An Ethernet device can surely go into a low power state, at runtime >> suspend, when the interface is down, for example. > > But then it is not expected to generate wakeup signals I suppose. Correct. > > [BTW, I wonder how it detects when the cable is connected again to it. > I know what happens in PCI NICs, but that clearly is not the case > here.] > > Well, anyway, it looks like the case when the device is > runtime-suspended right before system suspend and it is going to stay > suspended is not interesting here, because the state will be retained > whatever it is then. Correct. > > An interesting case seems to be when the device is not > runtime-suspended when system suspend triggers. > Yes. The principle is that the driver needs to runtime resume its device, if not already, during system suspend, as to be able to configure the in-band interrupt, then instruct the upper layers that the device needs to stay in its current power state. >>> >>> Does it then mean that the middle layer is expected to simply avoid >>> changing the power state of the device when enabled to wake up the >>> system, or is there more to that? In the former case, it may be >>> better to rename the flag to something along the lines of "don't >>> change power state if wakeup enabled". >> >> Yes, correct. >> >> I can try to clarify that in the description and unless you have a >> suggestion for a better name of the flag, I try to come up with >> something for that too. > > I was thinking about something like DPM_FLAG_IN_BAND_WAKEUP or similar. Perfect! > > But please note that there are cases in which the middle layer has > information on what power states to put devices into for wakeup to > work and IMO that should take precedence over the flag as a rule. Yeah, let me try to clarify that in the doc. [...] Kind regards Uffe
diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst index 53c1b0b..1ca2d0f 100644 --- a/Documentation/driver-api/pm/devices.rst +++ b/Documentation/driver-api/pm/devices.rst @@ -414,6 +414,18 @@ when the system is in the sleep state. For example, :c:func:`enable_irq_wake()` might identify GPIO signals hooked up to a switch or other external hardware, and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal. +Moreover, in case wakeup signals are enabled for a device, some bus types and +PM domains may manage the device slightly differently during system suspend. For +example, sometimes the device needs to stay in full power state, to have wakeup +signals enabled for it. In cases when the wakeup settings are mostly managed by +the driver, it may not be sufficient for bus types and PM domains to only check +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in +:c:member:`power.driver_flags`, by passing the flag to +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM +domains to leave the device in full power state, when wakeup signals are enabled +for it. + If any of these callbacks returns an error, the system won't enter the desired low-power state. Instead, the PM core will unwind its actions by resuming all the devices that were suspended. diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 8089e72..f64f945 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device *dev) spin_lock_irq(&parent->power.lock); parent->power.direct_complete = false; - if (dev->power.wakeup_path && !parent->power.ignore_children) + if (dev->power.wakeup_path && !parent->power.ignore_children) { parent->power.wakeup_path = true; + if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED)) + parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED; + } + spin_unlock_irq(&parent->power.lock); } diff --git a/include/linux/pm.h b/include/linux/pm.h index 65d3911..34c2404 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -559,6 +559,7 @@ struct pm_subsys_data { * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device. * SMART_PREPARE: Check the return value of the driver's ->prepare callback. * SMART_SUSPEND: No need to resume the device from runtime suspend. + * WAKEUP_POWERED: Keep the device powered if it has wakeup enabled. * * Setting SMART_PREPARE instructs bus types and PM domains which may want * system suspend/resume callbacks to be skipped for the device to return 0 from @@ -572,10 +573,14 @@ struct pm_subsys_data { * necessary from the driver's perspective. It also may cause them to skip * invocations of the ->suspend_late and ->suspend_noirq callbacks provided by * the driver if they decide to leave the device in runtime suspend. + * + * Setting WAKEUP_POWERED instructs bus types and PM domains that the device + * needs to remain powered in system suspend, in case wakeup is enabled for it. */ #define DPM_FLAG_NEVER_SKIP BIT(0) #define DPM_FLAG_SMART_PREPARE BIT(1) #define DPM_FLAG_SMART_SUSPEND BIT(2) +#define DPM_FLAG_WAKEUP_POWERED BIT(3) struct dev_pm_info { pm_message_t power_state;
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. In particular, sometimes the device may need to stay in full power state, to have wakeup signals enabled for it. Therefore, define and document a WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains exactly about that. During __device_suspend() in the PM core, let's make sure to also propagate the setting of the flag to the parent device, when wakeup signals are enabled and unless the parent has the "ignore_children" flag set. This makes it also consistent with how the existing "wakeup_path" flag is being assigned. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- Documentation/driver-api/pm/devices.rst | 12 ++++++++++++ drivers/base/power/main.c | 6 +++++- include/linux/pm.h | 5 +++++ 3 files changed, 22 insertions(+), 1 deletion(-) -- 2.7.4