Messages in this thread | | | From | Pali Rohár <> | Subject | Re: [PATCH v2 1/2] i8k: Autodetect maximal fan speed and fan RPM multiplier | Date | Sat, 20 Dec 2014 09:57:38 +0100 |
| |
On Friday 19 December 2014 20:28:08 Guenter Roeck wrote: > On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár wrote: > > On Friday 19 December 2014 19:32:37 Guenter Roeck wrote: > > > > -static int i8k_fan_mult; > > > > -static int i8k_pwm_mult; > > > > -static int i8k_fan_max = I8K_FAN_HIGH; > > > > +static int i8k_fan_mult[2]; > > > > +static int i8k_pwm_mult[2]; > > > > +static int i8k_fan_max[2]; > > > > > > The rationale for this change is not explained in the > > > commit log. > > > > > > Do you have any indication that those values would ever be > > > different for the two fans, ie that you actually need > > > arrays here ? > > > > I do not know... But if we decide to use only single value > > for multiplier and max value which fan to use for > > autodetection? > > That does not answer my question. That you can not decide > which fan to use for auto-detection does not mean that the > result of that auto-detection would be different for > different fans. >
Really I do not know if some dell products which have more fans (some Precision models have 2) and each fan is using different multiplier or has different max speed value.
> > > > @@ -271,8 +274,25 @@ static int i8k_get_fan_speed(int > > > > fan) > > > > > > > > { > > > > > > > > struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, }; > > > > > > > > + if (fan < 0 || fan >= ARRAY_SIZE(i8k_fan_mult)) > > > > + return -EINVAL; > > > > + > > > > > > This range check (and probably others) is still > > > unnecessary. > > > > > > Guenter > > > > No, it is necessary. Function i8k_get_fan_speed is called > > from ioctl callback with value which comes from userspace. > > If userspace specify fan out of that array we can get > > kernel panic. > > Yes, but just because you introduced an array for various > variables, and you still have the unnecessary check for other > callers. > > If you want to return -EINVAL for bad ioctl parameters, add a > range check check there. But that would be a separate patch. > > Guenter >
So you want to move that checks into ioctl code and do not do checks in smm functions?
> > > > regs.ebx = fan & 0xff; > > > > > > > > - return i8k_smm(®s) ? : (regs.eax & 0xffff) * > > > > i8k_fan_mult; + return i8k_smm(®s) ? : (regs.eax & > > > > 0xffff) * i8k_fan_mult[fan]; +}
-- Pali Rohár pali.rohar@gmail.com [unhandled content-type:application/pgp-signature] | |