Re: [PATCH] driver core: move device->knode_class to device_private

[Rafael J. Wysocki]

On Thu, Jan 17, 2019 at 6:57 AM Wei Yang <richardw.yang@xxxxxxxxxxxxxxx> wrote:
>
> As the description of struct device_private says, it stores data which
> is private to driver core. And it already has similar fields like:
> knode_parent, knode_driver, knode_driver and knode_bus. This look it is
> more proper to put knode_class together with those fields to make it
> private to driver core.
>
> This patch move device->knode_class to device_private to make it comply
> with code convention.
>
> Signed-off-by: Wei Yang <richardw.yang@xxxxxxxxxxxxxxx>

As a rule, I'm not a big fan of changes like this, because the only
reason for doing it seems to be aesthetics and it may be problematic
for people doing various types of backports, live patching etc, but
this particular one makes sense to me.

> ---
>  drivers/base/base.h    |  4 ++++
>  drivers/base/class.c   | 12 ++++++++----
>  drivers/base/core.c    |  4 ++--
>  include/linux/device.h |  1 -
>  4 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 7a419a7a6235..37329a668935 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -60,6 +60,7 @@ struct driver_private {
>   * @knode_parent - node in sibling list
>   * @knode_driver - node in driver list
>   * @knode_bus - node in bus list
> + * @knode_class - node in class list
>   * @deferred_probe - entry in deferred_probe_list which is used to retry the
>   *     binding of drivers which were unable to get all the resources needed by
>   *     the device; typically because it depends on another driver getting
> @@ -74,6 +75,7 @@ struct device_private {
>         struct klist_node knode_parent;
>         struct klist_node knode_driver;
>         struct klist_node knode_bus;
> +       struct klist_node knode_class;
>         struct list_head deferred_probe;
>         struct device *device;
>  };
> @@ -83,6 +85,8 @@ struct device_private {
>         container_of(obj, struct device_private, knode_driver)
>  #define to_device_private_bus(obj)     \
>         container_of(obj, struct device_private, knode_bus)
> +#define to_device_private_class(obj)   \
> +       container_of(obj, struct device_private, knode_class)
>
>  /* initialisation functions */
>  extern int devices_init(void);
> diff --git a/drivers/base/class.c b/drivers/base/class.c
> index 54def4e02f00..df7fde599407 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -119,14 +119,16 @@ static void class_put(struct class *cls)
>
>  static void klist_class_dev_get(struct klist_node *n)
>  {
> -       struct device *dev = container_of(n, struct device, knode_class);
> +       struct device_private *p = to_device_private_class(n);
> +       struct device *dev = p->device;

the

p = to_device_private_class(n)
dev = p->device

pattern is repeated below for at least a couple of times.

Care to add a static helper for this to reduce the code duplication somewhat?

>
>         get_device(dev);
>  }
>
>  static void klist_class_dev_put(struct klist_node *n)
>  {
> -       struct device *dev = container_of(n, struct device, knode_class);
> +       struct device_private *p = to_device_private_class(n);
> +       struct device *dev = p->device;
>
>         put_device(dev);
>  }
> @@ -277,7 +279,7 @@ void class_dev_iter_init(struct class_dev_iter *iter, struct class *class,
>         struct klist_node *start_knode = NULL;
>
>         if (start)
> -               start_knode = &start->knode_class;
> +               start_knode = &start->p->knode_class;
>         klist_iter_init_node(&class->p->klist_devices, &iter->ki, start_knode);
>         iter->type = type;
>  }
> @@ -298,13 +300,15 @@ EXPORT_SYMBOL_GPL(class_dev_iter_init);
>  struct device *class_dev_iter_next(struct class_dev_iter *iter)
>  {
>         struct klist_node *knode;
> +       struct device_private *p;
>         struct device *dev;
>
>         while (1) {
>                 knode = klist_next(&iter->ki);
>                 if (!knode)
>                         return NULL;
> -               dev = container_of(knode, struct device, knode_class);
> +               p = to_device_private_class(knode);
> +               dev = p->device;
>                 if (!iter->type || iter->type == dev->type)
>                         return dev;
>         }
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 04bbcd779e11..a624d27d11c0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1932,7 +1932,7 @@ int device_add(struct device *dev)
>         if (dev->class) {
>                 mutex_lock(&dev->class->p->mutex);
>                 /* tie the class to the device */
> -               klist_add_tail(&dev->knode_class,
> +               klist_add_tail(&dev->p->knode_class,
>                                &dev->class->p->klist_devices);
>
>                 /* notify any interfaces that the device is here */
> @@ -2069,7 +2069,7 @@ void device_del(struct device *dev)
>                         if (class_intf->remove_dev)
>                                 class_intf->remove_dev(dev, class_intf);
>                 /* remove the device from the class list */
> -               klist_del(&dev->knode_class);
> +               klist_del(&dev->p->knode_class);
>                 mutex_unlock(&dev->class->p->mutex);
>         }
>         device_remove_file(dev, &dev_attr_uevent);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1b25c7a43f4c..040e97669f94 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1035,7 +1035,6 @@ struct device {
>         spinlock_t              devres_lock;
>         struct list_head        devres_head;
>
> -       struct klist_node       knode_class;
>         struct class            *class;
>         const struct attribute_group **groups;  /* optional groups */
>
> --
> 2.19.1
>