From owner-freebsd-acpi@FreeBSD.ORG Tue Jul 12 15:20:12 2011 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 3411E1065670 for ; Tue, 12 Jul 2011 15:20:12 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 7406E8FC16 for ; Tue, 12 Jul 2011 15:20:11 +0000 (UTC) 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 SAA15608; Tue, 12 Jul 2011 18:20:07 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <4E1C6626.7030108@FreeBSD.org> Date: Tue, 12 Jul 2011 18:20:06 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:5.0) Gecko/20110705 Thunderbird/5.0 MIME-Version: 1.0 To: Vitaly Magerya References: <4E05EB91.9090509@FreeBSD.org> <4E0862A0.7060405@FreeBSD.org> <4E09BADF.7050702@FreeBSD.org> <4E0A41C8.3000904@FreeBSD.org> <4E0CE158.6030804@FreeBSD.org> <4E0DB58F.4070906@FreeBSD.org> <4E130154.9080809@FreeBSD.org> <4E146FDB.2020602@FreeBSD.org> <4E16F61E.80201@FreeBSD.org> In-Reply-To: X-Enigmail-Version: 1.2pre Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: freebsd-acpi@FreeBSD.org Subject: Re: (Missing) power states of an Atom N455-based netbook 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: Tue, 12 Jul 2011 15:20:12 -0000 on 11/07/2011 19:07 Vitaly Magerya said the following: > Andriy Gapon wrote: >> on 06/07/2011 22:20 Vitaly Magerya said the following: >>> --- acpi_cpu.c.orig 2011-07-05 19:50:31.000000000 +0000 >>> +++ acpi_cpu.c 2011-07-06 17:23:16.000000000 +0000 >>> @@ -1194,7 +1194,7 @@ >>> if (strlen(state) < 2 || toupper(state[0]) != 'C') >>> return (EINVAL); >>> val = (int) strtol(state + 1, NULL, 10) - 1; >>> - if (val < 0 || val > cpu_cx_count - 1) >>> + if (val < 0) >>> return (EINVAL); >>> cpu_cx_lowest = val; >> >> This change is a little bit more intrusive than I would like. >> There are some things about cpu_cx_lowest handling in the code that make me a bit >> unsure if this change is completely safe. > > Can you elaborate? From my reading, the only place cpu_cx_lowest > is used is in acpi_cpu_notify, where sc->cpu_cx_lowest is optionally > increased to min(cpu_cx_lowest, sc->cpu_cx_count - 1), which should > be safe in any situation. This is exactly the place that I am concerned about. Probably my mind is clouded but I can not understand why acpi_cpu_set_cx_lowest() call is under the condition: if (sc->cpu_cx_lowest < cpu_cx_lowest) acpi_cpu_set_cx_lowest(sc, min(cpu_cx_lowest, sc->cpu_cx_count - 1)); If you or someone else can explain to me why that condition is there... > Also note that we currently do not update cpu_cx_lowest immediately > when the number of available states changes (only devd+power_profile > does that). For example, if I kill devd and plug the power cord > off, cpu_cx_lowest remains at C3, even though only C2 is reported. > This is why the above patch shouldn't introduce situations we don't > already have. Yes, quite a good point. Although I am not sure yet if what you describe is not a bug that should be fixed. >> I suspect that there could be problems >> on systems where number Cx states becomes smaller after some events (e.g. AC connection). > > I have such a system; if there are situations you'd like me to test, > I can do so (so far it looks good). I am not exactly sure what to look for... Perhaps something like this (if your system would allow it): - place the system in a state where at least C3 is supported - set global cx_lowest to C3 - set per-CPU cx_lowest for one CPU to C2 - place a system in a state where only C1 is supported This testcase is only tangentially related to your proposed change. It's more about that code that I don't understand. -- Andriy Gapon