Re: [lm-sensors] [RFC] Asus ATK0110 ACPI hwmon driver

[Hans de Goede <hdegoede@redhat.com>]

Luca Tettamanti wrote:

<snip>

>> So summarizing:
>> 1) I like it :)
>> 2) I see some opportunities for removing duplicate code /
>>    simplifying (only one list).
>> 3) Blocker: must protect against userspace abuse causing lots of IO /
>>    an ACPI call storm
>> 4) Blocker: we must make sure we do not have ACPI and native IC drivers
>>    banging IO's of the same IC
> 
> 2 has been taken care of

Nice! One small mistake though, you forgot to do the value *= 100 when its a 
temp sensor in the show_limitX functions.

> will do 3.

Ok.

> About 4: not my fault ;) The IO region
> of the SMBUS controller is reserved by the firmware; once the resource
> enforcing is set to strict the drivers will be exclusive.

Ack, I'm not claiming that this is your fault. Still this is something which 
must be fixed before we can drop the atk0110 driver in to the mainline, esp. as 
it will autoload when build.

> There's is of
> course the possibility of broken firmwares where the IO range is not
> marked as reserved (I wouldn't be suprised...).

Yeah I know, well since this is all Asus only, lets hope the IO reservations 
are done properly on all Asus motherboards. I prefer not to try and think about 
what if there are Asus boards where this does not hold true.

> Here's another iteration of the patch, I'll implement caching ASAP.

Besides the value *= 100 in case of temp sensor for the limits, I have one 
small nitpick left, I think that the atk_create_files and atk_remove_files have 
lost their value as separate functions, and that it would be better to just 
move the code to where they all called all in all this would be a reduction in 
the number of lines, and more importantly if someone tries to follow the code 
flow while reading through the code it is one less function call / jump to 
follow, while figuring out what is exactly happening.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors