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

[Pali RohÃr]

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.

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.