tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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

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,
You've already exported the limit values, so let envsys's code do
the comparisons.


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;
                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   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at |
| Kernel Developer |                          | pgoyette at  |

Home | Main Index | Thread Index | Old Index