From owner-freebsd-acpi@FreeBSD.ORG Mon Jul 11 16:07:51 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 B01B61065672; Mon, 11 Jul 2011 16:07:51 +0000 (UTC) (envelope-from vmagerya@gmail.com) Received: from mail-vx0-f182.google.com (mail-vx0-f182.google.com [209.85.220.182]) by mx1.freebsd.org (Postfix) with ESMTP id 4C39C8FC13; Mon, 11 Jul 2011 16:07:50 +0000 (UTC) Received: by vxg33 with SMTP id 33so3839838vxg.13 for ; Mon, 11 Jul 2011 09:07:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=kKsWA+1RvyJSC7hfvgxS3G7wjm4M1uHmC/9IYeE1vrk=; b=cl9b7UZvzRzrU89P3U6cWo35HbSdc62r7KTARVRlDEJEaKSdCMayWCV5j3tuejNm73 m22N/edJTgpKO7fO0n8MVyqMfzecl92vJz2PEiGSYXqVuK0WdzNBjbLYI9I6K9KV3Pfs /7t0U8nvmKRiFQ22xcVoHBHXbIwdUhpk1/1Ds= MIME-Version: 1.0 Received: by 10.52.67.97 with SMTP id m1mr4482583vdt.300.1310400470378; Mon, 11 Jul 2011 09:07:50 -0700 (PDT) Received: by 10.52.188.193 with HTTP; Mon, 11 Jul 2011 09:07:50 -0700 (PDT) In-Reply-To: <4E16F61E.80201@FreeBSD.org> 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> Date: Mon, 11 Jul 2011 19:07:50 +0300 Message-ID: From: Vitaly Magerya To: Andriy Gapon Content-Type: text/plain; charset=UTF-8 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: Mon, 11 Jul 2011 16:07:51 -0000 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. 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. > 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 would prefer other developers to also comment on this. > Maybe it's worth while opening a PR for this proposed change. > >> You can even simplify power_profile with this change: >> >> --- power_profile.orig 2011-07-06 18:39:27.000000000 +0000 >> +++ power_profile 2011-07-06 18:40:20.000000000 +0000 >> @@ -81,8 +81,7 @@ >> # Set the various sysctls based on the profile's values. >> node="hw.acpi.cpu.cx_lowest" >> highest_value="C1" >> -lowest_value="`(sysctl -n dev.cpu.0.cx_supported | \ >> - awk '{ print "C" split($0, a) }' -) 2> /dev/null`" >> +lowest_value="C99" >> eval value=\$${profile}_cx_lowest >> sysctl_set > > C99 looks too scary (and too familiar) :-) > I think that C6 would be sufficient here. I expect people to get confused over such change to power_profile, so I'm not sure if I like it myself.