Re: [PATCH net-next v3 6/9] drivers/base/power: add dpm_sysfs_change_owner()

[Christian Brauner <christian.brauner@ubuntu.com>]

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