[PATCH] hwmon/abituguru: handle sysfs errors

[PATCH] hwmon/abituguru: handle sysfs errors

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jeff@garzik.org>

---

 drivers/hwmon/abituguru.c |   30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

2b10f648c8ed965369976eb7925b922ee187ce21
diff --git a/drivers/hwmon/abituguru.c b/drivers/hwmon/abituguru.c
index e5cb0fd..3ded982 100644
--- a/drivers/hwmon/abituguru.c
+++ b/drivers/hwmon/abituguru.c
@@ -1271,14 +1271,34 @@ static int __devinit abituguru_probe(str
 		res = PTR_ERR(data->class_dev);
 		goto abituguru_probe_error;
 	}
-	for (i = 0; i < sysfs_attr_i; i++)
-		device_create_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
-	for (i = 0; i < ARRAY_SIZE(abituguru_sysfs_attr); i++)
-		device_create_file(&pdev->dev,
-			&abituguru_sysfs_attr[i].dev_attr);
+	for (i = 0; i < sysfs_attr_i; i++) {
+		res = device_create_file(&pdev->dev,
+					 &data->sysfs_attr[i].dev_attr);
+		if (res) {
+			for (j = 0; j < i; j++)
+				device_remove_file(&pdev->dev,
+					 	&data->sysfs_attr[j].dev_attr);
+			goto err_devreg;
+		}
+	}
+	for (i = 0; i < ARRAY_SIZE(abituguru_sysfs_attr); i++) {
+		res = device_create_file(&pdev->dev,
+					 &abituguru_sysfs_attr[i].dev_attr);
+		if (res) {
+			for (j = 0; j < i; j++)
+				device_remove_file(&pdev->dev,
+					 &abituguru_sysfs_attr[j].dev_attr);
+			goto err_attr_i;
+		}
+	}
 
 	return 0;
 
+err_attr_i:
+	for (i = 0; i < sysfs_attr_i; i++)
+		device_remove_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
+err_devreg:
+	hwmon_device_unregister(data->class_dev);
 abituguru_probe_error:
 	kfree(data);
 	return res;

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Hans de Goede]

Hi Jean, Jeff,

You (Jean) already mailed me about this and it was on my todo list, but I'm currently rather busy with work. So it looks like Jeff beat me to it.

Jeff's patch looks fine, please apply. Thanks Jeff!

Regards,

Hans

Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
Signed-off-by: Jeff Garzik <jeff@garzik.org>

---

 drivers/hwmon/abituguru.c |   30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

2b10f648c8ed965369976eb7925b922ee187ce21
diff --git a/drivers/hwmon/abituguru.c b/drivers/hwmon/abituguru.c
index e5cb0fd..3ded982 100644
--- a/drivers/hwmon/abituguru.c
+++ b/drivers/hwmon/abituguru.c
@@ -1271,14 +1271,34 @@ static int __devinit abituguru_probe(str
 		res = PTR_ERR(data->class_dev);
 		goto abituguru_probe_error;
 	}
-	for (i = 0; i < sysfs_attr_i; i++)
-		device_create_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
-	for (i = 0; i < ARRAY_SIZE(abituguru_sysfs_attr); i++)
-		device_create_file(&pdev->dev,
-			&abituguru_sysfs_attr[i].dev_attr);
+	for (i = 0; i < sysfs_attr_i; i++) {
+		res = device_create_file(&pdev->dev,
+					 &data->sysfs_attr[i].dev_attr);
+		if (res) {
+			for (j = 0; j < i; j++)
+				device_remove_file(&pdev->dev,
+					 	&data->sysfs_attr[j].dev_attr);
+			goto err_devreg;
+		}
+	}
+	for (i = 0; i < ARRAY_SIZE(abituguru_sysfs_attr); i++) {
+		res = device_create_file(&pdev->dev,
+					 &abituguru_sysfs_attr[i].dev_attr);
+		if (res) {
+			for (j = 0; j < i; j++)
+				device_remove_file(&pdev->dev,
+					 &abituguru_sysfs_attr[j].dev_attr);
+			goto err_attr_i;
+		}
+	}
 
 	return 0;
 
+err_attr_i:
+	for (i = 0; i < sysfs_attr_i; i++)
+		device_remove_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
+err_devreg:
+	hwmon_device_unregister(data->class_dev);
 abituguru_probe_error:
 	kfree(data);
 	return res;

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Jean Delvare]

Hi Hans, Jeff,

> You (Jean) already mailed me about this and it was on my todo list,
> but I'm currently rather busy with work. So it looks like Jeff beat
> me to it.
> 
> Jeff's patch looks fine, please apply. Thanks Jeff!

The patch isn't wrong per se, but it could be made more simple, and is
incomplete in comparison to what was done for all other hardware
monitoring drivers:

* We want to create all the files before registering with the hwmon
  class, this closes a race condition.
* We want to delete all the device files at regular cleanup time (after
  unregistering with the hwmon class).
* It's OK to call device_create_file() on a non-existent file, so the
  error path can be simplified.

I'd like the abituguru driver to behave the same as all other hardware
monitoring drivers to lower the maintenance effort. Can either you
or Jeff work up a compliant patch? 

Thanks.

>  drivers/hwmon/abituguru.c |   30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> 2b10f648c8ed965369976eb7925b922ee187ce21
> diff --git a/drivers/hwmon/abituguru.c b/drivers/hwmon/abituguru.c
> index e5cb0fd..3ded982 100644
> --- a/drivers/hwmon/abituguru.c
> +++ b/drivers/hwmon/abituguru.c
> @@ -1271,14 +1271,34 @@ static int __devinit abituguru_probe(str
>  		res = PTR_ERR(data->class_dev);
>  		goto abituguru_probe_error;
>  	}
> -	for (i = 0; i < sysfs_attr_i; i++)
> -		device_create_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
> -	for (i = 0; i < ARRAY_SIZE(abituguru_sysfs_attr); i++)
> -		device_create_file(&pdev->dev,
> -			&abituguru_sysfs_attr[i].dev_attr);
> +	for (i = 0; i < sysfs_attr_i; i++) {
> +		res = device_create_file(&pdev->dev,
> +					 &data->sysfs_attr[i].dev_attr);
> +		if (res) {
> +			for (j = 0; j < i; j++)
> +				device_remove_file(&pdev->dev,
> +					 	&data->sysfs_attr[j].dev_attr);
> +			goto err_devreg;
> +		}
> +	}
> +	for (i = 0; i < ARRAY_SIZE(abituguru_sysfs_attr); i++) {
> +		res = device_create_file(&pdev->dev,
> +					 &abituguru_sysfs_attr[i].dev_attr);
> +		if (res) {
> +			for (j = 0; j < i; j++)
> +				device_remove_file(&pdev->dev,
> +					 &abituguru_sysfs_attr[j].dev_attr);
> +			goto err_attr_i;
> +		}
> +	}
>  
>  	return 0;
>  
> +err_attr_i:
> +	for (i = 0; i < sysfs_attr_i; i++)
> +		device_remove_file(&pdev->dev, &data->sysfs_attr[i].dev_attr);
> +err_devreg:
> +	hwmon_device_unregister(data->class_dev);
>  abituguru_probe_error:
>  	kfree(data);
>  	return res;


-- 
Jean Delvare

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans, Jeff,
> 
>> You (Jean) already mailed me about this and it was on my todo list,
>> but I'm currently rather busy with work. So it looks like Jeff beat
>> me to it.
>>
>> Jeff's patch looks fine, please apply. Thanks Jeff!
> 
> The patch isn't wrong per se, but it could be made more simple, and is
> incomplete in comparison to what was done for all other hardware
> monitoring drivers:
> 
> * We want to create all the files before registering with the hwmon
>   class, this closes a race condition.

OK

> * We want to delete all the device files at regular cleanup time (after
>   unregistering with the hwmon class).

Is this really nescesarry? AFAIK the files get deleted when the device gets deleted.

> * It's OK to call device_create_file() on a non-existent file, so the
>   error path can be simplified.
> 

?? You mean device_remove_file I assume?

> I'd like the abituguru driver to behave the same as all other hardware
> monitoring drivers to lower the maintenance effort. Can either you
> or Jeff work up a compliant patch? 
> 

I understand Jeff any chance you can do a new revision of your patch? Otherwise I'll take care of it as time permits.

Regards,

Hans

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Jean Delvare]

Hans,

> > * We want to delete all the device files at regular cleanup time (after
> >   unregistering with the hwmon class).
> 
> Is this really nescesarry? AFAIK the files get deleted when the device
> gets deleted.

The point is that the device shouldn't be deleted if we were following
the device driver model. After all the uGuru device exists in your
system before the abituguru driver is loaded, and is still there after
the driver is unloaded.

Now I agree that for now it won't make a difference, be we are
preparing for the future, and all other hardware monitoring drivers
were updated that way already.

> > * It's OK to call device_create_file() on a non-existent file, so the
> >   error path can be simplified.
> 
> ?? You mean device_remove_file I assume?

Oops, yes, that was a typo, sorry.

Thanks,
-- 
Jean Delvare

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Dmitry Torokhov]

On Tuesday 10 October 2006 05:34, Jean Delvare wrote:
> Hans,
> 
> > > * We want to delete all the device files at regular cleanup time (after
> > >   unregistering with the hwmon class).
> > 
> > Is this really nescesarry? AFAIK the files get deleted when the device
> > gets deleted.
> 
> The point is that the device shouldn't be deleted if we were following
> the device driver model. After all the uGuru device exists in your
> system before the abituguru driver is loaded, and is still there after
> the driver is unloaded.
> 
> Now I agree that for now it won't make a difference, be we are
> preparing for the future, and all other hardware monitoring drivers
> were updated that way already.
> 
> > > * It's OK to call device_create_file() on a non-existent file, so the
> > >   error path can be simplified.
> > 
> > ?? You mean device_remove_file I assume?
> 
> Oops, yes, that was a typo, sorry.
> 

I know I sound like a roken record but this driver would benefit from
using attribute_group.

-- 
Dmitry

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Jean Delvare]

Hi Dmitry,

Dmitry Torokhov wrote:
> I know I sound like a roken record but this driver would benefit from
> using attribute_group.

What about sending a patch then?

-- 
Jean Delvare

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Dmitry Torokhov]

On Thursday 12 October 2006 11:42, Jean Delvare wrote:
> Hi Dmitry,
> 
> Dmitry Torokhov wrote:
> > I know I sound like a roken record but this driver would benefit from
> > using attribute_group.
> 
> What about sending a patch then?
> 

Sorry, spoke too soon. Because the driver uses non-satic attributes
converting it to use attribute_group would probably be messier than
the present code.

-- 
Dmitry

Re: [PATCH] hwmon/abituguru: handle sysfs errors

[Jean Delvare]

Hans,

On Tue, 10 Oct 2006 11:18:33 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > The patch isn't wrong per se, but it could be made more simple, and is
> > incomplete in comparison to what was done for all other hardware
> > monitoring drivers:
> > 
> > * We want to create all the files before registering with the hwmon
> >   class, this closes a race condition.
> > * We want to delete all the device files at regular cleanup time (after
> >   unregistering with the hwmon class).
> > * It's OK to call device_remove_file() on a non-existent file, so the
> >   error path can be simplified.
> >
> > I'd like the abituguru driver to behave the same as all other hardware
> > monitoring drivers to lower the maintenance effort. Can either you
> > or Jeff work up a compliant patch? 
> 
> I understand Jeff any chance you can do a new revision of
> your patch? Otherwise I'll take care of it as time permits.

I'm still waiting for this patch. It'd be nice to have the abituguru
driver fixed in 2.6.20.

Thanks,
-- 
Jean Delvare