DragonFlyBSD

Alexander,

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

Thanks,
Sascha

+ manual page
+ openbsd/netbsd copyright information

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

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.

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.

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

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.

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

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 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.

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

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).

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.

On Mon, May 11, 2009 at 5:38 AM, Alexander Polakov (via DragonFly
issue tracker) <sinknull@leaf.dragonflybsd.org> wrote:

Alexander Polakov <polachok@gmail.com> 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

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.

Hi there again.

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

Grab. I will take care of the patch.

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

:Alexander Polakov <polachok@gmail.com> 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

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

On Fri, Jul 24, 2009 at 11:24:05AM +0000, Alexander Polakov (via DragonFly issue tracker) wrote:

Alexander Polakov <polachok@gmail.com> 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?

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

commited in 231d986092a296778bf9a368a27fb50a92116a75