[PATCH] Device class reference counting

[PATCH] Device class reference counting

[Thomas Koeller]

Hi,

I found a little issue with reference counting for
device classes in 2.6.8-rc2. Patch attached. Please
cc me on any responses, as I am not subsribed to
this list.

tk



--- linux-mips/drivers/base/class.c	2004-07-14 16:21:33.000000000 +0200
+++ linux-mips-work/drivers/base/class.c	2004-07-30 17:51:09.477331128 +0200
@@ -353,8 +353,8 @@
 	struct class_interface * class_intf;
 	int error;
 
-	class_dev = class_device_get(class_dev);
-	if (!class_dev || !strlen(class_dev->class_id))
+	if (!strlen(class_dev->class_id)
+		|| !(class_dev = class_device_get(class_dev)))
 		return -EINVAL;
 
 	parent = class_get(class_dev->class);

-- 
--------------------------------------------------

Thomas Koeller, Software Development
Basler Vision Technologies

thomas dot koeller at baslerweb dot com
http://www.baslerweb.com

==============================

Re: [PATCH] Device class reference counting

[Greg KH]

On Fri, Jul 30, 2004 at 06:03:00PM +0200, Thomas Koeller wrote:
> Hi,
> 
> I found a little issue with reference counting for
> device classes in 2.6.8-rc2. Patch attached.

> --- linux-mips/drivers/base/class.c	2004-07-14 16:21:33.000000000 +0200
> +++ linux-mips-work/drivers/base/class.c	2004-07-30 17:51:09.477331128 +0200
> @@ -353,8 +353,8 @@
>  	struct class_interface * class_intf;
>  	int error;
>  
> -	class_dev = class_device_get(class_dev);
> -	if (!class_dev || !strlen(class_dev->class_id))
> +	if (!strlen(class_dev->class_id)
> +		|| !(class_dev = class_device_get(class_dev)))
>  		return -EINVAL;

I don't understand what you are trying to fix here.  In fact, if
class_dev is NULL, you will now oops.

Hm, I guess if class_dev->class_id is null, we will exit with an extra
reference grabbed on the class_dev.  Is that what you are trying to fix
here?

If so, please rework the patch.

thanks,

greg k-h

Re: [PATCH] Device class reference counting

[Thomas Koeller]

On Friday 06 August 2004 00:46, Greg KH wrote:
> On Fri, Jul 30, 2004 at 06:03:00PM +0200, Thomas Koeller wrote:
> > Hi,
> >
> > I found a little issue with reference counting for
> > device classes in 2.6.8-rc2. Patch attached.
> >
> > --- linux-mips/drivers/base/class.c	2004-07-14 16:21:33.000000000 +0200
> > +++ linux-mips-work/drivers/base/class.c	2004-07-30 17:51:09.477331128
> > +0200 @@ -353,8 +353,8 @@
> >  	struct class_interface * class_intf;
> >  	int error;
> >
> > -	class_dev = class_device_get(class_dev);
> > -	if (!class_dev || !strlen(class_dev->class_id))
> > +	if (!strlen(class_dev->class_id)
> > +		|| !(class_dev = class_device_get(class_dev)))
> >  		return -EINVAL;
>
> I don't understand what you are trying to fix here.  In fact, if
> class_dev is NULL, you will now oops.
>
> Hm, I guess if class_dev->class_id is null, we will exit with an extra
> reference grabbed on the class_dev.  Is that what you are trying to fix
> here?
>
> If so, please rework the patch.
>
> thanks,
>
> greg k-h

You guessed it - pretty obvious, isn't it? Well, I assumed the case
where class_dev is NULL didn't matter, as this could only happen if
there is a bug in some other place - seems I was wrong. So how's
about this patch:


--- linux-mips/drivers/base/class.c     2004-07-14 16:21:33.000000000 +0200
+++ linux-mips-work/drivers/base/class.c        2004-08-06 11:06:10.983688216 +0200
@@ -349,14 +349,19 @@

 int class_device_add(struct class_device *class_dev)
 {
-       struct class * parent;
+       struct class * parent = NULL;
        struct class_interface * class_intf;
        int error;

        class_dev = class_device_get(class_dev);
-       if (!class_dev || !strlen(class_dev->class_id))
+       if (!class_dev)
                return -EINVAL;

+       if (!strlen(class_dev->class_id)) {
+               error = -EINVAL;
+               goto register_done;
+       }
+
        parent = class_get(class_dev->class);

        pr_debug("CLASS: registering class device: ID = '%s'\n",



-- 
--------------------------------------------------

Thomas Koeller, Software Development
Basler Vision Technologies

thomas dot koeller at baslerweb dot com
http://www.baslerweb.com

==============================

Re: [PATCH] Device class reference counting

[Thomas Koeller]

Sorry,

seems the patch got messsed up somehow, so I am
resending it:

--- linux-mips/drivers/base/class.c     2004-07-14 16:21:33.000000000 +0200
+++ linux-mips-work/drivers/base/class.c        2004-08-06 11:06:10.983688216 +0200
@@ -349,14 +349,19 @@

 int class_device_add(struct class_device *class_dev)
 {
-       struct class * parent;
+       struct class * parent = NULL;
        struct class_interface * class_intf;
        int error;

        class_dev = class_device_get(class_dev);
-       if (!class_dev || !strlen(class_dev->class_id))
+       if (!class_dev)
                return -EINVAL;

+       if (!strlen(class_dev->class_id)) {
+               error = -EINVAL;
+               goto register_done;
+       }
+
        parent = class_get(class_dev->class);

        pr_debug("CLASS: registering class device: ID = '%s'\n",

-- 
--------------------------------------------------

Thomas Koeller, Software Development
Basler Vision Technologies

thomas dot koeller at baslerweb dot com
http://www.baslerweb.com

==============================

Re: [PATCH] Device class reference counting

[Greg KH]

On Fri, Aug 06, 2004 at 11:43:47AM +0200, Thomas Koeller wrote:
> Sorry,
> 
> seems the patch got messsed up somehow, so I am
> resending it:

This patch looks good.  But it has the tabs stripped out of it, and no
Signed-off-by: line.  Care to resend it with that fixed up?

thanks,

greg k-h

Re: [PATCH] Device class reference counting

[Thomas Koeller]

On Friday 06 August 2004 21:47, Greg KH wrote:
> On Fri, Aug 06, 2004 at 11:43:47AM +0200, Thomas Koeller wrote:
> > Sorry,
> >
> > seems the patch got messsed up somehow, so I am
> > resending it:
>
> This patch looks good.  But it has the tabs stripped out of it, and no
> Signed-off-by: line.  Care to resend it with that fixed up?
>
> thanks,
>
> greg k-h

Hi greg,

here's the patch again, re-created against 2.6.8-rc3.
This time I even managed to read
Documentation/SubmittingPatches ;-)

The tabs are all there, if they are stripped again somewhere
along the way, it is beyond my control. If this should happen,
could you fix that on your side?

thanks,
Thomas



Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>

--- linux-2.6.8-rc3-orig/drivers/base/class.c	2004-08-07 12:19:01.000000000 +0200
+++ linux-2.6.8-rc3/drivers/base/class.c	2004-08-07 12:27:32.112631077 +0200
@@ -349,15 +349,20 @@ void class_device_initialize(struct clas
 
 int class_device_add(struct class_device *class_dev)
 {
-	struct class * parent;
+	struct class * parent = NULL;
 	struct class_interface * class_intf;
 	int error;
 
 	class_dev = class_device_get(class_dev);
-	if (!class_dev || !strlen(class_dev->class_id))
+	if (!class_dev)
 		return -EINVAL;
 
-	parent = class_get(class_dev->class);
+	if (!strlen(class_dev->class_id)) {
+		error = -EINVAL;
+		goto register_done;
+	}
+
+ 	parent = class_get(class_dev->class);
 
 	pr_debug("CLASS: registering class device: ID = '%s'\n",
 		 class_dev->class_id);

-- 
--------------------------------------------------

Thomas Koeller, Software Development
Basler Vision Technologies

thomas dot koeller at baslerweb dot com
http://www.baslerweb.com

==============================

Re: [PATCH] Device class reference counting

[Greg KH]

On Tue, Aug 10, 2004 at 02:09:43PM +0200, Thomas Koeller wrote:
> On Friday 06 August 2004 21:47, Greg KH wrote:
> > On Fri, Aug 06, 2004 at 11:43:47AM +0200, Thomas Koeller wrote:
> > > Sorry,
> > >
> > > seems the patch got messsed up somehow, so I am
> > > resending it:
> >
> > This patch looks good.  But it has the tabs stripped out of it, and no
> > Signed-off-by: line.  Care to resend it with that fixed up?
> >
> > thanks,
> >
> > greg k-h
> 
> Hi greg,
> 
> here's the patch again, re-created against 2.6.8-rc3.
> This time I even managed to read
> Documentation/SubmittingPatches ;-)
> 
> The tabs are all there, if they are stripped again somewhere
> along the way, it is beyond my control. If this should happen,
> could you fix that on your side?

Looks good, I've applied this, thanks.

greg k-h