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

From: Guenter Roeck <linux@roeck-us.net>
To: "Pali Rohár" <pali.rohar@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jean Delvare <jdelvare@suse.de>,
	Gabriele Mazzotta <gabriele.mzt@gmail.com>,
	Steven Honeyman <stevenhoneyman@gmail.com>,
	Jochen Eisinger <jochen@penguin-breeder.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] i8k: Autodetect maximal fan speed and fan RPM multiplier
Date: Wed, 10 Dec 2014 06:08:11 -0800	[thread overview]
Message-ID: <548853CB.7000704@roeck-us.net> (raw)
In-Reply-To: <201412101250.13891@pali>

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
>>>>> used.
>>>>>
>>>>> 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@gmail.com>
>>>>> ---
>>>>> 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.
>>>>
>>>> Guenter
>>>
>>> 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.
>>
>> Guenter
>>
>
> 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.

Guenter