Messages in this thread | | | From | Pali Rohár <> | Subject | Re: [PATCH 2/3] i8k: Autodetect maximal fan speed and fan RPM multiplier | Date | Thu, 18 Dec 2014 12:13:35 +0100 |
| |
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@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
Ok, I will rework this patch and drop that first cosmetic.
-- Pali Rohár pali.rohar@gmail.com [unhandled content-type:application/pgp-signature] | |