(Missing) power states of an Atom N455-based netbook
Vitaly Magerya
vmagerya at gmail.com
Mon Jul 11 16:07:51 UTC 2011
Andriy Gapon <avg at 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.
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.
More information about the freebsd-acpi
mailing list