2009/12/31 Paul Goyette <[email protected]>: > 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() ...
No, very few do it by #define, actually, since the conversion doesn't occur all that often: http://opengrok.netbsd.org/search?q=273150000&project=%2Fsrc > + 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! Yes, it is. :-) See the comment! >From my original driver for OpenBSD: bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xf0000 (77 entries) bios0: vendor Phoenix Technologies, LTD version "ASUS StrikerExtreme ACPI BIOS Revision 1801" date 10/23/2008 bios0: ASUSTeK Computer INC. StrikerExtreme ... aibs0 at acpi0 aibs0: TSIF 0: CPUT: 0x06030000 CPU Temperature 900 / 1250 0x10001 aibs0: TSIF 1: MBTP: 0x06030001 MB Temperature 450 / 900 0x10001 aibs0: TSIF 2: ST01: 0x06030021 OPT1 Temperature 900 / 1250 0x10001 aibs0: TSIF 3: ST02: 0x06030022 OPT2 Temperature 900 / 1250 0x10001 aibs0: TSIF 4: ST03: 0x06030023 OPT3 Temperature 900 / 1250 0x10001 aibs0: FSIF: misformed package: 7/8, assume 8 aibs0: FSIF 0: CPUF: 0x06040000 CPU FAN Speed 0 / 1800 0x10001 aibs0: FSIF 1: CHAF: 0x06040001 CHASSIS FAN Speed 0 / 1800 0x1 aibs0: FSIF 2: SF01: 0x06040021 OPT1 FAN Speed 0 / 1800 0x1 aibs0: FSIF 3: SF02: 0x06040022 OPT2 FAN Speed 0 / 1800 0x1 aibs0: FSIF 4: SF03: 0x06040023 OPT3 FAN Speed 0 / 1800 0x1 aibs0: FSIF 5: SF04: 0x06040024 OPT4 FAN Speed 0 / 1800 0x1 aibs0: FSIF 6: SF05: 0x06040025 OPT5 FAN Speed 0 / 1800 0x1 aibs0: FSIF 7: TF03: 0x06040033 PWR FAN Speed 0 / 1800 0x1 aibs0: VSIF 0: VCRE: 0x06020000 Vcore Voltage 850 / 1600 0x1 aibs0: VSIF 1: V333: 0x06020001 +3.3 Voltage 3000 / 3600 0x1 aibs0: VSIF 2: V500: 0x06020002 +5.0 Voltage 4500 / 5500 0x1 aibs0: VSIF 3: V120: 0x06020003 +12.0 Voltage 11200 / 13200 0x1 aibs0: VSIF 4: SV01: 0x06020021 1.2VHT Voltage 1000 / 1400 0x1 aibs0: VSIF 5: SV02: 0x06020022 SB CORE Voltage 1300 / 1700 0x1 aibs0: VSIF 6: SV03: 0x06020023 CPU VTT Voltage 1100 / 1400 0x1 aibs0: VSIF 7: SV04: 0x06020024 DDR2 TERM Voltage 500 / 1300 0x1 aibs0: VSIF 8: SV06: 0x06020026 NB CORE Voltage 1100 / 1600 0x1 aibs0: VSIF 9: SV07: 0x06020027 MEMORY Voltage 1600 / 2500 0x1 But then the newer boards have: bios0 at mainbus0: SMBIOS rev. 2.5 @ 0xf0720 (50 entries) bios0: vendor American Megatrends Inc. version "0703" date 02/26/2009 bios0: ASUSTeK Computer INC. P5N64 WS PRO ... aibs0 at acpi0 aibs0: TSIF 0: \\_SB_.PCI0.SBRG.ASOC.CPUT: 0x06030000 CPU Temperature 600 / 950 0x10001 aibs0: TSIF 1: \\_SB_.PCI0.SBRG.ASOC.MBTP: 0x06030001 MB Temperature 450 / 950 0x10001 aibs0: FSIF 0: \\_SB_.PCI0.SBRG.ASOC.CPUF: 0x06040000 CPU FAN Speed 800 / 7200 0x10001 aibs0: FSIF 1: \\_SB_.PCI0.SBRG.ASOC.CHAF: 0x06040001 CHASSIS FAN Speed 1200 / 7200 0x10001 aibs0: FSIF 2: \\_SB_.PCI0.SBRG.ASOC.CHF2: 0x06040002 CHASSIS FAN 2 Speed 800 / 7200 0x10001 aibs0: FSIF 3: \\_SB_.PCI0.SBRG.ASOC.CHF3: 0x06040003 CHASSIS FAN 3 Speed 800 / 7200 0x10001 aibs0: FSIF 4: \\_SB_.PCI0.SBRG.ASOC.PWRF: 0x06040004 POWER FAN Speed 1800 / 7200 0x10001 aibs0: VSIF 0: \\_SB_.PCI0.SBRG.ASOC.CORV: 0x06020000 Vcore Voltage 850 / 1600 0x1 aibs0: VSIF 1: \\_SB_.PCI0.SBRG.ASOC.V3VV: 0x06020001 +3.3 Voltage 2970 / 3630 0x1 aibs0: VSIF 2: \\_SB_.PCI0.SBRG.ASOC.V5VV: 0x06020002 +5 Voltage 4500 / 5500 0x1 aibs0: VSIF 3: \\_SB_.PCI0.SBRG.ASOC.VV12: 0x06020003 +12 Voltage 10200 / 13800 0x1 > Enjoy your holiday, including tonight's "blue moon". :) Thank you, you too! Cheers, Constantine. http://cnst.su/ -- Constantine A. Murenin David R. Cheriton School of Computer Science University of Waterloo 200 University Avenue West Waterloo, Ontario N2L 3G1 Canada
