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

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

[Tony Lindgren]

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 <robherring2@gmail.com> 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 <ccross@android.com>, 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 <robherring2@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -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                    = <...>,
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -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@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;
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -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@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;
 
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -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;

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

[Tony Lindgren]

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 <robherring2@gmail.com> 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 <ccross@android.com>, 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 <robherring2@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -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                    = <...>,
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -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;
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -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;
 
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -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;

Re: [PATCH] pstore/ram_core: Fix hang on ARMs because of pgprot_noncached

[Arnd Bergmann]

On Monday 25 August 2014 16:14:50 Tony Lindgren wrote:
> -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) {

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?

> @@ -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@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;
>  }

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

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

[Arnd Bergmann]

On Monday 25 August 2014 16:14:50 Tony Lindgren wrote:
> -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) {

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?

> @@ -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;
>  }

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

Re: [PATCH] pstore/ram_core: Fix hang on ARMs because of pgprot_noncached

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [140826 02:16]:
> On Monday 25 August 2014 16:14:50 Tony Lindgren wrote:
> > -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) {
> 
> 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.
 
> > @@ -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@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;
> >  }
> 
> 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.
 
> 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

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

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [140826 02:16]:
> On Monday 25 August 2014 16:14:50 Tony Lindgren wrote:
> > -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) {
> 
> 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.
 
> > @@ -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;
> >  }
> 
> 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.
 
> 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