Re: aibs(4): ASUSTeK AI Booster (ACPI ATK0110) hardware monitor with limit support

[Paul Goyette <paul%whooppee.com@localhost>]

On Thu, 31 Dec 2009, Constantine Aleksandrovich Murenin wrote:

> > In the aibs_refresh() routine, you really shouldn't be setting the
> > sensor state to anything other than SVALID or SINVALID.  (Yes, I
> > know that I did the same thing in acpi_tz, but I need to fix that,
> > too!)
> > You've already exported the limit values, so let envsys's code do
> > the comparisons.
> >
> > <snip>
>
> Yes, that all makes sense in general, however, it doesn't appear to work in 
> practice: 
> * before applying the attached patch (that no longer overwrites states in 
> refresh) only the user-set limits were ignored and never reported; 
> * after applying the attached patch, neither user-set limits nor driver-set 
> limits are reported. 
>
> I'm attaching envstat -x -d aibs0 output, too, from which you can see the 
> following: 
> * there's a fan sensor with a 0 speed which is below warning-min; 
> * there's a temperature sensor which is under the user-set warning-min, too. 
>
> Note that for both of these the states are not set correctly. Perhaps there's 
> some bug in the framework?  I'll try to go further into the code sometime 
> later, probably after the New Year.

Definitely a bug in the framework.  Silly me, I've made the assumption 
that if a driver can provide initial/default limit values, then it also 
handles the comparing of values - even user-supplied values.  This is 
simply not true.

The code in sysmon_envsys_events.c around line 501 currently looks like

        if (lims.sel_flags)
                lims.sel_flags |= PROP_DRIVER_LIMITS;
        else
                sed_t->sed_edata->flags &= ~ENVSYS_FMONLIMITS;

It should probably look more like

        if (lims.sel_flags == 0)
                sed_t->sed_edata->flags &= ~ENVSYS_FMONLIMITS;
        else if (sed_t->sed_sme->sme_set_limits != NULL)
                lims.sel_flags |= PROP_DRIVER_LIMITS;

This requires that the driver _also_ have a routine to process limit 
values when specified by the user.  (If the driver cannot "install" the 
user-provided values, then the *_set_limits() routine needs to clear 
PROP_DRIVER_LIMITS flag - see the sdtemp_set_limits() call in the 
sdtemp(4) driver.)

I'll think this through a little bit more before committing the change.
(I'll attempt to make the preceeding comment more informative.)


BTW, a couple more minor comments on the aibs code.

+       case ENVSYS_STEMP:
+               li->sel_critmax = h * 100 * 1000 + 273150000;
+               li->sel_warnmax = l * 100 * 1000 + 273150000;

Most of the NetBSD drivers that I've looked at #define a mnacro to 
convert device values to micro-Kelvins.  (Also the conversion in 
aibs_referesh() ...

+               li->sel_flags = PROP_CRITMAX | PROP_WARNMAX;
+               break;
+       case ENVSYS_SFANRPM:
+               /* some boards have strange limits for fans */
+               if (l == 0) {
+                       li->sel_warnmin = h;
+                       li->sel_flags = PROP_WARNMIN;
+               } else {
+                       li->sel_warnmin = l;
+                       li->sel_warnmax = h;
+                       li->sel_flags = PROP_WARNMIN | PROP_WARNMAX;
+               }
+               break;

Hmmm, is this really correct?  If (l == 0) then h is the warnmin value, 
but if (l != 0) then h is warnmax and l is warnmin?  That is very 
strange indeed!


Enjoy your holiday, including tonight's "blue moon".  :)


-------------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:      |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul at whooppee.com   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |                          | pgoyette at netbsd.org  |
-------------------------------------------------------------------------