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

From: Andi Kleen (ak_at_suse.de)
Date: 10/04/05

  • Next message: Bill Davidsen: "Re: what's next for the linux kernel?" 
    To: discuss@x86-64.org
Date:	Tue, 4 Oct 2005 19:09:00 +0200

    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  • Next message: Bill Davidsen: "Re: what's next for the linux kernel?"

    Relevant Pages