[PATCH 0/2] Some cleanup in driver core

[PATCH 0/2] Some cleanup in driver core

[Wei Yang]

These two patches do some cleanup in the driver core.

They are tested on:
Hardware:       Lenovo T420
Kernel Version: 3.8
Result:
  * /sys/devices/pci0000:00/ directory is correct
  * /sys/devices/pci0000:00/0000:00:19.0/ contains the sysfs files for a pci
    device

Wei Yang (2):
  driver core: remove device_add_attrs() in drivers/base/bus.c
  driver-core: remove the duplicate assignment of kobj->parent in
    device_add

 drivers/base/bus.c     |   21 +--------------------
 drivers/base/core.c    |    6 ++----
 include/linux/device.h |    2 ++
 3 files changed, 5 insertions(+), 24 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] driver core: remove device_add_attrs() in drivers/base/bus.c

[Wei Yang]

Originally, we have two functions named device_add_attrs() in
drivers/base/bus.c and drivers/base/core.c. This will cause some confusion
when reading the code.

And the one in drivers/base/bus.c do the same action as device_add_attributes()
in drivers/base/core.c. That would be better to reuse this code.

This patch just do some cleanup. Remove the device_add_attrs() in
drivers/base/bus.c and call device_add_attributes() directly.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/base/bus.c     |   21 +--------------------
 drivers/base/core.c    |    2 +-
 include/linux/device.h |    2 ++
 3 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 24eb078..79c2a48 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -452,25 +452,6 @@ int bus_for_each_drv(struct bus_type *bus, struct device_driver *start,
 }
 EXPORT_SYMBOL_GPL(bus_for_each_drv);
 
-static int device_add_attrs(struct bus_type *bus, struct device *dev)
-{
-	int error = 0;
-	int i;
-
-	if (!bus->dev_attrs)
-		return 0;
-
-	for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
-		error = device_create_file(dev, &bus->dev_attrs[i]);
-		if (error) {
-			while (--i >= 0)
-				device_remove_file(dev, &bus->dev_attrs[i]);
-			break;
-		}
-	}
-	return error;
-}
-
 static void device_remove_attrs(struct bus_type *bus, struct device *dev)
 {
 	int i;
@@ -496,7 +477,7 @@ int bus_add_device(struct device *dev)
 
 	if (bus) {
 		pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
-		error = device_add_attrs(bus, dev);
+		error = device_add_attributes(dev, bus->dev_attrs);
 		if (error)
 			goto out_put;
 		error = sysfs_create_link(&bus->p->devices_kset->kobj,
diff --git a/drivers/base/core.c b/drivers/base/core.c
index a235085..369ae4e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -397,7 +397,7 @@ static ssize_t store_uevent(struct device *dev, struct device_attribute *attr,
 static struct device_attribute uevent_attr =
 	__ATTR(uevent, S_IRUGO | S_IWUSR, show_uevent, store_uevent);
 
-static int device_add_attributes(struct device *dev,
+int device_add_attributes(struct device *dev,
 				 struct device_attribute *attrs)
 {
 	int error = 0;
diff --git a/include/linux/device.h b/include/linux/device.h
index 43dcda9..9cfe8e6 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -518,6 +518,8 @@ ssize_t device_store_bool(struct device *dev, struct device_attribute *attr,
 	struct device_attribute dev_attr_##_name =		\
 		__ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
 
+extern int device_add_attributes(struct device *dev,
+				 struct device_attribute *attrs);
 extern int device_create_file(struct device *device,
 			      const struct device_attribute *entry);
 extern void device_remove_file(struct device *dev,
-- 
1.7.5.4

Re: [PATCH 1/2] driver core: remove device_add_attrs() in drivers/base/bus.c

[Greg KH]

On Thu, Mar 07, 2013 at 10:22:45AM +0800, Wei Yang wrote:
> Originally, we have two functions named device_add_attrs() in
> drivers/base/bus.c and drivers/base/core.c. This will cause some confusion
> when reading the code.

Why?  It's just static functions.

> And the one in drivers/base/bus.c do the same action as device_add_attributes()
> in drivers/base/core.c. That would be better to reuse this code.

Possibly, yes, but you put the .h entry in the wrong place, thereby
exposing a previously static function to the entire kernel, which is not
good at all.  Why did you do that?  Are you now going to want to call
that function from some driver?

greg k-h

Re: [PATCH 1/2] driver core: remove device_add_attrs() in drivers/base/bus.c

[Wei Yang]

On Mon, Mar 18, 2013 at 05:18:22PM -0700, Greg KH wrote:
>On Thu, Mar 07, 2013 at 10:22:45AM +0800, Wei Yang wrote:
>> Originally, we have two functions named device_add_attrs() in
>> drivers/base/bus.c and drivers/base/core.c. This will cause some confusion
>> when reading the code.
>
>Why?  It's just static functions.

Hmm, yes, they are static functions.

The confusion comes from myself. When I look for the definitions of the
function in cscope, it pop out two line with the same name. So I just think
whether we can help the reader, if there is only one pop up.

Well, I have to admit this is my personal feeling and due to my lack of
experence.

>
>> And the one in drivers/base/bus.c do the same action as device_add_attributes()
>> in drivers/base/core.c. That would be better to reuse this code.
>
>Possibly, yes, but you put the .h entry in the wrong place, thereby
>exposing a previously static function to the entire kernel, which is not

Agree, this is not a good idea to expose an extra interface to the entire
kernel.

>good at all.  Why did you do that?  Are you now going to want to call
>that function from some driver?

The reason for this patch is, the logic in these two functions are the same.
My purpose is to reuse the code and save some space for the kernel image,
well, I have to admit it is just a very small piece of space.

>
>greg k-h

-- 
Richard Yang
Help you, Help me

[PATCH 2/2] driver-core: remove the duplicate assignment of kobj->parent in device_add

[Wei Yang]

kobject_add() will setup the kobject parent correctly.

This patch removes the redundant code.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/base/core.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 369ae4e..6b0a859 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1024,8 +1024,6 @@ int device_add(struct device *dev)
 
 	parent = get_device(dev->parent);
 	kobj = get_device_parent(dev, parent);
-	if (kobj)
-		dev->kobj.parent = kobj;
 
 	/* use parent numa_node */
 	if (parent)
@@ -1033,7 +1031,7 @@ int device_add(struct device *dev)
 
 	/* first, register with generic layer. */
 	/* we require the name to be set before, and pass NULL */
-	error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
+	error = kobject_add(&dev->kobj, kobj, NULL);
 	if (error)
 		goto Error;
 
-- 
1.7.5.4

Re: [PATCH 2/2] driver-core: remove the duplicate assignment of kobj->parent in device_add

[Bjørn Mork]

Wei Yang <weiyang@linux.vnet.ibm.com> writes:

> kobject_add() will setup the kobject parent correctly.
>
> This patch removes the redundant code.
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/base/core.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 369ae4e..6b0a859 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1024,8 +1024,6 @@ int device_add(struct device *dev)
>  
>  	parent = get_device(dev->parent);
>  	kobj = get_device_parent(dev, parent);
> -	if (kobj)
> -		dev->kobj.parent = kobj;
>  
>  	/* use parent numa_node */
>  	if (parent)
> @@ -1033,7 +1031,7 @@ int device_add(struct device *dev)
>  
>  	/* first, register with generic layer. */
>  	/* we require the name to be set before, and pass NULL */
> -	error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
> +	error = kobject_add(&dev->kobj, kobj, NULL);
>  	if (error)
>  		goto Error;


You've submitted this exact same patch before and Greg asked a couple of
questions about it: https://lkml.org/lkml/2013/1/25/599

If this is a resubmission, then it would sure be nice if that was clear
from the subject and on.  And I believe any previously asked questions
should be answered in some way.  I see that you have responded to one of
them by moving part of the commit message to the cover letter, but still
without any answer to the question.

To me this looks like you completely ignored Greg's review questions. Is
that so?



Bjørn

Re: [PATCH 2/2] driver-core: remove the duplicate assignment of kobj->parent in device_add

[Bjørn Mork]

[ not stripping any quoting to restore context for linux-kernel]

Wei Yang <weiyang@linux.vnet.ibm.com> writes:
> On Thu, Mar 07, 2013 at 12:33:19PM +0100, Bjørn Mork wrote:
>>Wei Yang <weiyang@linux.vnet.ibm.com> writes:
>>
>>> kobject_add() will setup the kobject parent correctly.
>>>
>>> This patch removes the redundant code.
>>>
>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>> ---
>>>  drivers/base/core.c |    4 +---
>>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 369ae4e..6b0a859 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -1024,8 +1024,6 @@ int device_add(struct device *dev)
>>>  
>>>  	parent = get_device(dev->parent);
>>>  	kobj = get_device_parent(dev, parent);
>>> -	if (kobj)
>>> -		dev->kobj.parent = kobj;
>>>  
>>>  	/* use parent numa_node */
>>>  	if (parent)
>>> @@ -1033,7 +1031,7 @@ int device_add(struct device *dev)
>>>  
>>>  	/* first, register with generic layer. */
>>>  	/* we require the name to be set before, and pass NULL */
>>> -	error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
>>> +	error = kobject_add(&dev->kobj, kobj, NULL);
>>>  	if (error)
>>>  		goto Error;
>>
>>
>>You've submitted this exact same patch before and Greg asked a couple of
>>questions about it: https://lkml.org/lkml/2013/1/25/599
>>
>>If this is a resubmission, then it would sure be nice if that was clear
>>from the subject and on.  And I believe any previously asked questions
>>should be answered in some way.  I see that you have responded to one of
>>them by moving part of the commit message to the cover letter, but still
>>without any answer to the question.
>>
>>To me this looks like you completely ignored Greg's review questions. Is
>>that so?
>
> Bjør,
>
> Thanks for your response. 
>
> In my mail box, I just see one mail from Greg and replied. Below is my screen
> in my mutt. You see I replied and wait for the further comment for several
> days and send out another reply on Feb 02. Well not receive further comment.
>
> 976     Jan 23 Wei Yang        (  37) [PATCH] driver-core: remove the duplicate assignment of kobj->parent in device_add
> 977 r   Jan 25 Greg KH         (  17) `->
> 978 r   Jan 28 Wei Yang        (  43)   `->
> 979     Feb 02 Wei Yang        (  52)     `->
>
> Hmm... I am confused with this, maybe my mail box get some problem.
> Could you see this mail? Or if someone could see this mail, would you please
> reply so that I will be sure that I have really sent it out .
>
> BTW, no one see my reply? I looked in the url you mentioned, really not see my
> reply. I am sorry for that. 

I cannot answer where your reply may have gone.  I do however note that
this reply to me went to every address on the CC list *except*
linux-kernel@vger.kernel.org.  Why?  That would make it appear as you
never answered me to 99.999997% of the linux-kernel readers...

Anyway, the main point is really not whether your mail went anywhere or
not.  If this was indeed an intentional resend, then I do expect it to
say so and to contain information about how previous review comments
were resolved.  Most people would not rememeber your answer even it was
sent to the mailing list.



Bjørn

Re: [PATCH 0/2] Some cleanup in driver core

[Greg KH]

On Thu, Mar 07, 2013 at 10:22:44AM +0800, Wei Yang wrote:
> These two patches do some cleanup in the driver core.

No they do not.

I understand the need to fix code, but please, do so if it is broken.
These patches don't help, and in fact, they hurt things.

Next time, if you wish to send me patches like this, please get someone
else at IBM, who is experienced, to ack them, before sending them on to
me.

greg k-h

Re: [PATCH 0/2] Some cleanup in driver core

[Wei Yang]

On Mon, Mar 18, 2013 at 05:17:08PM -0700, Greg KH wrote:
>On Thu, Mar 07, 2013 at 10:22:44AM +0800, Wei Yang wrote:
>> These two patches do some cleanup in the driver core.
>
>No they do not.
>
>I understand the need to fix code, but please, do so if it is broken.
>These patches don't help, and in fact, they hurt things.

Sorry for my bad patch.

>
>Next time, if you wish to send me patches like this, please get someone
>else at IBM, who is experienced, to ack them, before sending them on to
>me.

Sorry for doing that, I will contact the experienced first next time.

>
>greg k-h

-- 
Richard Yang
Help you, Help me