Re: SMP support for swsusp (this one actually works for me)

[Patrick Mochel]

> Here's SMP support for swsusp; this one actually works for me [with
> keyboard hack], but I'd like more testers. If it looks okay, I'll
> merge simple pieces with andrew.

This looks cool, but I have some aesthetic nits about it:

> --- linux.orig/drivers/input/serio/i8042.c	2004-06-22 12:53:19.000000000 +0200
> +++ linux/drivers/input/serio/i8042.c	2004-06-22 12:38:06.000000000 +0200
> @@ -841,12 +841,13 @@
>  		return -1;
>  	}
>
> +#if 0
...
> +#endif
> +#if 0
...
> +#endif

If that's dead code, why not remove it?

> +#ifdef CONFIG_SMP
> +extern void smp_freeze(void);
> +extern void smp_restart(void);
> +#else
> +static inline void smp_freeze(void) {}
> +static inline void smp_restart(void) {}
> +#endif

Could you name those something more explicit, like swsusp_smp_freeze(),
etc, so you don't have potential namespace conflicts?

> +#if 1
>  			do_magic(0);
> +#else
> +			device_resume();
> +#endif

What is this? It looks completely gratuitous.

> --- linux.orig/arch/i386/power/cpu.c	2004-06-22 12:53:19.000000000 +0200
> +++ linux/arch/i386/power/cpu.c	2004-06-09 14:38:54.000000000 +0200

> -void save_processor_state(void)
> +void __save_processor_state(struct saved_context *ctxt)

> +void save_processor_state(void)
> +{
> +	__save_processor_state(&saved_context);
>  }

This also looks completely gratuitous and confusing - if you're not doing
anything else but calling the __function, then why even create __function?

>  EXPORT_SYMBOL(save_processor_state);
>  EXPORT_SYMBOL(restore_processor_state);

And, why are they exported in the first place?

> %diffstat
>  Documentation/power/swsusp.txt   |    5 +
>  Documentation/power/video.txt    |    4 +
>  arch/i386/kernel/cpu/mtrr/main.c |    3 +
>  arch/i386/kernel/signal.c        |    3 -
>  arch/i386/power/cpu.c            |  109 +++++++++++++++++++++------------------
>  arch/i386/power/swsusp.S         |    4 -
>  arch/x86_64/kernel/time.c        |   31 +++++++++--
>  drivers/acpi/event.c             |   24 +++-----
>  drivers/acpi/thermal.c           |   15 +++++
>  drivers/input/power.c            |   12 ++++
>  drivers/input/serio/i8042.c      |    6 +-
>  include/linux/suspend.h          |    8 ++
>  kernel/power/Makefile            |    1
>  kernel/power/process.c           |    1
>  kernel/power/smp.c               |   85 ++++++++++++++++++++++++++++++
>  kernel/power/swsusp.c            |   78 +++++++++++++++++++--------
>  kernel/signal.c                  |    6 +-
>  17 files changed, 293 insertions(+), 102 deletions(-)

Were there more files to the patch? Some of the ones listed here were not
in the email?

BTW, nice work.


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