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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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 :)