Re: [discuss] [RFC][PATCH][Fix] swsusp: Yet another attempt to fix Bug #4959

[Andi Kleen]

On Saturday 01 October 2005 18:13, Rafael J. Wysocki wrote:

> Your comments, criticisms and (preferably) suggestions will be appreciated.

First always write a full description of the problem and the rationale
of the change and a overview what it changes. Also please add Signed-off-by 
lines.

> +#ifdef CONFIG_SOFTWARE_SUSPEND
> +extern unsigned long resume_table_start, resume_table_end;

These should be all in some include. Adding externs in C files is near
always wrong because it avoids cross file type checking.

Also the convention is to add _pfn to variables that are in PFNs,
otherwise it's full addresses.


> 10:40:03.000000000 +0200 +++
> linux-2.6.14-rc3/arch/x86_64/mm/init.c	2005-10-01 14:31:34.000000000 +0200
> @@ -260,6 +260,9 @@
>  	pmds = (end + PMD_SIZE - 1) >> PMD_SHIFT;
>  	tables = round_up(puds * sizeof(pud_t), PAGE_SIZE) +
>  		 round_up(pmds * sizeof(pmd_t), PAGE_SIZE);
> +#ifdef CONFIG_SOFTWARE_SUSPEND
> +	tables += tables;
> +#endif

This needs a comment. Also I would still prefer if it was allocated
only when suspend is actually attempted.


>  	table_start = find_e820_area(0x8000, __pa_symbol(&_text), tables);
>  	if (table_start == -1UL)
> @@ -272,6 +275,7 @@
>  /* Setup the direct mapping of the physical memory at PAGE_OFFSET.
>     This runs before bootmem is initialized and gets pages directly from
> the physical memory. To access them they are temporarily mapped. */
> +#ifndef CONFIG_SOFTWARE_SUSPEND
>  void __init init_memory_mapping(unsigned long start, unsigned long end)
>  {
>  	unsigned long next;
> @@ -307,6 +311,69 @@
>  	       table_start<<PAGE_SHIFT,
>  	       table_end<<PAGE_SHIFT);
>  }
> +#else
> +
> +extern pgd_t resume_level4_pgt[];

These should be in some include again.

I don't like it that you duplicated the function fully. Is that really 
needed?


-Andi
-
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/