From owner-freebsd-acpi@FreeBSD.ORG Thu Mar 26 15:41:19 2009 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 431111065691; Thu, 26 Mar 2009 15:41:19 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 3C80A8FC13; Thu, 26 Mar 2009 15:41:17 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id RAA29798; Thu, 26 Mar 2009 17:41:16 +0200 (EET) (envelope-from avg@icyb.net.ua) Message-ID: <49CBA21B.5050207@icyb.net.ua> Date: Thu, 26 Mar 2009 17:41:15 +0200 From: Andriy Gapon User-Agent: Thunderbird 2.0.0.21 (X11/20090323) MIME-Version: 1.0 To: John Baldwin References: <200903200030.n2K0U3iG011009@freefall.freebsd.org> <20090326151035.51e4196e@gluon.draftnet> <49CB9C1B.4070308@icyb.net.ua> <200903261129.50419.jhb@freebsd.org> In-Reply-To: <200903261129.50419.jhb@freebsd.org> X-Enigmail-Version: 0.95.7 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: freebsd-acpi@freebsd.org Subject: Re: kern/108581: [sysctl] sysctl: hw.acpi.cpu.cx_lowest: Invalid argument X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Mar 2009 15:41:21 -0000 on 26/03/2009 17:29 John Baldwin said the following: > 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 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. Perfect! > --- //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; > } > > /* > > -- Andriy Gapon