On Sat, Dec 23, 2017 at 1:50 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> On 22 December 2017 at 20:12, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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@xxxxxxxxxx> wrote: >>>> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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
