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

[Rafael J. Wysocki]

On Thu, Feb 20, 2020 at 11:21 AM Christian Brauner
<christian.brauner@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > /* v2 */
> > > - "Rafael J. Wysocki" <rafael@xxxxxxxxxx>:
> > >   -  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@xxxxxxxxxx>:
> > >   - 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@xxxxxxxxxxxxxxxxxxx>:
> > >    - 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.