lkml.org 
[lkml]   [2014]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 1/2] i8k: Autodetect maximal fan speed and fan RPM multiplier
    Date
    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@gmail.com
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2014-12-21 10:41    [W:3.515 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site