Issue with envsys2 (and formerly with apm@acpi)

Hi,

I've spent most of the last 24 hours tracking a bug that
made envstat(8)
show bogus values for the status of my battery.

(Very) long story short, the reason for that behaviour is
that the
sensors from acpi_bat(4) and acpi_acad(4) (and maybe
acpi_tz(4), too)
are queried before interrupts are enabled.

I haven't finished tracking exactly why they need
interrupts, but I
guess the ACPI embedded controller kind of reports back to
the driver
at this point, and when that reporting fails, I end up with
bogus
values.

Initially I only config_interrupts()'d the envsys attachment
for
acpi_bat(4) and it wasn't enough, so I did the same for
acpi_acad(4)
and acpi_tz(4), and now it just works.

I'm wondering if other people are in a similar situation,
and whether
or not we should accept that kind of requirement from an
ACPI
implementation, and of course, if we should comply.

While deferring the attachment to envsys(9) is very easy to
do and not
really meaningless, it obviously break apmacpi, and
I don't see an easy
way of working around that.  Not that I care a lot about
that hack,
though.  Besides, I lost a lot of time because of it.

Thoughts?

-- 
Quentin Garnier - cubecubidou.net - cubeNetBSD.org
"You could have made it, spitting out benchmarks
Owe it to yourself not to fail"
Amplifico, Spitting Out Benchmarks, Hometakes Vol. 2,
2005.

On Sun, 29 Jul 2007 06:00:09 +0200
Quentin Garnier <cubecubidou.net> wrote:

> Hi,
> 
> I've spent most of the last 24 hours tracking a bug
that made envstat
> (8) show bogus values for the status of my battery.
> 
> (Very) long story short, the reason for that behaviour
is that the
> sensors from acpi_bat(4) and acpi_acad(4) (and maybe
acpi_tz(4), too)
> are queried before interrupts are enabled.
> 
> I haven't finished tracking exactly why they need
interrupts, but I
> guess the ACPI embedded controller kind of reports back
to the driver
> at this point, and when that reporting fails, I end up
with bogus
> values.
> 
> Initially I only config_interrupts()'d the envsys
attachment for
> acpi_bat(4) and it wasn't enough, so I did the same for
acpi_acad(4)
> and acpi_tz(4), and now it just works.
> 
> I'm wondering if other people are in a similar
situation, and whether
> or not we should accept that kind of requirement from
an ACPI
> implementation, and of course, if we should comply.
> 
> While deferring the attachment to envsys(9) is very
easy to do and not
> really meaningless, it obviously break apmacpi, and
I don't see an
> easy way of working around that.  Not that I care a lot
about that
> hack, though.  Besides, I lost a lot of time because of
it.
> 
> Thoughts ?

Understood... at least you found what was the problem. The
following
patch avoids calling the sme_gtredata function callback at
register time
(sysmon_envsys_register()). This should fix at least the
problem with
envsys2:

Index: sysmon_envsys.c
============================================================
=======
RCS file: /cvsroot/src/sys/dev/sysmon/sysmon_envsys.c,v
retrieving revision 1.44
diff -b -u -r1.44 sysmon_envsys.c
--- sysmon_envsys.c	27 Jul 2007 11:59:09 -0000	1.44
+++ sysmon_envsys.c	29 Jul 2007 10:01:08 -0000
 -636,7
+636,6 
 	 * values specified by the sysmon envsys driver.
 	 */
 	for (i = 0; i < sme->sme_nsensors; i++) {
-		mutex_enter(&sme_mtx);
 		/*
 		 * XXX:
 		 *
 -651,19
+650,6 
 				sme->sme_sensor_data[0].sensor = 0;
 
 		edata = &sme->sme_sensor_data[i];
-		/* 
-		 * refresh sensor data via sme_gtredata only if the
-		 * flag is not set.
-		 */
-		if ((sme->sme_flags & SME_DISABLE_GTREDATA) == 0)
{
-			if ((*sme->sme_gtredata)(sme, edata)) {
-				DPRINTF(("%s: sme->sme_gtredata[%d]n",
-				    __func__, i));
-				mutex_exit(&sme_mtx);
-				continue;
-			}
-		}
-		mutex_exit(&sme_mtx);
 		sme_make_dictionary(sme, array, edata);
 	}
 

Not sure, what to do about acpi_apm.c...

-- 
Juan Romero Pardines 	- The NetBSD Project
http://plog.xtrarom.org
	- NetBSD/pkgsrc news in Spanish