| Message ID | 1513353391-30806-2-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / core: Extend behaviour for wakeup paths | expand |
On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > The PM core in the device_prepare() phase, resets the wakeup_path status > flag to the value of device_may_wakeup(). This means if a ->prepare() or a > ->suspend() callback for the device would update the device's wakeup > setting, this doesn't become reflected in the wakeup_path status flag. > > In general this isn't a problem, because wakeup settings isn't supposed to > be changed during those system suspend phases. Nevertheless, there are a > cases not conforming to that behaviour, as device_set_wakeup_enable() is > indeed called from ->suspend() callbacks. And why is this regarded as correct?
On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> The PM core in the device_prepare() phase, resets the wakeup_path status >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a >> ->suspend() callback for the device would update the device's wakeup >> setting, this doesn't become reflected in the wakeup_path status flag. >> >> In general this isn't a problem, because wakeup settings isn't supposed to >> be changed during those system suspend phases. Nevertheless, there are a >> cases not conforming to that behaviour, as device_set_wakeup_enable() is >> indeed called from ->suspend() callbacks. > > And why is this regarded as correct? I am not saying that this behavior is correct. However, I am trying to improve the situation, which doesn't hurt or does it? More importantly, the next patch, which is about the wakeup path, depends on this. Kind regards Uffe
On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote: > On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> The PM core in the device_prepare() phase, resets the wakeup_path status > >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a > >> ->suspend() callback for the device would update the device's wakeup > >> setting, this doesn't become reflected in the wakeup_path status flag. > >> > >> In general this isn't a problem, because wakeup settings isn't supposed to > >> be changed during those system suspend phases. Nevertheless, there are a > >> cases not conforming to that behaviour, as device_set_wakeup_enable() is > >> indeed called from ->suspend() callbacks. > > > > And why is this regarded as correct? > > I am not saying that this behavior is correct. However, I am trying to > improve the situation, which doesn't hurt or does it? Adding a workaround for them kind of encourages new code to do the same thing, which actually may really hurt. So I assume that these call sites are all legacy and that's why you don't want to touch them for now, but in that case the commit message should make it very clear that this is about legacy only and new code should not call device_set_wakeup_enable() during suspend. [Something should be printed to the log if wakeup source objects are created while system suspend is in progress I guess or similar.] > More importantly, the next patch, which is about the wakeup path, > depends on this. Honestly, this sounds like "We have this change we really really would like to make, but there's incorrect code getting in the way, so let's paper over it and be done." Not very nice. :-/ How many drivers actually do call device_set_wakeup_enable() during suspend? Thanks, Rafael
On 22 December 2017 at 20:12, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote: >> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote: >> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> The PM core in the device_prepare() phase, resets the wakeup_path status >> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a >> >> ->suspend() callback for the device would update the device's wakeup >> >> setting, this doesn't become reflected in the wakeup_path status flag. >> >> >> >> In general this isn't a problem, because wakeup settings isn't supposed to >> >> be changed during those system suspend phases. Nevertheless, there are a >> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is >> >> indeed called from ->suspend() callbacks. >> > >> > And why is this regarded as correct? >> >> I am not saying that this behavior is correct. However, I am trying to >> improve the situation, which doesn't hurt or does it? > > Adding a workaround for them kind of encourages new code to do the same > thing, which actually may really hurt. So I assume that these call sites are > all legacy and that's why you don't want to touch them for now, but in that > case the commit message should make it very clear that this is about legacy > only and new code should not call device_set_wakeup_enable() during suspend. Yeah, makes sense. Let me clarify that! > > [Something should be printed to the log if wakeup source objects > are created while system suspend is in progress I guess or similar.] Yes, good idea. Let me cook a patch for that as well. > >> More importantly, the next patch, which is about the wakeup path, >> depends on this. > > Honestly, this sounds like "We have this change we really really would like to > make, but there's incorrect code getting in the way, so let's paper over it > and be done." Not very nice. :-/ Yeah it sounds a bit weird, I agree. However, maybe if I update the changelog and fold in a patch printing an error in case the APIs is being abused, that would sort out the confusion? Another option is simply to squash patch 1 and patch2, what do you prefer? > > How many drivers actually do call device_set_wakeup_enable() during suspend? There are some ethernet/wifi drivers, although it hard to say how many without a more thorough investigation. Besides those I found these more obvious ones: drivers/ssb/pcihost_wrapper.c drivers/staging/rtlwifi/core.c drivers/tty/serial/atmel_serial.c drivers/usb/core/hcd-pci.c Kind regards Uffe
On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 22 December 2017 at 20:12, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote: >>> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> >> The PM core in the device_prepare() phase, resets the wakeup_path status >>> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a >>> >> ->suspend() callback for the device would update the device's wakeup >>> >> setting, this doesn't become reflected in the wakeup_path status flag. >>> >> >>> >> In general this isn't a problem, because wakeup settings isn't supposed to >>> >> be changed during those system suspend phases. Nevertheless, there are a >>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is >>> >> indeed called from ->suspend() callbacks. >>> > >>> > And why is this regarded as correct? >>> >>> I am not saying that this behavior is correct. However, I am trying to >>> improve the situation, which doesn't hurt or does it? >> >> Adding a workaround for them kind of encourages new code to do the same >> thing, which actually may really hurt. So I assume that these call sites are >> all legacy and that's why you don't want to touch them for now, but in that >> case the commit message should make it very clear that this is about legacy >> only and new code should not call device_set_wakeup_enable() during suspend. > > Yeah, makes sense. Let me clarify that! > >> >> [Something should be printed to the log if wakeup source objects >> are created while system suspend is in progress I guess or similar.] > > Yes, good idea. Let me cook a patch for that as well. > >> >>> More importantly, the next patch, which is about the wakeup path, >>> depends on this. >> >> Honestly, this sounds like "We have this change we really really would like to >> make, but there's incorrect code getting in the way, so let's paper over it >> and be done." Not very nice. :-/ > > Yeah it sounds a bit weird, I agree. > > However, maybe if I update the changelog and fold in a patch printing > an error in case the APIs is being abused, that would sort out the > confusion? That should be sufficient IMO. > Another option is simply to squash patch 1 and patch2, what do you prefer? > >> >> How many drivers actually do call device_set_wakeup_enable() during suspend? > > There are some ethernet/wifi drivers, although it hard to say how many > without a more thorough investigation. > > Besides those I found these more obvious ones: > drivers/ssb/pcihost_wrapper.c > drivers/staging/rtlwifi/core.c > drivers/tty/serial/atmel_serial.c > drivers/usb/core/hcd-pci.c Ugh. I need to look at the last one at least ... Thanks, Rafael
On Sat, Dec 23, 2017 at 1:50 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 22 December 2017 at 20:12, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote: >>>> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote: >>>> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> >> The PM core in the device_prepare() phase, resets the wakeup_path status >>>> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a >>>> >> ->suspend() callback for the device would update the device's wakeup >>>> >> setting, this doesn't become reflected in the wakeup_path status flag. >>>> >> >>>> >> In general this isn't a problem, because wakeup settings isn't supposed to >>>> >> be changed during those system suspend phases. Nevertheless, there are a >>>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is >>>> >> indeed called from ->suspend() callbacks. >>>> > >>>> > And why is this regarded as correct? >>>> >>>> I am not saying that this behavior is correct. However, I am trying to >>>> improve the situation, which doesn't hurt or does it? >>> >>> Adding a workaround for them kind of encourages new code to do the same >>> thing, which actually may really hurt. So I assume that these call sites are >>> all legacy and that's why you don't want to touch them for now, but in that >>> case the commit message should make it very clear that this is about legacy >>> only and new code should not call device_set_wakeup_enable() during suspend. >> >> Yeah, makes sense. Let me clarify that! >> >>> >>> [Something should be printed to the log if wakeup source objects >>> are created while system suspend is in progress I guess or similar.] >> >> Yes, good idea. Let me cook a patch for that as well. >> >>> >>>> More importantly, the next patch, which is about the wakeup path, >>>> depends on this. >>> >>> Honestly, this sounds like "We have this change we really really would like to >>> make, but there's incorrect code getting in the way, so let's paper over it >>> and be done." Not very nice. :-/ >> >> Yeah it sounds a bit weird, I agree. >> >> However, maybe if I update the changelog and fold in a patch printing >> an error in case the APIs is being abused, that would sort out the >> confusion? > > That should be sufficient IMO. > >> Another option is simply to squash patch 1 and patch2, what do you prefer? >> >>> >>> How many drivers actually do call device_set_wakeup_enable() during suspend? >> >> There are some ethernet/wifi drivers, although it hard to say how many >> without a more thorough investigation. >> >> Besides those I found these more obvious ones: >> drivers/ssb/pcihost_wrapper.c >> drivers/staging/rtlwifi/core.c >> drivers/tty/serial/atmel_serial.c >> drivers/usb/core/hcd-pci.c > > Ugh. I need to look at the last one at least ... The drivers/usb/core/hcd-pci.c instance is actually valid, because it calls device_set_wakeup_enable() to remove the wakeup source object in case the device is dead. Moreover, it does that in the "noirq" phase which is not covered by your patch anyway. Thanks, Rafael
[...] >>>> How many drivers actually do call device_set_wakeup_enable() during suspend? >>> >>> There are some ethernet/wifi drivers, although it hard to say how many >>> without a more thorough investigation. >>> >>> Besides those I found these more obvious ones: >>> drivers/ssb/pcihost_wrapper.c >>> drivers/staging/rtlwifi/core.c >>> drivers/tty/serial/atmel_serial.c >>> drivers/usb/core/hcd-pci.c >> >> Ugh. I need to look at the last one at least ... > > The drivers/usb/core/hcd-pci.c instance is actually valid, because it > calls device_set_wakeup_enable() to remove the wakeup source object in > case the device is dead. Moreover, it does that in the "noirq" phase > which is not covered by your patch anyway. Right, but this puzzles me a bit. Could you explain what you mean by valid here? Is it okay to call device_set_wakeup_enable() in the suspend phase after all? No? My very vague idea at this point is, if we find cases where device_set_wakeup_enable() makes sense to call during system suspend, probably those could/should be replaces by using the "wakeup_path" flag instead, somehow. Kind regards Uffe
On Saturday, December 23, 2017 4:17:58 PM CET Ulf Hansson wrote: > [...] > > >>>> How many drivers actually do call device_set_wakeup_enable() during suspend? > >>> > >>> There are some ethernet/wifi drivers, although it hard to say how many > >>> without a more thorough investigation. > >>> > >>> Besides those I found these more obvious ones: > >>> drivers/ssb/pcihost_wrapper.c > >>> drivers/staging/rtlwifi/core.c > >>> drivers/tty/serial/atmel_serial.c > >>> drivers/usb/core/hcd-pci.c > >> > >> Ugh. I need to look at the last one at least ... > > > > The drivers/usb/core/hcd-pci.c instance is actually valid, because it > > calls device_set_wakeup_enable() to remove the wakeup source object in > > case the device is dead. Moreover, it does that in the "noirq" phase > > which is not covered by your patch anyway. > > Right, but this puzzles me a bit. > > Could you explain what you mean by valid here? Is it okay to call > device_set_wakeup_enable() in the suspend phase after all? No? This is an exception and it would be better to call device_wakeup_disable() directly here. In general, calling device_set_wakeup_enable() during system suspend (to "enable" device wakeup) and then during system resume (to "disable" it) may be problematic. First, if device_wakeup_enable() is run during system suspend, it will create a wakeup source object, which then, quite obviously, can only cover the time after its creation, so there is a window (between the start of the system suspend transition and the creation of the wakeup source) during which wakeup events may be missed. That is not only relevant for devices supporting runtime PM, because input device drivers, say, may choose to report system wakeup events every time they see input (eg. from their interrupt handlers or similar) and wakeup sources used for that should be present before system suspend begins. Also, if the wakeup source is not present already, this effectively overrides the choice made by user space which didn't set the device's "wakeup" attribute in sysfs to "enabled", so it should not wake up the system. Second, if device_wakeup_disable() is run during system resume, it effectively destroys information that can be used to figure out why the system woke up and, again, it may cause the choice made by user space to be effectively overridden. However, if the device is dead, calling device_wakeup_disable() on it is fine at any time (as dead devices do not wake up the system as a rule). > My very vague idea at this point is, if we find cases where > device_set_wakeup_enable() makes sense to call during system suspend, > probably those could/should be replaces by using the "wakeup_path" > flag instead, somehow. Not really in this particular case. Clearing the wakeup enable bit for the device plays the role of "don't bother any more" here. Thanks, Rafael
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 6e8cc5d..810e5fb 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1620,6 +1620,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async) End: if (!error) { dev->power.is_suspended = true; + if (device_may_wakeup(dev)) + dev->power.wakeup_path = true; dpm_propagate_to_parent(dev); dpm_clear_suppliers_direct_complete(dev); } @@ -1744,7 +1746,7 @@ static int device_prepare(struct device *dev, pm_message_t state) device_lock(dev); - dev->power.wakeup_path = device_may_wakeup(dev); + dev->power.wakeup_path = false; if (dev->power.no_pm_callbacks) { ret = 1; /* Let device go direct_complete */
The PM core in the device_prepare() phase, resets the wakeup_path status flag to the value of device_may_wakeup(). This means if a ->prepare() or a ->suspend() callback for the device would update the device's wakeup setting, this doesn't become reflected in the wakeup_path status flag. In general this isn't a problem, because wakeup settings isn't supposed to be changed during those system suspend phases. Nevertheless, there are a cases not conforming to that behaviour, as device_set_wakeup_enable() is indeed called from ->suspend() callbacks. To address this, let's move the assignment of the wakeup_path status flag to the __device_suspend() phase and more precisely, after the ->suspend() callback has been invoked. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/base/power/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- 2.7.4