Subject: Re: CVS commit: src
To: Juan Romero Pardines <>
From: Quentin Garnier <>
List: source-changes
Date: 08/06/2006 19:38:41
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Aug 06, 2006 at 03:37:22PM +0000, Juan Romero Pardines wrote:
> Module Name:	src
> Committed By:	xtraeme
> Date:		Sun Aug  6 15:37:22 UTC 2006
> Modified Files:
> 	src/doc: CHANGES
> 	src/share/man/man4: options.4
> 	src/sys/arch/amd64/amd64: identcpu.c
> 	src/sys/arch/amd64/conf: GENERIC files.amd64
> 	src/sys/arch/amd64/include: cpu.h
> 	src/sys/arch/x86/conf: files.x86
> Added Files:
> 	src/sys/arch/amd64/amd64: powernow_k8.c
> 	src/sys/arch/x86/include: powernow.h
> 	src/sys/arch/x86/x86: powernow_common.c
> Log Message:
> AMD PowerNow!/Cool`n'Quiet driver for NetBSD/amd64,
> adapted from OpenBSD.
> Tested on a few machines:
> Thanks to cube, elad and others for testing and fixes.

Hmm, why do I get thanked?  Sure I made a few comments after a quick
glance at the code but you ignored them apparently.

I expect this to break on EM64T, but I told you that already...

Also, why didn't you try adding support for that on i386, too?  I
haven't seen anything in the spec that restricts Cool'n'Quiet to long
mode.  And I still feel bitmask_snprintf would be more suitable than
your own equivalent.

Some other comments:

 - k8_powernow_setperf could return meaningful errors (e.g., EBUSY in
   the PN8_STA_PENDING(status) case), which would then be returned by
   the sysctl handler

 - you don't free freq_names if k8pnow_current_state is NULL (what if
   the PSB doesn't list the current state?  you know vendors...  I
   certainly expect one to ship a laptop with a broken PSB but a correct
   ACPI table)

 - the spec indicates that FID and VID changes of a dual-core processor
   must me co-ordinated.

Glad I could help.  Please have your code reviewed on tech-kern before
you commit.

Quentin Garnier - -
"When I find the controls, I'll go where I like, I'll know where I want
to be, but maybe for now I'll stay right here on a silent sea."
KT Tunstall, Silent Sea, Eye to the Telescope, 2004.

Content-Type: application/pgp-signature
Content-Disposition: inline

Version: GnuPG v1.4.3 (NetBSD)