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

[Guenter Roeck]

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

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