Date: Thu, 26 Mar 2009 11:29:50 -0400 From: John Baldwin <jhb@freebsd.org> To: Andriy Gapon <avg@icyb.net.ua> Cc: freebsd-acpi@freebsd.org Subject: Re: kern/108581: [sysctl] sysctl: hw.acpi.cpu.cx_lowest: Invalid argument Message-ID: <200903261129.50419.jhb@freebsd.org> In-Reply-To: <49CB9C1B.4070308@icyb.net.ua> References: <200903200030.n2K0U3iG011009@freefall.freebsd.org> <20090326151035.51e4196e@gluon.draftnet> <49CB9C1B.4070308@icyb.net.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 26 March 2009 11:15:39 am Andriy Gapon wrote:
> on 26/03/2009 17:10 Bruce Cran said the following:
> > On Thu, 26 Mar 2009 17:04:19 +0200
> > Andriy Gapon <avg@icyb.net.ua> wrote:
> >
> >> on 26/03/2009 16:41 Bruce Cran said the following:
> >
> >>> I added lots of printfs to acpi_cpu.c and found that it's occuring
> >>> in acpi_cpu_startup; initializing it to 3 in that function (which I
> >>> wrongly assumed was the lowest Cx state supported in ACPI) fixed
> >>> the problem on my Athlon XP PC because the generic cx handling code
> >>> then lowered cpu_cx_count to 1 based on the fact that
> >>> sc->cpu_cx_count was also 1.
> >>>
> >> Ok, yes, the real issue is in acpi_cpu_generic_cx_probe, namely in
> >> early exits from it. So, sc->cpu_cx_count is always set to at least
> >> 1, but if we exit via one of the returns before the end of function,
> >> then global cpu_cx_count is never updated.
> >>
> >
> > Exactly:
> >
> > acpi: acpi_cpu_startup: initializing cpu_cx_count to 0
> > acpi_cpu_generic_cx_probe
> > if sc->cpu_p_blk_len < 5 [sc->cpu_p_blk_len = 0]
> > acpi: acpi_cpu_startup: cpu 0,cpu_cx_count = 0,sc->cpu_cx_count = 1
> >
> > So we're hitting an early exit in acpi_cpu_generic_cx_probe.
> >
>
> John, what would be a better fix - initialize the global variable to 1 or
use goto
> in acpi_cpu_generic_cx_probe?
> I think the latter is more consistent and obvious, the former is simpler and
> safer, though.
I would rather move the cpu_cx_count code out into acpi_cpu_startup()
completely. It would more closely match the _CST code in that case. It is
also easier to follow the logic this way as well as it is only modified in
one place and not via a secret side-effect.
--- //depot/vendor/freebsd/src/sys/dev/acpica/acpi_cpu.c 2009/02/19 14:40:18
+++ //depot/user/jhb/acpipci/dev/acpica/acpi_cpu.c 2009/03/26 15:28:32
@@ -609,10 +609,6 @@
sc->cpu_cx_count++;
}
}
-
- /* Update the largest cx_count seen so far */
- if (sc->cpu_cx_count > cpu_cx_count)
- cpu_cx_count = sc->cpu_cx_count;
}
/*
@@ -752,6 +748,8 @@
for (i = 0; i < cpu_ndevices; i++) {
sc = device_get_softc(cpu_devices[i]);
acpi_cpu_generic_cx_probe(sc);
+ if (sc->cpu_cx_count > cpu_cx_count)
+ cpu_cx_count = sc->cpu_cx_count;
}
/*
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200903261129.50419.jhb>