Re: [PATCH 2/3] i8k: Autodetect maximal fan speed and fan RPM multiplier

From: Guenter Roeck
Date: Wed Dec 10 2014 - 09:08:23 EST

On 12/10/2014 03:50 AM, Pali RohÃr wrote:
On Tuesday 09 December 2014 23:42:08 Guenter Roeck wrote:
On Tue, Dec 09, 2014 at 09:23:22PM +0100, Pali RohÃr wrote:
On Tuesday 09 December 2014 21:20:23 Guenter Roeck wrote:
On Tue, Dec 09, 2014 at 09:07:00PM +0100, Pali RohÃr wrote:
This patch adds new function i8k_get_fan_nominal_rpm()
for doing SMM call which will return nominal fan RPM
for specified fan speed. It returns nominal RPM value
at which fan operate when speed is set. It looks like
RPM value is not accurate, but still provides very
useful information.

First it can be used to validate if certain fan speed
could be accepted by SMM for setting fan speed and we
can use this routine to detect maximal fan speed.

Second it returns RPM value, so we can check if value
looks correct with multiplier 30 or multiplier 1 (until
now only these two multiplier was used). If RPM value
with multiplier 30 is too high, then multiplier 1 is

In case when SMM reports that new function is not
supported we will fallback to old hardcoded values.
Maximal fan speed would be 2 and RPM multiplier 30.

Signed-off-by: Pali RohÃr <pali.rohar@xxxxxxxxx>
I tested this patch only on my Dell Latitude E6440 and
autodetection worked fine Before appying this patch it
should be tested on some other dell machines too but if
machine does not support i8k_get_fan_nominal_rpm()
driver should fallback to old values. So patch should
be without regressions.

It looks like many of your error checks are unnecessary.
Why did you add those ?

Please refrain from adding unnecessary code.


Which error checks do you mean?

There are several you added. I noticed the ones around
'index', which would only be hit on coding errors. At that
point I stopped looking further and did not verify which of
the other added error checks are unnecessary as well.

A quick additional check reveals that the fan variable range
check in i8k_get_fan_nominal_rpm is completely unnecessary -
if the range was wrong, the calling code would fail as well,
since you unconditionally write into an array indexed by the
very same variable. Given the simplicity of the calling code,
it can even be mathematically proven that the error condition
you are checking can never happen.

With that I really stopped looking further.


Should I remove those access out-of-array checks?

If you want me to look into it further. In general, I don't accept
code like this, since it increases kernel size for no good reason.
It also makes it more difficult to find _real_ problems in the code
since it distracts from seeing those.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at