Messages in this thread | | | Date | Thu, 20 Feb 2020 11:35:26 +0100 | From | Christian Brauner <> | Subject | Re: [PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner() |
| |
On Thu, Feb 20, 2020 at 11:30:32AM +0100, Rafael J. Wysocki wrote: > On Thu, Feb 20, 2020 at 11:21 AM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > > > On Thu, Feb 20, 2020 at 11:02:04AM +0100, Rafael J. Wysocki wrote: > > > On Tue, Feb 18, 2020 at 5:30 PM Christian Brauner > > > <christian.brauner@ubuntu.com> wrote: > > > > > > > > Add a helper to change the owner of a device's power entries. This > > > > needs to happen when the ownership of a device is changed, e.g. when > > > > moving network devices between network namespaces. > > > > This function will be used to correctly account for ownership changes, > > > > e.g. when moving network devices between network namespaces. > > > > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > > > > --- > > > > /* v2 */ > > > > - "Rafael J. Wysocki" <rafael@kernel.org>: > > > > - Fold if (dev->power.wakeup && dev->power.wakeup->dev) check into > > > > if (device_can_wakeup(dev)) check since the former can never be true if > > > > the latter is false. > > > > > > > > - Christian Brauner <christian.brauner@ubuntu.com>: > > > > - Place (dev->power.wakeup && dev->power.wakeup->dev) check under > > > > CONFIG_PM_SLEEP ifdefine since it will wakeup_source will only be available > > > > when this config option is set. > > > > > > > > /* v3 */ > > > > - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > > > > - Add explicit uid/gid parameters. > > > > --- > > > > drivers/base/core.c | 4 ++++ > > > > drivers/base/power/power.h | 3 +++ > > > > drivers/base/power/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 49 insertions(+) > > > > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > > > index ec0d5e8cfd0f..efec2792f5d7 100644 > > > > --- a/drivers/base/core.c > > > > +++ b/drivers/base/core.c > > > > @@ -3522,6 +3522,10 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid) > > > > if (error) > > > > goto out; > > > > > > > > + error = dpm_sysfs_change_owner(dev, kuid, kgid); > > > > + if (error) > > > > + goto out; > > > > + > > > > #ifdef CONFIG_BLOCK > > > > if (sysfs_deprecated && dev->class == &block_class) > > > > goto out; > > > > diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h > > > > index 444f5c169a0b..54292cdd7808 100644 > > > > --- a/drivers/base/power/power.h > > > > +++ b/drivers/base/power/power.h > > > > @@ -74,6 +74,7 @@ extern int pm_qos_sysfs_add_flags(struct device *dev); > > > > extern void pm_qos_sysfs_remove_flags(struct device *dev); > > > > extern int pm_qos_sysfs_add_latency_tolerance(struct device *dev); > > > > extern void pm_qos_sysfs_remove_latency_tolerance(struct device *dev); > > > > +extern int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid); > > > > > > > > #else /* CONFIG_PM */ > > > > > > > > @@ -88,6 +89,8 @@ static inline void pm_runtime_remove(struct device *dev) {} > > > > > > > > static inline int dpm_sysfs_add(struct device *dev) { return 0; } > > > > static inline void dpm_sysfs_remove(struct device *dev) {} > > > > +static inline int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, > > > > + kgid_t kgid) { return 0; } > > > > > > > > #endif > > > > > > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > > > > index d7d82db2e4bc..4e79afcd5ca8 100644 > > > > --- a/drivers/base/power/sysfs.c > > > > +++ b/drivers/base/power/sysfs.c > > > > @@ -684,6 +684,48 @@ int dpm_sysfs_add(struct device *dev) > > > > return rc; > > > > } > > > > > > > > +int dpm_sysfs_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid) > > > > +{ > > > > + int rc; > > > > + > > > > + if (device_pm_not_required(dev)) > > > > + return 0; > > > > + > > > > + rc = sysfs_group_change_owner(&dev->kobj, &pm_attr_group, kuid, kgid); > > > > + if (rc) > > > > + return rc; > > > > + > > > > + if (pm_runtime_callbacks_present(dev)) { > > > > + rc = sysfs_group_change_owner( > > > > + &dev->kobj, &pm_runtime_attr_group, kuid, kgid); > > > > + if (rc) > > > > + return rc; > > > > + } > > > > + if (device_can_wakeup(dev)) { > > > > + rc = sysfs_group_change_owner(&dev->kobj, &pm_wakeup_attr_group, > > > > + kuid, kgid); > > > > + if (rc) > > > > + return rc; > > > > + > > > > +#ifdef CONFIG_PM_SLEEP > > > > + if (dev->power.wakeup && dev->power.wakeup->dev) { > > > > + rc = device_change_owner(dev->power.wakeup->dev, kuid, > > > > + kgid); > > > > + if (rc) > > > > + return rc; > > > > + } > > > > +#endif > > > > > > First off, I don't particularly like #ifdefs in function bodies. In > > > particular, there is a CONFIG_PM_SLEEP block in this file already and > > > you could define a new function in there to carry out the above > > > operations, and provide an empty stub of it for the "unset" case. > > > Failing to do so is somewhat on the "rushing things in" side in my > > > view. > > > > How ifdefines are used is highly dependent on the subsystem; networking > > ofen uses in-place ifdefines in some parts and not in others. That has > > nothing to do with rushing things. I'm happy to change it to your > > preferences. > > Thanks! > > > Thanks for pointing out your expectations. But please don't > > assume bad intentions on my part because I'm not meeting them right > > away. It often is the case that adding a helper that is called in one > > place is not well-received. > > Fair enough, sorry for being harsh.
Np, I didn't read it as such. I was really just worried you thought that I was trying to rush things in. It's Thursday anyway, usually about the time where we're all grumpy because we can't wait until we made it to Friday. :)
Christian
| |