[PATCH 1/3] drivers/base/core: improve device_add() error handling, fix bugs

[PATCH 1/3] drivers/base/core: improve device_add() error handling, fix bugs

[Jeff Garzik]

The following change was checked into 'warnings' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git

commit 5330916f5a11d8b7def46a6b3e14a831684032f5
Author: Jeff Garzik <jeff@garzik.org>
Date:   Wed Jul 18 01:41:29 2007 -0400

    drivers/base/core: improve device_add() error handling, fix bugs
    
    * silence several warnings by handling sysfs_create_link() failure
    
    * re-order some operations in error handling, to precisely reverse
      the order of operations, i.e. unwind properly.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 drivers/base/core.c |   46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

5330916f5a11d8b7def46a6b3e14a831684032f5
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0455aa7..f26c005 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -698,16 +698,26 @@ int device_add(struct device *dev)
 	}
 
 	if (dev->class) {
-		sysfs_create_link(&dev->kobj, &dev->class->subsys.kobj,
-				  "subsystem");
+		error = sysfs_create_link(&dev->kobj, &dev->class->subsys.kobj,
+					  "subsystem");
+		if (error)
+			goto ClassSubsysError;
+
 		/* If this is not a "fake" compatible device, then create the
 		 * symlink from the class to the device. */
-		if (dev->kobj.parent != &dev->class->subsys.kobj)
-			sysfs_create_link(&dev->class->subsys.kobj,
-					  &dev->kobj, dev->bus_id);
+		if (dev->kobj.parent != &dev->class->subsys.kobj) {
+			error = sysfs_create_link(&dev->class->subsys.kobj,
+						  &dev->kobj, dev->bus_id);
+			if (error)
+				goto ClassDevLinkError;
+		}
+
 		if (parent) {
-			sysfs_create_link(&dev->kobj, &dev->parent->kobj,
-							"device");
+			error = sysfs_create_link(&dev->kobj,&dev->parent->kobj,
+						  "device");
+			if (error)
+				goto ParentDevLinkError;
+
 #ifdef CONFIG_SYSFS_DEPRECATED
 			class_name = make_class_name(dev->class->name,
 							&dev->kobj);
@@ -755,16 +765,7 @@ int device_add(struct device *dev)
 					     BUS_NOTIFY_DEL_DEVICE, dev);
 	device_remove_attrs(dev);
  AttrsError:
-	if (MAJOR(dev->devt))
-		device_remove_file(dev, &devt_attr);
-
 	if (dev->class) {
-		sysfs_remove_link(&dev->kobj, "subsystem");
-		/* If this is not a "fake" compatible device, remove the
-		 * symlink from the class to the device. */
-		if (dev->kobj.parent != &dev->class->subsys.kobj)
-			sysfs_remove_link(&dev->class->subsys.kobj,
-					  dev->bus_id);
 		if (parent) {
 #ifdef CONFIG_SYSFS_DEPRECATED
 			char *class_name = make_class_name(dev->class->name,
@@ -776,7 +777,20 @@ int device_add(struct device *dev)
 #endif
 			sysfs_remove_link(&dev->kobj, "device");
 		}
+
+ ParentDevLinkError:
+		/* If this is not a "fake" compatible device, remove the
+		 * symlink from the class to the device. */
+		if (dev->kobj.parent != &dev->class->subsys.kobj)
+			sysfs_remove_link(&dev->class->subsys.kobj,
+					  dev->bus_id);
+
+ ClassDevLinkError:
+		sysfs_remove_link(&dev->kobj, "subsystem");
 	}
+ ClassSubsysError:
+	if (MAJOR(dev->devt))
+		device_remove_file(dev, &devt_attr);
  ueventattrError:
 	device_remove_file(dev, &uevent_attr);
  attrError:

[PATCH 2/3] drivers/video/aty/radeon_base: fix radeonfb_pci_register() err handling

[Jeff Garzik]

The following change was checked into 'warnings' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git

commit 61470a24062de01853fb922ec4a81dcd1c0ba1d0
Author: Jeff Garzik <jeff@garzik.org>
Date:   Wed Jul 18 01:43:05 2007 -0400

    drivers/video/aty/radeon_base: fix radeonfb_pci_register() err handling
    
    * silence warnings by handling failure of sysfs_create_bin_file()
    
    * fix leak-on-error if register_framebuffer() fails, and sysfs is active
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 drivers/video/aty/radeon_base.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

61470a24062de01853fb922ec4a81dcd1c0ba1d0
diff --git a/drivers/video/aty/radeon_base.c b/drivers/video/aty/radeon_base.c
index 47ca62f..5a5458b 100644
--- a/drivers/video/aty/radeon_base.c
+++ b/drivers/video/aty/radeon_base.c
@@ -2326,10 +2326,16 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 	radeon_check_modes(rinfo, mode_option);
 
 	/* Register some sysfs stuff (should be done better) */
-	if (rinfo->mon1_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
-	if (rinfo->mon2_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+	if (rinfo->mon1_EDID) {
+		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj,&edid1_attr);
+		if (ret)
+			goto err_unmap_fb;
+	}
+	if (rinfo->mon2_EDID) {
+		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj,&edid2_attr);
+		if (ret)
+			goto err_free_mon1;
+	}
 
 	/* save current mode regs before we switch into the new one
 	 * so we can restore this upon __exit
@@ -2353,7 +2359,7 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 	if (ret < 0) {
 		printk (KERN_ERR "radeonfb (%s): could not register framebuffer\n",
 			pci_name(rinfo->pdev));
-		goto err_unmap_fb;
+		goto err_free_mon2;
 	}
 
 #ifdef CONFIG_MTRR
@@ -2372,6 +2378,13 @@ static int __devinit radeonfb_pci_register (struct pci_dev *pdev,
 	RTRACE("radeonfb_pci_register END\n");
 
 	return 0;
+
+err_free_mon2:
+	if (rinfo->mon2_EDID)
+		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+err_free_mon1:
+	if (rinfo->mon1_EDID)
+		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
 err_unmap_fb:
 	iounmap(rinfo->fb_base);
 err_unmap_rom:

[PATCH 3/3] x86-64: make flush_tlb_kernel_range() a static inline function

[Jeff Garzik]

The following change was checked into 'warnings' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git

commit b1c9131343597967d8fcc042f59e4ed36ea2855f
Author: Jeff Garzik <jeff@garzik.org>
Date:   Wed Jul 18 01:47:39 2007 -0400

    [X86-64] make flush_tlb_kernel_range() a static inline function
    
    On x86-64, without this change, the following warning is emitted
    
    mm/vmalloc.c: In function ‘unmap_kernel_range’:
    mm/vmalloc.c:75: warning: unused variable ‘start’
    
    because, x86-64, flush_tlb_kernel_range(start,end) evaluates down to
    flush_tlb_all(void) in a macro.
    
    Avoid this warning by implementing x86-64's flush_tlb_kernel_range() as
    a type-safe static inline, rather than a macro.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 include/asm-x86_64/tlbflush.h |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

b1c9131343597967d8fcc042f59e4ed36ea2855f
diff --git a/include/asm-x86_64/tlbflush.h b/include/asm-x86_64/tlbflush.h
index 8516225..a82464c 100644
--- a/include/asm-x86_64/tlbflush.h
+++ b/include/asm-x86_64/tlbflush.h
@@ -92,7 +92,11 @@ static inline void flush_tlb_range(struct vm_area_struct * vma, unsigned long st
 
 #endif
 
-#define flush_tlb_kernel_range(start, end) flush_tlb_all()
+static inline void flush_tlb_kernel_range(unsigned long start,
+					  unsigned long end)
+{
+	flush_tlb_all();
+}
 
 static inline void flush_tlb_pgtables(struct mm_struct *mm,
 				      unsigned long start, unsigned long end)

Re: [PATCH 1/3] drivers/base/core: improve device_add() error handling, fix bugs

[Cornelia Huck]

On Wed, 18 Jul 2007 01:53:48 -0400,
Jeff Garzik <jeff@garzik.org> wrote:

> 
> The following change was checked into 'warnings' branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git
> 
> commit 5330916f5a11d8b7def46a6b3e14a831684032f5
> Author: Jeff Garzik <jeff@garzik.org>
> Date:   Wed Jul 18 01:41:29 2007 -0400
> 
>     drivers/base/core: improve device_add() error handling, fix bugs
>     
>     * silence several warnings by handling sysfs_create_link() failure
>     
>     * re-order some operations in error handling, to precisely reverse
>       the order of operations, i.e. unwind properly.
>     
>     Signed-off-by: Jeff Garzik <jeff@garzik.org>
> 
>  drivers/base/core.c |   46 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 16 deletions(-)

Hm, what about driver-core-check-return-code-of-sysfs_create_link.patch
(merged with ...-fix.patch) which has been in -mm for a while?

Re: [PATCH 1/3] drivers/base/core: improve device_add() error handling, fix bugs

[Jeff Garzik]

Cornelia Huck wrote:
> On Wed, 18 Jul 2007 01:53:48 -0400,
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> The following change was checked into 'warnings' branch of
>> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git
>>
>> commit 5330916f5a11d8b7def46a6b3e14a831684032f5
>> Author: Jeff Garzik <jeff@garzik.org>
>> Date:   Wed Jul 18 01:41:29 2007 -0400
>>
>>     drivers/base/core: improve device_add() error handling, fix bugs
>>     
>>     * silence several warnings by handling sysfs_create_link() failure
>>     
>>     * re-order some operations in error handling, to precisely reverse
>>       the order of operations, i.e. unwind properly.
>>     
>>     Signed-off-by: Jeff Garzik <jeff@garzik.org>
>>
>>  drivers/base/core.c |   46 ++++++++++++++++++++++++++++++----------------
>>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> Hm, what about driver-core-check-return-code-of-sysfs_create_link.patch
> (merged with ...-fix.patch) which has been in -mm for a while?

No idea, I didn't check there.   If someone else has a better patch... 
great!  Let's merge it :)

	Jeff

Re: [PATCH 1/3] drivers/base/core: improve device_add() error handling, fix bugs

[Greg KH]

On Wed, Jul 18, 2007 at 04:16:10AM -0400, Jeff Garzik wrote:
>  Cornelia Huck wrote:
> > On Wed, 18 Jul 2007 01:53:48 -0400,
> > Jeff Garzik <jeff@garzik.org> wrote:
> >> The following change was checked into 'warnings' branch of
> >> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git
> >>
> >> commit 5330916f5a11d8b7def46a6b3e14a831684032f5
> >> Author: Jeff Garzik <jeff@garzik.org>
> >> Date:   Wed Jul 18 01:41:29 2007 -0400
> >>
> >>     drivers/base/core: improve device_add() error handling, fix bugs
> >>         * silence several warnings by handling sysfs_create_link() failure
> >>         * re-order some operations in error handling, to precisely reverse
> >>       the order of operations, i.e. unwind properly.
> >>         Signed-off-by: Jeff Garzik <jeff@garzik.org>
> >>
> >>  drivers/base/core.c |   46 ++++++++++++++++++++++++++++++----------------
> >>  1 file changed, 30 insertions(+), 16 deletions(-)
> > Hm, what about driver-core-check-return-code-of-sysfs_create_link.patch
> > (merged with ...-fix.patch) which has been in -mm for a while?
> 
>  No idea, I didn't check there.   If someone else has a better patch... 
>  great!  Let's merge it :)

It's in my queue.

Or it should be, hm, Cornelia, can you resend it, I seem to have dropped
it :(

thanks,

greg k-h

Re: [PATCH 1/3] drivers/base/core: improve device_add() error handling, fix bugs

[Cornelia Huck]

On Wed, 18 Jul 2007 01:36:00 -0700,
Greg KH <greg@kroah.com> wrote:

> Or it should be, hm, Cornelia, can you resend it, I seem to have dropped
> it :(

Andrew just did :)

(For reference, here's the patch:)

> From: akpm@linux-foundation.org
> To: greg@kroah.com
> Cc: akpm@linux-foundation.org, cornelia.huck@de.ibm.com
> Subject: [patch 1/1] Driver core: check return code of sysfs_create_link()
> Date: Wed, 18 Jul 2007 01:43:47 -0700
> 
> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Check for return value of sysfs_create_link() in device_add() and
> device_rename().  Add helper functions device_add_class_symlinks() and
> device_remove_class_symlinks() to make the code easier to read.
> 
> [akpm@linux-foundation.org: fix unused var warnings]
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Greg KH <greg@kroah.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/base/core.c |  145 ++++++++++++++++++++++++++++++------------
>  1 file changed, 105 insertions(+), 40 deletions(-)
> 
> diff -puN drivers/base/core.c~driver-core-check-return-code-of-sysfs_create_link drivers/base/core.c
> --- a/drivers/base/core.c~driver-core-check-return-code-of-sysfs_create_link
> +++ a/drivers/base/core.c
> @@ -643,6 +643,82 @@ static int setup_parent(struct device *d
>  	return 0;
>  }
> 
> +static int device_add_class_symlinks(struct device *dev)
> +{
> +	int error;
> +
> +	if (!dev->class)
> +		return 0;
> +	error = sysfs_create_link(&dev->kobj, &dev->class->subsys.kobj,
> +				  "subsystem");
> +	if (error)
> +		goto out;
> +	/*
> +	 * If this is not a "fake" compatible device, then create the
> +	 * symlink from the class to the device.
> +	 */
> +	if (dev->kobj.parent != &dev->class->subsys.kobj) {
> +		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
> +					  dev->bus_id);
> +		if (error)
> +			goto out_subsys;
> +	}
> +	/* only bus-device parents get a "device"-link */
> +	if (dev->parent && dev->parent->bus) {
> +		error = sysfs_create_link(&dev->kobj, &dev->parent->kobj,
> +					  "device");
> +		if (error)
> +			goto out_busid;
> +#ifdef CONFIG_SYSFS_DEPRECATED
> +		{
> +			char * class_name = make_class_name(dev->class->name,
> +								&dev->kobj);
> +			if (class_name)
> +				error = sysfs_create_link(&dev->parent->kobj,
> +							&dev->kobj, class_name);
> +			kfree(class_name);
> +			if (error)
> +				goto out_device;
> +		}
> +#endif
> +	}
> +	return 0;
> +
> +#ifdef CONFIG_SYSFS_DEPRECATED
> +out_device:
> +	if (dev->parent)
> +		sysfs_remove_link(&dev->kobj, "device");
> +#endif
> +out_busid:
> +	if (dev->kobj.parent != &dev->class->subsys.kobj)
> +		sysfs_remove_link(&dev->class->subsys.kobj, dev->bus_id);
> +out_subsys:
> +	sysfs_remove_link(&dev->kobj, "subsystem");
> +out:
> +	return error;
> +}
> +
> +static void device_remove_class_symlinks(struct device *dev)
> +{
> +	if (!dev->class)
> +		return;
> +	if (dev->parent) {
> +#ifdef CONFIG_SYSFS_DEPRECATED
> +		char *class_name;
> +
> +		class_name = make_class_name(dev->class->name, &dev->kobj);
> +		if (class_name) {
> +			sysfs_remove_link(&dev->parent->kobj, class_name);
> +			kfree(class_name);
> +		}
> +#endif
> +		sysfs_remove_link(&dev->kobj, "device");
> +	}
> +	if (dev->kobj.parent != &dev->class->subsys.kobj)
> +		sysfs_remove_link(&dev->class->subsys.kobj, dev->bus_id);
> +	sysfs_remove_link(&dev->kobj, "subsystem");
> +}
> +
>  /**
>   *	device_add - add device to device hierarchy.
>   *	@dev:	device.
> @@ -657,7 +733,6 @@ static int setup_parent(struct device *d
>  int device_add(struct device *dev)
>  {
>  	struct device *parent = NULL;
> -	char *class_name = NULL;
>  	struct class_interface *class_intf;
>  	int error = -EINVAL;
> 
> @@ -697,27 +772,9 @@ int device_add(struct device *dev)
>  			goto ueventattrError;
>  	}
> 
> -	if (dev->class) {
> -		sysfs_create_link(&dev->kobj, &dev->class->subsys.kobj,
> -				  "subsystem");
> -		/* If this is not a "fake" compatible device, then create the
> -		 * symlink from the class to the device. */
> -		if (dev->kobj.parent != &dev->class->subsys.kobj)
> -			sysfs_create_link(&dev->class->subsys.kobj,
> -					  &dev->kobj, dev->bus_id);
> -		if (parent) {
> -			sysfs_create_link(&dev->kobj, &dev->parent->kobj,
> -							"device");
> -#ifdef CONFIG_SYSFS_DEPRECATED
> -			class_name = make_class_name(dev->class->name,
> -							&dev->kobj);
> -			if (class_name)
> -				sysfs_create_link(&dev->parent->kobj,
> -						  &dev->kobj, class_name);
> -#endif
> -		}
> -	}
> -
> +	error = device_add_class_symlinks(dev);
> +	if (error)
> +		goto SymlinkError;
>  	error = device_add_attrs(dev);
>  	if (error)
>  		goto AttrsError;
> @@ -744,7 +801,6 @@ int device_add(struct device *dev)
>  		up(&dev->class->sem);
>  	}
>   Done:
> -	kfree(class_name);
>  	put_device(dev);
>  	return error;
>   BusError:
> @@ -755,6 +811,8 @@ int device_add(struct device *dev)
>  					     BUS_NOTIFY_DEL_DEVICE, dev);
>  	device_remove_attrs(dev);
>   AttrsError:
> +	device_remove_class_symlinks(dev);
> + SymlinkError:
>  	if (MAJOR(dev->devt))
>  		device_remove_file(dev, &devt_attr);
> 
> @@ -1140,7 +1198,7 @@ int device_rename(struct device *dev, ch
>  {
>  	char *old_class_name = NULL;
>  	char *new_class_name = NULL;
> -	char *old_symlink_name = NULL;
> +	char *old_device_name = NULL;
>  	int error;
> 
>  	dev = get_device(dev);
> @@ -1154,42 +1212,49 @@ int device_rename(struct device *dev, ch
>  		old_class_name = make_class_name(dev->class->name, &dev->kobj);
>  #endif
> 
> -	if (dev->class) {
> -		old_symlink_name = kmalloc(BUS_ID_SIZE, GFP_KERNEL);
> -		if (!old_symlink_name) {
> -			error = -ENOMEM;
> -			goto out_free_old_class;
> -		}
> -		strlcpy(old_symlink_name, dev->bus_id, BUS_ID_SIZE);
> +	old_device_name = kmalloc(BUS_ID_SIZE, GFP_KERNEL);
> +	if (!old_device_name) {
> +		error = -ENOMEM;
> +		goto out;
>  	}
> -
> +	strlcpy(old_device_name, dev->bus_id, BUS_ID_SIZE);
>  	strlcpy(dev->bus_id, new_name, BUS_ID_SIZE);
> 
>  	error = kobject_rename(&dev->kobj, new_name);
> +	if (error) {
> +		strlcpy(dev->bus_id, old_device_name, BUS_ID_SIZE);
> +		goto out;
> +	}
> 
>  #ifdef CONFIG_SYSFS_DEPRECATED
>  	if (old_class_name) {
>  		new_class_name = make_class_name(dev->class->name, &dev->kobj);
>  		if (new_class_name) {
> -			sysfs_create_link(&dev->parent->kobj, &dev->kobj,
> -					  new_class_name);
> +			error = sysfs_create_link(&dev->parent->kobj,
> +						  &dev->kobj, new_class_name);
> +			if (error)
> +				goto out;
>  			sysfs_remove_link(&dev->parent->kobj, old_class_name);
>  		}
>  	}
>  #endif
> 
>  	if (dev->class) {
> -		sysfs_remove_link(&dev->class->subsys.kobj,
> -				  old_symlink_name);
> -		sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
> -				  dev->bus_id);
> +		sysfs_remove_link(&dev->class->subsys.kobj, old_device_name);
> +		error = sysfs_create_link(&dev->class->subsys.kobj, &dev->kobj,
> +					  dev->bus_id);
> +		if (error) {
> +			/* Uh... how to unravel this if restoring can fail? */
> +			dev_err(dev, "%s: sysfs_create_symlink failed (%d)\n",
> +				__FUNCTION__, error);
> +		}
>  	}
> +out:
>  	put_device(dev);
> 
>  	kfree(new_class_name);
> -	kfree(old_symlink_name);
> - out_free_old_class:
>  	kfree(old_class_name);
> +	kfree(old_device_name);
> 
>  	return error;
>  }
> _

Re: [PATCH 1/3] drivers/base/core: improve device_add() error handling, fix bugs

[Jeff Garzik]

Cornelia Huck wrote:
> On Wed, 18 Jul 2007 01:36:00 -0700,
> Greg KH <greg@kroah.com> wrote:
> 
>> Or it should be, hm, Cornelia, can you resend it, I seem to have dropped
>> it :(
> 
> Andrew just did :)
> 
> (For reference, here's the patch:)
> 
>> From: akpm@linux-foundation.org
>> To: greg@kroah.com
>> Cc: akpm@linux-foundation.org, cornelia.huck@de.ibm.com
>> Subject: [patch 1/1] Driver core: check return code of sysfs_create_link()
>> Date: Wed, 18 Jul 2007 01:43:47 -0700
>>
>> From: Cornelia Huck <cornelia.huck@de.ibm.com>
>>
>> Check for return value of sysfs_create_link() in device_add() and
>> device_rename().  Add helper functions device_add_class_symlinks() and
>> device_remove_class_symlinks() to make the code easier to read.
>>
>> [akpm@linux-foundation.org: fix unused var warnings]
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Greg KH <greg@kroah.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>
>>  drivers/base/core.c |  145 ++++++++++++++++++++++++++++++------------
>>  1 file changed, 105 insertions(+), 40 deletions(-)

ACK