[PATCH resend 0/3] pstore/ram: Configurable ECC size

[PATCH resend 0/3] pstore/ram: Configurable ECC size

[Anton Vorontsov]

Hi all,

Just a few patches left from the series that used to add configurable
ECC size for pstore/ram backend. Most patches were merged into -next,
and this is just a resend of the leftovers.

(Note that pstore/trace patches go on top of this series.)

Thanks,

---
 fs/pstore/ram.c            |   14 +++++++-------
 fs/pstore/ram_core.c       |   30 ++++++++++++++----------------
 include/linux/pstore_ram.h |    7 ++-----
 3 files changed, 23 insertions(+), 28 deletions(-)

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH resend 0/3] pstore/ram: Configurable ECC size

[Anton Vorontsov]

On Mon, Jul 09, 2012 at 04:45:59PM -0700, Anton Vorontsov wrote:
> Hi all,
> 
> Just a few patches left from the series that used to add configurable
> ECC size for pstore/ram backend. Most patches were merged into -next,
> and this is just a resend of the leftovers.

Oh, I fogot: actually, it's not a plain resend, I fixed Dan Carpenter's
comment (thanks!): added more text about special ecc=1 case, both
as a module param description and as a comment in the code.

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[PATCH 1/3] pstore/ram_core: Get rid of prz->ecc_symsize and prz->ecc_poly

[Anton Vorontsov]

The struct members were never used anywhere outside of
persistent_ram_init_ecc(), so there's actually no need for them
to be in the struct.

If we ever want to make polynomial or symbol size configurable,
it would make more sense to just pass initialized rs_decoder
to the persistent_ram init functions.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram_core.c       |    7 +++----
 include/linux/pstore_ram.h |    2 --
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index a5a7b13..3f4d6e6 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -177,14 +177,14 @@ static int persistent_ram_init_ecc(struct persistent_ram_zone *prz)
 	struct persistent_ram_buffer *buffer = prz->buffer;
 	int ecc_blocks;
 	size_t ecc_total;
+	int ecc_symsize = 8;
+	int ecc_poly = 0x11d;
 
 	if (!prz->ecc)
 		return 0;
 
 	prz->ecc_block_size = 128;
 	prz->ecc_size = 16;
-	prz->ecc_symsize = 8;
-	prz->ecc_poly = 0x11d;
 
 	ecc_blocks = DIV_ROUND_UP(prz->buffer_size, prz->ecc_block_size);
 	ecc_total = (ecc_blocks + 1) * prz->ecc_size;
@@ -202,8 +202,7 @@ static int persistent_ram_init_ecc(struct persistent_ram_zone *prz)
 	 * first consecutive root is 0
 	 * primitive element to generate roots = 1
 	 */
-	prz->rs_decoder = init_rs(prz->ecc_symsize, prz->ecc_poly, 0, 1,
-				  prz->ecc_size);
+	prz->rs_decoder = init_rs(ecc_symsize, ecc_poly, 0, 1, prz->ecc_size);
 	if (prz->rs_decoder == NULL) {
 		pr_info("persistent_ram: init_rs failed\n");
 		return -EINVAL;
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index e681af9..a0975c0 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -41,8 +41,6 @@ struct persistent_ram_zone {
 	int bad_blocks;
 	int ecc_block_size;
 	int ecc_size;
-	int ecc_symsize;
-	int ecc_poly;
 
 	char *old_log;
 	size_t old_log_size;
-- 
1.7.10.4

[PATCH 2/3] pstore/ram: Make ECC size configurable

[Anton Vorontsov]

This is now pretty straightforward: instead of using bool, just pass
an integer. For backwards compatibility ramoops.ecc=1 means 16 bytes
ECC (using 1 byte for ECC isn't much of use anyway).

Suggested-by: Arve Hjønnevåg <arve@android.com>
Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c            |   22 ++++++++++++++--------
 fs/pstore/ram_core.c       |   15 ++++++++-------
 include/linux/pstore_ram.h |    4 ++--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 58b93fb..b39aebb 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -63,7 +63,9 @@ MODULE_PARM_DESC(dump_oops,
 static int ramoops_ecc;
 module_param_named(ecc, ramoops_ecc, int, 0600);
 MODULE_PARM_DESC(ramoops_ecc,
-		"set to 1 to enable ECC support");
+		"if non-zero, the option enables ECC support and specifies "
+		"ECC buffer size in bytes (1 is a special value, means 16 "
+		"bytes ECC)");
 
 struct ramoops_context {
 	struct persistent_ram_zone **przs;
@@ -73,7 +75,7 @@ struct ramoops_context {
 	size_t record_size;
 	size_t console_size;
 	int dump_oops;
-	bool ecc;
+	int ecc_size;
 	unsigned int max_dump_cnt;
 	unsigned int dump_write_cnt;
 	unsigned int dump_read_cnt;
@@ -288,7 +290,7 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
 	for (i = 0; i < cxt->max_dump_cnt; i++) {
 		size_t sz = cxt->record_size;
 
-		cxt->przs[i] = persistent_ram_new(*paddr, sz, cxt->ecc);
+		cxt->przs[i] = persistent_ram_new(*paddr, sz, cxt->ecc_size);
 		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",
@@ -314,7 +316,7 @@ static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt,
 	if (*paddr + sz > *paddr + cxt->size)
 		return -ENOMEM;
 
-	*prz = persistent_ram_new(*paddr, sz, cxt->ecc);
+	*prz = persistent_ram_new(*paddr, sz, cxt->ecc_size);
 	if (IS_ERR(*prz)) {
 		int err = PTR_ERR(*prz);
 
@@ -361,7 +363,7 @@ static int __devinit ramoops_probe(struct platform_device *pdev)
 	cxt->record_size = pdata->record_size;
 	cxt->console_size = pdata->console_size;
 	cxt->dump_oops = pdata->dump_oops;
-	cxt->ecc = pdata->ecc;
+	cxt->ecc_size = pdata->ecc_size;
 
 	paddr = cxt->phys_addr;
 
@@ -411,9 +413,9 @@ static int __devinit ramoops_probe(struct platform_device *pdev)
 	record_size = pdata->record_size;
 	dump_oops = pdata->dump_oops;
 
-	pr_info("attached 0x%lx@0x%llx, ecc: %s\n",
+	pr_info("attached 0x%lx@0x%llx, ecc: %d\n",
 		cxt->size, (unsigned long long)cxt->phys_addr,
-		ramoops_ecc ? "on" : "off");
+		cxt->ecc_size);
 
 	return 0;
 
@@ -478,7 +480,11 @@ static void ramoops_register_dummy(void)
 	dummy_data->record_size = record_size;
 	dummy_data->console_size = ramoops_console_size;
 	dummy_data->dump_oops = dump_oops;
-	dummy_data->ecc = ramoops_ecc;
+	/*
+	 * For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
+	 * (using 1 byte for ECC isn't much of use anyway).
+	 */
+	dummy_data->ecc_size = ramoops_ecc == 1 ? 16 : ramoops_ecc;
 
 	dummy = platform_device_register_data(NULL, "ramoops", -1,
 			dummy_data, sizeof(struct ramoops_platform_data));
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 3f4d6e6..7e5a2a9 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -171,7 +171,8 @@ static void persistent_ram_ecc_old(struct persistent_ram_zone *prz)
 	}
 }
 
-static int persistent_ram_init_ecc(struct persistent_ram_zone *prz)
+static int persistent_ram_init_ecc(struct persistent_ram_zone *prz,
+				   int ecc_size)
 {
 	int numerr;
 	struct persistent_ram_buffer *buffer = prz->buffer;
@@ -184,7 +185,7 @@ static int persistent_ram_init_ecc(struct persistent_ram_zone *prz)
 		return 0;
 
 	prz->ecc_block_size = 128;
-	prz->ecc_size = 16;
+	prz->ecc_size = ecc_size;
 
 	ecc_blocks = DIV_ROUND_UP(prz->buffer_size, prz->ecc_block_size);
 	ecc_total = (ecc_blocks + 1) * prz->ecc_size;
@@ -390,13 +391,13 @@ static int persistent_ram_buffer_map(phys_addr_t start, phys_addr_t size,
 }
 
 static int __devinit persistent_ram_post_init(struct persistent_ram_zone *prz,
-					      bool ecc)
+					      int ecc_size)
 {
 	int ret;
 
-	prz->ecc = ecc;
+	prz->ecc = ecc_size;
 
-	ret = persistent_ram_init_ecc(prz);
+	ret = persistent_ram_init_ecc(prz, ecc_size);
 	if (ret)
 		return ret;
 
@@ -444,7 +445,7 @@ void persistent_ram_free(struct persistent_ram_zone *prz)
 
 struct persistent_ram_zone * __devinit persistent_ram_new(phys_addr_t start,
 							  size_t size,
-							  bool ecc)
+							  int ecc_size)
 {
 	struct persistent_ram_zone *prz;
 	int ret = -ENOMEM;
@@ -459,7 +460,7 @@ struct persistent_ram_zone * __devinit persistent_ram_new(phys_addr_t start,
 	if (ret)
 		goto err;
 
-	ret = persistent_ram_post_init(prz, ecc);
+	ret = persistent_ram_post_init(prz, ecc_size);
 	if (ret)
 		goto err;
 
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index a0975c0..94b79f1 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -48,7 +48,7 @@ struct persistent_ram_zone {
 
 struct persistent_ram_zone * __devinit persistent_ram_new(phys_addr_t start,
 							  size_t size,
-							  bool ecc);
+							  int ecc_size);
 void persistent_ram_free(struct persistent_ram_zone *prz);
 void persistent_ram_zap(struct persistent_ram_zone *prz);
 
@@ -74,7 +74,7 @@ struct ramoops_platform_data {
 	unsigned long	record_size;
 	unsigned long	console_size;
 	int		dump_oops;
-	bool		ecc;
+	int		ecc_size;
 };
 
 #endif
-- 
1.7.10.4

[PATCH 3/3] pstore/ram_core: Get rid of prz->ecc enable/disable flag

[Anton Vorontsov]

Nowadays we can use prz->ecc_size as a flag, no need for the special
member in the prz struct.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram_core.c       |   10 ++++------
 include/linux/pstore_ram.h |    1 -
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 7e5a2a9..4dabbb8 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -114,7 +114,7 @@ static void notrace persistent_ram_update_ecc(struct persistent_ram_zone *prz,
 	int ecc_size = prz->ecc_size;
 	int size = prz->ecc_block_size;
 
-	if (!prz->ecc)
+	if (!prz->ecc_size)
 		return;
 
 	block = buffer->data + (start & ~(ecc_block_size - 1));
@@ -133,7 +133,7 @@ static void persistent_ram_update_header_ecc(struct persistent_ram_zone *prz)
 {
 	struct persistent_ram_buffer *buffer = prz->buffer;
 
-	if (!prz->ecc)
+	if (!prz->ecc_size)
 		return;
 
 	persistent_ram_encode_rs8(prz, (uint8_t *)buffer, sizeof(*buffer),
@@ -146,7 +146,7 @@ static void persistent_ram_ecc_old(struct persistent_ram_zone *prz)
 	uint8_t *block;
 	uint8_t *par;
 
-	if (!prz->ecc)
+	if (!prz->ecc_size)
 		return;
 
 	block = buffer->data;
@@ -181,7 +181,7 @@ static int persistent_ram_init_ecc(struct persistent_ram_zone *prz,
 	int ecc_symsize = 8;
 	int ecc_poly = 0x11d;
 
-	if (!prz->ecc)
+	if (!ecc_size)
 		return 0;
 
 	prz->ecc_block_size = 128;
@@ -395,8 +395,6 @@ static int __devinit persistent_ram_post_init(struct persistent_ram_zone *prz,
 {
 	int ret;
 
-	prz->ecc = ecc_size;
-
 	ret = persistent_ram_init_ecc(prz, ecc_size);
 	if (ret)
 		return ret;
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 94b79f1..dcf805f 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -33,7 +33,6 @@ struct persistent_ram_zone {
 	size_t buffer_size;
 
 	/* ECC correction */
-	bool ecc;
 	char *par_buffer;
 	char *par_header;
 	struct rs_control *rs_decoder;
-- 
1.7.10.4

Re: [PATCH resend 0/3] pstore/ram: Configurable ECC size

[Kees Cook]

On Mon, Jul 9, 2012 at 4:45 PM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> Just a few patches left from the series that used to add configurable
> ECC size for pstore/ram backend. Most patches were merged into -next,
> and this is just a resend of the leftovers.

Feel free to add my:

Acked-by: Kees Cook <keescook@chromium.org>

to this series. Looks good to me.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH resend 0/3] pstore/ram: Configurable ECC size

[Anton Vorontsov]

On Tue, Jul 10, 2012 at 01:17:48PM -0700, Kees Cook wrote:
> On Mon, Jul 9, 2012 at 4:45 PM, Anton Vorontsov
> <anton.vorontsov@linaro.org> wrote:
> > Just a few patches left from the series that used to add configurable
> > ECC size for pstore/ram backend. Most patches were merged into -next,
> > and this is just a resend of the leftovers.
> 
> Feel free to add my:
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> to this series. Looks good to me.

Thanks for looking at this, Kees!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com