2022-06-07 07:11:35

by Bjorn Helgaas

[permalink] [raw]
Subject: [PATCH] PM / wakeup: Unify device_init_wakeup() for PM_SLEEP and !PM_SLEEP

From: Bjorn Helgaas <[email protected]>

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 <[email protected]>
---
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 */
--
2.25.1


2022-06-10 14:55:31

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] PM / wakeup: Unify device_init_wakeup() for PM_SLEEP and !PM_SLEEP

On Mon, Jun 06, 2022 at 10:51:58PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2022-06-10 15:57:09

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH] PM / wakeup: Unify device_init_wakeup() for PM_SLEEP and !PM_SLEEP

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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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

2022-06-12 06:44:18

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] PM / wakeup: Unify device_init_wakeup() for PM_SLEEP and !PM_SLEEP

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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 :)