[PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

Warn when sysfs_add_file_to_group fails.

Also add missing curly braces.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..4c3cd62 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -211,20 +211,26 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
 	if (err)
 		return err;
 
-	if (cpu_has(c, X86_FEATURE_PLN))
+	if (cpu_has(c, X86_FEATURE_PLN)) {
 		err = sysfs_add_file_to_group(&sys_dev->kobj,
 					      &attr_core_power_limit_count.attr,
 					      thermal_attr_group.name);
-	if (cpu_has(c, X86_FEATURE_PTS))
+		WARN_ON(err);
+	}
+	if (cpu_has(c, X86_FEATURE_PTS)) {
 		err = sysfs_add_file_to_group(&sys_dev->kobj,
 					      &attr_package_throttle_count.attr,
 					      thermal_attr_group.name);
-		if (cpu_has(c, X86_FEATURE_PLN))
+		WARN_ON(err);
+		if (cpu_has(c, X86_FEATURE_PLN)) {
 			err = sysfs_add_file_to_group(&sys_dev->kobj,
 					&attr_package_power_limit_count.attr,
 					thermal_attr_group.name);
+			WARN_ON(err);
+		}
+	}
 
-	return err;
+	return 0;
 }
 
 static __cpuinit void thermal_throttle_remove_dev(struct sys_device *sys_dev)
-- 
1.6.0.3

Re: [PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[Jean Delvare]

On Thu,  2 Sep 2010 17:45:26 -0700, Fenghua Yu wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Warn when sysfs_add_file_to_group fails.
> 
> Also add missing curly braces.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Acked-by: Jean Delvare <khali@linux-fr.org>

> ---
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index c2a8b26..4c3cd62 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -211,20 +211,26 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
>  	if (err)
>  		return err;
>  
> -	if (cpu_has(c, X86_FEATURE_PLN))
> +	if (cpu_has(c, X86_FEATURE_PLN)) {
>  		err = sysfs_add_file_to_group(&sys_dev->kobj,
>  					      &attr_core_power_limit_count.attr,
>  					      thermal_attr_group.name);
> -	if (cpu_has(c, X86_FEATURE_PTS))
> +		WARN_ON(err);
> +	}
> +	if (cpu_has(c, X86_FEATURE_PTS)) {
>  		err = sysfs_add_file_to_group(&sys_dev->kobj,
>  					      &attr_package_throttle_count.attr,
>  					      thermal_attr_group.name);
> -		if (cpu_has(c, X86_FEATURE_PLN))
> +		WARN_ON(err);
> +		if (cpu_has(c, X86_FEATURE_PLN)) {
>  			err = sysfs_add_file_to_group(&sys_dev->kobj,
>  					&attr_package_power_limit_count.attr,
>  					thermal_attr_group.name);
> +			WARN_ON(err);
> +		}
> +	}
>  
> -	return err;
> +	return 0;
>  }
>  
>  static __cpuinit void thermal_throttle_remove_dev(struct sys_device *sys_dev)


-- 
Jean Delvare

Re: [PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[Ingo Molnar]

* Fenghua Yu <fenghua.yu@intel.com> wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Warn when sysfs_add_file_to_group fails.
> 
> Also add missing curly braces.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)

> +               WARN_ON(err);

Hm, we tend to use WARN_ON_ONCE(), to avoid repeat spamming of the 
syslog. Also, and perhaps more importantly, WARN_ON() is not a 
particularly smart way to handle errors. How do other drivers handle 
sysfs registration failures?

Also, that's not the only thing the patch does:

> @@ -211,20 +211,26 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)

> -	return err;
> +	return 0;

How is the ignoring of an error and turning it into a WARN_ON() a fix? 
Either it makes no sense to return errors - in which case the whole 
add_dev method needs to be fixed in all drivers - or it makes sense, in 
which case the behavior here is inconsistent.

At minimum more explanation is needed in the changelog.

Thanks,

	Ingo

Re: [PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[Fenghua Yu]

On Fri, Sep 03, 2010 at 02:35:53AM -0700, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > From: Fenghua Yu <fenghua.yu@intel.com>
> > 
> > Warn when sysfs_add_file_to_group fails.
> > 
> > Also add missing curly braces.
> > 
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > ---
> >  arch/x86/kernel/cpu/mcheck/therm_throt.c |   14 ++++++++++----
> >  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> > +               WARN_ON(err);
> 
> Hm, we tend to use WARN_ON_ONCE(), to avoid repeat spamming of the 
> syslog. Also, and perhaps more importantly, WARN_ON() is not a 
> particularly smart way to handle errors. How do other drivers handle 
> sysfs registration failures?

drivers/pci/pcie/aspm.c doesn't collect error from sysfs_add_file_to_group. This
driver is similar to our case because neither of sysfs_add_file_to_group error
is fatal and we can ignore the error and continue to add other files.

Other drivers exit function when sysfs_add_file_to_group fails. This assumes
there is dependency among each devices and an earlier failure prevents from
later sysfs_add_file_to_group. This is different from our case.

> 
> Also, that's not the only thing the patch does:
> 
> > @@ -211,20 +211,26 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
> 
> > -	return err;
> > +	return 0;
> 
> How is the ignoring of an error and turning it into a WARN_ON() a fix? 
> Either it makes no sense to return errors - in which case the whole 
> add_dev method needs to be fixed in all drivers - or it makes sense, in 
> which case the behavior here is inconsistent.
> 
> At minimum more explanation is needed in the changelog.

"return err" always returns the err value from the last sysfs_add_file_to_group
I.e. one earlier err value is not reflected by thermal_throttle_add_dev(). If
you think the original code is ok, I can keep the original error handling code
and just have the missing curly braces fix.

To log an error, the patch just warns on each error but returns 0 because
neither of the error is fatal. If you think this option is ok, I can change
WAR_ON to WARN_ON_ONCE.

Or other options could be:

1. Just calling sysfs_add_file_to_group() without collecting returned error and
return 0 at the end (driver/pci/pcie/aspm.c does like this). The drawback is
there is no error logged if an unlikely errorr occurs. But user can see some
files are missing in sysfs.
2. Or collect errors in err1, err2, etc for each sysfs_add_file_to_group. At
the end, return -ENODEV(??) if any err1, err2, etc is not 0. This option makes
code unreasonable complex to handle unlikely errors.

What do you think?

Thanks.

-Fenghua

Re: [PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[Ingo Molnar]

* Fenghua Yu <fenghua.yu@intel.com> wrote:

> Or other options could be:
> 
> 1. Just calling sysfs_add_file_to_group() without collecting returned error and
> return 0 at the end (driver/pci/pcie/aspm.c does like this). The drawback is
> there is no error logged if an unlikely errorr occurs. But user can see some
> files are missing in sysfs.
>
> 2. Or collect errors in err1, err2, etc for each sysfs_add_file_to_group. At
> the end, return -ENODEV(??) if any err1, err2, etc is not 0. This option makes
> code unreasonable complex to handle unlikely errors.

Well, the usual way to handle errors is to abort the operation when it 
occurs, and return the error code that sysfs_add_file_to_group() gave.

The error is not 'fatal' but missing sysfs files sure are confusing, and 
might break user-land which depends on them. So we should either 
initialize a driver fully - or not intialize it at all.

Now, a sub-case is the question whether to emit something more than the 
return code from sysfs_add_file_to_group(). If it's exceedingly rare 
(and subsequently poorly tested) then adding a WARN_ON_ONCE(ret) is OK - 
but that error code should be returned.

Am i missing any detail?

Thanks,

	Ingo

Re: [lm-sensors] [PATCH] therm_throt.c: Fix error handling in thermal_throttle_add_dev

[Guenter Roeck]

On Sun, Sep 05, 2010 at 08:32:29AM -0400, Ingo Molnar wrote:
> 
> * Fenghua Yu <fenghua.yu@intel.com> wrote:
> 
> > Or other options could be:
> > 
> > 1. Just calling sysfs_add_file_to_group() without collecting returned error and
> > return 0 at the end (driver/pci/pcie/aspm.c does like this). The drawback is
> > there is no error logged if an unlikely errorr occurs. But user can see some
> > files are missing in sysfs.
> >
> > 2. Or collect errors in err1, err2, etc for each sysfs_add_file_to_group. At
> > the end, return -ENODEV(??) if any err1, err2, etc is not 0. This option makes
> > code unreasonable complex to handle unlikely errors.
> 
> Well, the usual way to handle errors is to abort the operation when it 
> occurs, and return the error code that sysfs_add_file_to_group() gave.
> 
> The error is not 'fatal' but missing sysfs files sure are confusing, and 
> might break user-land which depends on them. So we should either 
> initialize a driver fully - or not intialize it at all.
> 
> Now, a sub-case is the question whether to emit something more than the 
> return code from sysfs_add_file_to_group(). If it's exceedingly rare 
> (and subsequently poorly tested) then adding a WARN_ON_ONCE(ret) is OK - 
> but that error code should be returned.
> 
> Am i missing any detail?
> 
Unless I am missing something, the problem here is that if driver initialization
code returns an error, the driver won't be installed. If device entries are retained,
this will ultimately cause the kernel to crash.

>From my perspective, the usual handling is just fine - ie not install the driver
at all. Everything else just causes confusion.

Guenter