Re: [PATCH] amd76x_pm port to 2.5.70

Dave Jones (davej@codemonkey.org.uk)
Sat, 31 May 2003 22:42:23 +0100


On Sat, May 31, 2003 at 09:33:21PM +0300, Pasi Savolainen wrote:
> + /* Clear W4SG, and set PMIOEN, if using a 765/766 set STPGNT as well.
> + * AMD-766: C3A41; page 59 in AMD-766 doc
> + * AMD-768: DevB:3x41C; page 94 in AMD-768 doc */
> + pci_read_config_byte(pdev_sb, 0x41, &regbyte);
> + if(enable) {
> + regbyte |= ((0 << 0) | (is_766?1:0 << 1) | (1 << 7));
^^^^^^

This looks totally bogus. If you want that bit clearing, you need
to AND its inverse, not OR it. The second statement also looks a
bit funny to the eye.

> + }
> + else {
> + regbyte |= (0 << 7);

Ditto.

> +/*
> + * Idle loop for SMP systems, supports currently only 2 processors.
> + *
> + * Note; for 2.5 folks - not pre-empt safe

Indeed.

> +static void
> +amd76x_smp_idle(void)
> +{
> + /*
> + * Exit idle mode immediately if the CPU does not change.
> + * Usually that means that we have some load on another CPU.
> + */
> +
> + if (prs[0].idle && prs[1].idle && amd76x_pm_cfg.last_pr == smp_processor_id()) {
> + prs[0].idle = 0;
> + prs[1].idle = 0;
> + /* This looks redundent as it was just checked in the if() */
> + /* amd76x_pm_cfg.last_pr = smp_processor_id(); */

except with preemption, it may have changed. This needs to be fixed,
as noted in the function header. Either get_cpu(),put_cpu() or explicit
preempt_enable/disable()

> +
> +/*
> + * Info exported through "/proc/driver/amd76x_pm"

This should really be using sysfs. Adding extra junk to /proc is
somewhat frowned upon, and with the other PM stuff now living in
sysfs, that seems to be the way forward.

> +static void __exit
> +amd76x_pm_cleanup(void)
> +{
> +#ifndef AMD76X_NTH
> + pm_idle = amd76x_pm_cfg.orig_idle;
> + wmb();
> + //__asm__ __volatile__ ("wbinvd;"); // propagate through SMP

Not sure why this was there, is it noted in the 766 spec to flush
the cache ? if so, it needs to be reenabled, and used with on_each_cpu()

> + /*
> + * FIXME: We want to wait until all CPUs have set the new
> + * idle function, otherwise we will oops. This may not be
> + * the right way to do it, but seems to work.
> + *
> + * - Best answer is going to be to ban unload, but when its debugged
> + * --- Alan
> + */
> + schedule();
> + mdelay(1000);

Ummmm, oh-kay. That's a little funky. Look at how the MTRR driver
does its synchronisation, it maybe sucky, but it looks a little
cleaner than this IMO.

> +#define LAZY_IDLE_DELAY 800 /* 0: Best savings, 3000: More responsive */

Sounds like possible candidate for something that should be exposed
via sysfs.

Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/