* [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path
@ 2010-11-09 5:18 Axel Lin
2010-11-09 7:59 ` Jean Delvare
2010-11-09 8:01 ` Jean Delvare
0 siblings, 2 replies; 6+ messages in thread
From: Axel Lin @ 2010-11-09 5:18 UTC (permalink / raw)
To: linux-kernel; +Cc: Jean Delvare, Guenter Roeck, Simon Guinot, lm-sensors
Remove sysfs entries before return -ENODEV.
Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
drivers/hwmon/gpio-fan.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index aa701a1..d2e66b4 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -388,11 +388,13 @@ static int fan_ctrl_init(struct gpio_fan_data *fan_data,
fan_data->speed_index = get_fan_speed_index(fan_data);
if (fan_data->speed_index < 0) {
err = -ENODEV;
- goto err_free_gpio;
+ goto err_remove_sysfs;
}
return 0;
+err_remove_sysfs:
+ sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
err_free_gpio:
for (i = i - 1; i >= 0; i--)
gpio_free(ctrl[i]);
--
1.7.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path
2010-11-09 5:18 [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path Axel Lin
@ 2010-11-09 7:59 ` Jean Delvare
2010-11-09 8:01 ` Jean Delvare
1 sibling, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2010-11-09 7:59 UTC (permalink / raw)
To: Axel Lin; +Cc: linux-kernel, Guenter Roeck, Simon Guinot, lm-sensors
Hi Alex,
On Tue, 09 Nov 2010 13:18:39 +0800, Axel Lin wrote:
> Remove sysfs entries before return -ENODEV.
>
> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> ---
> drivers/hwmon/gpio-fan.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index aa701a1..d2e66b4 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -388,11 +388,13 @@ static int fan_ctrl_init(struct gpio_fan_data *fan_data,
> fan_data->speed_index = get_fan_speed_index(fan_data);
> if (fan_data->speed_index < 0) {
> err = -ENODEV;
> - goto err_free_gpio;
> + goto err_remove_sysfs;
> }
>
> return 0;
>
> +err_remove_sysfs:
> + sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> err_free_gpio:
> for (i = i - 1; i >= 0; i--)
> gpio_free(ctrl[i]);
This is certainly better than the current situation, however the fact
that you need to do this strongly suggests that the initialization
order is wrong in the first place. Creating the sysfs attribute should
be the last thing done by the function, after all the rest has been
successful. Otherwise there is a small window during which user-space
can access the attribute but the driver isn't ready to deal with the
requests.
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path
2010-11-09 5:18 [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path Axel Lin
2010-11-09 7:59 ` Jean Delvare
@ 2010-11-09 8:01 ` Jean Delvare
2010-11-09 8:22 ` Axel Lin
1 sibling, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2010-11-09 8:01 UTC (permalink / raw)
To: Axel Lin; +Cc: linux-kernel, Guenter Roeck, Simon Guinot, lm-sensors
On Tue, 09 Nov 2010 13:18:39 +0800, Axel Lin wrote:
> Remove sysfs entries before return -ENODEV.
>
> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> ---
> drivers/hwmon/gpio-fan.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index aa701a1..d2e66b4 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -388,11 +388,13 @@ static int fan_ctrl_init(struct gpio_fan_data *fan_data,
> fan_data->speed_index = get_fan_speed_index(fan_data);
> if (fan_data->speed_index < 0) {
> err = -ENODEV;
> - goto err_free_gpio;
> + goto err_remove_sysfs;
> }
>
> return 0;
>
> +err_remove_sysfs:
> + sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> err_free_gpio:
> for (i = i - 1; i >= 0; i--)
> gpio_free(ctrl[i]);
Oh, and while you're here, fan_alarm_init() needs some love as well.
There's a "return 0" in the middle which clearly can't be correct.
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path
2010-11-09 8:01 ` Jean Delvare
@ 2010-11-09 8:22 ` Axel Lin
2010-11-09 9:14 ` Jean Delvare
2010-11-09 9:25 ` Simon Guinot
0 siblings, 2 replies; 6+ messages in thread
From: Axel Lin @ 2010-11-09 8:22 UTC (permalink / raw)
To: Jean Delvare; +Cc: linux-kernel, Guenter Roeck, Simon Guinot, lm-sensors
2010/11/9 Jean Delvare <khali@linux-fr.org>:
> On Tue, 09 Nov 2010 13:18:39 +0800, Axel Lin wrote:
>> Remove sysfs entries before return -ENODEV.
>>
>> Signed-off-by: Axel Lin <axel.lin@gmail.com>
>> ---
>> drivers/hwmon/gpio-fan.c | 4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
>> index aa701a1..d2e66b4 100644
>> --- a/drivers/hwmon/gpio-fan.c
>> +++ b/drivers/hwmon/gpio-fan.c
>> @@ -388,11 +388,13 @@ static int fan_ctrl_init(struct gpio_fan_data *fan_data,
>> fan_data->speed_index = get_fan_speed_index(fan_data);
>> if (fan_data->speed_index < 0) {
>> err = -ENODEV;
>> - goto err_free_gpio;
>> + goto err_remove_sysfs;
>> }
>>
>> return 0;
>>
>> +err_remove_sysfs:
>> + sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
>> err_free_gpio:
>> for (i = i - 1; i >= 0; i--)
>> gpio_free(ctrl[i]);
>
> Oh, and while you're here, fan_alarm_init() needs some love as well.
> There's a "return 0" in the middle which clearly can't be correct.
I think this part is correct. see the comment:
/*
* If the alarm GPIO don't support interrupts, just leave
* without initializing the fail notification support.
*/
Regards,
Axel
>
> --
> Jean Delvare
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path
2010-11-09 8:22 ` Axel Lin
@ 2010-11-09 9:14 ` Jean Delvare
2010-11-09 9:25 ` Simon Guinot
1 sibling, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2010-11-09 9:14 UTC (permalink / raw)
To: axel.lin; +Cc: linux-kernel, Guenter Roeck, Simon Guinot, lm-sensors
On Tue, 9 Nov 2010 16:22:02 +0800, Axel Lin wrote:
> 2010/11/9 Jean Delvare <khali@linux-fr.org>:
> > On Tue, 09 Nov 2010 13:18:39 +0800, Axel Lin wrote:
> >> Remove sysfs entries before return -ENODEV.
> >>
> >> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> >> ---
> >> drivers/hwmon/gpio-fan.c | 4 +++-
> >> 1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> >> index aa701a1..d2e66b4 100644
> >> --- a/drivers/hwmon/gpio-fan.c
> >> +++ b/drivers/hwmon/gpio-fan.c
> >> @@ -388,11 +388,13 @@ static int fan_ctrl_init(struct gpio_fan_data *fan_data,
> >> fan_data->speed_index = get_fan_speed_index(fan_data);
> >> if (fan_data->speed_index < 0) {
> >> err = -ENODEV;
> >> - goto err_free_gpio;
> >> + goto err_remove_sysfs;
> >> }
> >>
> >> return 0;
> >>
> >> +err_remove_sysfs:
> >> + sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> >> err_free_gpio:
> >> for (i = i - 1; i >= 0; i--)
> >> gpio_free(ctrl[i]);
> >
> > Oh, and while you're here, fan_alarm_init() needs some love as well.
> > There's a "return 0" in the middle which clearly can't be correct.
> I think this part is correct. see the comment:
> /*
> * If the alarm GPIO don't support interrupts, just leave
> * without initializing the fail notification support.
> */
Oops, my bad. I should have read the comments before blaming the
author :(
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path
2010-11-09 8:22 ` Axel Lin
2010-11-09 9:14 ` Jean Delvare
@ 2010-11-09 9:25 ` Simon Guinot
1 sibling, 0 replies; 6+ messages in thread
From: Simon Guinot @ 2010-11-09 9:25 UTC (permalink / raw)
To: Axel Lin
Cc: Jean Delvare, linux-kernel, Guenter Roeck, Simon Guinot, lm-sensors
[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]
On Tue, Nov 09, 2010 at 04:22:02PM +0800, Axel Lin wrote:
> 2010/11/9 Jean Delvare <khali@linux-fr.org>:
> > On Tue, 09 Nov 2010 13:18:39 +0800, Axel Lin wrote:
> >> Remove sysfs entries before return -ENODEV.
> >>
> >> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> >> ---
> >> drivers/hwmon/gpio-fan.c | 4 +++-
> >> 1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> >> index aa701a1..d2e66b4 100644
> >> --- a/drivers/hwmon/gpio-fan.c
> >> +++ b/drivers/hwmon/gpio-fan.c
> >> @@ -388,11 +388,13 @@ static int fan_ctrl_init(struct gpio_fan_data *fan_data,
> >> fan_data->speed_index = get_fan_speed_index(fan_data);
> >> if (fan_data->speed_index < 0) {
> >> err = -ENODEV;
> >> - goto err_free_gpio;
> >> + goto err_remove_sysfs;
> >> }
> >>
> >> return 0;
> >>
> >> +err_remove_sysfs:
> >> + sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_ctrl_group);
> >> err_free_gpio:
> >> for (i = i - 1; i >= 0; i--)
> >> gpio_free(ctrl[i]);
> >
> > Oh, and while you're here, fan_alarm_init() needs some love as well.
> > There's a "return 0" in the middle which clearly can't be correct.
> I think this part is correct. see the comment:
> /*
> * If the alarm GPIO don't support interrupts, just leave
> * without initializing the fail notification support.
> */
>
Yes, GPIO interrupts could not be supported. That's not an error case.
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-09 9:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-09 5:18 [PATCH] hwmon: (gpio-fan) Fix fan_ctrl_init error path Axel Lin
2010-11-09 7:59 ` Jean Delvare
2010-11-09 8:01 ` Jean Delvare
2010-11-09 8:22 ` Axel Lin
2010-11-09 9:14 ` Jean Delvare
2010-11-09 9:25 ` Simon Guinot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome