Date: Tue, 12 Jul 2011 18:20:06 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: Vitaly Magerya <vmagerya@gmail.com> Cc: freebsd-acpi@FreeBSD.org Subject: Re: (Missing) power states of an Atom N455-based netbook Message-ID: <4E1C6626.7030108@FreeBSD.org> In-Reply-To: <CAL409KzZFHfrEcABv%2B6ZzYUxmLKCbPOR_3sWALrqvmwCD8dxVQ@mail.gmail.com> References: <BANLkTim%2B1UwquMJ32WP8wZBGkYxPv78MLA@mail.gmail.com> <4E05EB91.9090509@FreeBSD.org> <BANLkTi=dyNx=TjyEqYMhSkRtddjVA4nAtw@mail.gmail.com> <4E0862A0.7060405@FreeBSD.org> <BANLkTikmVUtLyANBSqYb%2BL-xkwQ4Zo51Eg@mail.gmail.com> <4E09BADF.7050702@FreeBSD.org> <BANLkTin_%2BZH%2Bo7rdR9ijHMtrXcSdH9ZSdQ@mail.gmail.com> <4E0A41C8.3000904@FreeBSD.org> <BANLkTikwgy%2BKuA5E5zXQKGT-eyV35YAVag@mail.gmail.com> <4E0CE158.6030804@FreeBSD.org> <BANLkTinRY-h%2BkpXtwWJ_L86qVRdoynFSdg@mail.gmail.com> <4E0DB58F.4070906@FreeBSD.org> <CAL409Kw=rUnm9D56KvYiFWiU-bp59KqKnPcUXL38rZsW_Qh8AQ@mail.gmail.com> <4E130154.9080809@FreeBSD.org> <CAL409KyX0jDd9U=7GExvyPR1cDxPwsMHm2b%2B1Tvmijhpg0iWDQ@mail.gmail.com> <4E146FDB.2020602@FreeBSD.org> <CAL409KxL8S%2BzMOmSXghAQHLJRCS19ft%2BLiCKMPPm1dgEyMV%2BSQ@mail.gmail.com> <4E16F61E.80201@FreeBSD.org> <CAL409KzZFHfrEcABv%2B6ZzYUxmLKCbPOR_3sWALrqvmwCD8dxVQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
on 11/07/2011 19:07 Vitaly Magerya said the following:
> Andriy Gapon <avg@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E1C6626.7030108>