| Message ID | 20220607035158.308111-1-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | PM / wakeup: Unify device_init_wakeup() for PM_SLEEP and !PM_SLEEP | expand |
On Mon, Jun 06, 2022 at 10:51:58PM -0500, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > Previously the CONFIG_PM_SLEEP and !CONFIG_PM_SLEEP device_init_wakeup() > implementations differed in confusing ways: > > - The PM_SLEEP version checked for a NULL device pointer and returned > -EINVAL, while the !PM_SLEEP version did not and would simply > dereference a NULL pointer. > > - When called with "false", the !PM_SLEEP version cleared "capable" and > "enable" in the opposite order of the PM_SLEEP version. That was > harmless because for !PM_SLEEP they're simple assignments, but it's > unnecessary confusion. > > Use a simplified version of the PM_SLEEP implementation for both cases. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/base/power/wakeup.c | 30 ------------------------------ > include/linux/pm_wakeup.h | 31 +++++++++++++++++++++++-------- > 2 files changed, 23 insertions(+), 38 deletions(-) > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > index 11a4ffe91367..e3befa2c1b66 100644 > --- a/drivers/base/power/wakeup.c > +++ b/drivers/base/power/wakeup.c > @@ -500,36 +500,6 @@ void device_set_wakeup_capable(struct device *dev, bool capable) > } > EXPORT_SYMBOL_GPL(device_set_wakeup_capable); > > -/** > - * device_init_wakeup - Device wakeup initialization. > - * @dev: Device to handle. > - * @enable: Whether or not to enable @dev as a wakeup device. > - * > - * By default, most devices should leave wakeup disabled. The exceptions are > - * devices that everyone expects to be wakeup sources: keyboards, power buttons, > - * possibly network interfaces, etc. Also, devices that don't generate their > - * own wakeup requests but merely forward requests from one bus to another > - * (like PCI bridges) should have wakeup enabled by default. > - */ > -int device_init_wakeup(struct device *dev, bool enable) > -{ > - int ret = 0; > - > - if (!dev) > - return -EINVAL; > - > - if (enable) { > - device_set_wakeup_capable(dev, true); > - ret = device_wakeup_enable(dev); > - } else { > - device_wakeup_disable(dev); > - device_set_wakeup_capable(dev, false); > - } > - > - return ret; > -} > -EXPORT_SYMBOL_GPL(device_init_wakeup); > - > /** > * device_set_wakeup_enable - Enable or disable a device to wake up the system. > * @dev: Device to handle. > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > index 196a157456aa..77f4849e3418 100644 > --- a/include/linux/pm_wakeup.h > +++ b/include/linux/pm_wakeup.h > @@ -109,7 +109,6 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws); > extern int device_wakeup_enable(struct device *dev); > extern int device_wakeup_disable(struct device *dev); > extern void device_set_wakeup_capable(struct device *dev, bool capable); > -extern int device_init_wakeup(struct device *dev, bool val); > extern int device_set_wakeup_enable(struct device *dev, bool enable); > extern void __pm_stay_awake(struct wakeup_source *ws); > extern void pm_stay_awake(struct device *dev); > @@ -167,13 +166,6 @@ static inline int device_set_wakeup_enable(struct device *dev, bool enable) > return 0; > } > > -static inline int device_init_wakeup(struct device *dev, bool val) > -{ > - device_set_wakeup_capable(dev, val); > - device_set_wakeup_enable(dev, val); > - return 0; > -} > - > static inline bool device_may_wakeup(struct device *dev) > { > return dev->power.can_wakeup && dev->power.should_wakeup; > @@ -217,4 +209,27 @@ static inline void pm_wakeup_hard_event(struct device *dev) > return pm_wakeup_dev_event(dev, 0, true); > } > > +/** > + * device_init_wakeup - Device wakeup initialization. > + * @dev: Device to handle. > + * @enable: Whether or not to enable @dev as a wakeup device. > + * > + * By default, most devices should leave wakeup disabled. The exceptions are > + * devices that everyone expects to be wakeup sources: keyboards, power buttons, > + * possibly network interfaces, etc. Also, devices that don't generate their > + * own wakeup requests but merely forward requests from one bus to another > + * (like PCI bridges) should have wakeup enabled by default. > + */ > +static inline int device_init_wakeup(struct device *dev, bool enable) > +{ > + if (enable) { > + device_set_wakeup_capable(dev, true); > + return device_wakeup_enable(dev); > + } else { > + device_wakeup_disable(dev); > + device_set_wakeup_capable(dev, false); > + return 0; > + } > +} > + Why are you moving this to be inline? Why not just drop the "other" version and stick with the .c version? thanks, greg k-h
On Fri, Jun 10, 2022 at 04:42:16PM +0200, Greg Kroah-Hartman wrote: > On Mon, Jun 06, 2022 at 10:51:58PM -0500, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > Previously the CONFIG_PM_SLEEP and !CONFIG_PM_SLEEP device_init_wakeup() > > implementations differed in confusing ways: > > > > - The PM_SLEEP version checked for a NULL device pointer and returned > > -EINVAL, while the !PM_SLEEP version did not and would simply > > dereference a NULL pointer. > > > > - When called with "false", the !PM_SLEEP version cleared "capable" and > > "enable" in the opposite order of the PM_SLEEP version. That was > > harmless because for !PM_SLEEP they're simple assignments, but it's > > unnecessary confusion. > > > > Use a simplified version of the PM_SLEEP implementation for both cases. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > --- > > drivers/base/power/wakeup.c | 30 ------------------------------ > > include/linux/pm_wakeup.h | 31 +++++++++++++++++++++++-------- > > 2 files changed, 23 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > > index 11a4ffe91367..e3befa2c1b66 100644 > > --- a/drivers/base/power/wakeup.c > > +++ b/drivers/base/power/wakeup.c > > @@ -500,36 +500,6 @@ void device_set_wakeup_capable(struct device *dev, bool capable) > > } > > EXPORT_SYMBOL_GPL(device_set_wakeup_capable); > > > > -/** > > - * device_init_wakeup - Device wakeup initialization. > > - * @dev: Device to handle. > > - * @enable: Whether or not to enable @dev as a wakeup device. > > - * > > - * By default, most devices should leave wakeup disabled. The exceptions are > > - * devices that everyone expects to be wakeup sources: keyboards, power buttons, > > - * possibly network interfaces, etc. Also, devices that don't generate their > > - * own wakeup requests but merely forward requests from one bus to another > > - * (like PCI bridges) should have wakeup enabled by default. > > - */ > > -int device_init_wakeup(struct device *dev, bool enable) > > -{ > > - int ret = 0; > > - > > - if (!dev) > > - return -EINVAL; > > - > > - if (enable) { > > - device_set_wakeup_capable(dev, true); > > - ret = device_wakeup_enable(dev); > > - } else { > > - device_wakeup_disable(dev); > > - device_set_wakeup_capable(dev, false); > > - } > > - > > - return ret; > > -} > > -EXPORT_SYMBOL_GPL(device_init_wakeup); > > - > > /** > > * device_set_wakeup_enable - Enable or disable a device to wake up the system. > > * @dev: Device to handle. > > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > > index 196a157456aa..77f4849e3418 100644 > > --- a/include/linux/pm_wakeup.h > > +++ b/include/linux/pm_wakeup.h > > @@ -109,7 +109,6 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws); > > extern int device_wakeup_enable(struct device *dev); > > extern int device_wakeup_disable(struct device *dev); > > extern void device_set_wakeup_capable(struct device *dev, bool capable); > > -extern int device_init_wakeup(struct device *dev, bool val); > > extern int device_set_wakeup_enable(struct device *dev, bool enable); > > extern void __pm_stay_awake(struct wakeup_source *ws); > > extern void pm_stay_awake(struct device *dev); > > @@ -167,13 +166,6 @@ static inline int device_set_wakeup_enable(struct device *dev, bool enable) > > return 0; > > } > > > > -static inline int device_init_wakeup(struct device *dev, bool val) > > -{ > > - device_set_wakeup_capable(dev, val); > > - device_set_wakeup_enable(dev, val); > > - return 0; > > -} > > - > > static inline bool device_may_wakeup(struct device *dev) > > { > > return dev->power.can_wakeup && dev->power.should_wakeup; > > @@ -217,4 +209,27 @@ static inline void pm_wakeup_hard_event(struct device *dev) > > return pm_wakeup_dev_event(dev, 0, true); > > } > > > > +/** > > + * device_init_wakeup - Device wakeup initialization. > > + * @dev: Device to handle. > > + * @enable: Whether or not to enable @dev as a wakeup device. > > + * > > + * By default, most devices should leave wakeup disabled. The exceptions are > > + * devices that everyone expects to be wakeup sources: keyboards, power buttons, > > + * possibly network interfaces, etc. Also, devices that don't generate their > > + * own wakeup requests but merely forward requests from one bus to another > > + * (like PCI bridges) should have wakeup enabled by default. > > + */ > > +static inline int device_init_wakeup(struct device *dev, bool enable) > > +{ > > + if (enable) { > > + device_set_wakeup_capable(dev, true); > > + return device_wakeup_enable(dev); > > + } else { > > + device_wakeup_disable(dev); > > + device_set_wakeup_capable(dev, false); > > + return 0; > > + } > > +} > > + > > Why are you moving this to be inline? Why not just drop the "other" > version and stick with the .c version? That would definitely be simpler but drivers/base/power/wakeup.c is only built when CONFIG_PM_SLEEP=y, so a .c version there by itself isn't sufficient. I should have mentioned that in the commit log. Bjorn
On Fri, Jun 10, 2022 at 10:06:28AM -0500, Bjorn Helgaas wrote: > On Fri, Jun 10, 2022 at 04:42:16PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Jun 06, 2022 at 10:51:58PM -0500, Bjorn Helgaas wrote: > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Previously the CONFIG_PM_SLEEP and !CONFIG_PM_SLEEP device_init_wakeup() > > > implementations differed in confusing ways: > > > > > > - The PM_SLEEP version checked for a NULL device pointer and returned > > > -EINVAL, while the !PM_SLEEP version did not and would simply > > > dereference a NULL pointer. > > > > > > - When called with "false", the !PM_SLEEP version cleared "capable" and > > > "enable" in the opposite order of the PM_SLEEP version. That was > > > harmless because for !PM_SLEEP they're simple assignments, but it's > > > unnecessary confusion. > > > > > > Use a simplified version of the PM_SLEEP implementation for both cases. > > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > --- > > > drivers/base/power/wakeup.c | 30 ------------------------------ > > > include/linux/pm_wakeup.h | 31 +++++++++++++++++++++++-------- > > > 2 files changed, 23 insertions(+), 38 deletions(-) > > > > > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > > > index 11a4ffe91367..e3befa2c1b66 100644 > > > --- a/drivers/base/power/wakeup.c > > > +++ b/drivers/base/power/wakeup.c > > > @@ -500,36 +500,6 @@ void device_set_wakeup_capable(struct device *dev, bool capable) > > > } > > > EXPORT_SYMBOL_GPL(device_set_wakeup_capable); > > > > > > -/** > > > - * device_init_wakeup - Device wakeup initialization. > > > - * @dev: Device to handle. > > > - * @enable: Whether or not to enable @dev as a wakeup device. > > > - * > > > - * By default, most devices should leave wakeup disabled. The exceptions are > > > - * devices that everyone expects to be wakeup sources: keyboards, power buttons, > > > - * possibly network interfaces, etc. Also, devices that don't generate their > > > - * own wakeup requests but merely forward requests from one bus to another > > > - * (like PCI bridges) should have wakeup enabled by default. > > > - */ > > > -int device_init_wakeup(struct device *dev, bool enable) > > > -{ > > > - int ret = 0; > > > - > > > - if (!dev) > > > - return -EINVAL; > > > - > > > - if (enable) { > > > - device_set_wakeup_capable(dev, true); > > > - ret = device_wakeup_enable(dev); > > > - } else { > > > - device_wakeup_disable(dev); > > > - device_set_wakeup_capable(dev, false); > > > - } > > > - > > > - return ret; > > > -} > > > -EXPORT_SYMBOL_GPL(device_init_wakeup); > > > - > > > /** > > > * device_set_wakeup_enable - Enable or disable a device to wake up the system. > > > * @dev: Device to handle. > > > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > > > index 196a157456aa..77f4849e3418 100644 > > > --- a/include/linux/pm_wakeup.h > > > +++ b/include/linux/pm_wakeup.h > > > @@ -109,7 +109,6 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws); > > > extern int device_wakeup_enable(struct device *dev); > > > extern int device_wakeup_disable(struct device *dev); > > > extern void device_set_wakeup_capable(struct device *dev, bool capable); > > > -extern int device_init_wakeup(struct device *dev, bool val); > > > extern int device_set_wakeup_enable(struct device *dev, bool enable); > > > extern void __pm_stay_awake(struct wakeup_source *ws); > > > extern void pm_stay_awake(struct device *dev); > > > @@ -167,13 +166,6 @@ static inline int device_set_wakeup_enable(struct device *dev, bool enable) > > > return 0; > > > } > > > > > > -static inline int device_init_wakeup(struct device *dev, bool val) > > > -{ > > > - device_set_wakeup_capable(dev, val); > > > - device_set_wakeup_enable(dev, val); > > > - return 0; > > > -} > > > - > > > static inline bool device_may_wakeup(struct device *dev) > > > { > > > return dev->power.can_wakeup && dev->power.should_wakeup; > > > @@ -217,4 +209,27 @@ static inline void pm_wakeup_hard_event(struct device *dev) > > > return pm_wakeup_dev_event(dev, 0, true); > > > } > > > > > > +/** > > > + * device_init_wakeup - Device wakeup initialization. > > > + * @dev: Device to handle. > > > + * @enable: Whether or not to enable @dev as a wakeup device. > > > + * > > > + * By default, most devices should leave wakeup disabled. The exceptions are > > > + * devices that everyone expects to be wakeup sources: keyboards, power buttons, > > > + * possibly network interfaces, etc. Also, devices that don't generate their > > > + * own wakeup requests but merely forward requests from one bus to another > > > + * (like PCI bridges) should have wakeup enabled by default. > > > + */ > > > +static inline int device_init_wakeup(struct device *dev, bool enable) > > > +{ > > > + if (enable) { > > > + device_set_wakeup_capable(dev, true); > > > + return device_wakeup_enable(dev); > > > + } else { > > > + device_wakeup_disable(dev); > > > + device_set_wakeup_capable(dev, false); > > > + return 0; > > > + } > > > +} > > > + > > > > Why are you moving this to be inline? Why not just drop the "other" > > version and stick with the .c version? > > That would definitely be simpler but drivers/base/power/wakeup.c is > only built when CONFIG_PM_SLEEP=y, so a .c version there by itself > isn't sufficient. I should have mentioned that in the commit log. Ah, missed that, sorry, nevermind :)
On Tue, Jun 7, 2022 at 5:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > Previously the CONFIG_PM_SLEEP and !CONFIG_PM_SLEEP device_init_wakeup() > implementations differed in confusing ways: > > - The PM_SLEEP version checked for a NULL device pointer and returned > -EINVAL, while the !PM_SLEEP version did not and would simply > dereference a NULL pointer. > > - When called with "false", the !PM_SLEEP version cleared "capable" and > "enable" in the opposite order of the PM_SLEEP version. That was > harmless because for !PM_SLEEP they're simple assignments, but it's > unnecessary confusion. > > Use a simplified version of the PM_SLEEP implementation for both cases. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/base/power/wakeup.c | 30 ------------------------------ > include/linux/pm_wakeup.h | 31 +++++++++++++++++++++++-------- > 2 files changed, 23 insertions(+), 38 deletions(-) > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > index 11a4ffe91367..e3befa2c1b66 100644 > --- a/drivers/base/power/wakeup.c > +++ b/drivers/base/power/wakeup.c > @@ -500,36 +500,6 @@ void device_set_wakeup_capable(struct device *dev, bool capable) > } > EXPORT_SYMBOL_GPL(device_set_wakeup_capable); > > -/** > - * device_init_wakeup - Device wakeup initialization. > - * @dev: Device to handle. > - * @enable: Whether or not to enable @dev as a wakeup device. > - * > - * By default, most devices should leave wakeup disabled. The exceptions are > - * devices that everyone expects to be wakeup sources: keyboards, power buttons, > - * possibly network interfaces, etc. Also, devices that don't generate their > - * own wakeup requests but merely forward requests from one bus to another > - * (like PCI bridges) should have wakeup enabled by default. > - */ > -int device_init_wakeup(struct device *dev, bool enable) > -{ > - int ret = 0; > - > - if (!dev) > - return -EINVAL; > - > - if (enable) { > - device_set_wakeup_capable(dev, true); > - ret = device_wakeup_enable(dev); > - } else { > - device_wakeup_disable(dev); > - device_set_wakeup_capable(dev, false); > - } > - > - return ret; > -} > -EXPORT_SYMBOL_GPL(device_init_wakeup); > - > /** > * device_set_wakeup_enable - Enable or disable a device to wake up the system. > * @dev: Device to handle. > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > index 196a157456aa..77f4849e3418 100644 > --- a/include/linux/pm_wakeup.h > +++ b/include/linux/pm_wakeup.h > @@ -109,7 +109,6 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws); > extern int device_wakeup_enable(struct device *dev); > extern int device_wakeup_disable(struct device *dev); > extern void device_set_wakeup_capable(struct device *dev, bool capable); > -extern int device_init_wakeup(struct device *dev, bool val); > extern int device_set_wakeup_enable(struct device *dev, bool enable); > extern void __pm_stay_awake(struct wakeup_source *ws); > extern void pm_stay_awake(struct device *dev); > @@ -167,13 +166,6 @@ static inline int device_set_wakeup_enable(struct device *dev, bool enable) > return 0; > } > > -static inline int device_init_wakeup(struct device *dev, bool val) > -{ > - device_set_wakeup_capable(dev, val); > - device_set_wakeup_enable(dev, val); > - return 0; > -} > - > static inline bool device_may_wakeup(struct device *dev) > { > return dev->power.can_wakeup && dev->power.should_wakeup; > @@ -217,4 +209,27 @@ static inline void pm_wakeup_hard_event(struct device *dev) > return pm_wakeup_dev_event(dev, 0, true); > } > > +/** > + * device_init_wakeup - Device wakeup initialization. > + * @dev: Device to handle. > + * @enable: Whether or not to enable @dev as a wakeup device. > + * > + * By default, most devices should leave wakeup disabled. The exceptions are > + * devices that everyone expects to be wakeup sources: keyboards, power buttons, > + * possibly network interfaces, etc. Also, devices that don't generate their > + * own wakeup requests but merely forward requests from one bus to another > + * (like PCI bridges) should have wakeup enabled by default. > + */ > +static inline int device_init_wakeup(struct device *dev, bool enable) > +{ > + if (enable) { > + device_set_wakeup_capable(dev, true); > + return device_wakeup_enable(dev); > + } else { > + device_wakeup_disable(dev); > + device_set_wakeup_capable(dev, false); > + return 0; > + } > +} > + > #endif /* _LINUX_PM_WAKEUP_H */ > -- Applied as 5.20 material, sorry for the delay. Thanks!
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index 11a4ffe91367..e3befa2c1b66 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -500,36 +500,6 @@ void device_set_wakeup_capable(struct device *dev, bool capable) } EXPORT_SYMBOL_GPL(device_set_wakeup_capable); -/** - * device_init_wakeup - Device wakeup initialization. - * @dev: Device to handle. - * @enable: Whether or not to enable @dev as a wakeup device. - * - * By default, most devices should leave wakeup disabled. The exceptions are - * devices that everyone expects to be wakeup sources: keyboards, power buttons, - * possibly network interfaces, etc. Also, devices that don't generate their - * own wakeup requests but merely forward requests from one bus to another - * (like PCI bridges) should have wakeup enabled by default. - */ -int device_init_wakeup(struct device *dev, bool enable) -{ - int ret = 0; - - if (!dev) - return -EINVAL; - - if (enable) { - device_set_wakeup_capable(dev, true); - ret = device_wakeup_enable(dev); - } else { - device_wakeup_disable(dev); - device_set_wakeup_capable(dev, false); - } - - return ret; -} -EXPORT_SYMBOL_GPL(device_init_wakeup); - /** * device_set_wakeup_enable - Enable or disable a device to wake up the system. * @dev: Device to handle. diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h index 196a157456aa..77f4849e3418 100644 --- a/include/linux/pm_wakeup.h +++ b/include/linux/pm_wakeup.h @@ -109,7 +109,6 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws); extern int device_wakeup_enable(struct device *dev); extern int device_wakeup_disable(struct device *dev); extern void device_set_wakeup_capable(struct device *dev, bool capable); -extern int device_init_wakeup(struct device *dev, bool val); extern int device_set_wakeup_enable(struct device *dev, bool enable); extern void __pm_stay_awake(struct wakeup_source *ws); extern void pm_stay_awake(struct device *dev); @@ -167,13 +166,6 @@ static inline int device_set_wakeup_enable(struct device *dev, bool enable) return 0; } -static inline int device_init_wakeup(struct device *dev, bool val) -{ - device_set_wakeup_capable(dev, val); - device_set_wakeup_enable(dev, val); - return 0; -} - static inline bool device_may_wakeup(struct device *dev) { return dev->power.can_wakeup && dev->power.should_wakeup; @@ -217,4 +209,27 @@ static inline void pm_wakeup_hard_event(struct device *dev) return pm_wakeup_dev_event(dev, 0, true); } +/** + * device_init_wakeup - Device wakeup initialization. + * @dev: Device to handle. + * @enable: Whether or not to enable @dev as a wakeup device. + * + * By default, most devices should leave wakeup disabled. The exceptions are + * devices that everyone expects to be wakeup sources: keyboards, power buttons, + * possibly network interfaces, etc. Also, devices that don't generate their + * own wakeup requests but merely forward requests from one bus to another + * (like PCI bridges) should have wakeup enabled by default. + */ +static inline int device_init_wakeup(struct device *dev, bool enable) +{ + if (enable) { + device_set_wakeup_capable(dev, true); + return device_wakeup_enable(dev); + } else { + device_wakeup_disable(dev); + device_set_wakeup_capable(dev, false); + return 0; + } +} + #endif /* _LINUX_PM_WAKEUP_H */