Skip to contentSkip to conversation

MIPS: Fix memory reservation in bootmem_init for certain non-usermem setups

PATCH Superseded Tobias Wolf 6 participants 9 messages Aug 20, 2018 – Aug 21, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 MIPS: Fix memory reservation in bootmem_init for certain non-usermem setups

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

This patch adds the originally intended prerequisite again.

---
 arch/mips/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/mips/kernel/setup.c +0 -0
index 563188ac6fa2..c3ca55128926 100644
@@ -371,6 +371,8 @@ static unsigned long __init bootmap_bytes(unsigned long 

pages)
return ALIGN(bytes, sizeof(long));
}

+static int usermem __initdata;
+
static void __init bootmem_init(void)
{
unsigned long reserved_end;
@@ -444,7 +446,7 @@ static void __init bootmem_init(void)
/*
* Reserve any memory between the start of RAM and PHYS_OFFSET */
- if (ramstart > PHYS_OFFSET)
+ if (usermem == 0 && ramstart > PHYS_OFFSET)
add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
BOOT_MEM_RESERVED);

@@ -654,8 +656,6 @@ static void __init bootmem_init(void)
* initialization hook for anything else was introduced. */

-static int usermem __initdata;
-
static int __init early_parse_mem(char *p)
{
phys_addr_t start, size;

Signature · 1 line
2.18.0
#1 lore raw

PATCH v2 1/1 MIPS: Fix memory reservation in bootmem_init for certain non-usermem setups

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

This patch adds the originally intended prerequisite again.

v2: Correctly compare that usermem is not null.

---
 arch/mips/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/mips/kernel/setup.c +0 -0
index 563188ac6fa2..c3ca55128926 100644
@@ -371,6 +371,8 @@ static unsigned long __init bootmap_bytes(unsigned long 

pages)
return ALIGN(bytes, sizeof(long));
}

+static int usermem __initdata;
+
static void __init bootmem_init(void)
{
unsigned long reserved_end;
@@ -444,7 +446,7 @@ static void __init bootmem_init(void)
/*
* Reserve any memory between the start of RAM and PHYS_OFFSET */
- if (ramstart > PHYS_OFFSET)
+ if (usermem && ramstart > PHYS_OFFSET)
add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
BOOT_MEM_RESERVED);

@@ -654,8 +656,6 @@ static void __init bootmem_init(void)
* initialization hook for anything else was introduced. */

-static int usermem __initdata;
-
static int __init early_parse_mem(char *p)
{
phys_addr_t start, size;

Acked-by on v2 1/1
#2 lore raw
Quoted 6 lines · Tobias Wolf

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

Looks good to me.

Quoted 3 lines

This patch adds the originally intended prerequisite again.

v2: Correctly compare that usermem is not null.

Could you please CC: [email protected] and add the missing #version

Quoted 1 line

nit:
One of the MIPS maintainer may complain your From: is different from your Signed-off-by:

Quoted 40 lines
---
 arch/mips/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/mips/kernel/setup.c +2 -0
index 563188ac6fa2..c3ca55128926 100644
@@ -371,6 +371,8 @@ static unsigned long __init bootmap_bytes(unsigned long
 pages)
        return ALIGN(bytes, sizeof(long));
 }
 
+static int usermem __initdata;
+
 static void __init bootmem_init(void)
 {
        unsigned long reserved_end;
arch/mips/kernel/setup.c +1 -3
@@ -444,7 +446,7 @@ static void __init bootmem_init(void)
        /*
         * Reserve any memory between the start of RAM and PHYS_OFFSET
         */
-       if (ramstart > PHYS_OFFSET)
+       if (usermem && ramstart > PHYS_OFFSET)
                add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
                                  BOOT_MEM_RESERVED);
 
@@ -654,8 +656,6 @@ static void __init bootmem_init(void)
  * initialization hook for anything else was introduced.
  */
 
-static int usermem __initdata;
-
 static int __init early_parse_mem(char *p)
 {
        phys_addr_t start, size;
#3 lore raw

PATCH v3 1/1 MIPS: Fix memory reservation in bootmem_init for certain non-usermem setups

In reply to Tobias Wolf

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

This patch adds the originally intended prerequisite again.

---
 arch/mips/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/mips/kernel/setup.c +0 -0
index 563188ac6fa2..c3ca55128926 100644

v2: Correctly compare that usermem is not null.
v3: Added/changed position of changelog and fixed sender address.

arch/mips/kernel/setup.c +0 -0
@@ -371,6 +371,8 @@ static unsigned long __init bootmap_bytes(unsigned long 

pages)
return ALIGN(bytes, sizeof(long));
}

+static int usermem __initdata;
+
static void __init bootmem_init(void)
{
unsigned long reserved_end;
@@ -444,7 +446,7 @@ static void __init bootmem_init(void)
/*
* Reserve any memory between the start of RAM and PHYS_OFFSET */
- if (ramstart > PHYS_OFFSET)
+ if (usermem && ramstart > PHYS_OFFSET)
add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
BOOT_MEM_RESERVED);

@@ -654,8 +656,6 @@ static void __init bootmem_init(void)
* initialization hook for anything else was introduced. */

-static int usermem __initdata;
-
static int __init early_parse_mem(char *p)
{
phys_addr_t start, size;

#4 lore raw
Quoted 10 lines · Tobias Wolf

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

This patch adds the originally intended prerequisite again.

You are missing the Fixes: line

Ref:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

You could use 67a3ba25aa95 as an example.

Quoted 43 lines
---
 arch/mips/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/mips/kernel/setup.c +0 -0
index 563188ac6fa2..c3ca55128926 100644

v2: Correctly compare that usermem is not null.
v3: Added/changed position of changelog and fixed sender address.

arch/mips/kernel/setup.c +2 -0
@@ -371,6 +371,8 @@ static unsigned long __init bootmap_bytes(unsigned long
 pages)
        return ALIGN(bytes, sizeof(long));
 }
 
+static int usermem __initdata;
+
 static void __init bootmem_init(void)
 {
        unsigned long reserved_end;
arch/mips/kernel/setup.c +1 -3
@@ -444,7 +446,7 @@ static void __init bootmem_init(void)
        /*
         * Reserve any memory between the start of RAM and PHYS_OFFSET
         */
-       if (ramstart > PHYS_OFFSET)
+       if (usermem && ramstart > PHYS_OFFSET)
                add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
                                  BOOT_MEM_RESERVED);
 
@@ -654,8 +656,6 @@ static void __init bootmem_init(void)
  * initialization hook for anything else was introduced.
  */
 
-static int usermem __initdata;
-
 static int __init early_parse_mem(char *p)
 {
        phys_addr_t start, size;
Signature · 1 line
Mathieu
#5 lore raw
In reply to Tobias Wolf
Quoted 6 lines · Tobias Wolf

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

Quoted 6 lines

This patch adds the originally intended prerequisite again.

---
 arch/mips/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

#6 lore raw
In reply to Tobias Wolf

Hi Tobias,

Quoted 7 lines · Tobias Wolf

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

This patch adds the originally intended prerequisite again.

Could you please give an example of a typical memory layout on your platform, and what mem= arguments you're using? That would help a lot in being able to understand what's going wrong with the existing code.

Quoted 1 line

You'd want to add at least a fixes tag here:

Fixes 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling")

And ideally add the Cc: stable tag indicating the version the fix ought to be backported to, something like:

Cc [email protected] # v4.16+

As an aside, if your system generally has a non-zero ramstart perhaps it would be worth having it select MIPS_AUTO_PFN_OFFSET in Kconfig? (New in the 4.19 merge window.) That would effectively adjust PHYS_OFFSET to match ramstart, and not waste memory on struct page for the region below ramstart.

Thanks,
Paul

#7 lore raw

Dear Paul,

Am Dienstag, 21. August 2018, 01:31:11 CEST schrieb Paul Burton:
[...]

Quoted 4 lines

Could you please give an example of a typical memory layout on your platform, and what mem= arguments you're using? That would help a lot in being able to understand what's going wrong with the existing code.

Here is the boot log without the patch

[    0.000000] SoC Type: Ralink RT2880 id:2 rev:1
[...]
[    0.000000] CPU0 revision is: 0001906c (MIPS 4KEc)
[    0.000000] MIPS: machine is Belkin F5D8235 v1
[    0.000000] Determined physical RAM map:
[    0.000000]  memory: 02000000 @ 08000000 (usable)
[    0.000000] Wasting 1048576 bytes for tracking 32768 unused pages
[...]
[    0.000000] Zone ranges:
[    0.000000]   Normal   [mem 0x0000000000000000-0x0000000009ffffff]
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000000000000-0x0000000009ffffff]
[    0.000000] Initmem setup node 0 [mem 
0x0000000000000000-0x0000000009ffffff]
[...]
[    0.000000] Memory: 23712K/163840K available (3334K kernel code, 162K 
rwdata, 808K rodata, 2984K init, 208K bss, 140128K reserved, 0K cma-reserved)
[...]
[    0.000000] NR_IRQS: 256
[    0.000000] Failed to request intc memory
[    0.000000] Kernel panic - not syncing: Failed to request resources for 
ralink,rt2880-sysc
[    0.000000] Rebooting in 1 seconds..
[    0.000000] Reboot failed -- System halted

... and here with ...

[    0.000000] SoC Type: Ralink RT2880 id:2 rev:1
[...]
[    0.000000] CPU0 revision is: 0001906c (MIPS 4KEc)
[    0.000000] MIPS: machine is Belkin F5D8235 v1
[    0.000000] Determined physical RAM map:
[    0.000000]  memory: 02000000 @ 08000000 (usable)
[    0.000000] Wasting 1048576 bytes for tracking 32768 unused pages
[    0.000000] Initrd not found or empty - disabling initrd
[...]
[    0.000000] Zone ranges:
[    0.000000]   Normal   [mem 0x0000000008000000-0x0000000009ffffff]
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000008000000-0x0000000009ffffff]
[    0.000000] Initmem setup node 0 [mem 
0x0000000008000000-0x0000000009ffffff]
[...]
[    0.000000] Memory: 26700K/32768K available (3334K kernel code, 162K 
rwdata, 808K rodata, 1192K init, 208K bss, 6068K reserved, 0K cma-reserved)
[...]
[    0.000000] NR_IRQS: 256
[    0.000000] CPU Clock: 266MHz

The command line does not contain any custom "mem=" entries, that's why the patch checks "usermem". Please be aware that the platform already has a memory related issue I tried to fix with another patch "mm: Fix alloc_node_mem_map with ARCH_PFN_OFFSET calculation" [1]. So you might be right that this is not the root cause for both issues. I guess the patch might be useful anyway as it originated from the "early_parse_mem" function and memory was not reserved for non usermem setups anyway.

Currently I'm testing this device with an OpenWRT based image and compiled a vanilla kernel separately to validate that this is not triggered outside the mainline one.

Best regards
Tobias

[1] https://patchwork.linux-mips.org/patch/14627/

#8 lore raw

PATCH v4 1/1 MIPS: Fix memory reservation in bootmem_init for certain non-usermem setups

In reply to Tobias Wolf

Commit 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling") introduced a new issue for rt288x where "PHYS_OFFSET" is 0x0 but the calculated "ramstart" is not. As the prerequisite of custom memory map has been removed, this results in the full memory range of 0x0 - 0x8000000 to be marked as reserved for this platform.

This patch adds the originally intended prerequisite again.

Fixes 67a3ba25aa95 ("MIPS: Fix incorrect mem=X@Y handling")
Cc [email protected] # v4.14+
---
 arch/mips/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/mips/kernel/setup.c +0 -0
index 563188ac6fa2..c3ca55128926 100644

v2: Correctly compare that usermem is not null.

v3: Added/changed position of changelog and fixed sender address.

v4: Added "Fixes" and "Cc" tags.

arch/mips/kernel/setup.c +0 -0
@@ -371,6 +371,8 @@ static unsigned long __init bootmap_bytes(unsigned long 

pages)
return ALIGN(bytes, sizeof(long));
}

+static int usermem __initdata;
+
static void __init bootmem_init(void)
{
unsigned long reserved_end;
@@ -444,7 +446,7 @@ static void __init bootmem_init(void)
/*
* Reserve any memory between the start of RAM and PHYS_OFFSET */
- if (ramstart > PHYS_OFFSET)
+ if (usermem && ramstart > PHYS_OFFSET)
add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
BOOT_MEM_RESERVED);

@@ -654,8 +656,6 @@ static void __init bootmem_init(void)
* initialization hook for anything else was introduced. */

-static int usermem __initdata;
-
static int __init early_parse_mem(char *p)
{
phys_addr_t start, size;