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

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

[Wei Yang]

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@linux.intel.com>
---
 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;
 
 	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

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@linux.intel.com> 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@linux.intel.com>

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
>

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

[Wei Yang]

On Thu, Jan 17, 2019 at 10:38:13AM +0100, Rafael J. Wysocki wrote:
>On Thu, Jan 17, 2019 at 6:57 AM Wei Yang <richardw.yang@linux.intel.com> 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@linux.intel.com>
>
>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.

Rafael,

Thanks for your comment. 

To be honest, I didn't realized the trouble it will introduce to backport and
live patching. :-)

>
>> ---
>>  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?
>

Yep, this is reasonable. Let me put them in a helper.

>>
>>         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
>>

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

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@linux.intel.com>

---
v2: add a helper to get dev from klist_class
---
 drivers/base/base.h    |  4 ++++
 drivers/base/class.c   | 14 ++++++++++----
 drivers/base/core.c    |  4 ++--
 include/linux/device.h |  1 -
 4 files changed, 16 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..d8a6a5864c2e 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -117,16 +117,22 @@ static void class_put(struct class *cls)
 		kset_put(&cls->p->subsys);
 }
 
+static struct device *klist_class_to_dev(struct klist_node *n)
+{
+	struct device_private *p = to_device_private_class(n);
+	return p->device;
+}
+
 static void klist_class_dev_get(struct klist_node *n)
 {
-	struct device *dev = container_of(n, struct device, knode_class);
+	struct device *dev = klist_class_to_dev(n);
 
 	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 *dev = klist_class_to_dev(n);
 
 	put_device(dev);
 }
@@ -277,7 +283,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;
 }
@@ -304,7 +310,7 @@ struct device *class_dev_iter_next(struct class_dev_iter *iter)
 		knode = klist_next(&iter->ki);
 		if (!knode)
 			return NULL;
-		dev = container_of(knode, struct device, knode_class);
+		dev = klist_class_to_dev(knode);
 		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

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

[Rafael J. Wysocki]

On Fri, Jan 18, 2019 at 3:35 AM Wei Yang <richardw.yang@linux.intel.com> 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@linux.intel.com>
>
> ---
> v2: add a helper to get dev from klist_class

Looks better now.

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/base/base.h    |  4 ++++
>  drivers/base/class.c   | 14 ++++++++++----
>  drivers/base/core.c    |  4 ++--
>  include/linux/device.h |  1 -
>  4 files changed, 16 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..d8a6a5864c2e 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -117,16 +117,22 @@ static void class_put(struct class *cls)
>                 kset_put(&cls->p->subsys);
>  }
>
> +static struct device *klist_class_to_dev(struct klist_node *n)
> +{
> +       struct device_private *p = to_device_private_class(n);
> +       return p->device;
> +}
> +
>  static void klist_class_dev_get(struct klist_node *n)
>  {
> -       struct device *dev = container_of(n, struct device, knode_class);
> +       struct device *dev = klist_class_to_dev(n);
>
>         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 *dev = klist_class_to_dev(n);
>
>         put_device(dev);
>  }
> @@ -277,7 +283,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;
>  }
> @@ -304,7 +310,7 @@ struct device *class_dev_iter_next(struct class_dev_iter *iter)
>                 knode = klist_next(&iter->ki);
>                 if (!knode)
>                         return NULL;
> -               dev = container_of(knode, struct device, knode_class);
> +               dev = klist_class_to_dev(knode);
>                 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
>

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

[Greg KH]

On Fri, Jan 18, 2019 at 10:34:59AM +0800, Wei Yang 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@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks for this.  There are a number of other variables that should move
into this structure, if you want to take a look into doing that.

greg k-h

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

[Wei Yang]

On Fri, Jan 18, 2019 at 11:06:23AM +0100, Rafael J. Wysocki wrote:
>On Fri, Jan 18, 2019 at 3:35 AM Wei Yang <richardw.yang@linux.intel.com> 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@linux.intel.com>
>>
>> ---
>> v2: add a helper to get dev from klist_class
>
>Looks better now.
>
>Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks :-)

>
>> ---
>>  drivers/base/base.h    |  4 ++++
>>  drivers/base/class.c   | 14 ++++++++++----
>>  drivers/base/core.c    |  4 ++--
>>  include/linux/device.h |  1 -
>>  4 files changed, 16 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..d8a6a5864c2e 100644
>> --- a/drivers/base/class.c
>> +++ b/drivers/base/class.c
>> @@ -117,16 +117,22 @@ static void class_put(struct class *cls)
>>                 kset_put(&cls->p->subsys);
>>  }
>>
>> +static struct device *klist_class_to_dev(struct klist_node *n)
>> +{
>> +       struct device_private *p = to_device_private_class(n);
>> +       return p->device;
>> +}
>> +
>>  static void klist_class_dev_get(struct klist_node *n)
>>  {
>> -       struct device *dev = container_of(n, struct device, knode_class);
>> +       struct device *dev = klist_class_to_dev(n);
>>
>>         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 *dev = klist_class_to_dev(n);
>>
>>         put_device(dev);
>>  }
>> @@ -277,7 +283,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;
>>  }
>> @@ -304,7 +310,7 @@ struct device *class_dev_iter_next(struct class_dev_iter *iter)
>>                 knode = klist_next(&iter->ki);
>>                 if (!knode)
>>                         return NULL;
>> -               dev = container_of(knode, struct device, knode_class);
>> +               dev = klist_class_to_dev(knode);
>>                 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
>>

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Fri, Jan 18, 2019 at 04:55:41PM +0100, Greg KH wrote:
>On Fri, Jan 18, 2019 at 10:34:59AM +0800, Wei Yang 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@linux.intel.com>
>> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
>Thanks for this.  There are a number of other variables that should move
>into this structure, if you want to take a look into doing that.

Really? I may not notice that yet.

I may try to take a look into this, but not sure I would have enough time :-)

>
>greg k-h

-- 
Wei Yang
Help you, Help me