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? 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
