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

[Pali RohÃr]

On Saturday 20 December 2014 19:38:32 Guenter Roeck wrote:
> > +	if (fan_mult <= 0) {
> 
> Wonder what to do in the < 0 case. It might be better to make
> the variable and the module parameter an unsigned int to
> cover that case, since a negative multiplier doesn't really
> make sense. Same for fan_max.
> 

In some other kernel modules negative value means autodetect. I 
think we can do that too.

> > +		/*
> > +		 * Autodetect fan multiplier for each fan based on
> > nominal rpm +		 * First set default fan multiplier for each
> > fan +		 * And if it reports rpm value too high then set
> > multiplier to 1 +		 *
> > +		 * Try also setting multiplier from current rpm, but 
this
> > will +		 * work only in case when fan is not turned off. It
> > is better +		 * then nothing for machines which does not
> > support nominal rpm +		 * SMM function.
> > +		 */
> > +		for (fan = 0; fan < ARRAY_SIZE(i8k_fan_mult); ++fan) {
> > +			i8k_fan_mult[fan] = I8K_FAN_DEFAULT_MULT;
> > +			if (i8k_get_fan_speed(fan) > I8K_FAN_MAX_RPM) {
> > +				i8k_fan_mult[fan] = 1;
> > +				continue;
> > +			}
> 
> What if i8k_get_fan_speed(fan) returns an error ? Doesn't that
> imply that the fan does not exist, and that you would not
> need the second loop in the first place ?
> 

See my other email "[PATCH] i8k: Add support for fan labels". DOS 
binary check for fan presence by another function. I bet it is 
because fan could have same problem as temperature sensors. If 
fan is on GPU and GPU is turned off reading fan rpm will fail. 
But my laptop (with GPU which can be turned on/off) does not have 
GPU fan so this is just my assumption.

> > +			for (val = 0; val < 256; ++val) {
> > +				ret = i8k_get_fan_nominal_speed(fan, val);
> > +				if (ret > I8K_FAN_MAX_RPM) {
> > +					i8k_fan_mult[fan] = 1;
> > +					break;
> > +				} else if (ret < 0) {
> > +					break;
> > +				}
> 
> Traditionally error checks come first.
> 

Ok.

> > +		for (fan = 0; fan < ARRAY_SIZE(i8k_fan_max); ++fan) {
> > +			for (val = 0; val < 256; ++val) {
> > +				if (i8k_get_fan_nominal_speed(fan, val) < 0)
> > +					break;
> > +			}
> > +			--val; /* set last value which not failed */
> > +			if (val <= 0) /* Must not be 0 (and non negative) 
*/
> > +				val = I8K_FAN_HIGH;
> > +			i8k_fan_max[fan] = val;
> 
> I kind of dislike that you are (at least potentially) looping
> through all the nominal speeds twice; not sure how long that
> will delay the boot process. I don't know if or how that can
> be solved easily, though. Either case, I would prefer
> something like
> 			i8k_fan_max = I8K_FAN_HIGH;
> 			for (val = 0; val < 256; ++val) {
> 				if (i8k_get_fan_nominal_speed(fan, val) < 0)
> 					break;
> 				i8k_fan_max = val;
> 			}
> 
> since that would take care of all the meddling at the end.
> 

Ok, I will change code.

> > +		}
> > +	} else {
> > +		/* Maximal fan speed was specified in module param or 
in
> > dmi */ +		for (fan = 0; fan < ARRAY_SIZE(i8k_fan_max);
> > ++fan) +			i8k_fan_max[fan] = fan_max;
> > 
> >   	}
> 
> Overall I think it would make sense to move the auto-detection
> code to a separate function. This is getting a bit large to
> have it all in a single function.
> 

Now when I reduced fan detection for first available fan code is 
smaller.

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

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