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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/