lkml.org 
[lkml]   [2020]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2020-02-20 11:35    [W:0.344 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site