pstore/ram_core: Fix hang on ARMs because of pgprot_noncached

PATCH In review Tony Lindgren 2 participants 3 messages Aug 25, 2014 – Aug 26, 2014

Subsystems (2)

Documentation Pstore filesystem

lore

Share & tools

lore

Tony Lindgren

author

2014-08-25 23:14 UTC #0 lore raw

PATCH 1/1 pstore/ram_core: Fix hang on ARMs because of pgprot_noncached

Currently trying to use pstore on ARMs can hang as we're mapping the peristent RAM with pgprot_noncached(). On ARMs, this will actually make the memory strongly ordered, and as the atomic operations pstore uses are implementation defined for strongly ordered memory, they may not work.

An earlier fix was done by Rob Herring <[email protected]> to change the mapping to be pgprot_writecombine() instead as that's often defined as pgprot_noncached() anyways. However, as pointed out by Colin Cross <[email protected]>, in some cases you do want to use pgprot_noncached() on ARMs if the SoC supports it to see a debug printk just before a write hanging the system.

So let's fix this issue by adding a module parameter for mem_cached, and default to having it set. This fixes pstore at least for omaps, and does not change the default for x86. And people debugging system hangs can set the mem_cached to zero as needed.

Cc Rob Herring <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

Documentation/ramoops.txt +10 -2

@@ -14,11 +14,18 @@ survive after a restart.
 
 1. Ramoops concepts
 
-Ramoops uses a predefined memory area to store the dump. The start and size of
-the memory area are set using two variables:
+Ramoops uses a predefined memory area to store the dump. The start and size
+and type of the memory area are set using three variables:
   * "mem_address" for the start
   * "mem_size" for the size. The memory size will be rounded down to a
   power of two.
+  * "mem_cached" to specifiy if the memory is cached or not.
+
+Note disabling "mem_cached" may not be supported on all architectures as
+pstore depends on atomic operations. At least on ARM, clearing "mem_cached"
+will cause the memory to be mapped strongly ordered. And atomic operations
+on strongly ordered memory are implementation defined, and won't work on
+many ARMs such as omap.
 
 The memory area is divided into "record_size" chunks (also rounded down to
 power of two) and each oops/panic writes a "record_size" chunk of

@@ -55,6 +62,7 @@ Setting the ramoops parameters can be done in 2 different manners:
 static struct ramoops_platform_data ramoops_data = {
         .mem_size               = <...>,
         .mem_address            = <...>,
+        .mem_cached             = <...>,
         .record_size            = <...>,
         .dump_oops              = <...>,
         .ecc                    = <...>,

fs/pstore/ram.c +10 -2

@@ -61,6 +61,11 @@ module_param(mem_size, ulong, 0400);
 MODULE_PARM_DESC(mem_size,
 		"size of reserved RAM used to store oops/panic logs");
 
+static unsigned int mem_cached = 1;
+module_param(mem_cached, uint, 0600);
+MODULE_PARM_DESC(mem_cached,
+		"set to 1 to use cached memory, 0 to use uncached (default 1)");
+
 static int dump_oops = 1;
 module_param(dump_oops, int, 0600);
 MODULE_PARM_DESC(dump_oops,

@@ -79,6 +84,7 @@ struct ramoops_context {
 	struct persistent_ram_zone *fprz;
 	phys_addr_t phys_addr;
 	unsigned long size;
+	unsigned int cached;
 	size_t record_size;
 	size_t console_size;
 	size_t ftrace_size;

@@ -358,7 +364,8 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
 		size_t sz = cxt->record_size;
 
 		cxt->przs[i] = persistent_ram_new(*paddr, sz, 0,
-						  &cxt->ecc_info);
+						  &cxt->ecc_info,
+						  cxt->cached);
 		if (IS_ERR(cxt->przs[i])) {
 			err = PTR_ERR(cxt->przs[i]);
 			dev_err(dev, "failed to request mem region (0x%zx at 0x%llx): %d\n",

@@ -388,7 +395,7 @@ static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
 		return -ENOMEM;
 	}
 
-	*prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info);
+	*prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info, cxt->cached);
 	if (IS_ERR(*prz)) {
 		int err = PTR_ERR(*prz);

@@ -435,6 +442,7 @@ static int ramoops_probe(struct platform_device *pdev)
 
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
+	cxt->cached = pdata->mem_cached;
 	cxt->record_size = pdata->record_size;
 	cxt->console_size = pdata->console_size;
 	cxt->ftrace_size = pdata->ftrace_size;

fs/pstore/ram_core.c +22 -9

@@ -380,7 +380,8 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
 	persistent_ram_update_header_ecc(prz);
 }
 
-static void *persistent_ram_vmap(phys_addr_t start, size_t size)
+static void *persistent_ram_vmap(phys_addr_t start, size_t size,
+		unsigned int cached)
 {
 	struct page **pages;
 	phys_addr_t page_start;

@@ -392,7 +393,10 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size)
 	page_start = start - offset_in_page(start);
 	page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
 
-	prot = pgprot_noncached(PAGE_KERNEL);
+	if (cached)
+		prot = pgprot_writecombine(PAGE_KERNEL);
+	else
+		prot = pgprot_noncached(PAGE_KERNEL);
 
 	pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
 	if (!pages) {

@@ -411,8 +415,11 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size)
 	return vaddr;
 }
 
-static void *persistent_ram_iomap(phys_addr_t start, size_t size)
+static void *persistent_ram_iomap(phys_addr_t start, size_t size,
+		unsigned int cached)
 {
+	void *va;
+
 	if (!request_mem_region(start, size, "persistent_ram")) {
 		pr_err("request mem region (0x%llx at 0x%llx) failed\n",
 			(unsigned long long)size, (unsigned long long)start);

@@ -422,19 +429,24 @@ static void *persistent_ram_iomap(phys_addr_t start, size_t size)
 	buffer_start_add = buffer_start_add_locked;
 	buffer_size_add = buffer_size_add_locked;
 
-	return ioremap(start, size);
+	if (cached)
+		va = ioremap(start, size);
+	else
+		va = ioremap_wc(start, size);
+
+	return va;
 }
 
 static int persistent_ram_buffer_map(phys_addr_t start, phys_addr_t size,
-		struct persistent_ram_zone *prz)
+		struct persistent_ram_zone *prz, int cached)
 {
 	prz->paddr = start;
 	prz->size = size;
 
 	if (pfn_valid(start >> PAGE_SHIFT))
-		prz->vaddr = persistent_ram_vmap(start, size);
+		prz->vaddr = persistent_ram_vmap(start, size, cached);
 	else
-		prz->vaddr = persistent_ram_iomap(start, size);
+		prz->vaddr = persistent_ram_iomap(start, size, cached);
 
 	if (!prz->vaddr) {
 		pr_err("%s: Failed to map 0x%llx pages at 0x%llx\n", __func__,

@@ -500,7 +512,8 @@ void persistent_ram_free(struct persistent_ram_zone *prz)
 }
 
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
-			u32 sig, struct persistent_ram_ecc_info *ecc_info)
+			u32 sig, struct persistent_ram_ecc_info *ecc_info,
+			unsigned int cached)
 {
 	struct persistent_ram_zone *prz;
 	int ret = -ENOMEM;

@@ -511,7 +524,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 		goto err;
 	}
 
-	ret = persistent_ram_buffer_map(start, size, prz);
+	ret = persistent_ram_buffer_map(start, size, prz, cached);
 	if (ret)
 		goto err;

include/linux/pstore_ram.h +3 -1

@@ -53,7 +53,8 @@ struct persistent_ram_zone {
 };
 
 struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
-			u32 sig, struct persistent_ram_ecc_info *ecc_info);
+			u32 sig, struct persistent_ram_ecc_info *ecc_info,
+			unsigned int cached);
 void persistent_ram_free(struct persistent_ram_zone *prz);
 void persistent_ram_zap(struct persistent_ram_zone *prz);

@@ -76,6 +77,7 @@ ssize_t persistent_ram_ecc_string(struct persistent_ram_zone *prz,
 struct ramoops_platform_data {
 	unsigned long	mem_size;
 	unsigned long	mem_address;
+	unsigned int	mem_cached;
 	unsigned long	record_size;
 	unsigned long	console_size;
 	unsigned long	ftrace_size;

Arnd Bergmann

2014-08-26 09:16 UTC #1 lore raw

Quoted 8 lines · On Monday 25 August 2014 16:14:50 Tony Lindgren wrote:

fs/pstore/ram_core.c +2 -1

-static void *persistent_ram_vmap(phys_addr_t start, size_t size)
+static void *persistent_ram_vmap(phys_addr_t start, size_t size,
+               unsigned int cached)
 {
        struct page **pages;
        phys_addr_t page_start;

@@ -392,7 +393,10 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size)
        page_start = start - offset_in_page(start);

fs/pstore/ram_core.c static void *persistent_ram_vmap(phys_addr_t start, size_t size) · line 402

Quoted 10 lines

fs/pstore/ram_core.c +4 -1

        page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
 
-       prot = pgprot_noncached(PAGE_KERNEL);
+       if (cached)
+               prot = pgprot_writecombine(PAGE_KERNEL);
+       else
+               prot = pgprot_noncached(PAGE_KERNEL);
 
        pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
        if (!pages) {

If you have a 'struct page', you also have a cacheable mapping in the kernel already, so you are not really supposed to add another uncached mapping. On some architectures (e.g. powerpc) that will cause the CPU to checkstop, on others it is undefined behavior. What is the reason for using an uncached mapping here in the first place?

Quoted 15 lines

fs/pstore/ram_core.c +4 -1

@@ -411,8 +415,11 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size)
        return vaddr;
 }
 
-static void *persistent_ram_iomap(phys_addr_t start, size_t size)
+static void *persistent_ram_iomap(phys_addr_t start, size_t size,
+               unsigned int cached)
 {
+       void *va;
+
        if (!request_mem_region(start, size, "persistent_ram")) {
                pr_err("request mem region (0x%llx at 0x%llx) failed\n",
                        (unsigned long long)size, (unsigned long long)start);

@@ -422,19 +429,24 @@ static void *persistent_ram_iomap(phys_addr_t start, size_t size)
        buffer_start_add = buffer_start_add_locked;

fs/pstore/ram_core.c static void *persistent_ram_iomap(phys_addr_t start, size_t size) · line 438

Quoted 10 lines

fs/pstore/ram_core.c +6 -1

        buffer_size_add = buffer_size_add_locked;
 
-       return ioremap(start, size);
+       if (cached)
+               va = ioremap(start, size);
+       else
+               va = ioremap_wc(start, size);
+
+       return va;
 }

This seems confusing at best, but is probably just wrong: so you use an uncached mapping if someone asks for cached, but use a (more relaxed) write-combining mapping if someone asked for a stricter mapping? It's also the other way round for persistent_ram_vmap above.

According to your description, the intention is to make atomic operations work, however most architectures don't allow atomics on either type of uncached mapping, since atomicity is a feature of the cache coherency fabric.

The only way I see to actually make atomics work here is to use a cached mapping and explicit dcache flushes to actually force the data into persistent storage.

Arnd

Tony Lindgren

author

2014-08-26 14:53 UTC #2 lore raw

Quoted 17 lines · Arnd Bergmann

Quoted 16 lines · On Monday 25 August 2014 16:14:50 Tony Lindgren wrote:

fs/pstore/ram_core.c +6 -2

-static void *persistent_ram_vmap(phys_addr_t start, size_t size)
+static void *persistent_ram_vmap(phys_addr_t start, size_t size,
+               unsigned int cached)
 {
        struct page **pages;
        phys_addr_t page_start;

@@ -392,7 +393,10 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size)
        page_start = start - offset_in_page(start);
        page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
 
-       prot = pgprot_noncached(PAGE_KERNEL);
+       if (cached)
+               prot = pgprot_writecombine(PAGE_KERNEL);
+       else
+               prot = pgprot_noncached(PAGE_KERNEL);

Quoted 7 lines

fs/pstore/ram_core.c static void *persistent_ram_vmap(phys_addr_t start, size_t size) · line 402
Quoted 2 lines
fs/pstore/ram_core.c +0 -0
        pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
        if (!pages) {
If you have a 'struct page', you also have a cacheable mapping in the kernel already, so you are not really supposed to add another uncached mapping. On some architectures (e.g. powerpc) that will cause the CPU to checkstop, on others it is undefined behavior. What is the reason for using an uncached mapping here in the first place?

The reason for using uncached mapping (really strongly ordered for ARM) here is because Colin observed lost debug prints just before hanging register writes because of the write buffer.

But it also sounds like pstore is broken for powerpc in addition to a bunch of ARMs, and possibly other architectures too.

Quoted 17 lines

fs/pstore/ram_core.c +4 -1

@@ -411,8 +415,11 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t size)
        return vaddr;
 }
 
-static void *persistent_ram_iomap(phys_addr_t start, size_t size)
+static void *persistent_ram_iomap(phys_addr_t start, size_t size,
+               unsigned int cached)
 {
+       void *va;
+
        if (!request_mem_region(start, size, "persistent_ram")) {
                pr_err("request mem region (0x%llx at 0x%llx) failed\n",
                        (unsigned long long)size, (unsigned long long)start);

@@ -422,19 +429,24 @@ static void *persistent_ram_iomap(phys_addr_t start, size_t size)
        buffer_start_add = buffer_start_add_locked;
        buffer_size_add = buffer_size_add_locked;

Quoted 13 lines

fs/pstore/ram_core.c static void *persistent_ram_iomap(phys_addr_t start, size_t size) · line 438
Quoted 8 lines
fs/pstore/ram_core.c +6 -1
-       return ioremap(start, size);
+       if (cached)
+               va = ioremap(start, size);
+       else
+               va = ioremap_wc(start, size);
+
+       return va;
 }
This seems confusing at best, but is probably just wrong: so you use an uncached mapping if someone asks for cached, but use a (more relaxed) write-combining mapping if someone asked for a stricter mapping? It's also the other way round for persistent_ram_vmap above.

Indeed, the cached test for the ioremap is the wrong way around here.

Quoted 8 lines

According to your description, the intention is to make atomic operations work, however most architectures don't allow atomics on either type of uncached mapping, since atomicity is a feature of the cache coherency fabric.
The only way I see to actually make atomics work here is to use a cached mapping and explicit dcache flushes to actually force the data into persistent storage.

Right, that's what Rob attempted to patch a while back:

https://lkml.org/lkml/2013/4/9/831

See also the comments from Colin in that thread:

https://lkml.org/lkml/2013/4/9/854

The reason why I added the module_param is because Rob's fix did not go anywhere for over a year now. And adding the module_param seemed to help with the concerns Colin had. Personally I don't need the strongly ordered option though, I just need pgprot_writecombine :)

It's starting to sound that we should first apply Rob's original fix to get pstore working. Then we can figure out how to deal with the unbuffered mapping for architectures and SoCs that support it.

Regards,

Tony