[PATCH] gpiolib: fix device_create() result check

[PATCH] gpiolib: fix device_create() result check

[Sergei Shtylyov]

In case of failure, device_create() returns not NULL but the error code.
The current code checks for non-NULL though which causes kernel oops in
sysfs_create_group() when device_create() fails.  Check for error using
IS_ERR() and propagate the error value using PTR_ERR() instead of fixed
-ENODEV code returned now...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

 drivers/gpio/gpiolib.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/gpio/gpiolib.c
===================================================================
--- linux-2.6.orig/drivers/gpio/gpiolib.c
+++ linux-2.6/drivers/gpio/gpiolib.c
@@ -661,7 +661,7 @@ int gpio_export(unsigned gpio, bool dire
 
 		dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
 				desc, ioname ? ioname : "gpio%d", gpio);
-		if (dev) {
+		if (!IS_ERR(dev)) {
 			if (direction_may_change)
 				status = sysfs_create_group(&dev->kobj,
 						&gpio_attr_group);
@@ -679,7 +679,7 @@ int gpio_export(unsigned gpio, bool dire
 			if (status != 0)
 				device_unregister(dev);
 		} else
-			status = -ENODEV;
+			status = PTR_ERR(dev);
 		if (status == 0)
 			set_bit(FLAG_EXPORT, &desc->flags);
 	}
@@ -800,11 +800,11 @@ static int gpiochip_export(struct gpio_c
 	mutex_lock(&sysfs_lock);
 	dev = device_create(&gpio_class, chip->dev, MKDEV(0, 0), chip,
 				"gpiochip%d", chip->base);
-	if (dev) {
+	if (!IS_ERR(dev)) {
 		status = sysfs_create_group(&dev->kobj,
 				&gpiochip_attr_group);
 	} else
-		status = -ENODEV;
+		status = PTR_ERR(dev);
 	chip->exported = (status == 0);
 	mutex_unlock(&sysfs_lock);
 

Re: [PATCH] gpiolib: fix device_create() result check

[Andrew Morton]

On Tue, 20 Oct 2009 20:35:23 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> In case of failure, device_create() returns not NULL but the error code.
> The current code checks for non-NULL though which causes kernel oops in
> sysfs_create_group() when device_create() fails.  Check for error using
> IS_ERR() and propagate the error value using PTR_ERR() instead of fixed
> -ENODEV code returned now...

Does anyone notice any missing information here?

/**
 * device_create - creates a device and registers it with sysfs
 * @class: pointer to the struct class that this device should be registered to
 * @parent: pointer to the parent struct device of this new device, if any
 * @devt: the dev_t for the char device to be added
 * @drvdata: the data to be added to the device for callbacks
 * @fmt: string for the device's name
 *
 * This function can be used by char device classes.  A struct device
 * will be created in sysfs, registered to the specified class.
 *
 * A "dev" file will be created, showing the dev_t for the device, if
 * the dev_t is not 0,0.
 * If a pointer to a parent struct device is passed in, the newly created
 * struct device will be a child of that device in sysfs.
 * The pointer to the struct device will be returned from the call.
 * Any further sysfs files that might be required can be created using this
 * pointer.
 *
 * Note: the struct class passed to this function must have previously
 * been created with a call to class_create().
 */


Why do we do this to ourselves?

Re: [PATCH] gpiolib: fix device_create() result check

[Alan Cox]

On Tue, 3 Nov 2009 14:44:01 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 20 Oct 2009 20:35:23 +0400
> Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> 
> > In case of failure, device_create() returns not NULL but the error code.
> > The current code checks for non-NULL though which causes kernel oops in
> > sysfs_create_group() when device_create() fails.  Check for error using
> > IS_ERR() and propagate the error value using PTR_ERR() instead of fixed
> > -ENODEV code returned now...
> 
> Does anyone notice any missing information here?

its what you get for cheap hacks instead of having a ptr_err_t which is a
union so you have to get it right ;)

Alan

Re: [PATCH] gpiolib: fix device_create() result check

[Greg KH]

On Tue, Nov 03, 2009 at 02:44:01PM -0800, Andrew Morton wrote:
> On Tue, 20 Oct 2009 20:35:23 +0400
> Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> 
> > In case of failure, device_create() returns not NULL but the error code.
> > The current code checks for non-NULL though which causes kernel oops in
> > sysfs_create_group() when device_create() fails.  Check for error using
> > IS_ERR() and propagate the error value using PTR_ERR() instead of fixed
> > -ENODEV code returned now...
> 
> Does anyone notice any missing information here?
> 
> /**
>  * device_create - creates a device and registers it with sysfs
>  * @class: pointer to the struct class that this device should be registered to
>  * @parent: pointer to the parent struct device of this new device, if any
>  * @devt: the dev_t for the char device to be added
>  * @drvdata: the data to be added to the device for callbacks
>  * @fmt: string for the device's name
>  *
>  * This function can be used by char device classes.  A struct device
>  * will be created in sysfs, registered to the specified class.
>  *
>  * A "dev" file will be created, showing the dev_t for the device, if
>  * the dev_t is not 0,0.
>  * If a pointer to a parent struct device is passed in, the newly created
>  * struct device will be a child of that device in sysfs.
>  * The pointer to the struct device will be returned from the call.
>  * Any further sysfs files that might be required can be created using this
>  * pointer.
>  *
>  * Note: the struct class passed to this function must have previously
>  * been created with a call to class_create().
>  */
> 
> 
> Why do we do this to ourselves?

Because we suck at writing documentation?  :)

Patches to add the one line:
	"If an error happens, a PTR_ERR is returned instead of a pointer
	to the device"
would be appreciated.

And yes, I like Alan's idea, that would be much nicer overall for lots
of functions like this.

thanks,

greg k-h