Re: [PATCH v2] PM / wakeup: Register wakeup class kobj after device is added
From: Stephen Boyd
Date: Mon Aug 19 2019 - 13:15:51 EST
Quoting Tri Vo (2019-08-19 10:04:56)
> On Mon, Aug 19, 2019 at 8:06 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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@xxxxxx>
> > Cc: Qian Cai <cai@xxxxxx>
> > Cc: Tri Vo <trong@xxxxxxxxxxx>
> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > ---
> >
> > 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.