[lm-sensors] PATCH: abituguru driver

[lm-sensors] PATCH: abituguru driver

[Hans de Goede]

Hi all,

This one should be it. I've converted the driver to implement the new
sysfs alarm interface and I've created a proper patch against the latest
mm kernel tree, including the nescesarry makefile & kconfig mods,
MAINTAINERS entry, docs etc.

Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>

Regards,

Hans de Goede
-------------- next part --------------
A non-text attachment was scrubbed...
Name: abituguru.patch
Type: text/x-patch
Size: 67018 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060521/719708c8/abituguru-0001.bin

[lm-sensors] PATCH: abituguru driver

[Jean Delvare]

Hi Hans,

> This one should be it. I've converted the driver to implement the new
> sysfs alarm interface and I've created a proper patch against the latest
> mm kernel tree, including the nescesarry makefile & kconfig mods,
> MAINTAINERS entry, docs etc.
> 
> Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>

OK, I applied this. At last! :)

I did many cosmetic changes, fixing coding style and spelling all
around the place.

I didn't do a full review, given that the code was already reviewed
several times. If things need to be fixed, they always can be fixed
afterwards. And this will happen ;) From now on, any further change
should be provided as an incremental patch. The exact patch I applied
can be seen here:

http://khali.linux-fr.org/devel/i2c/linux-2.6/hwmon-abituguru-new-driver.patch

Now, before I push this upwards, there's at least one thing which I
would like you change/fix: the sysfs interface files handling. I see
that you are creating the sysfs file attributes dynamically, and store
the names in a large array in the device private data structure:

> struct abituguru_data {
> (...)
> 	/* The sysfs attr and their names are generated automatically, for bank1
> 	   we cannot use a predefined array because we don't know beforehand
> 	   of a sensor is a volt or a temp sensor, for bank2 and the pwms its
> 	   easier todo things the same way.  For in sensors we have 9 (temp 7)
> 	   sysfs entries per sensor, for bank2 and pwms 6. */
> 	struct sensor_device_attribute_2 sysfs_attr[16 * 9 +
> 		ABIT_UGURU_MAX_BANK2_SENSORS * 6 + ABIT_UGURU_MAX_PWMS * 6];
> 	/* Buffer to store the dynamically generated sysfs names, we need 2120
> 	   bytes for bank1 (worst case scenario of 16 in sensors), 444 bytes
> 	   for fan1-6 and 738 bytes for pwm1-6 + some room to spare in case I
> 	   miscounted :) */
> 	char bank1_names[3400];

Why do you want to do this in the first place? All other hardware
monitoring drivers use static attributes. Within a given driver, not
all supported chips use all attributes, but that's OK. What makes the
uGuru different?

Your dynamic implementation has "overflow" written on it in big block
letters. What will happen when we later need to add files? Or if we
change the names and the new ones are longer? I'd need to be convinced
that everything is safe and under control. "3400" hardcoded doesn't
qualify, let alone the comment itself ;) At the very least, you should
express the array size from clearly defined constants. And you should
use snprintf, not sprintf, when filling the array.

But before that you really need to convince me that this is needed at
all, because the code complexity must be justified by a significant
benefit, else it's just not worth it.

I won't push the driver upwards until this is sorted out.

Other cleanups I'd like you to consider:

* Use the ARRAY_SIZE macro instead of hardcoded values wherever
  possible.
* In abituguru_probe(), refactor the error path so that you have a
  single call to kfree().

Thanks,
-- 
Jean Delvare

[lm-sensors] PATCH: abituguru driver

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
>> This one should be it. I've converted the driver to implement the new
>> sysfs alarm interface and I've created a proper patch against the latest
>> mm kernel tree, including the nescesarry makefile & kconfig mods,
>> MAINTAINERS entry, docs etc.
>>
>> Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>
> 
> OK, I applied this. At last! :)
> 
> I did many cosmetic changes, fixing coding style and spelling all
> around the place.
> 
> I didn't do a full review, given that the code was already reviewed
> several times. If things need to be fixed, they always can be fixed
> afterwards. And this will happen ;) From now on, any further change
> should be provided as an incremental patch. The exact patch I applied
> can be seen here:
> 
> http://khali.linux-fr.org/devel/i2c/linux-2.6/hwmon-abituguru-new-driver.patch
> 
> Now, before I push this upwards, there's at least one thing which I
> would like you change/fix: the sysfs interface files handling. I see
> that you are creating the sysfs file attributes dynamically, and store
> the names in a large array in the device private data structure:
> 
>> struct abituguru_data {
>> (...)
>> 	/* The sysfs attr and their names are generated automatically, for bank1
>> 	   we cannot use a predefined array because we don't know beforehand
>> 	   of a sensor is a volt or a temp sensor, for bank2 and the pwms its
>> 	   easier todo things the same way.  For in sensors we have 9 (temp 7)
>> 	   sysfs entries per sensor, for bank2 and pwms 6. */
>> 	struct sensor_device_attribute_2 sysfs_attr[16 * 9 +
>> 		ABIT_UGURU_MAX_BANK2_SENSORS * 6 + ABIT_UGURU_MAX_PWMS * 6];
>> 	/* Buffer to store the dynamically generated sysfs names, we need 2120
>> 	   bytes for bank1 (worst case scenario of 16 in sensors), 444 bytes
>> 	   for fan1-6 and 738 bytes for pwm1-6 + some room to spare in case I
>> 	   miscounted :) */
>> 	char bank1_names[3400];
> 
> Why do you want to do this in the first place? All other hardware
> monitoring drivers use static attributes. Within a given driver, not
> all supported chips use all attributes, but that's OK. What makes the
> uGuru different?
> 

That in bank1 a certain sensor address within the bank sometimes is a 
temp sensor and sometimes an in sensor, hence the 2 different sysfs attr 
"templates" for bank1 sensors. In my previous version I already had 
dynamicly generated sysfs attr because of this, but I used a large const 
array with all the names to assign the names. However with the new per 
sensor alarm files, this array became huge (9 entries per sensor * 16 
sensors!) and the array became a maintainance problem, having to 
manually type in 144 names in an array is no fun!

This change has actually reduced the size of the driver by 150 lines or 
so compared to the old one file for all alarms version, I don't want to 
know how much the line increase would have been if I had kept static 
sysfs attr arrays.

The uguru is a special beast, currently it has 134 sysfs entries on my 
system, but the theoretical maximum is:
9x16 (16 in sensors) + 6x6 (6 fan sensors) + 6x5 (5 pwm outputs) = 210 
entries.

Since I don't know beforehand wether a sensor is a temp or in sensor, I 
would need an additional 7x16 (16 temp sensors) sysfs attr in the 
driver, for a grand total of 322 sysfs attr! I've always learned that 
computers are good at repetetive jobs and that you shouldn't have 
repetitive patterns in your code but instead let the computer do the 
repetitions which is what I've done.

> Your dynamic implementation has "overflow" written on it in big block
> letters. What will happen when we later need to add files? Or if we
> change the names and the new ones are longer? I'd need to be convinced
> that everything is safe and under control. "3400" hardcoded doesn't
> qualify, let alone the comment itself ;) At the very least, you should
> express the array size from clearly defined constants. And you should
> use snprintf, not sprintf, when filling the array.
> 

Ok, I'll add a bunch of defines, is 1 define for the total names length 
(including \0) per sensor type acceptable so say (numbers are fiction):
ABITUGURU_IN_NAMES_LENGTH   150
ABITUGURU_TEMP_NAMES_LENGTH 120
ABITUGURU_FAN_NAMES_LENGTH   80
ABITUGURU_PWM_NAMES_LENGTH  100

And I'll switch to snprintf

> But before that you really need to convince me that this is needed at
> all, because the code complexity must be justified by a significant
> benefit, else it's just not worth it.
> 

I call not having to have an array with 322 entries + const 
initialization for 322 entries a significant benefit.

If you agree please let me know then I'll add the defines and switch to 
snprintf.

> I won't push the driver upwards until this is sorted out.
> 
> Other cleanups I'd like you to consider:
> 
> * Use the ARRAY_SIZE macro instead of hardcoded values wherever
>   possible.
> * In abituguru_probe(), refactor the error path so that you have a
>   single call to kfree().
> 

I'll take a look at these when I'm fixing the showstopper.

Regards,

Hans

[lm-sensors] PATCH: abituguru driver

[Jean Delvare]

Hi Hans,

> > Now, before I push this upwards, there's at least one thing which I
> > would like you change/fix: the sysfs interface files handling. I see
> > that you are creating the sysfs file attributes dynamically, and store
> > the names in a large array in the device private data structure:
> > 
> > > struct abituguru_data {
> > > (...)
> > > 	/* The sysfs attr and their names are generated automatically, for bank1
> > > 	   we cannot use a predefined array because we don't know beforehand
> > > 	   of a sensor is a volt or a temp sensor, for bank2 and the pwms its
> > > 	   easier todo things the same way.  For in sensors we have 9 (temp 7)
> > > 	   sysfs entries per sensor, for bank2 and pwms 6. */
> > > 	struct sensor_device_attribute_2 sysfs_attr[16 * 9 +
> > > 		ABIT_UGURU_MAX_BANK2_SENSORS * 6 + ABIT_UGURU_MAX_PWMS * 6];
> > > 	/* Buffer to store the dynamically generated sysfs names, we need 2120
> > > 	   bytes for bank1 (worst case scenario of 16 in sensors), 444 bytes
> > > 	   for fan1-6 and 738 bytes for pwm1-6 + some room to spare in case I
> > > 	   miscounted :) */
> > > 	char bank1_names[3400];
> > 
> > Why do you want to do this in the first place? All other hardware
> > monitoring drivers use static attributes. Within a given driver, not
> > all supported chips use all attributes, but that's OK. What makes the
> > uGuru different?
> 
> That in bank1 a certain sensor address within the bank sometimes is a 
> temp sensor and sometimes an in sensor, hence the 2 different sysfs attr 
> "templates" for bank1 sensors. In my previous version I already had 
> dynamicly generated sysfs attr because of this, but I used a large const 
> array with all the names to assign the names. However with the new per 
> sensor alarm files, this array became huge (9 entries per sensor * 16 
> sensors!) and the array became a maintainance problem, having to 
> manually type in 144 names in an array is no fun!

We have other drivers where channels can be of different nature
(vt8231, vt1211), this is nothing new. I didn't suggest that you
manually type the names, you could use macros. Not that I particularly
like macros, but this is an alternative.

> This change has actually reduced the size of the driver by 150 lines or 
> so compared to the old one file for all alarms version, I don't want to 
> know how much the line increase would have been if I had kept static 
> sysfs attr arrays.
> 
> The uguru is a special beast, currently it has 134 sysfs entries on my 
> system, but the theoretical maximum is:
> 9x16 (16 in sensors) + 6x6 (6 fan sensors) + 6x5 (5 pwm outputs) = 210 
> entries.
> 
> Since I don't know beforehand wether a sensor is a temp or in sensor, I 
> would need an additional 7x16 (16 temp sensors) sysfs attr in the 
> driver, for a grand total of 322 sysfs attr! I've always learned that 
> computers are good at repetetive jobs and that you shouldn't have 
> repetitive patterns in your code but instead let the computer do the 
> repetitions which is what I've done.

Cut'n'paste is a third way to let your computer do the repetitive job ;)

> > Your dynamic implementation has "overflow" written on it in big block
> > letters. What will happen when we later need to add files? Or if we
> > change the names and the new ones are longer? I'd need to be convinced
> > that everything is safe and under control. "3400" hardcoded doesn't
> > qualify, let alone the comment itself ;) At the very least, you should
> > express the array size from clearly defined constants. And you should
> > use snprintf, not sprintf, when filling the array.
> 
> Ok, I'll add a bunch of defines, is 1 define for the total names length 
> (including \0) per sensor type acceptable so say (numbers are fiction):
> ABITUGURU_IN_NAMES_LENGTH   150
> ABITUGURU_TEMP_NAMES_LENGTH 120
> ABITUGURU_FAN_NAMES_LENGTH   80
> ABITUGURU_PWM_NAMES_LENGTH  100
> 
> And I'll switch to snprintf

No, actually I think you should define a maximum length per file name
first. Then multiply by the number of files per input, then by the max
number of inputs, and finally add everything to get the total array
size. It doesn't matter if it makes you define a dozen constants, as
long as the final result can be easily verified by the reviewer. Feel
free to make the define names a bit shorter if it makes the expressions
more readable.

Of course, the size parameter you pass to snprintf will need to be
expressed in terms of these constants.

> > But before that you really need to convince me that this is needed at
> > all, because the code complexity must be justified by a significant
> > benefit, else it's just not worth it.
> 
> I call not having to have an array with 322 entries + const 
> initialization for 322 entries a significant benefit.
> 
> If you agree please let me know then I'll add the defines and switch to 
> snprintf.

I would have done it differently, but this is your driver ;) If you can
guarantee that the arrays containing the attributes and names won't
overflow, I'm fine with your approach.

> > I won't push the driver upwards until this is sorted out.
> > 
> > Other cleanups I'd like you to consider:
> > 
> > * Use the ARRAY_SIZE macro instead of hardcoded values wherever
> >   possible.
> > * In abituguru_probe(), refactor the error path so that you have a
> >   single call to kfree().
> 
> I'll take a look at these when I'm fixing the showstopper.

Thanks,
-- 
Jean Delvare

[lm-sensors] PATCH: abituguru driver

[Jean Delvare]

Quoting myself:
> > Ok, I'll add a bunch of defines, is 1 define for the total names length 
> > (including \0) per sensor type acceptable so say (numbers are fiction):
> > ABITUGURU_IN_NAMES_LENGTH   150
> > ABITUGURU_TEMP_NAMES_LENGTH 120
> > ABITUGURU_FAN_NAMES_LENGTH   80
> > ABITUGURU_PWM_NAMES_LENGTH  100
> > 
> > And I'll switch to snprintf
> 
> No, actually I think you should define a maximum length per file name
> first. Then multiply by the number of files per input, then by the max
> number of inputs, and finally add everything to get the total array
> size. It doesn't matter if it makes you define a dozen constants, as
> long as the final result can be easily verified by the reviewer. Feel
> free to make the define names a bit shorter if it makes the expressions
> more readable.

I might sound a bit extreme here. What I really mean is that values
need to be explained, not that you must have one define for each number
taking part in the computation. In other words, I am fine with:
#define UGURU_IN_NAMES_LENGTH	(18 * 3 + 32 * 3 + 14) 
As long as there is some comment explaining what 18, 32 and 14 are.

> Of course, the size parameter you pass to snprintf will need to be
> expressed in terms of these constants.

As for this, the only thing you really need to check against the total
size of the array, for which I guess you'll have a (computed) define.

-- 
Jean Delvare

[lm-sensors] PATCH: abituguru driver

[Hans de Goede]

Jean Delvare wrote:
> Quoting myself:
>>> Ok, I'll add a bunch of defines, is 1 define for the total names length 
>>> (including \0) per sensor type acceptable so say (numbers are fiction):
>>> ABITUGURU_IN_NAMES_LENGTH   150
>>> ABITUGURU_TEMP_NAMES_LENGTH 120
>>> ABITUGURU_FAN_NAMES_LENGTH   80
>>> ABITUGURU_PWM_NAMES_LENGTH  100
>>>
>>> And I'll switch to snprintf
>> No, actually I think you should define a maximum length per file name
>> first. Then multiply by the number of files per input, then by the max
>> number of inputs, and finally add everything to get the total array
>> size. It doesn't matter if it makes you define a dozen constants, as
>> long as the final result can be easily verified by the reviewer. Feel
>> free to make the define names a bit shorter if it makes the expressions
>> more readable.
> 
> I might sound a bit extreme here. What I really mean is that values
> need to be explained, not that you must have one define for each number
> taking part in the computation. In other words, I am fine with:
> #define UGURU_IN_NAMES_LENGTH	(18 * 3 + 32 * 3 + 14) 
> As long as there is some comment explaining what 18, 32 and 14 are.
> 
>> Of course, the size parameter you pass to snprintf will need to be
>> expressed in terms of these constants.
> 
> As for this, the only thing you really need to check against the total
> size of the array, for which I guess you'll have a (computed) define.
> 

This is what I had in mind too, done. The attached patch fixes this as
requested and also fixes the requested should fixes, changes:
 - exactly calculate the sysfs_names array length using macro
 - use snprintf when generating names to double check that the
   sysfs_names array does not overflow.
 - use ARRAY_SIZE and / or defines to determine number of loops in for
   loops instead of using hardcoded values.
 - In abituguru_probe(), refactor the error path leaving a single call
   to kfree

Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>


Regards,

Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-abituguru-fixes.patch
Type: text/x-patch
Size: 15648 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060528/8a747e8c/hwmon-abituguru-fixes-0001.bin

[lm-sensors] PATCH: abituguru driver

[Jean Delvare]

Hi Hans,

> This is what I had in mind too, done. The attached patch fixes this as
> requested and also fixes the requested should fixes, changes:
>  - exactly calculate the sysfs_names array length using macro
>  - use snprintf when generating names to double check that the
>    sysfs_names array does not overflow.
>  - use ARRAY_SIZE and / or defines to determine number of loops in for
>    loops instead of using hardcoded values.
>  - In abituguru_probe(), refactor the error path leaving a single call
>    to kfree

Looks good, thanks! I'll push the abituguru driver upstream in the next
batch of patches (not scheduled yet.)

-- 
Jean Delvare