pstore/ram: Configurable ECC size

PATCH RESEND In review Anton Vorontsov 3 participants 7 messages Jul 9, 2012 – Jul 11, 2012 Pstore filesystem

lore

Share & tools

lore

Anton Vorontsov

author

2012-07-09 23:48 UTC #0 lore raw

cover pstore/ram: Configurable ECC size

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(-)

Signature · 2 lines

Anton Vorontsov
Email: [email protected]

Anton Vorontsov

author

2012-07-10 00:03 UTC #1 lore raw

Quoted 5 lines · 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.

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.

Signature · 2 lines

Anton Vorontsov
Email: [email protected]

Anton Vorontsov

author

2012-07-10 00:06 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 fs/pstore/ram_core.c       |    7 +++----
 include/linux/pstore_ram.h |    2 --
 2 files changed, 3 insertions(+), 6 deletions(-)

fs/pstore/ram_core.c +3 -4

index a5a7b13..3f4d6e6 100644
@@ -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;

include/linux/pstore_ram.h +0 -2

index e681af9..a0975c0 100644
@@ -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;

Signature · 1 line

1.7.10.4

Anton Vorontsov

author

2012-07-10 00:06 UTC #3 lore raw

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

In reply to 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).

Signed-off-by Anton Vorontsov <[email protected]>

---
 fs/pstore/ram.c            |   22 ++++++++++++++--------
 fs/pstore/ram_core.c       |   15 ++++++++-------
 include/linux/pstore_ram.h |    4 ++--
 3 files changed, 24 insertions(+), 17 deletions(-)

fs/pstore/ram.c +14 -8

index 58b93fb..b39aebb 100644
@@ -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));

fs/pstore/ram_core.c +8 -7

index 3f4d6e6..7e5a2a9 100644
@@ -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;

include/linux/pstore_ram.h +2 -2

index a0975c0..94b79f1 100644
@@ -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

Signature · 1 line

1.7.10.4

Anton Vorontsov

author

2012-07-10 00:06 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 fs/pstore/ram_core.c       |   10 ++++------
 include/linux/pstore_ram.h |    1 -
 2 files changed, 4 insertions(+), 7 deletions(-)

fs/pstore/ram_core.c +4 -6

index 7e5a2a9..4dabbb8 100644
@@ -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;

include/linux/pstore_ram.h +0 -1

index 94b79f1..dcf805f 100644
@@ -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;

Signature · 1 line

1.7.10.4

Kees Cook

Acked-by on the v1 series

2012-07-10 20:17 UTC #5 lore raw

In reply to Anton Vorontsov

Quoted 3 lines · Anton Vorontsov

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 <[email protected]>

to this series. Looks good to me.

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Anton Vorontsov

2012-07-11 06:30 UTC #6 lore raw

Quoted 11 lines · Kees Cook

Quoted 3 lines · Anton Vorontsov
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 <[email protected]>
to this series. Looks good to me.

Thanks for looking at this, Kees!

Signature · 2 lines

Anton Vorontsov
Email: [email protected]