Re: [PATCH v2] PM / wakeup: Register wakeup class kobj after device is added

[Stephen Boyd <swboyd@chromium.org>]

Quoting Tri Vo (2019-08-19 10:04:56)
> On Mon, Aug 19, 2019 at 8:06 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > The device_set_wakeup_enable() function can be called on a device that
> > hasn't been registered with device_add() yet. This allows the device to
> > be in a state where wakeup is enabled for it but the device isn't
> > published to userspace in sysfs yet.
> >
> > After commit 986845e747af ("PM / wakeup: Show wakeup sources stats in
> > sysfs"), calling device_set_wakeup_enable() will fail for a device that
> > hasn't been registered with the driver core via device_add(). This is
> > because we try to create sysfs entries for the device and associate a
> > wakeup class kobject with it before the device has been registered.
> > Let's follow a similar approach that device_set_wakeup_capable() takes
> > here and register the wakeup class either from
> > device_set_wakeup_enable() when the device is already registered, or
> > from dpm_sysfs_add() when the device is being registered with the driver
> > core via device_add().
> >
> > Fixes: 986845e747af ("PM / wakeup: Show wakeup sources stats in sysfs")
> > Reported-by: Qian Cai <cai@lca.pw>
> > Cc: Qian Cai <cai@lca.pw>
> > Cc: Tri Vo <trong@android.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >
> > Changes from v1:
> >  * Export wakeup_source_sysfs_add/remove stubs
> >  * New function to check if we should add the device from
> >    dpm_sysfs_add()
> >
> >  drivers/base/power/power.h  |  9 +++++++++
> >  drivers/base/power/sysfs.c  | 10 +++++++++-
> >  drivers/base/power/wakeup.c | 10 ++++++----
> >  include/linux/pm_wakeup.h   | 10 ++++++++++
> >  4 files changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> > index 57b1d1d88c8e..22a1533ec56b 100644
> > --- a/drivers/base/power/power.h
> > +++ b/drivers/base/power/power.h
> > @@ -156,5 +156,14 @@ static inline void device_pm_init(struct device *dev)
> >  extern int wakeup_source_sysfs_add(struct device *parent,
> >                                    struct wakeup_source *ws);
> >  extern void wakeup_source_sysfs_remove(struct wakeup_source *ws);
> > +#else /* !CONFIG_PM_SLEEP */
> > +
> > +static inline int wakeup_source_sysfs_add(struct device *parent,
> > +                                         struct wakeup_source *ws)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline void wakeup_source_sysfs_remove(struct wakeup_source *ws) {}
> >
> >  #endif /* CONFIG_PM_SLEEP */
> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > index 1b9c281cbe41..1468d03ae9fb 100644
> > --- a/drivers/base/power/sysfs.c
> > +++ b/drivers/base/power/sysfs.c
> > @@ -5,6 +5,7 @@
> >  #include <linux/export.h>
> >  #include <linux/pm_qos.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/pm_wakeup.h>
> >  #include <linux/atomic.h>
> >  #include <linux/jiffies.h>
> >  #include "power.h"
> > @@ -661,14 +662,21 @@ int dpm_sysfs_add(struct device *dev)
> >                 if (rc)
> >                         goto err_runtime;
> >         }
> > +       if (!device_has_wakeup_dev(dev)) {
> 
> This evaluates to true if dev->power.wakeup is NULL, which will result
> in a null pointer dereference later in wakeup_source_sysfs_add().
> 
> I think the condition you want to check for is the one you pointed out
> in previous patch.
> 
>         if (dev->power.wakeup && !dev->power.wakeup->dev)

Aha thanks. I need to wrapper it because CONFIG_PM_SLEEP is the one that
defines the power path.

> 
> > +               rc = wakeup_source_sysfs_add(dev, dev->power.wakeup);

But also, dev->power.wakeup doesn't exist with !CONFIG_PM_SLEEP.