| Message ID | 20190816145602.231163-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | PM / wakeup: Register wakeup class kobj after device is added | expand |
On Fri, Aug 16, 2019 at 7:56 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> > --- > drivers/base/power/sysfs.c | 10 +++++++++- > drivers/base/power/wakeup.c | 10 ++++++---- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > index 1b9c281cbe41..27ee00f50bd7 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 (dev->power.wakeup) { This conditional checks for the situation when wakeup source registration have been previously attempted, but failed at wakeup_source_sysfs_add(). My concern is that it's not easy to understand what this check does without knowing exactly what device_wakeup_enable() does to dev->power.wakeup before we reach this point. > + rc = wakeup_source_sysfs_add(dev, dev->power.wakeup); > + if (rc) > + goto err_wakeup; > + } > if (dev->power.set_latency_tolerance) { > rc = sysfs_merge_group(&dev->kobj, > &pm_qos_latency_tolerance_attr_group); > if (rc) > - goto err_wakeup; > + goto err_wakeup_source; > } > return 0; > > + err_wakeup_source: > + wakeup_source_sysfs_remove(dev->power.wakeup); > err_wakeup: > sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group); > err_runtime: > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > index f7925820b5ca..5817b51d2b15 100644 > --- a/drivers/base/power/wakeup.c > +++ b/drivers/base/power/wakeup.c > @@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct device *dev, > > ws = wakeup_source_create(name); > if (ws) { > - ret = wakeup_source_sysfs_add(dev, ws); > - if (ret) { > - wakeup_source_free(ws); > - return NULL; > + if (!dev || device_is_registered(dev)) { Is there a possible race condition here? If dev->power.wakeup check in dpm_sysfs_add() is done at the same time as device_is_registered(dev) check here, then wakeup_source_sysfs_add() won't ever be called? > + ret = wakeup_source_sysfs_add(dev, ws); > + if (ret) { > + wakeup_source_free(ws); > + return NULL; > + } > } > wakeup_source_add(ws); > } > -- > Sent by a computer through tubes >
Quoting Tri Vo (2019-08-16 14:27:35) > On Fri, Aug 16, 2019 at 7:56 AM Stephen Boyd <swboyd@chromium.org> wrote: > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > > index 1b9c281cbe41..27ee00f50bd7 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 (dev->power.wakeup) { > > This conditional checks for the situation when wakeup source > registration have been previously attempted, but failed at > wakeup_source_sysfs_add(). My concern is that it's not easy to > understand what this check does without knowing exactly what > device_wakeup_enable() does to dev->power.wakeup before we reach this > point. Oh, actually this is wrong. It should be a check for dev->power.wakeup->dev being non-NULL. That's the variable that's set by wakeup_source_sysfs_add() upon success. So I should make it: if (dev->power.wakeup && !dev->power.wakeup->dev) And there's the problem that CONFIG_PM_SLEEP could be unset. Let me fix it up with a new inline function like device_has_wakeup_dev(). > > > + rc = wakeup_source_sysfs_add(dev, dev->power.wakeup); > > + if (rc) > > + goto err_wakeup; > > + } > > if (dev->power.set_latency_tolerance) { > > rc = sysfs_merge_group(&dev->kobj, > > &pm_qos_latency_tolerance_attr_group); > > if (rc) > > - goto err_wakeup; > > + goto err_wakeup_source; > > } > > return 0; > > > > + err_wakeup_source: > > + wakeup_source_sysfs_remove(dev->power.wakeup); > > err_wakeup: > > sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group); > > err_runtime: > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > > index f7925820b5ca..5817b51d2b15 100644 > > --- a/drivers/base/power/wakeup.c > > +++ b/drivers/base/power/wakeup.c > > @@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct device *dev, > > > > ws = wakeup_source_create(name); > > if (ws) { > > - ret = wakeup_source_sysfs_add(dev, ws); > > - if (ret) { > > - wakeup_source_free(ws); > > - return NULL; > > + if (!dev || device_is_registered(dev)) { > > Is there a possible race condition here? If dev->power.wakeup check in > dpm_sysfs_add() is done at the same time as device_is_registered(dev) > check here, then wakeup_source_sysfs_add() won't ever be called? The same race exists for device_set_wakeup_capable() so I didn't bother to try to avoid it. I suppose wakeup_source_sysfs_add() could run completely, allocate the device and set the name, etc., but not call device_add() and then we can set ws->dev and call device_add() under a mutex so that we keep a very small window where the wakeup class is published to sysfs. Or just throw a big mutex around the whole wakeup class creation path so that there isn't a chance of a race. But really, is anyone going to call device_set_wakeup_*() on a device that is also being added to the system? Seems unlikely. > > > + ret = wakeup_source_sysfs_add(dev, ws); > > + if (ret) { > > + wakeup_source_free(ws);
On Fri, Aug 16, 2019 at 2:46 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Tri Vo (2019-08-16 14:27:35) > > On Fri, Aug 16, 2019 at 7:56 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > > > index 1b9c281cbe41..27ee00f50bd7 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 (dev->power.wakeup) { > > > > This conditional checks for the situation when wakeup source > > registration have been previously attempted, but failed at > > wakeup_source_sysfs_add(). My concern is that it's not easy to > > understand what this check does without knowing exactly what > > device_wakeup_enable() does to dev->power.wakeup before we reach this > > point. > > Oh, actually this is wrong. It should be a check for > dev->power.wakeup->dev being non-NULL. That's the variable that's set by > wakeup_source_sysfs_add() upon success. So I should make it: > > if (dev->power.wakeup && !dev->power.wakeup->dev) Oh ok, this makes more sense now :) > > And there's the problem that CONFIG_PM_SLEEP could be unset. Let me fix > it up with a new inline function like device_has_wakeup_dev(). > > > > > > + rc = wakeup_source_sysfs_add(dev, dev->power.wakeup); > > > + if (rc) > > > + goto err_wakeup; > > > + } > > > if (dev->power.set_latency_tolerance) { > > > rc = sysfs_merge_group(&dev->kobj, > > > &pm_qos_latency_tolerance_attr_group); > > > if (rc) > > > - goto err_wakeup; > > > + goto err_wakeup_source; > > > } > > > return 0; > > > > > > + err_wakeup_source: > > > + wakeup_source_sysfs_remove(dev->power.wakeup); > > > err_wakeup: > > > sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group); > > > err_runtime: > > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > > > index f7925820b5ca..5817b51d2b15 100644 > > > --- a/drivers/base/power/wakeup.c > > > +++ b/drivers/base/power/wakeup.c > > > @@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct device *dev, > > > > > > ws = wakeup_source_create(name); > > > if (ws) { > > > - ret = wakeup_source_sysfs_add(dev, ws); > > > - if (ret) { > > > - wakeup_source_free(ws); > > > - return NULL; > > > + if (!dev || device_is_registered(dev)) { > > > > Is there a possible race condition here? If dev->power.wakeup check in > > dpm_sysfs_add() is done at the same time as device_is_registered(dev) > > check here, then wakeup_source_sysfs_add() won't ever be called? > > The same race exists for device_set_wakeup_capable() so I didn't bother > to try to avoid it. I suppose wakeup_source_sysfs_add() could run > completely, allocate the device and set the name, etc., but not call > device_add() and then we can set ws->dev and call device_add() under a > mutex so that we keep a very small window where the wakeup class is > published to sysfs. Or just throw a big mutex around the whole wakeup > class creation path so that there isn't a chance of a race. But really, > is anyone going to call device_set_wakeup_*() on a device that is also > being added to the system? Seems unlikely. True. I don't have a strong opinion. > > > > > > + ret = wakeup_source_sysfs_add(dev, ws); > > > + if (ret) { > > > + wakeup_source_free(ws);
On Friday, August 16, 2019 11:46:19 PM CEST Stephen Boyd wrote: > Quoting Tri Vo (2019-08-16 14:27:35) > > On Fri, Aug 16, 2019 at 7:56 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > > > index 1b9c281cbe41..27ee00f50bd7 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 (dev->power.wakeup) { > > > > This conditional checks for the situation when wakeup source > > registration have been previously attempted, but failed at > > wakeup_source_sysfs_add(). My concern is that it's not easy to > > understand what this check does without knowing exactly what > > device_wakeup_enable() does to dev->power.wakeup before we reach this > > point. > > Oh, actually this is wrong. It should be a check for > dev->power.wakeup->dev being non-NULL. That's the variable that's set by > wakeup_source_sysfs_add() upon success. So I should make it: > > if (dev->power.wakeup && !dev->power.wakeup->dev) > > And there's the problem that CONFIG_PM_SLEEP could be unset. Let me fix > it up with a new inline function like device_has_wakeup_dev(). OK, fix that and resend the patch. I'll restore the series then.
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c index 1b9c281cbe41..27ee00f50bd7 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 (dev->power.wakeup) { + rc = wakeup_source_sysfs_add(dev, dev->power.wakeup); + if (rc) + goto err_wakeup; + } if (dev->power.set_latency_tolerance) { rc = sysfs_merge_group(&dev->kobj, &pm_qos_latency_tolerance_attr_group); if (rc) - goto err_wakeup; + goto err_wakeup_source; } return 0; + err_wakeup_source: + wakeup_source_sysfs_remove(dev->power.wakeup); err_wakeup: sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group); err_runtime: diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index f7925820b5ca..5817b51d2b15 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct device *dev, ws = wakeup_source_create(name); if (ws) { - ret = wakeup_source_sysfs_add(dev, ws); - if (ret) { - wakeup_source_free(ws); - return NULL; + if (!dev || device_is_registered(dev)) { + ret = wakeup_source_sysfs_add(dev, ws); + if (ret) { + wakeup_source_free(ws); + return NULL; + } } wakeup_source_add(ws); }
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> --- drivers/base/power/sysfs.c | 10 +++++++++- drivers/base/power/wakeup.c | 10 ++++++---- 2 files changed, 15 insertions(+), 5 deletions(-)