Re: [PATCH 2/2] pstore-ram: Allow optional mapping with pgprot_noncached
On Tue, Sep 16, 2014 at 1:50 PM, Tony Lindgren t...@atomide.com wrote:
From: Tony Lindgren t...@atomide.com
Date: Thu, 11 Sep 2014 15:02:37 -0700
Subject: [PATCH] pstore-ram: Allow optional mapping with pgprot_noncached
On some ARMs the memory can be mapped pgprot_noncached() and still
be working for atomic operations. As pointed out by Colin Cross
ccr...@android.com, in some cases you do want to use
pgprot_noncached() if the SoC supports it to see a debug printk
just before a write hanging the system.
On ARMs, the atomic operations on strongly ordered memory are
implementation defined. So let's provide an optional kernel parameter
for configuring pgprot_noncached(), and use pgprot_writecombine() by
default.
Cc: Arnd Bergmann a...@arndb.de
Cc: Rob Herring robherri...@gmail.com
Cc: Randy Dunlap rdun...@infradead.org
Cc: Anton Vorontsov an...@enomsg.org
Cc: Colin Cross ccr...@android.com
Cc: Kees Cook keesc...@chromium.org
Cc: Olof Johansson o...@lixom.net
Cc: Tony Luck tony.l...@intel.com
Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Tony Lindgren t...@atomide.com
Sorry this got missed! I think rmk's concerns were addressed in this
v2. Tony (Luck), can you take this into your tree?
Acked-by: Kees Cook keesc...@chromium.org
Thanks!
-Kees
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -14,11 +14,19 @@ 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_type to specifiy if the memory type (default is
pgprot_writecombine).
+
+Typically the default value of mem_type=0 should be used as that sets the
pstore
+mapping to pgprot_writecombine. Setting mem_type=1 attempts to use
+pgprot_noncached, which only works on some platforms. This is because pstore
+depends on atomic operations. At least on ARM, pgprot_noncached causes 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 omaps.
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 +63,7 @@ Setting the ramoops parameters can be done in 2 different
manners:
static struct ramoops_platform_data ramoops_data = {
.mem_size = ...,
.mem_address= ...,
+.mem_type = ...,
.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_type;
+module_param(mem_type, uint, 0600);
+MODULE_PARM_DESC(mem_type,
+ set to 1 to try to use unbuffered memory (default 0));
+
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 memtype;
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-memtype);
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-memtype);
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-memtype = pdata-mem_type;
cxt-record_size = pdata-record_size;
cxt-console_size = pdata-console_size;
cxt-ftrace_size = pdata-ftrace_size;
@@ -564,6 +572,7 @@ static void
Re: [PATCH 2/2] pstore-ram: Allow optional mapping with pgprot_noncached
* Kees Cook keesc...@chromium.org [141210 13:18]:
On Tue, Sep 16, 2014 at 1:50 PM, Tony Lindgren t...@atomide.com wrote:
From: Tony Lindgren t...@atomide.com
Date: Thu, 11 Sep 2014 15:02:37 -0700
Subject: [PATCH] pstore-ram: Allow optional mapping with pgprot_noncached
On some ARMs the memory can be mapped pgprot_noncached() and still
be working for atomic operations. As pointed out by Colin Cross
ccr...@android.com, in some cases you do want to use
pgprot_noncached() if the SoC supports it to see a debug printk
just before a write hanging the system.
On ARMs, the atomic operations on strongly ordered memory are
implementation defined. So let's provide an optional kernel parameter
for configuring pgprot_noncached(), and use pgprot_writecombine() by
default.
Cc: Arnd Bergmann a...@arndb.de
Cc: Rob Herring robherri...@gmail.com
Cc: Randy Dunlap rdun...@infradead.org
Cc: Anton Vorontsov an...@enomsg.org
Cc: Colin Cross ccr...@android.com
Cc: Kees Cook keesc...@chromium.org
Cc: Olof Johansson o...@lixom.net
Cc: Tony Luck tony.l...@intel.com
Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Tony Lindgren t...@atomide.com
Sorry this got missed! I think rmk's concerns were addressed in this
v2. Tony (Luck), can you take this into your tree?
Acked-by: Kees Cook keesc...@chromium.org
I take your ack covers patch 1/2 also. The first patch in this
series should be tagged cc stable when committing please.
Regards,
Tony
Thanks!
-Kees
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -14,11 +14,19 @@ 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_type to specifiy if the memory type (default is
pgprot_writecombine).
+
+Typically the default value of mem_type=0 should be used as that sets the
pstore
+mapping to pgprot_writecombine. Setting mem_type=1 attempts to use
+pgprot_noncached, which only works on some platforms. This is because
pstore
+depends on atomic operations. At least on ARM, pgprot_noncached causes 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
omaps.
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 +63,7 @@ Setting the ramoops parameters can be done in 2 different
manners:
static struct ramoops_platform_data ramoops_data = {
.mem_size = ...,
.mem_address= ...,
+.mem_type = ...,
.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_type;
+module_param(mem_type, uint, 0600);
+MODULE_PARM_DESC(mem_type,
+ set to 1 to try to use unbuffered memory (default 0));
+
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 memtype;
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-memtype);
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-memtype);
if (IS_ERR(*prz)) {
int err = PTR_ERR(*prz);
@@ -435,6 +442,7 @@ static int ramoops_probe(struct platform_device
Re: [PATCH 2/2] pstore-ram: Allow optional mapping with pgprot_noncached
* Russell King - ARM Linux li...@arm.linux.org.uk [140912 16:16]:
On Fri, Sep 12, 2014 at 11:32:25AM -0700, Tony Lindgren wrote:
On some ARMs at least the memory can be mapped pgprot_noncached()
and still be working for atomic operations. As pointed out by
Colin Cross ccr...@android.com, in some cases you do want to use
pgprot_noncached() if the SoC supports it to see a debug printk
just before a write hanging the system.
On ARMs, the atomic operations on strongly ordered memory are
implementation defined. So let's provide an optional kernel parameter
for configuring noncached memory, and use pgprot_writecombine() by
default.
Can we clean up this terminology please?
Writecombine memory is not cached - write combine memory bypasses the
caches entirely. What it doesn't bypass are store buffers, which may
combine two writes together. So, calling it cached is misleading.
Good point. I've pretty much s/cached/memtype/ and updated the
description too in the patch below.
Secondly, memory returned by ioremap() is not strongly ordered, it is
device memory. There's three main classifications of memory on ARM:
strongly ordered, device and normal memory. Normal memory has attributes
which define whether it is write combining, or cacheable in some way (and
if so, how it's cacheable.)
Exclusives are always permitted to normal memory. The other two are
implementation defined. While an implementation may offer it on
strongly ordered, that doesn't mean that it also supports it on device
memory.
Lastly, aliased mappings are something that ARM has always strongly
discouraged on ARMv6+ (it was plain down-right unpredictable, but it's
now strongly discouraged) and remapping memory with different memory
type should be avoided.
Yes thanks for the summary again. Maybe let's let this second patch float
on the lists for a while until we have somebody actually test it on
unbuffered memory.
Andrew, if no comments on 1/2 in this series, can you please pick it up
for fixes so we get pstore working?
Here's the updated version of the second patch for reference.
Regards,
Tony
8 -
From: Tony Lindgren t...@atomide.com
Date: Thu, 11 Sep 2014 15:02:37 -0700
Subject: [PATCH] pstore-ram: Allow optional mapping with pgprot_noncached
On some ARMs the memory can be mapped pgprot_noncached() and still
be working for atomic operations. As pointed out by Colin Cross
ccr...@android.com, in some cases you do want to use
pgprot_noncached() if the SoC supports it to see a debug printk
just before a write hanging the system.
On ARMs, the atomic operations on strongly ordered memory are
implementation defined. So let's provide an optional kernel parameter
for configuring pgprot_noncached(), and use pgprot_writecombine() by
default.
Cc: Arnd Bergmann a...@arndb.de
Cc: Rob Herring robherri...@gmail.com
Cc: Randy Dunlap rdun...@infradead.org
Cc: Anton Vorontsov an...@enomsg.org
Cc: Colin Cross ccr...@android.com
Cc: Kees Cook keesc...@chromium.org
Cc: Olof Johansson o...@lixom.net
Cc: Tony Luck tony.l...@intel.com
Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Tony Lindgren t...@atomide.com
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -14,11 +14,19 @@ 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_type to specifiy if the memory type (default is pgprot_writecombine).
+
+Typically the default value of mem_type=0 should be used as that sets the
pstore
+mapping to pgprot_writecombine. Setting mem_type=1 attempts to use
+pgprot_noncached, which only works on some platforms. This is because pstore
+depends on atomic operations. At least on ARM, pgprot_noncached causes 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 omaps.
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 +63,7 @@ Setting the ramoops parameters can be done in 2 different
manners:
static struct ramoops_platform_data ramoops_data = {
.mem_size = ...,
.mem_address= ...,
+.mem_type = ...,
.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_type;
[PATCH 2/2] pstore-ram: Allow optional mapping with pgprot_noncached
On some ARMs at least the memory can be mapped pgprot_noncached()
and still be working for atomic operations. As pointed out by
Colin Cross ccr...@android.com, in some cases you do want to use
pgprot_noncached() if the SoC supports it to see a debug printk
just before a write hanging the system.
On ARMs, the atomic operations on strongly ordered memory are
implementation defined. So let's provide an optional kernel parameter
for configuring noncached memory, and use pgprot_writecombine() by
default.
Cc: Arnd Bergmann a...@arndb.de
Cc: Rob Herring robherri...@gmail.com
Cc: Randy Dunlap rdun...@infradead.org
Cc: Anton Vorontsov an...@enomsg.org
Cc: Colin Cross ccr...@android.com
Cc: Kees Cook keesc...@chromium.org
Cc: Olof Johansson o...@lixom.net
Cc: Tony Luck tony.l...@intel.com
Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Tony Lindgren t...@atomide.com
---
Documentation/ramoops.txt | 12 ++--
fs/pstore/ram.c| 13 +++--
fs/pstore/ram_core.c | 31 ++-
include/linux/pstore_ram.h | 4 +++-
4 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 69b3cac..2dab135 100644
--- 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= ...,
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 3b57443..53ddcb2 100644
--- 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;
@@ -564,6 +572,7 @@ static void ramoops_register_dummy(void)
dummy_data-mem_size = mem_size;
dummy_data-mem_address = mem_address;
+ dummy_data-mem_cached = 1;
dummy_data-record_size = record_size;
dummy_data-console_size
Re: [PATCH 2/2] pstore-ram: Allow optional mapping with pgprot_noncached
On Fri, Sep 12, 2014 at 11:32:25AM -0700, Tony Lindgren wrote: On some ARMs at least the memory can be mapped pgprot_noncached() and still be working for atomic operations. As pointed out by Colin Cross ccr...@android.com, in some cases you do want to use pgprot_noncached() if the SoC supports it to see a debug printk just before a write hanging the system. On ARMs, the atomic operations on strongly ordered memory are implementation defined. So let's provide an optional kernel parameter for configuring noncached memory, and use pgprot_writecombine() by default. Can we clean up this terminology please? Writecombine memory is not cached - write combine memory bypasses the caches entirely. What it doesn't bypass are store buffers, which may combine two writes together. So, calling it cached is misleading. Secondly, memory returned by ioremap() is not strongly ordered, it is device memory. There's three main classifications of memory on ARM: strongly ordered, device and normal memory. Normal memory has attributes which define whether it is write combining, or cacheable in some way (and if so, how it's cacheable.) Exclusives are always permitted to normal memory. The other two are implementation defined. While an implementation may offer it on strongly ordered, that doesn't mean that it also supports it on device memory. Lastly, aliased mappings are something that ARM has always strongly discouraged on ARMv6+ (it was plain down-right unpredictable, but it's now strongly discouraged) and remapping memory with different memory type should be avoided. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
