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

[Guenter Roeck]

On Thu, Dec 18, 2014 at 12:13:35PM +0100, Pali Rohár wrote:
> On Wednesday 10 December 2014 15:08:11 Guenter Roeck wrote:
> > 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@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.
> > >>>> 
> > >>>> 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
> 
> Ok, I will rework this patch and drop that first cosmetic.
> 
Fine, but as mentioned before I still dislike unnecessary
value range checks.

Guenter
--
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/