| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 10 Dec 2014 06:08:11 -0800 From: Guenter Roeck <linux@...ck-us.net> To: Pali Rohár <pali.rohar@...il.com> CC: Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jean Delvare <jdelvare@...e.de>, Gabriele Mazzotta <gabriele.mzt@...il.com>, Steven Honeyman <stevenhoneyman@...il.com>, Jochen Eisinger <jochen@...guin-breeder.org>, linux-kernel@...r.kernel.org Subject: Re: [PATCH 2/3] i8k: Autodetect maximal fan speed and fan RPM multiplier 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@...il.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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists