[Patch-next] Package Level Power limit: fix the generation of package_power_limit_count sysfile.

[Patch-next] Package Level Power limit: fix the generation of package_power_limit_count sysfile.

[Jin Dongming]

 I read the source code of therm_throt.c. Most of checking codes for PLN
and PTS are like following:
if (PLN)
....
if (PTS) {
...
if (PLN)
...
}

But there is not checking code for the generation of package_power_limit_count
sysfile. And the code is like following:
if (PLN)
...
if (PTS)
...
if (PLN)
...

I don't think the sysfile package_power_limit_count should be generated,
when the feature PTS of CPU is not supported. The reasons are listed
as following:
1.The sysfile package_power_limit_count is used for counting the happened
time of PLN event of a package. If PTS is not supported by CPU,
IA32_PACKAGE_THERM_STATUS and IA32_PACKAGE_THERM_INTERRUPT MSRs are not
implemented on the package on which the CPU exists.The PLN interrupt
bit for package in IA32_PACKAGE_THERM_INTERRUPT could not be enabled, too.
Because the PLN event for package will never happen, the sysfile
package_power_limit_count loses the true meaning of its existence.
2.Even if package_power_limit_count sysfile could be generated,
if PTS is not supported by CPU, there is not any other source code
for updating the value of package_power_limit_count. So the sysfile
package_power_limit_count is not useful.

This patch is used for fixing it. But I have not confirmed this patch
because I don't have such machine.

Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
---
arch/x86/kernel/cpu/mcheck/therm_throt.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index d9368ee..169d880 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -216,7 +216,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
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))
+ 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);
@@ -224,6 +224,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
err = sysfs_add_file_to_group(&sys_dev->kobj,
&attr_package_power_limit_count.attr,
thermal_attr_group.name);
+ }

return err;
}
-- 
1.7.1.1

Re: [Patch-next] Package Level Power limit: fix the generation of package_power_limit_count sysfile.

[Fenghua Yu]

On Wed, Aug 25, 2010 at 07:39:00PM -0700, Jin Dongming wrote:
> 
> This patch is used for fixing it. But I have not confirmed this patch
> because I don't have such machine.
> 
> Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> ---
> arch/x86/kernel/cpu/mcheck/therm_throt.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index d9368ee..169d880 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -216,7 +216,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> 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))
> + 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);
> @@ -224,6 +224,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> err = sysfs_add_file_to_group(&sys_dev->kobj,
> &attr_package_power_limit_count.attr,
> thermal_attr_group.name);
> + }
> 
> return err;
> }

The argument is right. A concise message (e.g. missing { and }) would be better.

Format of this patch is broken. It would be applied to kernel tree. Could you
generate a patch with right format?

Thanks.

-Fenghua

[Patch-next] therm_throt.c: fix missing { and }.

[Jin Dongming]

When the feature PTS is not supported by CPU, the sysfile
package_power_limit_count for package should not be generated.

This patch is used for fixing missing { and }. But I have not confirmed
this patch because I don't have such machine.

Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index d9368ee..169d880 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -216,7 +216,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
 		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))
+	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);
@@ -224,6 +224,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
 			err = sysfs_add_file_to_group(&sys_dev->kobj,
 					&attr_package_power_limit_count.attr,
 					thermal_attr_group.name);
+	}
 
 	return err;
 }
-- 
1.7.1.1

Re: [Patch-next] therm_throt.c: fix missing { and }.

[Fenghua Yu]

On Thu, Aug 26, 2010 at 01:29:05AM -0700, Jin Dongming wrote:
> When the feature PTS is not supported by CPU, the sysfile
> package_power_limit_count for package should not be generated.
> 
> This patch is used for fixing missing { and }. But I have not confirmed
> this patch because I don't have such machine.
> 
> Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> ---
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index d9368ee..169d880 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -216,7 +216,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>  		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))
> +	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);
> @@ -224,6 +224,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>  			err = sysfs_add_file_to_group(&sys_dev->kobj,
>  					&attr_package_power_limit_count.attr,
>  					thermal_attr_group.name);
> +	}
>  
>  	return err;
>  }
> -- 
> 1.7.1.1
> 

Reviewed-by: Fenghua Yu <fenghua.yu@initel.com>

Re: [lm-sensors] [Patch-next] therm_throt.c: fix missing { and }.

[Jean Delvare]

On Thu, 26 Aug 2010 17:29:05 +0900, Jin Dongming wrote:
> When the feature PTS is not supported by CPU, the sysfile
> package_power_limit_count for package should not be generated.
> 
> This patch is used for fixing missing { and }. But I have not confirmed
> this patch because I don't have such machine.
> 
> Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> ---
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index d9368ee..169d880 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -216,7 +216,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>  		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))
> +	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);
> @@ -224,6 +224,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>  			err = sysfs_add_file_to_group(&sys_dev->kobj,
>  					&attr_package_power_limit_count.attr,
>  					thermal_attr_group.name);
> +	}
>  
>  	return err;
>  }

This is incomplete. Error handling in this function is totally broken.


-- 
Jean Delvare

Re: [lm-sensors] [Patch-next] therm_throt.c: fix missing { and }.

[Jin Dongming]

(2010/08/27 22:20), Jean Delvare wrote:
> On Thu, 26 Aug 2010 17:29:05 +0900, Jin Dongming wrote:
>> When the feature PTS is not supported by CPU, the sysfile
>> package_power_limit_count for package should not be generated.
>>
>> This patch is used for fixing missing { and }. But I have not confirmed
>> this patch because I don't have such machine.
>>
>> Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
>> ---
>>  arch/x86/kernel/cpu/mcheck/therm_throt.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
>> index d9368ee..169d880 100644
>> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
>> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
>> @@ -216,7 +216,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>>  		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))
>> +	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);
>> @@ -224,6 +224,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>>  			err = sysfs_add_file_to_group(&sys_dev->kobj,
>>  					&attr_package_power_limit_count.attr,
>>  					thermal_attr_group.name);
>> +	}
>>  
>>  	return err;
>>  }
> 
> This is incomplete. Error handling in this function is totally broken.
> 
> 
I am sorry for replying late.

Yes. What you thought is right.
But this patch is only used for fixing the problem that when the feature PTS is not
supported by CPU, the sysfile package_power_limit_count should not be generated.

So the patch for error handling should be provided as another one. I will make
a patch for it .

Thanks.

Best Regards,
Jin Dongming

RE: [lm-sensors] [Patch-next] therm_throt.c: fix missing { and }.

[Yu, Fenghua]

> So the patch for error handling should be provided as another one. I will make
> a patch for it .

These two patches could be sent as one patch since they are in the same small function.

Thanks.

-Fenghua

[Patch-next] Trival fixes in thermal_throttle_add_dev().

[Jin Dongming]

This patch fixed the following two problems.
  1. When the feature PTS is not supported by CPU, the sysfile
     package_power_limit_count for package should not be generated.
  2. No matter whether a sysfile is failed to be created or not,
     the next one will be created.

As the resolving methods:
  1. Add missing { and } after checking PTS feature.
  2. Fix the error handling.

Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
Reviewed-by: Fenghua Yu <fenghua.yu@initel.com>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index d9368ee..79d563a 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
 		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))
+	if (err)
+		goto error;
+
+	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 (err)
+			goto error;
+
 		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);
+		if (err)
+			goto error;
+	}
+
+	return 0;
+error:
+	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
 
 	return err;
 }
-- 
1.7.1.1

Re: [Patch-next] Trival fixes in thermal_throttle_add_dev().

[Fenghua Yu]

On Mon, Aug 30, 2010 at 05:55:48PM -0700, Jin Dongming wrote:
> This patch fixed the following two problems.
>   1. When the feature PTS is not supported by CPU, the sysfile
>      package_power_limit_count for package should not be generated.
>   2. No matter whether a sysfile is failed to be created or not,
>      the next one will be created.
> 
> As the resolving methods:
>   1. Add missing { and } after checking PTS feature.
>   2. Fix the error handling.
> 
> Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> Reviewed-by: Fenghua Yu <fenghua.yu@initel.com>
> ---
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index d9368ee..79d563a 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>  		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))
> +	if (err)
> +		goto error;
> +
> +	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 (err)
> +			goto error;
> +
>  		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);
> +		if (err)
> +			goto error;
> +	}
> +
> +	return 0;
> +error:
> +	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
>  
>  	return err;
>  }

This fix is incorrect. In this patch, a previous error prevents from adding any
further devices. There shouldn't be such dependency among the devices.

Thanks.

-Fenghua

Re: [Patch-next] Trival fixes in thermal_throttle_add_dev().

[Jean Delvare]

Hi Fenghua,

On Mon, 30 Aug 2010 18:02:52 -0700, Fenghua Yu wrote:
> On Mon, Aug 30, 2010 at 05:55:48PM -0700, Jin Dongming wrote:
> > This patch fixed the following two problems.
> >   1. When the feature PTS is not supported by CPU, the sysfile
> >      package_power_limit_count for package should not be generated.
> >   2. No matter whether a sysfile is failed to be created or not,
> >      the next one will be created.
> > 
> > As the resolving methods:
> >   1. Add missing { and } after checking PTS feature.
> >   2. Fix the error handling.
> > 
> > Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> > Reviewed-by: Fenghua Yu <fenghua.yu@initel.com>
> > ---
> >  arch/x86/kernel/cpu/mcheck/therm_throt.c |   15 ++++++++++++++-
> >  1 files changed, 14 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > index d9368ee..79d563a 100644
> > --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > @@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> >  		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))
> > +	if (err)
> > +		goto error;
> > +
> > +	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 (err)
> > +			goto error;
> > +
> >  		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);
> > +		if (err)
> > +			goto error;
> > +	}
> > +
> > +	return 0;
> > +error:
> > +	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
> >  
> >  	return err;
> >  }
> 
> This fix is incorrect. In this patch, a previous error prevents from adding any
> further devices. There shouldn't be such dependency among the devices.

I don't quite follow you. Did you mean to write that a previous error
prevents from creating further _attributes_ for the same device? This
would be true.

Now I don't think this is a problem because 1* such errors should never
happen anyway and 2* if they do happen then further attempts to create
the other attributes are unlikely to succeed either.

-- 
Jean Delvare

Re: [Patch-next] Trival fixes in thermal_throttle_add_dev().

[Fenghua Yu]

On Tue, Aug 31, 2010 at 12:07:25AM -0700, Jean Delvare wrote:
> On Mon, 30 Aug 2010 18:02:52 -0700, Fenghua Yu wrote:
> > On Mon, Aug 30, 2010 at 05:55:48PM -0700, Jin Dongming wrote:
> > > diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > index d9368ee..79d563a 100644
> > > --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > @@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> > >  		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))
> > > +	if (err)
> > > +		goto error;
> > > +
> > > +	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 (err)
> > > +			goto error;
> > > +
> > >  		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);
> > > +		if (err)
> > > +			goto error;
> > > +	}
> > > +
> > > +	return 0;
> > > +error:
> > > +	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
> > >  
> > >  	return err;
> > >  }
> > 
> > This fix is incorrect. In this patch, a previous error prevents from adding any
> > further devices. There shouldn't be such dependency among the devices.
> 
> I don't quite follow you. Did you mean to write that a previous error
> prevents from creating further _attributes_ for the same device? This
> would be true.
> 
> Now I don't think this is a problem because 1* such errors should never
> happen anyway and 2* if they do happen then further attempts to create
> the other attributes are unlikely to succeed either.

I don't think there is dependency among the count files, i.e. failure to add a
count file to the group shouldn't impact other files. Other filles can still be
added to the group. In this case, user application only sees part of count
numbers. And kernel may just warn on the failure instead of providing nothing
to user.

In the patch, any failure to add a file will remove the whole group. This is
too strict. Kernel doesn't provide any count number to user application.

Agree with you that such errors should never happen anyway. So original code
works fine.

If to be picky to the error handling, a patch may just ignore returning errors
from sysfs_add_file_to_group. Or use err |= sysfs_add_file_to_group to collect
all errors and return err but without calling sysfs_remove_group.

Thanks.

-Fenghua

Re: [Patch-next] Trival fixes in thermal_throttle_add_dev().

[Jean Delvare]

On Tue, 31 Aug 2010 10:04:43 -0700, Fenghua Yu wrote:
> On Tue, Aug 31, 2010 at 12:07:25AM -0700, Jean Delvare wrote:
> > On Mon, 30 Aug 2010 18:02:52 -0700, Fenghua Yu wrote:
> > > On Mon, Aug 30, 2010 at 05:55:48PM -0700, Jin Dongming wrote:
> > > > diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > > index d9368ee..79d563a 100644
> > > > --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > > +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> > > > @@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> > > >  		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))
> > > > +	if (err)
> > > > +		goto error;
> > > > +
> > > > +	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 (err)
> > > > +			goto error;
> > > > +
> > > >  		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);
> > > > +		if (err)
> > > > +			goto error;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +error:
> > > > +	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
> > > >  
> > > >  	return err;
> > > >  }
> > > 
> > > This fix is incorrect. In this patch, a previous error prevents from adding any
> > > further devices. There shouldn't be such dependency among the devices.
> > 
> > I don't quite follow you. Did you mean to write that a previous error
> > prevents from creating further _attributes_ for the same device? This
> > would be true.
> > 
> > Now I don't think this is a problem because 1* such errors should never
> > happen anyway and 2* if they do happen then further attempts to create
> > the other attributes are unlikely to succeed either.
> 
> I don't think there is dependency among the count files, i.e. failure to add a
> count file to the group shouldn't impact other files. Other filles can still be
> added to the group. In this case, user application only sees part of count
> numbers. And kernel may just warn on the failure instead of providing nothing
> to user.
> 
> In the patch, any failure to add a file will remove the whole group. This is
> too strict. Kernel doesn't provide any count number to user application.

Oh, my bad. I missed the call to sysfs_remove_group() when reviewing
the code. I agree with you that it shouldn't be added.

> Agree with you that such errors should never happen anyway. So original code
> works fine.

The original code works indeed (except for the missing curly braces)
but is confusing for the reader (which is why I raised the point and we
are discussing it now). If you are voluntarily ignoring errors, you
should add a comment saying so. And it is also a good practice to use a
dummy variable to store the error value you'll ignore, so that the
intent is clear.

> If to be picky to the error handling, a patch may just ignore returning errors
> from sysfs_add_file_to_group.

This is an option, yes. Unfortunately this also means that such errors
won't be even logged, while I think this would be desirable.

> Or use err |= sysfs_add_file_to_group to collect
> all errors and return err but without calling sysfs_remove_group.

Please never use |= on non-bitwise values, it can only lead to bugs and
confusion.

-- 
Jean Delvare

Re: [Patch-next] Trival fixes in thermal_throttle_add_dev().

[Jin Dongming]

Hi Jean and Fenghua,

Sorry for replying late.

(2010/09/01 4:30), Jean Delvare wrote:
> On Tue, 31 Aug 2010 10:04:43 -0700, Fenghua Yu wrote:
>> On Tue, Aug 31, 2010 at 12:07:25AM -0700, Jean Delvare wrote:
>>> On Mon, 30 Aug 2010 18:02:52 -0700, Fenghua Yu wrote:
>>>> On Mon, Aug 30, 2010 at 05:55:48PM -0700, Jin Dongming wrote:
>>>>> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
>>>>> index d9368ee..79d563a 100644
>>>>> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
>>>>> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
>>>>> @@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>>>>>  		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))
>>>>> +	if (err)
>>>>> +		goto error;
>>>>> +
>>>>> +	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 (err)
>>>>> +			goto error;
>>>>> +
>>>>>  		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);
>>>>> +		if (err)
>>>>> +			goto error;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +error:
>>>>> +	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
>>>>>  
>>>>>  	return err;
>>>>>  }
>>>>
>>>> This fix is incorrect. In this patch, a previous error prevents from adding any
>>>> further devices. There shouldn't be such dependency among the devices.
>>>
>>> I don't quite follow you. Did you mean to write that a previous error
>>> prevents from creating further _attributes_ for the same device? This
>>> would be true.
>>>
>>> Now I don't think this is a problem because 1* such errors should never
>>> happen anyway and 2* if they do happen then further attempts to create
>>> the other attributes are unlikely to succeed either.
>>
>> I don't think there is dependency among the count files, i.e. failure to add a
>> count file to the group shouldn't impact other files. Other filles can still be
>> added to the group. In this case, user application only sees part of count
>> numbers. And kernel may just warn on the failure instead of providing nothing
>> to user.
>>
>> In the patch, any failure to add a file will remove the whole group. This is
>> too strict. Kernel doesn't provide any count number to user application.
> 
> Oh, my bad. I missed the call to sysfs_remove_group() when reviewing
> the code. I agree with you that it shouldn't be added.
> 
>> Agree with you that such errors should never happen anyway. So original code
>> works fine.
> 
> The original code works indeed (except for the missing curly braces)
> but is confusing for the reader (which is why I raised the point and we
> are discussing it now). If you are voluntarily ignoring errors, you
> should add a comment saying so. And it is also a good practice to use a
> dummy variable to store the error value you'll ignore, so that the
> intent is clear.
> 
I agree with your option. So I will cancel error handling of this patch and
resend a patch just for missing { and } only.

And I think Fenghua could make a new patch for a comment.

Best Regards,
Jin Dongming

>> If to be picky to the error handling, a patch may just ignore returning errors
>> from sysfs_add_file_to_group.
> 
> This is an option, yes. Unfortunately this also means that such errors
> won't be even logged, while I think this would be desirable.
> 
>> Or use err |= sysfs_add_file_to_group to collect
>> all errors and return err but without calling sysfs_remove_group.
> 
> Please never use |= on non-bitwise values, it can only lead to bugs and
> confusion.
> 

Re: [Patch-next] Trival fixes in thermal_throttle_add_dev().

[Jean Delvare]

Hi Jin

On Tue, 31 Aug 2010 09:55:48 +0900, Jin Dongming wrote:
> This patch fixed the following two problems.
>   1. When the feature PTS is not supported by CPU, the sysfile
>      package_power_limit_count for package should not be generated.
>   2. No matter whether a sysfile is failed to be created or not,
>      the next one will be created.
> 
> As the resolving methods:
>   1. Add missing { and } after checking PTS feature.
>   2. Fix the error handling.
> 
> Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
> Reviewed-by: Fenghua Yu <fenghua.yu@initel.com>
> ---
>  arch/x86/kernel/cpu/mcheck/therm_throt.c |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index d9368ee..79d563a 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -216,14 +216,27 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
>  		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))
> +	if (err)
> +		goto error;
> +
> +	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 (err)
> +			goto error;
> +
>  		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);
> +		if (err)
> +			goto error;

Even though you are lucky that in the end it still works, this check is
misplaced. You should instead have:

		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);
			if (err)
				goto error;
		}


> +	}
> +
> +	return 0;
> +error:
> +	sysfs_remove_group(&sys_dev->kobj, &thermal_attr_group);
>  
>  	return err;
>  }


-- 
Jean Delvare

[tip:x86/urgent] x86, mce, therm_throt.c: Fix missing curly braces in error handling logic

[tip-bot for Jin Dongming]

Commit-ID:  b62be8ea9db4048112219ff6d6ce5f183179d4dc
Gitweb:     http://git.kernel.org/tip/b62be8ea9db4048112219ff6d6ce5f183179d4dc
Author:     Jin Dongming <jin.dongming@np.css.fujitsu.com>
AuthorDate: Thu, 26 Aug 2010 17:29:05 +0900
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 8 Oct 2010 10:29:20 +0200

x86, mce, therm_throt.c: Fix missing curly braces in error handling logic

When the feature PTS is not supported by CPU, the sysfile
package_power_limit_count for package should not be
generated.

This patch is used for fixing missing { and }.

The patch is not complete as there are other error handling
problems in this function - but that can wait until the
merge window.

Signed-off-by: Jin Dongming <jin.dongming@np.css.fujitsu.com>
Reviewed-by: Fenghua Yu <fenghua.yu@initel.com>
Acked-by: Jean Delvare <khali@linux-fr.org>
Cc: Brown Len <len.brown@intel.com>
Cc: Guenter Roeck <guenter.roeck@ericsson.com>
Cc: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
Cc: lm-sensors@lm-sensors.org <lm-sensors@lm-sensors.org>
LKML-Reference: <4C7625D1.4060201@np.css.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index d9368ee..169d880 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -216,7 +216,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
 		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))
+	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);
@@ -224,6 +224,7 @@ static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
 			err = sysfs_add_file_to_group(&sys_dev->kobj,
 					&attr_package_power_limit_count.attr,
 					thermal_attr_group.name);
+	}
 
 	return err;
 }