Bug #1359

PowerNow! support

Added by polachok about 5 years ago. Updated almost 5 years ago.

Status:ClosedStart date:
Priority:LowDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Hi.

I couldn't find any support for AMD PowerNow! technology in current DragonFly.
So here's some code attached. I'm not a hacker and don't know anything about
DragonFly internals, but it works fine for me (AMD Sempron Mobile 3300+).
Code is based on OpenBSD/NetBSD and est.c.

powernow.tgz (3.78 KB) polachok, 05/08/2009 04:37 PM

powernow.tgz (5.42 KB) polachok, 05/08/2009 07:47 PM

powernow.c Magnifier (15.5 KB) polachok, 05/10/2009 04:28 PM

powernow.c Magnifier (15.4 KB) polachok, 05/10/2009 07:48 PM

powernow.c Magnifier (15.7 KB) polachok, 05/10/2009 09:38 PM

pnow.tgz (20 KB) polachok, 05/25/2009 03:34 PM

History

#1 Updated by swildner about 5 years ago

Alexander,

could you submit a small manual page too. Else no one will find it later on. :)

Thanks,
Sascha

#2 Updated by polachok about 5 years ago

+ manual page
+ openbsd/netbsd copyright information

#3 Updated by swildner about 5 years ago

The driver panics here (trying to free NULL pointer) when I try to kldunload it
again.

#4 Updated by alexh about 5 years ago

From what I can see the code is not particularly safe:
1) you potentially lose reference to (Line 346) state = kmalloc(sizeof(struct
k8pnow_cpu_state), M_DEVBUF, M_NOWAIT); because you only assign it to
k8pnow_current_state if a certain condition (Line 363) is met. If it isn't, you
would have a memory leak.
2) From what Sascha experiences, if powernow_init() fails to allocate memory
(Line 346) it just returns and everything goes on normally. On unload then the
memory is freed no matter if it really was allocated or not.

PS: I might be wrong on some assumption; I only took a quick look.

#5 Updated by alexh about 5 years ago

By the way, so you don't have to worry about kmalloc returning NULL, you should
use M_WAITOK instead of M_NOWAIT. I don't see any reason to use M_NOWAIT here.

#6 Updated by polachok about 5 years ago

I tried to fix memory leak and null pointer free here.

#7 Updated by alexh about 5 years ago

I didn't explain myself well enough I guess: if you pass M_WAITOK to kmalloc you
won't get NULL back EVER; there is no need to check for it, so you can safely
remove the check after kmalloc.

For the rest at least that part looks fine to me. Thanks for your fixes. Hope
someone can try this? I don't have an AMD CPU.

#8 Updated by swildner about 5 years ago

Alex Hornung (via DragonFly issue tracker) schrieb:
> For the rest at least that part looks fine to me. Thanks for your fixes. Hope
> someone can try this? I don't have an AMD CPU.

I tried to try it but even though my BIOS has a PowerNow option and the
code looks like it applies to both PowerNow! and Cool'n'Quiet, it
detects nothing.

This is an Athlon X2 4850e. Alexander, any ideas if it can be made to
work with my CPU?

Sascha

#9 Updated by polachok about 5 years ago

I made some changes, don't know if it helps, but debugging should be easier.
Sorry, I don't have any experience in writing kernel drivers.

#10 Updated by alexh about 5 years ago

It seems that Sascha's BIOS doesn't provide AMDK7PNOW! string in PSB, so it
probably will never work.
But I have noticed that you currently only support version 0x14 of speedstep.
Looking through NetBSD's and FreeBSD's code it seems like 0x12 version has only
some slight variations and should be trivial to implement, too in
k8pnow_states().
What I'm getting at is that it would be nice if you could add support for that,
too.
As for the rest it seems ok now with that memory leak and null pointer free
fixed, but it would be great if some more people could try it.

#11 Updated by swildner about 5 years ago

Alexander Polakov (via DragonFly issue tracker) schrieb:
> I made some changes, don't know if it helps, but debugging should be easier.
> Sorry, I don't have any experience in writing kernel drivers.

It enters the for loop in k8pnow_states() but the memcmp() never succeeds.

But another thing: As you are the only person so far for whom this patch
works, can you check out if it works OK with sysutils/estd from pkgsrc,
which seems to support the machdep.powernow sysctls.

Regards,
Sascha

#12 Updated by polachok about 5 years ago

>But another thing: As you are the only person so far for whom this patch
>works, can you check out if it works OK with sysutils/estd from pkgsrc,
>which seems to support the machdep.powernow sysctls.
Attached one works nice with estd (just added a sysctl).

>It enters the for loop in k8pnow_states() but the memcmp() never succeeds.
ACPI tables can be used instead, but I have no idea how to implement it at the
moment. Anyway, I'm going to learn more about ACPI to fix acpi_ec and acpi_tz
spamming system console with NO_HARDWARE_RESPONSE. Btw I made
acpica-unix-20090422 port (which doesn't bring any benefit though).

#13 Updated by qhwt+dfly about 5 years ago

On Sun, May 10, 2009 at 10:30:39PM +0200, Sascha Wildner wrote:
> Alexander Polakov (via DragonFly issue tracker) schrieb:
>> I made some changes, don't know if it helps, but debugging should be easier.
>> Sorry, I don't have any experience in writing kernel drivers.
>
> But another thing: As you are the only person so far for whom this patch
> works, can you check out if it works OK with sysutils/estd from pkgsrc,
> which seems to support the machdep.powernow sysctls.

It seems to work for me, although it's not a laptop PC
(Athlon 64 X2 4400+ and a cheap mainboard from ASRock).

$ sysctl machdep.powernow
machdep.powernow.frequency.current: 1800
machdep.powernow.frequency.available: 2200 2000 1800 1000
$ estd -oa
estd: Not detaching from terminal
1000 MHz

and it prints a new frequency as the load average changes (and of course
machdep.powernow.frequency.current changes to that value). Apparently
get_cpuusage() in estd returns half the value of actual load average when
run on SMP, so the low-water and high-water values should be tweaked for it
to work correctly.

Thanks.

#14 Updated by sepherosa about 5 years ago

On Mon, May 11, 2009 at 5:38 AM, Alexander Polakov (via DragonFly
issue tracker) <> wrote:
>
> Alexander Polakov <> added the comment:
>
>>But another thing: As you are the only person so far for whom this patch
>>works, can you check out if it works OK with sysutils/estd from pkgsrc,
>>which seems to support the machdep.powernow sysctls.
> Attached one works nice with estd (just added a sysctl).
>
>>It enters the for loop in k8pnow_states() but the memcmp() never succeeds.
> ACPI tables can be used instead, but I have no idea how to implement it at the
> moment. Anyway, I'm going to learn more about ACPI to fix acpi_ec and acpi_tz
> spamming system console with NO_HARDWARE_RESPONSE. Btw I made
> acpica-unix-20090422 port (which doesn't bring any benefit though).

I am planning to add pstate kernel module, but the first step will be
splitting cstate support from acpi_cpu module; so pstate and cstate
could be children devices of acpi_cpu. Some BIOS only supplies
PCT/PSS/PPC stuffs through ACPI DSDT nowadays.

Best Regards,
sephe

#15 Updated by qhwt+dfly about 5 years ago

On Mon, May 11, 2009 at 11:06:01AM +0900, YONETANI Tomokazu wrote:
> Apparently
> get_cpuusage() in estd returns half the value of actual load average when
> run on SMP, so the low-water and high-water values should be tweaked for it
> to work correctly.

Oh, please disregard this part; I somehow believed that low- and hi- water
referred to load average, but I noticed they aren't after reading the man
page more carefully.

#16 Updated by polachok about 5 years ago

Hi there again.

I tried to add some code for pstates into acpi_cpu and changed powernow driver
to use them. Any review?

#17 Updated by sepherosa about 5 years ago

Grab. I will take care of the patch.

#18 Updated by sepherosa about 5 years ago

Major part of the patch (the frequency change) is committed! Thank you for the
submission. The PSB enumeration part will be committed soon!

#19 Updated by dillon about 5 years ago

:Alexander Polakov <> added the comment:
:
:Hi there again.
:
:I tried to add some code for pstates into acpi_cpu and changed powernow dri=
:ver
:to use them. Any review?

The code looks very clean. It detects "Cool'n Quiet" on my AMD
test box, and it seems to work. I can set the frequency to
1000, 1800, or 2000. However, when I load the module it seems
to default to the lowest frequency.

test28# /tmp/loop2
SMP contention, userland-only loop, duel-forks. Run just one
loop2/2xfork 0.361s 1000000 loops = 0.361uS/loop

test28# kldload powernow
test28# /tmp/loop2
SMP contention, userland-only loop, duel-forks. Run just one
loop2/2xfork 0.725s 1000000 loops = 0.725uS/loop

test28# sysctl machdep.powernow
machdep.powernow.frequency.target: 1000
machdep.powernow.frequency.current: 1000
machdep.powernow.frequency.available: 2000 1800 1000

test28# sysctl machdep.powernow.target=2000
test28# /tmp/loop2
SMP contention, userland-only loop, duel-forks. Run just one
loop2/2xfork 0.361s 1000000 loops = 0.361uS/loop

The only issue I see is that the frequency on module load is
being set to the lowest value. It should probably retain whatever
the existing frequency is (if possible).

-Matt

#20 Updated by polachok about 5 years ago

>The only issue I see is that the frequency on module load is
>being set to the lowest value. It should probably retain whatever
>the existing frequency is (if possible).

Should be fixed now. Try this http://leaf.dragonflybsd.org/~polachok/powernow.tgz

#21 Updated by qhwt+dfly almost 5 years ago

On Fri, Jul 24, 2009 at 11:24:05AM +0000, Alexander Polakov (via DragonFly issue tracker) wrote:
>
> Alexander Polakov <> added the comment:
>
> >The only issue I see is that the frequency on module load is
> >being set to the lowest value. It should probably retain whatever
> >the existing frequency is (if possible).
>
> Should be fixed now. Try this http://leaf.dragonflybsd.org/~polachok/powernow.tgz

Are you planning to commit this module sometime later?

#22 Updated by dillon almost 5 years ago

Ok, Alexander is going to commit the powernow stuff. ACPI has
(more portable) cpu management but there could be situations where
people want cpu management without ACPI, and since powernow is a
separate module it can't hurt anything. So we might as well throw it
in.

-Matt

#23 Updated by alexh almost 5 years ago

commited in 231d986092a296778bf9a368a27fb50a92116a75

Also available in: Atom PDF