[PATCH 0/3]: mtdoops fixes and improvements

[PATCH 0/3]: mtdoops fixes and improvements

[Simon Kagstrom]

Hi!

Here are a couple of patches to mtdoops which I've been working on for
a while. The patches are:

1. Make page size configurable (to support longer messages)

2. Use panic_write if panic_on_oops is set. This is essentially the
   same as Aaro sent earlier today (funny coincidence!), but contains
   some other fixes around the same area. This unbreaks the case when
   panic_on_oops is set.

3. Store all kernel messages in a circular buffer. This changes the
   behavior of mtdoops a bit by constantly recording messages (not only
   on oopses), which are written out on oopses and panics.

   The code is changed to register a panic handler which calls the
   write function.

// Simon

[PATCH 1/3]: mtdoops: Make page size configurable

[Simon Kagstrom]

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   62 ++++++++++++++++++++++++++----------------------
 1 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 1060337..244582c 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -34,7 +34,11 @@
 #include <linux/mtd/mtd.h>
 
 #define MTDOOPS_KERNMSG_MAGIC 0x5d005d00
-#define OOPS_PAGE_SIZE 4096
+
+static int mtdoops_page_size = 4096;
+module_param(mtdoops_page_size, int, 0);
+MODULE_PARM_DESC(mtdoops_page_size, "page size for MTD OOPS pages in bytes (default 4096)");
+
 
 static struct mtdoops_context {
 	int mtd_index;
@@ -42,6 +46,7 @@ static struct mtdoops_context {
 	struct work_struct work_write;
 	struct mtd_info *mtd;
 	int oops_pages;
+	size_t page_size;
 	int nextpage;
 	int nextcount;
 	char *name;
@@ -107,11 +112,11 @@ static void mtdoops_inc_counter(struct mtdoops_context *cxt)
 	if (cxt->nextcount == 0xffffffff)
 		cxt->nextcount = 0;
 
-	ret = mtd->read(mtd, cxt->nextpage * OOPS_PAGE_SIZE, 4,
+	ret = mtd->read(mtd, cxt->nextpage * cxt->page_size, 4,
 			&retlen, (u_char *) &count);
 	if ((retlen != 4) || ((ret < 0) && (ret != -EUCLEAN))) {
 		printk(KERN_ERR "mtdoops: Read failure at %d (%td of 4 read)"
-				", err %d.\n", cxt->nextpage * OOPS_PAGE_SIZE,
+				", err %d.\n", cxt->nextpage * cxt->page_size,
 				retlen, ret);
 		schedule_work(&cxt->work_erase);
 		return;
@@ -140,15 +145,15 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 	if (!mtd)
 		return;
 
-	mod = (cxt->nextpage * OOPS_PAGE_SIZE) % mtd->erasesize;
+	mod = (cxt->nextpage * cxt->page_size) % mtd->erasesize;
 	if (mod != 0) {
-		cxt->nextpage = cxt->nextpage + ((mtd->erasesize - mod) / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage + ((mtd->erasesize - mod) / cxt->page_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
 	}
 
 	while (mtd->block_isbad) {
-		ret = mtd->block_isbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_isbad(mtd, cxt->nextpage * cxt->page_size);
 		if (!ret)
 			break;
 		if (ret < 0) {
@@ -157,19 +162,19 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 		}
 badblock:
 		printk(KERN_WARNING "mtdoops: Bad block at %08x\n",
-				cxt->nextpage * OOPS_PAGE_SIZE);
+				cxt->nextpage * cxt->page_size);
 		i++;
-		cxt->nextpage = cxt->nextpage + (mtd->erasesize / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage + (mtd->erasesize / cxt->page_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
-		if (i == (cxt->oops_pages / (mtd->erasesize / OOPS_PAGE_SIZE))) {
+		if (i == (cxt->oops_pages / (mtd->erasesize / cxt->page_size))) {
 			printk(KERN_ERR "mtdoops: All blocks bad!\n");
 			return;
 		}
 	}
 
 	for (j = 0, ret = -1; (j < 3) && (ret < 0); j++)
-		ret = mtdoops_erase_block(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtdoops_erase_block(mtd, cxt->nextpage * cxt->page_size);
 
 	if (ret >= 0) {
 		printk(KERN_DEBUG "mtdoops: Ready %d, %d \n", cxt->nextpage, cxt->nextcount);
@@ -178,7 +183,7 @@ badblock:
 	}
 
 	if (mtd->block_markbad && (ret == -EIO)) {
-		ret = mtd->block_markbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_markbad(mtd, cxt->nextpage * cxt->page_size);
 		if (ret < 0) {
 			printk(KERN_ERR "mtdoops: block_markbad failed, aborting.\n");
 			return;
@@ -193,22 +198,22 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	size_t retlen;
 	int ret;
 
-	if (cxt->writecount < OOPS_PAGE_SIZE)
+	if (cxt->writecount < cxt->page_size)
 		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					OOPS_PAGE_SIZE - cxt->writecount);
+					cxt->page_size - cxt->writecount);
 
 	if (panic)
-		ret = mtd->panic_write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->panic_write(mtd, cxt->nextpage * cxt->page_size,
+					cxt->page_size, &retlen, cxt->oops_buf);
 	else
-		ret = mtd->write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->write(mtd, cxt->nextpage * cxt->page_size,
+					cxt->page_size, &retlen, cxt->oops_buf);
 
 	cxt->writecount = 0;
 
-	if ((retlen != OOPS_PAGE_SIZE) || (ret < 0))
+	if ((retlen != cxt->page_size) || (ret < 0))
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
-			cxt->nextpage * OOPS_PAGE_SIZE, retlen,	OOPS_PAGE_SIZE, ret);
+			cxt->nextpage * cxt->page_size, retlen,	cxt->page_size, ret);
 
 	mtdoops_inc_counter(cxt);
 }
@@ -230,10 +235,10 @@ static void find_next_position(struct mtdoops_context *cxt)
 	size_t retlen;
 
 	for (page = 0; page < cxt->oops_pages; page++) {
-		ret = mtd->read(mtd, page * OOPS_PAGE_SIZE, 8, &retlen, (u_char *) &count[0]);
+		ret = mtd->read(mtd, page * cxt->page_size, 8, &retlen, (u_char *) &count[0]);
 		if ((retlen != 8) || ((ret < 0) && (ret != -EUCLEAN))) {
 			printk(KERN_ERR "mtdoops: Read failure at %d (%td of 8 read)"
-				", err %d.\n", page * OOPS_PAGE_SIZE, retlen, ret);
+				", err %d.\n", page * cxt->page_size, retlen, ret);
 			continue;
 		}
 
@@ -286,7 +291,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 		return;
 	}
 
-	if (mtd->erasesize < OOPS_PAGE_SIZE) {
+	if (mtd->erasesize < cxt->page_size) {
 		printk(KERN_ERR "Eraseblock size of MTD partition %d too small\n",
 				mtd->index);
 		return;
@@ -294,9 +299,9 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 
 	cxt->mtd = mtd;
 	if (mtd->size > INT_MAX)
-		cxt->oops_pages = INT_MAX / OOPS_PAGE_SIZE;
+		cxt->oops_pages = INT_MAX / cxt->page_size;
 	else
-		cxt->oops_pages = (int)mtd->size / OOPS_PAGE_SIZE;
+		cxt->oops_pages = (int)mtd->size / cxt->page_size;
 
 	find_next_position(cxt);
 
@@ -373,15 +378,15 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		cxt->writecount = 8;
 	}
 
-	if ((count + cxt->writecount) > OOPS_PAGE_SIZE)
-		count = OOPS_PAGE_SIZE - cxt->writecount;
+	if ((count + cxt->writecount) > cxt->page_size)
+		count = cxt->page_size - cxt->writecount;
 
 	memcpy(cxt->oops_buf + cxt->writecount, s, count);
 	cxt->writecount += count;
 
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == OOPS_PAGE_SIZE)
+	if (cxt->writecount == cxt->page_size)
 		mtdoops_console_sync();
 }
 
@@ -421,7 +426,8 @@ static int __init mtdoops_console_init(void)
 	struct mtdoops_context *cxt = &oops_cxt;
 
 	cxt->mtd_index = -1;
-	cxt->oops_buf = vmalloc(OOPS_PAGE_SIZE);
+	cxt->page_size = mtdoops_page_size;
+	cxt->oops_buf = vmalloc(cxt->page_size);
 	spin_lock_init(&cxt->writecount_lock);
 
 	if (!cxt->oops_buf) {
-- 
1.6.0.4

Re: [PATCH 1/3]: mtdoops: Make page size configurable

[Artem Bityutskiy]

On Fri, 2009-10-02 at 16:06 +0200, Simon Kagstrom wrote:
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  drivers/mtd/mtdoops.c |   62 ++++++++++++++++++++++++++----------------------
>  1 files changed, 34 insertions(+), 28 deletions(-)

Would be nice to have better commit messages which describes why you
need this option.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH 2/3]: mtdoops: Use panic_write if panic_on_oops is set

[Simon Kagstrom]

mtdoops will not store the oops if panic_on_oops is set. This patch
makes use of panic_write if panic_on_oops is set. mtdoops_inc_counter is
also not good to call on panics, since the call to mtd->read suspends
the panic (at least with my NAND flash), so defer that. There is also no
point in doing it since we cannot recover from the panic anyway.

panic_on_oops is not exported to modules, so make mtdoops in-kernel only

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/Kconfig   |    2 +-
 drivers/mtd/mtdoops.c |    8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index ecf90f5..6b39a8b 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -305,7 +305,7 @@ config SSFDC
 	  flash. You can mount it with FAT file system.
 
 config MTD_OOPS
-	tristate "Log panic/oops to an MTD buffer"
+	bool "Log panic/oops to an MTD buffer"
 	depends on MTD
 	help
 	  This enables panic and oops messages to be logged to a circular
diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 244582c..cc2c187 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -215,7 +215,11 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
 			cxt->nextpage * cxt->page_size, retlen,	cxt->page_size, ret);
 
-	mtdoops_inc_counter(cxt);
+	/* Go to next page, but skip this if we are currently panicking.
+	 * We will not recover from that anyway, and the mtd->read call
+	 * causes the panic to suspend */
+	if (!in_interrupt() && !panic_on_oops)
+		mtdoops_inc_counter(cxt);
 }
 
 
@@ -340,7 +344,7 @@ static void mtdoops_console_sync(void)
 	cxt->ready = 0;
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (mtd->panic_write && in_interrupt())
+	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
 		/* Interrupt context, we're going to panic so try and log */
 		mtdoops_write(cxt, 1);
 	else
-- 
1.6.0.4

Re: [PATCH 2/3]: mtdoops: Use panic_write if panic_on_oops is set

[Artem Bityutskiy]

On Fri, 2009-10-02 at 16:06 +0200, Simon Kagstrom wrote:
> mtdoops will not store the oops if panic_on_oops is set. This patch
> makes use of panic_write if panic_on_oops is set. mtdoops_inc_counter is
> also not good to call on panics, since the call to mtd->read suspends
> the panic (at least with my NAND flash), so defer that. There is also no
> point in doing it since we cannot recover from the panic anyway.
> 
> panic_on_oops is not exported to modules, so make mtdoops in-kernel only
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  drivers/mtd/Kconfig   |    2 +-
>  drivers/mtd/mtdoops.c |    8 ++++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index ecf90f5..6b39a8b 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -305,7 +305,7 @@ config SSFDC
>  	  flash. You can mount it with FAT file system.
>  
>  config MTD_OOPS
> -	tristate "Log panic/oops to an MTD buffer"
> +	bool "Log panic/oops to an MTD buffer"
>  	depends on MTD
>  	help
>  	  This enables panic and oops messages to be logged to a circular
> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> index 244582c..cc2c187 100644
> --- a/drivers/mtd/mtdoops.c
> +++ b/drivers/mtd/mtdoops.c
> @@ -215,7 +215,11 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
>  		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
>  			cxt->nextpage * cxt->page_size, retlen,	cxt->page_size, ret);
>  
> -	mtdoops_inc_counter(cxt);
> +	/* Go to next page, but skip this if we are currently panicking.
> +	 * We will not recover from that anyway, and the mtd->read call
> +	 * causes the panic to suspend */
> +	if (!in_interrupt() && !panic_on_oops)
> +		mtdoops_inc_counter(cxt);

Hmm, what if we are in an oops we want to write more than one time,
because the oops output is large? With this change we will write twice
to the same flash area in that case, which is bad.

Basically, if you skip 'mtdoops_inc_counter()', you should disallow any
further mtdoops writes, so you need something like 'ctx->i_am_screwed'
flag, I guess.

Also, I think this kind of check should be inside
'mtdoops_inc_counter()', not outside. It is just cleaner.

BTW, 'mtdoops_inc_counter()' reads flash before each write, which is
strange. If an eraseblock was erased, everything is 0xFFed there. Why
not to maintain an internal array which contains erased/not erased
status of each eraseblock?

> @@ -340,7 +344,7 @@ static void mtdoops_console_sync(void)
>  	cxt->ready = 0;
>  	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
>  
> -	if (mtd->panic_write && in_interrupt())
> +	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
>  		/* Interrupt context, we're going to panic so try and log */
>  		mtdoops_write(cxt, 1);
>  	else

I agree with this part and will push the patch from Aaro to my tree for
now.

Please, post patches against:
git://git.infradead.org/users/dedekind/l2-mtd-2.6.git
so far. When we are done, I'll ping dwmw2 to pull them.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/3]: mtdoops: Use panic_write if panic_on_oops is set

[Simon Kagstrom]

Thanks for reviewing the patches!

On Thu, 08 Oct 2009 08:28:12 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> > -	mtdoops_inc_counter(cxt);
> > +	/* Go to next page, but skip this if we are currently panicking.
> > +	 * We will not recover from that anyway, and the mtd->read call
> > +	 * causes the panic to suspend */
> > +	if (!in_interrupt() && !panic_on_oops)
> > +		mtdoops_inc_counter(cxt);
> 
> Hmm, what if we are in an oops we want to write more than one time,
> because the oops output is large? With this change we will write twice
> to the same flash area in that case, which is bad.

No, cxt->ready is set by mtdoops_inc_counter, so there will only be one
write (no more messages are put in the buffer) until inc_counter is done.

> Also, I think this kind of check should be inside
> 'mtdoops_inc_counter()', not outside. It is just cleaner.

OK, I'll do that.

> BTW, 'mtdoops_inc_counter()' reads flash before each write, which is
> strange. If an eraseblock was erased, everything is 0xFFed there. Why
> not to maintain an internal array which contains erased/not erased
> status of each eraseblock?

Well, it reads the flash for the next block which will be written (on
the next oops), and schedules an erase if needed. But sure, since it
scans the partition at startup, an array could be used instead.

The check I added was because mtdoops_inc_counter caused my panic to
hang with my NAND flash (it sounds strange, but I never got the actual
restart). I haven't debugged why it happened, but it's caused by the
mtd->read call from mtdoops_inc_counter. 


Anyway, perhaps a cleaner solution to the entire issue is to put
mtdoops_inc_counter in a scheduled work instead so that we can skip
this check entirely.

> > @@ -340,7 +344,7 @@ static void mtdoops_console_sync(void)
> >  	cxt->ready = 0;
> >  	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
> >  
> > -	if (mtd->panic_write && in_interrupt())
> > +	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
> >  		/* Interrupt context, we're going to panic so try and log */
> >  		mtdoops_write(cxt, 1);
> >  	else
> 
> I agree with this part and will push the patch from Aaro to my tree for
> now.

Good, just bear in mind that adding the panic_on_oops check will make
it impossible to build as a module as panic_on_oops is not exported. I
disabled module-building in this patch because of this.

But perhaps the real solution is to export panic_on_oops, I just
assumed that there was some good reason why it wasn't exported so
far :-)

// Simon

Re: [PATCH 2/3]: mtdoops: Use panic_write if panic_on_oops is set

[Artem Bityutskiy]

On Thu, 2009-10-08 at 08:28 +0300, Artem Bityutskiy wrote:
> On Fri, 2009-10-02 at 16:06 +0200, Simon Kagstrom wrote:
> > mtdoops will not store the oops if panic_on_oops is set. This patch
> > makes use of panic_write if panic_on_oops is set. mtdoops_inc_counter is
> > also not good to call on panics, since the call to mtd->read suspends
> > the panic (at least with my NAND flash), so defer that. There is also no
> > point in doing it since we cannot recover from the panic anyway.
> > 
> > panic_on_oops is not exported to modules, so make mtdoops in-kernel only
> > 
> > Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> > ---
> >  drivers/mtd/Kconfig   |    2 +-
> >  drivers/mtd/mtdoops.c |    8 ++++++--
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> > index ecf90f5..6b39a8b 100644
> > --- a/drivers/mtd/Kconfig
> > +++ b/drivers/mtd/Kconfig
> > @@ -305,7 +305,7 @@ config SSFDC
> >  	  flash. You can mount it with FAT file system.
> >  
> >  config MTD_OOPS
> > -	tristate "Log panic/oops to an MTD buffer"
> > +	bool "Log panic/oops to an MTD buffer"
> >  	depends on MTD
> >  	help
> >  	  This enables panic and oops messages to be logged to a circular
> > diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> > index 244582c..cc2c187 100644
> > --- a/drivers/mtd/mtdoops.c
> > +++ b/drivers/mtd/mtdoops.c
> > @@ -215,7 +215,11 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
> >  		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
> >  			cxt->nextpage * cxt->page_size, retlen,	cxt->page_size, ret);
> >  
> > -	mtdoops_inc_counter(cxt);
> > +	/* Go to next page, but skip this if we are currently panicking.
> > +	 * We will not recover from that anyway, and the mtd->read call
> > +	 * causes the panic to suspend */
> > +	if (!in_interrupt() && !panic_on_oops)
> > +		mtdoops_inc_counter(cxt);
> 
> Hmm, what if we are in an oops we want to write more than one time,
> because the oops output is large? With this change we will write twice
> to the same flash area in that case, which is bad.
> 
> Basically, if you skip 'mtdoops_inc_counter()', you should disallow any
> further mtdoops writes, so you need something like 'ctx->i_am_screwed'
> flag, I guess.
> 
> Also, I think this kind of check should be inside
> 'mtdoops_inc_counter()', not outside. It is just cleaner.
> 
> BTW, 'mtdoops_inc_counter()' reads flash before each write, which is
> strange. If an eraseblock was erased, everything is 0xFFed there. Why
> not to maintain an internal array which contains erased/not erased
> status of each eraseblock?
> 
> > @@ -340,7 +344,7 @@ static void mtdoops_console_sync(void)
> >  	cxt->ready = 0;
> >  	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
> >  
> > -	if (mtd->panic_write && in_interrupt())
> > +	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
> >  		/* Interrupt context, we're going to panic so try and log */
> >  		mtdoops_write(cxt, 1);

Oh, panic_on_oops is not exported. So we should either export it or make
mtdoops to be always compiled-in.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/3]: mtdoops: Use panic_write if panic_on_oops is set

[Simon Kagstrom]

On Sun, 11 Oct 2009 09:00:20 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Thu, 2009-10-08 at 08:28 +0300, Artem Bityutskiy wrote:
>
> > > -	if (mtd->panic_write && in_interrupt())
> > > +	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
> > >  		/* Interrupt context, we're going to panic so try and log */
> > >  		mtdoops_write(cxt, 1);
> 
> Oh, panic_on_oops is not exported. So we should either export it or make
> mtdoops to be always compiled-in.

Yep, in the earlier patch series (this one!) I made it always
compiled-in, but for new one I kept the module support and also sent
a patch to LKML which exports panic_on_oops. I saw you did the same
thing now, thanks!

// Simon

[PATCH 3/3]: mtdoops: store all kernel messages in a circular buffer

[Simon Kagstrom]

The last messages which happens before a crash might contain interesting
information about the crash. This patch reworks mtdoops to keep a
circular buffer of _all_ kernel messages, not just those that are
printed when an oops is initiated.

A handler that is called on panic is also added instead of
mtdoops_console_sync so that panic_on_oops and true panics are stored.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |  110 ++++++++++++++++++++++++++++--------------------
 1 files changed, 64 insertions(+), 46 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index cc2c187..7664ed0 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -52,6 +52,7 @@ static struct mtdoops_context {
 	char *name;
 
 	void *oops_buf;
+	void *oops_buf_write;
 
 	/* writecount and disabling ready are spin lock protected */
 	spinlock_t writecount_lock;
@@ -196,20 +197,29 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	size_t retlen;
+	u32 * stamp;
 	int ret;
 
-	if (cxt->writecount < cxt->page_size)
-		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					cxt->page_size - cxt->writecount);
+	cxt->ready = 0;
+
+	BUG_ON(cxt->writecount > cxt->page_size - 8);
+
+	/* oops_write_buf = [:8] + [writecount:] + [:writecount] */
+	stamp = cxt->oops_buf_write;
+	*stamp++ = cxt->nextcount;
+	*stamp = MTDOOPS_KERNMSG_MAGIC;
+
+	memcpy(cxt->oops_buf_write + 8, cxt->oops_buf + cxt->writecount,
+			cxt->page_size - cxt->writecount);
+	memcpy(cxt->oops_buf_write + cxt->page_size - cxt->writecount,
+			cxt->oops_buf, cxt->writecount);
 
 	if (panic)
 		ret = mtd->panic_write(mtd, cxt->nextpage * cxt->page_size,
-					cxt->page_size, &retlen, cxt->oops_buf);
+					cxt->page_size, &retlen, cxt->oops_buf_write);
 	else
 		ret = mtd->write(mtd, cxt->nextpage * cxt->page_size,
-					cxt->page_size, &retlen, cxt->oops_buf);
-
-	cxt->writecount = 0;
+					cxt->page_size, &retlen, cxt->oops_buf_write);
 
 	if ((retlen != cxt->page_size) || (ret < 0))
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
@@ -222,6 +232,26 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 		mtdoops_inc_counter(cxt);
 }
 
+static int mtdoops_panic(struct notifier_block *this, unsigned long event,
+		void *ptr)
+{
+	struct mtdoops_context *cxt = &oops_cxt;
+
+	cancel_work_sync(&cxt->work_write);
+	cxt->ready = 0;
+	if (cxt->mtd->panic_write)
+		mtdoops_write(cxt, 1);
+	else
+		printk(KERN_WARNING "mtdoops: panic_write is not defined, "
+					"cannot store dump from panic\n");
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_block = {
+	.notifier_call = mtdoops_panic,
+};
+
 
 static void mtdoops_workfunc_write(struct work_struct *work)
 {
@@ -309,6 +339,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 
 	find_next_position(cxt);
 
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 	printk(KERN_INFO "mtdoops: Attached to MTD device %d\n", mtd->index);
 }
 
@@ -326,28 +357,9 @@ static void mtdoops_notify_remove(struct mtd_info *mtd)
 static void mtdoops_console_sync(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
-	struct mtd_info *mtd = cxt->mtd;
-	unsigned long flags;
 
-	if (!cxt->ready || !mtd || cxt->writecount == 0)
-		return;
-
-	/* 
-	 *  Once ready is 0 and we've held the lock no further writes to the 
-	 *  buffer will happen
-	 */
-	spin_lock_irqsave(&cxt->writecount_lock, flags);
-	if (!cxt->ready) {
-		spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-		return;
-	}
-	cxt->ready = 0;
-	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-
-	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
-		/* Interrupt context, we're going to panic so try and log */
-		mtdoops_write(cxt, 1);
-	else
+	/* Write out the buffer if we are called during an oops */
+	if (oops_in_progress)
 		schedule_work(&cxt->work_write);
 }
 
@@ -356,13 +368,11 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct mtdoops_context *cxt = co->data;
 	struct mtd_info *mtd = cxt->mtd;
+	int copy_from;
+	int copy_wrap = 0;
+	int copy_wrap_diff = 0;
 	unsigned long flags;
 
-	if (!oops_in_progress) {
-		mtdoops_console_sync();
-		return;
-	}
-
 	if (!cxt->ready || !mtd)
 		return;
 
@@ -375,23 +385,21 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		return;
 	}
 
-	if (cxt->writecount == 0) {
-		u32 *stamp = cxt->oops_buf;
-		*stamp++ = cxt->nextcount;
-		*stamp = MTDOOPS_KERNMSG_MAGIC;
+	/* Handle wraps */
+	if ((count + cxt->writecount) >= cxt->page_size) {
+		copy_wrap_diff = cxt->page_size - cxt->writecount;
+		copy_wrap = cxt->writecount;
+
 		cxt->writecount = 8;
+		count -= copy_wrap_diff;
 	}
-
-	if ((count + cxt->writecount) > cxt->page_size)
-		count = cxt->page_size - cxt->writecount;
-
-	memcpy(cxt->oops_buf + cxt->writecount, s, count);
+	copy_from = cxt->writecount;
 	cxt->writecount += count;
-
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == cxt->page_size)
-		mtdoops_console_sync();
+	if (copy_wrap)
+		memcpy(cxt->oops_buf + copy_wrap, s, copy_wrap_diff);
+	memcpy(cxt->oops_buf + copy_from, s, count);
 }
 
 static int __init mtdoops_console_setup(struct console *co, char *options)
@@ -429,15 +437,24 @@ static int __init mtdoops_console_init(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
 
+	cxt->writecount = 8; /* Start after the header */
 	cxt->mtd_index = -1;
 	cxt->page_size = mtdoops_page_size;
-	cxt->oops_buf = vmalloc(cxt->page_size);
 	spin_lock_init(&cxt->writecount_lock);
 
+	cxt->oops_buf = vmalloc(cxt->page_size);
 	if (!cxt->oops_buf) {
 		printk(KERN_ERR "Failed to allocate mtdoops buffer workspace\n");
 		return -ENOMEM;
 	}
+	cxt->oops_buf_write = vmalloc(cxt->page_size);
+	if (!cxt->oops_buf_write) {
+		printk(KERN_ERR "Failed to allocate mtdoops write buffer workspace\n");
+		vfree(cxt->oops_buf);
+		return -ENOMEM;
+	}
+	memset(cxt->oops_buf_write, 0xff, cxt->page_size);
+	memset(cxt->oops_buf, 0xff, cxt->page_size);
 
 	INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase);
 	INIT_WORK(&cxt->work_write, mtdoops_workfunc_write);
@@ -455,6 +472,7 @@ static void __exit mtdoops_console_exit(void)
 	unregister_console(&mtdoops_console);
 	kfree(cxt->name);
 	vfree(cxt->oops_buf);
+	vfree(cxt->oops_buf_write);
 }
 
 
-- 
1.6.0.4

Re: [PATCH 3/3]: mtdoops: store all kernel messages in a circular buffer

[Simon Kagstrom]

On Fri, 2 Oct 2009 16:07:41 +0200
Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> The last messages which happens before a crash might contain interesting
> information about the crash. This patch reworks mtdoops to keep a
> circular buffer of _all_ kernel messages, not just those that are
> printed when an oops is initiated.
> 
> A handler that is called on panic is also added instead of
> mtdoops_console_sync so that panic_on_oops and true panics are stored.

I found two small bugs in this patch, so a new version is coming up.

// Simon

[PATCH v2 3/3]: mtdoops: store all kernel messages in a circular buffer

[Simon Kagstrom]

The last messages which happens before a crash might contain interesting
information about the crash. This patch reworks mtdoops to keep a
circular buffer of _all_ kernel messages, not just those that are
printed when an oops is initiated.

A handler that is called on panic is also added instead of
mtdoops_console_sync so that panic_on_oops and true panics are stored.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
ChangeLog:

 v2:
   * Fix output bug when writing out on large pages (ff's were at the
     start of the printout).
   * Fix garbled text issues on wrapping
   * Add a BUG_ON assertion for writecount

 drivers/mtd/mtdoops.c |  117 +++++++++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 46 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index cc2c187..c24e11f 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -52,6 +52,7 @@ static struct mtdoops_context {
 	char *name;
 
 	void *oops_buf;
+	void *oops_buf_write;
 
 	/* writecount and disabling ready are spin lock protected */
 	spinlock_t writecount_lock;
@@ -196,20 +197,36 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	size_t retlen;
+	u32 * stamp;
+	int p;
 	int ret;
 
-	if (cxt->writecount < cxt->page_size)
-		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					cxt->page_size - cxt->writecount);
+	cxt->ready = 0;
+
+	BUG_ON(cxt->writecount < 8);
+	BUG_ON(cxt->writecount > cxt->page_size - 8);
+
+	/* oops_write_buf = [:8] + [writecount:] + [:writecount] */
+	stamp = cxt->oops_buf_write;
+	*stamp++ = cxt->nextcount;
+	*stamp = MTDOOPS_KERNMSG_MAGIC;
+
+	/* Find out the first non-0xff character */
+	for (p = cxt->writecount; p < cxt->page_size; p++) {
+		if ( ((u8*)cxt->oops_buf)[p] != 0xff)
+			break;
+	}
+	memcpy(cxt->oops_buf_write + 8, cxt->oops_buf + p,
+			cxt->page_size - p);
+	memcpy(cxt->oops_buf_write + 8 + cxt->page_size - p,
+			cxt->oops_buf + 8, p - 8);
 
 	if (panic)
 		ret = mtd->panic_write(mtd, cxt->nextpage * cxt->page_size,
-					cxt->page_size, &retlen, cxt->oops_buf);
+					cxt->page_size, &retlen, cxt->oops_buf_write);
 	else
 		ret = mtd->write(mtd, cxt->nextpage * cxt->page_size,
-					cxt->page_size, &retlen, cxt->oops_buf);
-
-	cxt->writecount = 0;
+					cxt->page_size, &retlen, cxt->oops_buf_write);
 
 	if ((retlen != cxt->page_size) || (ret < 0))
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
@@ -222,6 +239,26 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 		mtdoops_inc_counter(cxt);
 }
 
+static int mtdoops_panic(struct notifier_block *this, unsigned long event,
+		void *ptr)
+{
+	struct mtdoops_context *cxt = &oops_cxt;
+
+	cancel_work_sync(&cxt->work_write);
+	cxt->ready = 0;
+	if (cxt->mtd->panic_write)
+		mtdoops_write(cxt, 1);
+	else
+		printk(KERN_WARNING "mtdoops: panic_write is not defined, "
+					"cannot store dump from panic\n");
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_block = {
+	.notifier_call = mtdoops_panic,
+};
+
 
 static void mtdoops_workfunc_write(struct work_struct *work)
 {
@@ -309,6 +346,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 
 	find_next_position(cxt);
 
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 	printk(KERN_INFO "mtdoops: Attached to MTD device %d\n", mtd->index);
 }
 
@@ -326,28 +364,9 @@ static void mtdoops_notify_remove(struct mtd_info *mtd)
 static void mtdoops_console_sync(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
-	struct mtd_info *mtd = cxt->mtd;
-	unsigned long flags;
-
-	if (!cxt->ready || !mtd || cxt->writecount == 0)
-		return;
 
-	/* 
-	 *  Once ready is 0 and we've held the lock no further writes to the 
-	 *  buffer will happen
-	 */
-	spin_lock_irqsave(&cxt->writecount_lock, flags);
-	if (!cxt->ready) {
-		spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-		return;
-	}
-	cxt->ready = 0;
-	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-
-	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
-		/* Interrupt context, we're going to panic so try and log */
-		mtdoops_write(cxt, 1);
-	else
+	/* Write out the buffer if we are called during an oops */
+	if (oops_in_progress)
 		schedule_work(&cxt->work_write);
 }
 
@@ -356,13 +375,11 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct mtdoops_context *cxt = co->data;
 	struct mtd_info *mtd = cxt->mtd;
+	int copy_from;
+	int copy_wrap = 0;
+	int copy_wrap_diff = 0;
 	unsigned long flags;
 
-	if (!oops_in_progress) {
-		mtdoops_console_sync();
-		return;
-	}
-
 	if (!cxt->ready || !mtd)
 		return;
 
@@ -375,23 +392,21 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		return;
 	}
 
-	if (cxt->writecount == 0) {
-		u32 *stamp = cxt->oops_buf;
-		*stamp++ = cxt->nextcount;
-		*stamp = MTDOOPS_KERNMSG_MAGIC;
+	/* Handle wraps */
+	if ((count + cxt->writecount) >= cxt->page_size) {
+		copy_wrap_diff = cxt->page_size - cxt->writecount;
+		copy_wrap = cxt->writecount;
+
 		cxt->writecount = 8;
+		count -= copy_wrap_diff;
 	}
-
-	if ((count + cxt->writecount) > cxt->page_size)
-		count = cxt->page_size - cxt->writecount;
-
-	memcpy(cxt->oops_buf + cxt->writecount, s, count);
+	copy_from = cxt->writecount;
 	cxt->writecount += count;
-
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == cxt->page_size)
-		mtdoops_console_sync();
+	if (copy_wrap)
+		memcpy(cxt->oops_buf + copy_wrap, s, copy_wrap_diff);
+	memcpy(cxt->oops_buf + copy_from, s + copy_wrap_diff, count);
 }
 
 static int __init mtdoops_console_setup(struct console *co, char *options)
@@ -429,15 +444,24 @@ static int __init mtdoops_console_init(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
 
+	cxt->writecount = 8; /* Start after the header */
 	cxt->mtd_index = -1;
 	cxt->page_size = mtdoops_page_size;
-	cxt->oops_buf = vmalloc(cxt->page_size);
 	spin_lock_init(&cxt->writecount_lock);
 
+	cxt->oops_buf = vmalloc(cxt->page_size);
 	if (!cxt->oops_buf) {
 		printk(KERN_ERR "Failed to allocate mtdoops buffer workspace\n");
 		return -ENOMEM;
 	}
+	cxt->oops_buf_write = vmalloc(cxt->page_size);
+	if (!cxt->oops_buf_write) {
+		printk(KERN_ERR "Failed to allocate mtdoops write buffer workspace\n");
+		vfree(cxt->oops_buf);
+		return -ENOMEM;
+	}
+	memset(cxt->oops_buf_write, 0xff, cxt->page_size);
+	memset(cxt->oops_buf, 0xff, cxt->page_size);
 
 	INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase);
 	INIT_WORK(&cxt->work_write, mtdoops_workfunc_write);
@@ -455,6 +479,7 @@ static void __exit mtdoops_console_exit(void)
 	unregister_console(&mtdoops_console);
 	kfree(cxt->name);
 	vfree(cxt->oops_buf);
+	vfree(cxt->oops_buf_write);
 }
 
 
-- 
1.6.0.4

[PATCH v2 0/3]: mtdoops fixes and improvements

[Simon Kagstrom]

Hi!

Here are a couple of patches to mtdoops which I've been working on for
a while. They should be applied on top of Aaros fix for panic_on_oops.
That one technically breaks building mtdoops as a module, so I sent a
patch to LKML which exports panic_on_oops:

  http://patchwork.kernel.org/patch/52505/

The patches here are:

1. Keep track of mtdoops page cleanliness in an array. This allows
   mtdoops_inc_counter to be called during panic (which fails in my
   case, although I believe this is MTD-driver dependent).

2. Make page size configurable to support longer messages. Mainly
   needed for patch 3, but also allows longer messages to be stored
   during panics (when the next oops area cannot be erased).

3. Store all kernel messages in a circular buffer. This changes the
   behavior of mtdoops a bit by constantly recording messages (not only
   on oopses), which are written out on oopses and panics.

   The code is changed to register a panic handler which calls the
   write function, and on normal oopses scheduling a write work to
   store the buffer.

ChangeLog from last submit:

* Patches have been reordered to keep the least controversial (?) first.

* Implement Artems suggestion to keep cleanliness in an array

* Use Aaros patch to fix the panic_on_oops problem

// Simon

[PATCH v2 1/3]: mtdoops: Keep track of clean/dirty mtdoops pages in an array

[Simon Kagstrom]

This patch makes mtdoops keep track of clean/dirty pages in an array
instead of scanning the flash after a write. The advantage with this
approach is that it avoids calling mtd->read on a panic, which is not
possible for all mtd drivers.
---
 drivers/mtd/mtdoops.c |   50 +++++++++++++++++++++++++++++-------------------
 1 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index ac67833..19632d5 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -44,6 +44,7 @@ static struct mtdoops_context {
 	int oops_pages;
 	int nextpage;
 	int nextcount;
+	u8 *oops_page_dirty;
 	char *name;
 
 	void *oops_buf;
@@ -60,12 +61,17 @@ static void mtdoops_erase_callback(struct erase_info *done)
 	wake_up(wait_q);
 }
 
-static int mtdoops_erase_block(struct mtd_info *mtd, int offset)
+static int mtdoops_erase_block(struct mtdoops_context *cxt, int offset)
 {
+	struct mtd_info *mtd = cxt->mtd;
+	u32 start_page_offset = mtd_div_by_eb(offset, mtd) * mtd->erasesize;
+	u32 start_page = start_page_offset / OOPS_PAGE_SIZE;
+	u32 erase_pages = mtd->erasesize / OOPS_PAGE_SIZE;
 	struct erase_info erase;
 	DECLARE_WAITQUEUE(wait, current);
 	wait_queue_head_t wait_q;
 	int ret;
+	int page;
 
 	init_waitqueue_head(&wait_q);
 	erase.mtd = mtd;
@@ -90,16 +96,15 @@ static int mtdoops_erase_block(struct mtd_info *mtd, int offset)
 	schedule();  /* Wait for erase to finish. */
 	remove_wait_queue(&wait_q, &wait);
 
+	/* Mark pages as clean */
+	for (page = start_page; page < start_page + erase_pages; page++)
+		cxt->oops_page_dirty[page] = 0;
+
 	return 0;
 }
 
 static void mtdoops_inc_counter(struct mtdoops_context *cxt)
 {
-	struct mtd_info *mtd = cxt->mtd;
-	size_t retlen;
-	u32 count;
-	int ret;
-
 	cxt->nextpage++;
 	if (cxt->nextpage >= cxt->oops_pages)
 		cxt->nextpage = 0;
@@ -107,18 +112,7 @@ static void mtdoops_inc_counter(struct mtdoops_context *cxt)
 	if (cxt->nextcount == 0xffffffff)
 		cxt->nextcount = 0;
 
-	ret = mtd->read(mtd, cxt->nextpage * OOPS_PAGE_SIZE, 4,
-			&retlen, (u_char *) &count);
-	if ((retlen != 4) || ((ret < 0) && (ret != -EUCLEAN))) {
-		printk(KERN_ERR "mtdoops: Read failure at %d (%td of 4 read)"
-				", err %d.\n", cxt->nextpage * OOPS_PAGE_SIZE,
-				retlen, ret);
-		schedule_work(&cxt->work_erase);
-		return;
-	}
-
-	/* See if we need to erase the next block */
-	if (count != 0xffffffff) {
+	if (cxt->oops_page_dirty[cxt->nextpage]) {
 		schedule_work(&cxt->work_erase);
 		return;
 	}
@@ -169,7 +163,7 @@ badblock:
 	}
 
 	for (j = 0, ret = -1; (j < 3) && (ret < 0); j++)
-		ret = mtdoops_erase_block(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtdoops_erase_block(cxt, cxt->nextpage * OOPS_PAGE_SIZE);
 
 	if (ret >= 0) {
 		printk(KERN_DEBUG "mtdoops: Ready %d, %d \n", cxt->nextpage, cxt->nextcount);
@@ -230,13 +224,17 @@ static void find_next_position(struct mtdoops_context *cxt)
 	size_t retlen;
 
 	for (page = 0; page < cxt->oops_pages; page++) {
+		/* Assume it's dirty */
+		cxt->oops_page_dirty[page] = 1;
 		ret = mtd->read(mtd, page * OOPS_PAGE_SIZE, 8, &retlen, (u_char *) &count[0]);
+
 		if ((retlen != 8) || ((ret < 0) && (ret != -EUCLEAN))) {
 			printk(KERN_ERR "mtdoops: Read failure at %d (%td of 8 read)"
 				", err %d.\n", page * OOPS_PAGE_SIZE, retlen, ret);
 			continue;
 		}
-
+		if (count[0] == 0xffffffff && count[1] == 0xffffffff)
+			cxt->oops_page_dirty[page] = 0;
 		if (count[1] != MTDOOPS_KERNMSG_MAGIC)
 			continue;
 		if (count[0] == 0xffffffff)
@@ -273,6 +271,9 @@ static void find_next_position(struct mtdoops_context *cxt)
 static void mtdoops_notify_add(struct mtd_info *mtd)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
+	u64 mtdoops_pages = mtd->size;
+
+	do_div(mtdoops_pages, OOPS_PAGE_SIZE);
 
 	if (cxt->name && !strcmp(mtd->name, cxt->name))
 		cxt->mtd_index = mtd->index;
@@ -292,6 +293,12 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 		return;
 	}
 
+	cxt->oops_page_dirty = vmalloc(mtdoops_pages);
+	if (!cxt->oops_page_dirty) {
+		printk(KERN_ERR "Could not allocate page array (%Ld bytes)\n",
+				mtdoops_pages);
+		return;
+	}
 	cxt->mtd = mtd;
 	if (mtd->size > INT_MAX)
 		cxt->oops_pages = INT_MAX / OOPS_PAGE_SIZE;
@@ -428,6 +435,7 @@ static int __init mtdoops_console_init(void)
 		printk(KERN_ERR "Failed to allocate mtdoops buffer workspace\n");
 		return -ENOMEM;
 	}
+	cxt->oops_page_dirty = NULL;
 
 	INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase);
 	INIT_WORK(&cxt->work_write, mtdoops_workfunc_write);
@@ -445,6 +453,8 @@ static void __exit mtdoops_console_exit(void)
 	unregister_console(&mtdoops_console);
 	kfree(cxt->name);
 	vfree(cxt->oops_buf);
+	if (cxt->oops_page_dirty)
+		vfree(cxt->oops_page_dirty);
 }
 
 
-- 
1.6.0.4

[PATCH v2 2/3]: mtdoops: Make page size configurable

[Simon Kagstrom]

The main justification for this is to allow catching long messages
during a panic, where the top part might otherwise be lost since moving
to the next block can require a flash erase.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   71 +++++++++++++++++++++++++++++--------------------
 1 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 19632d5..4f2dc9c 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -32,9 +32,14 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/mtd/mtd.h>
+#include <linux/log2.h>
 
 #define MTDOOPS_KERNMSG_MAGIC 0x5d005d00
-#define OOPS_PAGE_SIZE 4096
+
+static int mtdoops_page_size = 4096;
+module_param(mtdoops_page_size, int, 0);
+MODULE_PARM_DESC(mtdoops_page_size, "page size for MTD OOPS pages in bytes (default 4096)");
+
 
 static struct mtdoops_context {
 	int mtd_index;
@@ -65,8 +70,8 @@ static int mtdoops_erase_block(struct mtdoops_context *cxt, int offset)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	u32 start_page_offset = mtd_div_by_eb(offset, mtd) * mtd->erasesize;
-	u32 start_page = start_page_offset / OOPS_PAGE_SIZE;
-	u32 erase_pages = mtd->erasesize / OOPS_PAGE_SIZE;
+	u32 start_page = start_page_offset / mtdoops_page_size;
+	u32 erase_pages = mtd->erasesize / mtdoops_page_size;
 	struct erase_info erase;
 	DECLARE_WAITQUEUE(wait, current);
 	wait_queue_head_t wait_q;
@@ -134,15 +139,15 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 	if (!mtd)
 		return;
 
-	mod = (cxt->nextpage * OOPS_PAGE_SIZE) % mtd->erasesize;
+	mod = (cxt->nextpage * mtdoops_page_size) % mtd->erasesize;
 	if (mod != 0) {
-		cxt->nextpage = cxt->nextpage + ((mtd->erasesize - mod) / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage + ((mtd->erasesize - mod) / mtdoops_page_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
 	}
 
 	while (mtd->block_isbad) {
-		ret = mtd->block_isbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_isbad(mtd, cxt->nextpage * mtdoops_page_size);
 		if (!ret)
 			break;
 		if (ret < 0) {
@@ -151,19 +156,19 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 		}
 badblock:
 		printk(KERN_WARNING "mtdoops: Bad block at %08x\n",
-				cxt->nextpage * OOPS_PAGE_SIZE);
+				cxt->nextpage * mtdoops_page_size);
 		i++;
-		cxt->nextpage = cxt->nextpage + (mtd->erasesize / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage + (mtd->erasesize / mtdoops_page_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
-		if (i == (cxt->oops_pages / (mtd->erasesize / OOPS_PAGE_SIZE))) {
+		if (i == (cxt->oops_pages / (mtd->erasesize / mtdoops_page_size))) {
 			printk(KERN_ERR "mtdoops: All blocks bad!\n");
 			return;
 		}
 	}
 
 	for (j = 0, ret = -1; (j < 3) && (ret < 0); j++)
-		ret = mtdoops_erase_block(cxt, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtdoops_erase_block(cxt, cxt->nextpage * mtdoops_page_size);
 
 	if (ret >= 0) {
 		printk(KERN_DEBUG "mtdoops: Ready %d, %d \n", cxt->nextpage, cxt->nextcount);
@@ -172,7 +177,7 @@ badblock:
 	}
 
 	if (mtd->block_markbad && (ret == -EIO)) {
-		ret = mtd->block_markbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_markbad(mtd, cxt->nextpage * mtdoops_page_size);
 		if (ret < 0) {
 			printk(KERN_ERR "mtdoops: block_markbad failed, aborting.\n");
 			return;
@@ -187,22 +192,22 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	size_t retlen;
 	int ret;
 
-	if (cxt->writecount < OOPS_PAGE_SIZE)
+	if (cxt->writecount < mtdoops_page_size)
 		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					OOPS_PAGE_SIZE - cxt->writecount);
+					mtdoops_page_size - cxt->writecount);
 
 	if (panic)
-		ret = mtd->panic_write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->panic_write(mtd, cxt->nextpage * mtdoops_page_size,
+					mtdoops_page_size, &retlen, cxt->oops_buf);
 	else
-		ret = mtd->write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->write(mtd, cxt->nextpage * mtdoops_page_size,
+					mtdoops_page_size, &retlen, cxt->oops_buf);
 
 	cxt->writecount = 0;
 
-	if ((retlen != OOPS_PAGE_SIZE) || (ret < 0))
+	if ((retlen != mtdoops_page_size) || (ret < 0))
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
-			cxt->nextpage * OOPS_PAGE_SIZE, retlen,	OOPS_PAGE_SIZE, ret);
+			cxt->nextpage * mtdoops_page_size, retlen,	mtdoops_page_size, ret);
 
 	mtdoops_inc_counter(cxt);
 }
@@ -226,11 +231,11 @@ static void find_next_position(struct mtdoops_context *cxt)
 	for (page = 0; page < cxt->oops_pages; page++) {
 		/* Assume it's dirty */
 		cxt->oops_page_dirty[page] = 1;
-		ret = mtd->read(mtd, page * OOPS_PAGE_SIZE, 8, &retlen, (u_char *) &count[0]);
+		ret = mtd->read(mtd, page * mtdoops_page_size, 8, &retlen, (u_char *) &count[0]);
 
 		if ((retlen != 8) || ((ret < 0) && (ret != -EUCLEAN))) {
 			printk(KERN_ERR "mtdoops: Read failure at %d (%td of 8 read)"
-				", err %d.\n", page * OOPS_PAGE_SIZE, retlen, ret);
+				", err %d.\n", page * mtdoops_page_size, retlen, ret);
 			continue;
 		}
 		if (count[0] == 0xffffffff && count[1] == 0xffffffff)
@@ -273,7 +278,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 	struct mtdoops_context *cxt = &oops_cxt;
 	u64 mtdoops_pages = mtd->size;
 
-	do_div(mtdoops_pages, OOPS_PAGE_SIZE);
+	do_div(mtdoops_pages, mtdoops_page_size);
 
 	if (cxt->name && !strcmp(mtd->name, cxt->name))
 		cxt->mtd_index = mtd->index;
@@ -287,7 +292,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 		return;
 	}
 
-	if (mtd->erasesize < OOPS_PAGE_SIZE) {
+	if (mtd->erasesize < mtdoops_page_size) {
 		printk(KERN_ERR "Eraseblock size of MTD partition %d too small\n",
 				mtd->index);
 		return;
@@ -301,9 +306,9 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 	}
 	cxt->mtd = mtd;
 	if (mtd->size > INT_MAX)
-		cxt->oops_pages = INT_MAX / OOPS_PAGE_SIZE;
+		cxt->oops_pages = INT_MAX / mtdoops_page_size;
 	else
-		cxt->oops_pages = (int)mtd->size / OOPS_PAGE_SIZE;
+		cxt->oops_pages = (int)mtd->size / mtdoops_page_size;
 
 	find_next_position(cxt);
 
@@ -380,15 +385,15 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		cxt->writecount = 8;
 	}
 
-	if ((count + cxt->writecount) > OOPS_PAGE_SIZE)
-		count = OOPS_PAGE_SIZE - cxt->writecount;
+	if ((count + cxt->writecount) > mtdoops_page_size)
+		count = mtdoops_page_size - cxt->writecount;
 
 	memcpy(cxt->oops_buf + cxt->writecount, s, count);
 	cxt->writecount += count;
 
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == OOPS_PAGE_SIZE)
+	if (cxt->writecount == mtdoops_page_size)
 		mtdoops_console_sync();
 }
 
@@ -427,8 +432,16 @@ static int __init mtdoops_console_init(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
 
+	if (!is_power_of_2(mtdoops_page_size)) {
+		printk(KERN_ERR "Error: mtdoops_page_size not a power of two\n");
+		return -EINVAL;
+	}
+	if (mtdoops_page_size < 4096) {
+		printk(KERN_ERR "Error: mtdoops_page_size must be over 4096 bytes\n");
+		return -EINVAL;
+	}
 	cxt->mtd_index = -1;
-	cxt->oops_buf = vmalloc(OOPS_PAGE_SIZE);
+	cxt->oops_buf = vmalloc(mtdoops_page_size);
 	spin_lock_init(&cxt->writecount_lock);
 
 	if (!cxt->oops_buf) {
-- 
1.6.0.4

[PATCH v2 3/3]: mtdoops: store all kernel messages in a circular buffer

[Simon Kagstrom]

The last messages which happens before a crash might contain interesting
information about the crash. This patch reworks mtdoops to keep a
circular buffer of _all_ kernel messages, not just those that are
printed when an oops is initiated.

A handler that is called on panic is also added instead of
mtdoops_console_sync so that panic_on_oops and true panics are stored
(regular oopses are stored via a scheduled work).

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |  116 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 45 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 4f2dc9c..c5c4977 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -53,6 +53,7 @@ static struct mtdoops_context {
 	char *name;
 
 	void *oops_buf;
+	void *oops_buf_write;
 
 	/* writecount and disabling ready are spin lock protected */
 	spinlock_t writecount_lock;
@@ -190,20 +191,36 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	size_t retlen;
+	u32 * stamp;
+	int p;
 	int ret;
 
-	if (cxt->writecount < mtdoops_page_size)
-		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					mtdoops_page_size - cxt->writecount);
+	cxt->ready = 0;
+
+	BUG_ON(cxt->writecount < 8);
+	BUG_ON(cxt->writecount > mtdoops_page_size - 8);
+
+	/* oops_write_buf = [:8] + [writecount:] + [:writecount] */
+	stamp = cxt->oops_buf_write;
+	*stamp++ = cxt->nextcount;
+	*stamp = MTDOOPS_KERNMSG_MAGIC;
+
+	/* Find out the first non-0xff character */
+	for (p = cxt->writecount; p < mtdoops_page_size; p++) {
+		if ( ((u8*)cxt->oops_buf)[p] != 0xff)
+			break;
+	}
+	memcpy(cxt->oops_buf_write + 8, cxt->oops_buf + p,
+			mtdoops_page_size - p);
+	memcpy(cxt->oops_buf_write + 8 + mtdoops_page_size - p,
+			cxt->oops_buf + 8, p - 8);
 
 	if (panic)
 		ret = mtd->panic_write(mtd, cxt->nextpage * mtdoops_page_size,
-					mtdoops_page_size, &retlen, cxt->oops_buf);
+					mtdoops_page_size, &retlen, cxt->oops_buf_write);
 	else
 		ret = mtd->write(mtd, cxt->nextpage * mtdoops_page_size,
-					mtdoops_page_size, &retlen, cxt->oops_buf);
-
-	cxt->writecount = 0;
+					mtdoops_page_size, &retlen, cxt->oops_buf_write);
 
 	if ((retlen != mtdoops_page_size) || (ret < 0))
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
@@ -212,6 +229,26 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	mtdoops_inc_counter(cxt);
 }
 
+static int mtdoops_panic(struct notifier_block *this, unsigned long event,
+		void *ptr)
+{
+	struct mtdoops_context *cxt = &oops_cxt;
+
+	cancel_work_sync(&cxt->work_write);
+	cxt->ready = 0;
+	if (cxt->mtd->panic_write)
+		mtdoops_write(cxt, 1);
+	else
+		printk(KERN_WARNING "mtdoops: panic_write is not defined, "
+					"cannot store dump from panic\n");
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_block = {
+	.notifier_call = mtdoops_panic,
+};
+
 
 static void mtdoops_workfunc_write(struct work_struct *work)
 {
@@ -312,6 +349,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 
 	find_next_position(cxt);
 
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 	printk(KERN_INFO "mtdoops: Attached to MTD device %d\n", mtd->index);
 }
 
@@ -329,28 +367,9 @@ static void mtdoops_notify_remove(struct mtd_info *mtd)
 static void mtdoops_console_sync(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
-	struct mtd_info *mtd = cxt->mtd;
-	unsigned long flags;
-
-	if (!cxt->ready || !mtd || cxt->writecount == 0)
-		return;
 
-	/* 
-	 *  Once ready is 0 and we've held the lock no further writes to the 
-	 *  buffer will happen
-	 */
-	spin_lock_irqsave(&cxt->writecount_lock, flags);
-	if (!cxt->ready) {
-		spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-		return;
-	}
-	cxt->ready = 0;
-	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-
-	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
-		/* Interrupt context, we're going to panic so try and log */
-		mtdoops_write(cxt, 1);
-	else
+	/* Write out the buffer if we are called during an oops */
+	if (oops_in_progress)
 		schedule_work(&cxt->work_write);
 }
 
@@ -359,13 +378,11 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct mtdoops_context *cxt = co->data;
 	struct mtd_info *mtd = cxt->mtd;
+	int copy_from;
+	int copy_wrap = 0;
+	int copy_wrap_diff = 0;
 	unsigned long flags;
 
-	if (!oops_in_progress) {
-		mtdoops_console_sync();
-		return;
-	}
-
 	if (!cxt->ready || !mtd)
 		return;
 
@@ -378,23 +395,21 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		return;
 	}
 
-	if (cxt->writecount == 0) {
-		u32 *stamp = cxt->oops_buf;
-		*stamp++ = cxt->nextcount;
-		*stamp = MTDOOPS_KERNMSG_MAGIC;
+	/* Handle wraps */
+	if ((count + cxt->writecount) >= mtdoops_page_size) {
+		copy_wrap_diff = mtdoops_page_size - cxt->writecount;
+		copy_wrap = cxt->writecount;
+
 		cxt->writecount = 8;
+		count -= copy_wrap_diff;
 	}
-
-	if ((count + cxt->writecount) > mtdoops_page_size)
-		count = mtdoops_page_size - cxt->writecount;
-
-	memcpy(cxt->oops_buf + cxt->writecount, s, count);
+	copy_from = cxt->writecount;
 	cxt->writecount += count;
-
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == mtdoops_page_size)
-		mtdoops_console_sync();
+	if (copy_wrap)
+		memcpy(cxt->oops_buf + copy_wrap, s, copy_wrap_diff);
+	memcpy(cxt->oops_buf + copy_from, s + copy_wrap_diff, count);
 }
 
 static int __init mtdoops_console_setup(struct console *co, char *options)
@@ -440,15 +455,25 @@ static int __init mtdoops_console_init(void)
 		printk(KERN_ERR "Error: mtdoops_page_size must be over 4096 bytes\n");
 		return -EINVAL;
 	}
+	cxt->writecount = 8; /* Start after the header */
 	cxt->mtd_index = -1;
 	cxt->oops_buf = vmalloc(mtdoops_page_size);
 	spin_lock_init(&cxt->writecount_lock);
 
+	cxt->oops_buf = vmalloc(mtdoops_page_size);
 	if (!cxt->oops_buf) {
 		printk(KERN_ERR "Failed to allocate mtdoops buffer workspace\n");
 		return -ENOMEM;
 	}
 	cxt->oops_page_dirty = NULL;
+	cxt->oops_buf_write = vmalloc(mtdoops_page_size);
+	if (!cxt->oops_buf_write) {
+		printk(KERN_ERR "Failed to allocate mtdoops write buffer workspace\n");
+		vfree(cxt->oops_buf);
+		return -ENOMEM;
+	}
+	memset(cxt->oops_buf_write, 0xff, mtdoops_page_size);
+	memset(cxt->oops_buf, 0xff, mtdoops_page_size);
 
 	INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase);
 	INIT_WORK(&cxt->work_write, mtdoops_workfunc_write);
@@ -468,6 +493,7 @@ static void __exit mtdoops_console_exit(void)
 	vfree(cxt->oops_buf);
 	if (cxt->oops_page_dirty)
 		vfree(cxt->oops_page_dirty);
+	vfree(cxt->oops_buf_write);
 }
 
 
-- 
1.6.0.4

Re: [PATCH v2 0/3]: mtdoops fixes and improvements

[Simon Kagstrom]

On Thu, 8 Oct 2009 16:42:43 +0200
Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> Here are a couple of patches to mtdoops which I've been working on for
> a while. They should be applied on top of Aaros fix for panic_on_oops.
> That one technically breaks building mtdoops as a module, so I sent a
> patch to LKML which exports panic_on_oops:
> 
>   http://patchwork.kernel.org/patch/52505/
> 
> The patches here are:

Argh, forgot to run checkpatch on that. I'll post a v3 which has the
checkpatch problems fixed.

Sorry about that.

// Simon

[PATCH v3 0/3]: mtdoops fixes and improvements

[Simon Kagstrom]

Hi!

Here are a couple of patches to mtdoops which I've been working on for
a while. They should be applied on top of Aaros fix for panic_on_oops.
That one technically breaks building mtdoops as a module, so I sent a
patch to LKML which exports panic_on_oops:

  http://patchwork.kernel.org/patch/52505/

The patches here are:

1. Keep track of mtdoops page cleanliness in an array. This allows
   mtdoops_inc_counter to be called during panic (which fails in my
   case, although I believe this is MTD-driver dependent).

2. Make page size configurable to support longer messages. Mainly
   needed for patch 3, but also allows longer messages to be stored
   during panics (when the next oops area cannot be erased).

3. Store all kernel messages in a circular buffer. This changes the
   behavior of mtdoops a bit by constantly recording messages (not only
   on oopses), which are written out on oopses and panics.

   The code is changed to register a panic handler which calls the
   write function, and on normal oopses scheduling a write work to
   store the buffer.

ChangeLog:

v2:
  * Patches have been reordered to keep the least controversial (?) first.

  * Implement Artems suggestion to keep cleanliness in an array

  * Use Aaros patch to fix the panic_on_oops problem

v3:

  * Correct checkpatch issues

// Simon

[PATCH v3 1/3]: mtdoops: Keep track of clean/dirty mtdoops pages in an array

[Simon Kagstrom]

This patch makes mtdoops keep track of clean/dirty pages in an array
instead of scanning the flash after a write. The advantage with this
approach is that it avoids calling mtd->read on a panic, which is not
possible for all mtd drivers.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index ac67833..435961e 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -44,6 +44,7 @@ static struct mtdoops_context {
 	int oops_pages;
 	int nextpage;
 	int nextcount;
+	u8 *oops_page_dirty;
 	char *name;
 
 	void *oops_buf;
@@ -60,12 +61,17 @@ static void mtdoops_erase_callback(struct erase_info *done)
 	wake_up(wait_q);
 }
 
-static int mtdoops_erase_block(struct mtd_info *mtd, int offset)
+static int mtdoops_erase_block(struct mtdoops_context *cxt, int offset)
 {
+	struct mtd_info *mtd = cxt->mtd;
+	u32 start_page_offset = mtd_div_by_eb(offset, mtd) * mtd->erasesize;
+	u32 start_page = start_page_offset / OOPS_PAGE_SIZE;
+	u32 erase_pages = mtd->erasesize / OOPS_PAGE_SIZE;
 	struct erase_info erase;
 	DECLARE_WAITQUEUE(wait, current);
 	wait_queue_head_t wait_q;
 	int ret;
+	int page;
 
 	init_waitqueue_head(&wait_q);
 	erase.mtd = mtd;
@@ -90,16 +96,15 @@ static int mtdoops_erase_block(struct mtd_info *mtd, int offset)
 	schedule();  /* Wait for erase to finish. */
 	remove_wait_queue(&wait_q, &wait);
 
+	/* Mark pages as clean */
+	for (page = start_page; page < start_page + erase_pages; page++)
+		cxt->oops_page_dirty[page] = 0;
+
 	return 0;
 }
 
 static void mtdoops_inc_counter(struct mtdoops_context *cxt)
 {
-	struct mtd_info *mtd = cxt->mtd;
-	size_t retlen;
-	u32 count;
-	int ret;
-
 	cxt->nextpage++;
 	if (cxt->nextpage >= cxt->oops_pages)
 		cxt->nextpage = 0;
@@ -107,18 +112,7 @@ static void mtdoops_inc_counter(struct mtdoops_context *cxt)
 	if (cxt->nextcount == 0xffffffff)
 		cxt->nextcount = 0;
 
-	ret = mtd->read(mtd, cxt->nextpage * OOPS_PAGE_SIZE, 4,
-			&retlen, (u_char *) &count);
-	if ((retlen != 4) || ((ret < 0) && (ret != -EUCLEAN))) {
-		printk(KERN_ERR "mtdoops: Read failure at %d (%td of 4 read)"
-				", err %d.\n", cxt->nextpage * OOPS_PAGE_SIZE,
-				retlen, ret);
-		schedule_work(&cxt->work_erase);
-		return;
-	}
-
-	/* See if we need to erase the next block */
-	if (count != 0xffffffff) {
+	if (cxt->oops_page_dirty[cxt->nextpage]) {
 		schedule_work(&cxt->work_erase);
 		return;
 	}
@@ -169,7 +163,7 @@ badblock:
 	}
 
 	for (j = 0, ret = -1; (j < 3) && (ret < 0); j++)
-		ret = mtdoops_erase_block(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtdoops_erase_block(cxt, cxt->nextpage * OOPS_PAGE_SIZE);
 
 	if (ret >= 0) {
 		printk(KERN_DEBUG "mtdoops: Ready %d, %d \n", cxt->nextpage, cxt->nextcount);
@@ -230,13 +224,18 @@ static void find_next_position(struct mtdoops_context *cxt)
 	size_t retlen;
 
 	for (page = 0; page < cxt->oops_pages; page++) {
+		/* Assume it's dirty */
+		cxt->oops_page_dirty[page] = 1;
 		ret = mtd->read(mtd, page * OOPS_PAGE_SIZE, 8, &retlen, (u_char *) &count[0]);
+
 		if ((retlen != 8) || ((ret < 0) && (ret != -EUCLEAN))) {
 			printk(KERN_ERR "mtdoops: Read failure at %d (%td of 8 read)"
 				", err %d.\n", page * OOPS_PAGE_SIZE, retlen, ret);
 			continue;
 		}
 
+		if (count[0] == 0xffffffff && count[1] == 0xffffffff)
+			cxt->oops_page_dirty[page] = 0;
 		if (count[1] != MTDOOPS_KERNMSG_MAGIC)
 			continue;
 		if (count[0] == 0xffffffff)
@@ -273,6 +272,9 @@ static void find_next_position(struct mtdoops_context *cxt)
 static void mtdoops_notify_add(struct mtd_info *mtd)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
+	u64 mtdoops_pages = mtd->size;
+
+	do_div(mtdoops_pages, OOPS_PAGE_SIZE);
 
 	if (cxt->name && !strcmp(mtd->name, cxt->name))
 		cxt->mtd_index = mtd->index;
@@ -292,6 +294,11 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 		return;
 	}
 
+	cxt->oops_page_dirty = vmalloc(mtdoops_pages);
+	if (!cxt->oops_page_dirty) {
+		printk(KERN_ERR "Could not allocate page array\n");
+		return;
+	}
 	cxt->mtd = mtd;
 	if (mtd->size > INT_MAX)
 		cxt->oops_pages = INT_MAX / OOPS_PAGE_SIZE;
@@ -428,6 +435,7 @@ static int __init mtdoops_console_init(void)
 		printk(KERN_ERR "Failed to allocate mtdoops buffer workspace\n");
 		return -ENOMEM;
 	}
+	cxt->oops_page_dirty = NULL;
 
 	INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase);
 	INIT_WORK(&cxt->work_write, mtdoops_workfunc_write);
@@ -445,6 +453,8 @@ static void __exit mtdoops_console_exit(void)
 	unregister_console(&mtdoops_console);
 	kfree(cxt->name);
 	vfree(cxt->oops_buf);
+	if (cxt->oops_page_dirty)
+		vfree(cxt->oops_page_dirty);
 }
 
 
-- 
1.6.0.4

Re: [PATCH v3 1/3]: mtdoops: Keep track of clean/dirty mtdoops pages in an array

[Artem Bityutskiy]

On Thu, 2009-10-08 at 17:26 +0200, Simon Kagstrom wrote:
> This patch makes mtdoops keep track of clean/dirty pages in an array
> instead of scanning the flash after a write. The advantage with this
> approach is that it avoids calling mtd->read on a panic, which is not
> possible for all mtd drivers.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  drivers/mtd/mtdoops.c |   48 +++++++++++++++++++++++++++++-------------------
>  1 files changed, 29 insertions(+), 19 deletions(-)

> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> index ac67833..435961e 100644
> --- a/drivers/mtd/mtdoops.c
> +++ b/drivers/mtd/mtdoops.c
> @@ -44,6 +44,7 @@ static struct mtdoops_context {
>  	int oops_pages;
>  	int nextpage;
>  	int nextcount;
> +	u8 *oops_page_dirty;
>  	char *name;

So basically, you want to keep a per-page array which contains free/used
mtd oops page status. I find the term "dirty" a bit confusing. Could we
please instead use "used" or "free" or something like that?

Also, is it possible to make it 1 bit per oops page, not 1 byte? Just
less wasteful.

> @@ -428,6 +435,7 @@ static int __init mtdoops_console_init(void)
>  		printk(KERN_ERR "Failed to allocate mtdoops buffer workspace\n");
>  		return -ENOMEM;
>  	}
> +	cxt->oops_page_dirty = NULL;

Minor nitpick: this initialization is unnecessary.

[snip]

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH v3 2/3]: mtdoops: Make page size configurable

[Simon Kagstrom]

The main justification for this is to allow catching long messages
during a panic, where the top part might otherwise be lost since moving
to the next block can require a flash erase.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   83 +++++++++++++++++++++++++++++++-----------------
 1 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 435961e..7045578 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -32,9 +32,14 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/mtd/mtd.h>
+#include <linux/log2.h>
 
 #define MTDOOPS_KERNMSG_MAGIC 0x5d005d00
-#define OOPS_PAGE_SIZE 4096
+
+static int mtdoops_page_size = 4096;
+module_param(mtdoops_page_size, int, 0);
+MODULE_PARM_DESC(mtdoops_page_size,
+		"page size for MTD OOPS pages in bytes (default 4096)");
 
 static struct mtdoops_context {
 	int mtd_index;
@@ -65,8 +70,8 @@ static int mtdoops_erase_block(struct mtdoops_context *cxt, int offset)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	u32 start_page_offset = mtd_div_by_eb(offset, mtd) * mtd->erasesize;
-	u32 start_page = start_page_offset / OOPS_PAGE_SIZE;
-	u32 erase_pages = mtd->erasesize / OOPS_PAGE_SIZE;
+	u32 start_page = start_page_offset / mtdoops_page_size;
+	u32 erase_pages = mtd->erasesize / mtdoops_page_size;
 	struct erase_info erase;
 	DECLARE_WAITQUEUE(wait, current);
 	wait_queue_head_t wait_q;
@@ -134,15 +139,16 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 	if (!mtd)
 		return;
 
-	mod = (cxt->nextpage * OOPS_PAGE_SIZE) % mtd->erasesize;
+	mod = (cxt->nextpage * mtdoops_page_size) % mtd->erasesize;
 	if (mod != 0) {
-		cxt->nextpage = cxt->nextpage + ((mtd->erasesize - mod) / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage +
+				((mtd->erasesize - mod) / mtdoops_page_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
 	}
 
 	while (mtd->block_isbad) {
-		ret = mtd->block_isbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_isbad(mtd, cxt->nextpage * mtdoops_page_size);
 		if (!ret)
 			break;
 		if (ret < 0) {
@@ -151,19 +157,22 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 		}
 badblock:
 		printk(KERN_WARNING "mtdoops: Bad block at %08x\n",
-				cxt->nextpage * OOPS_PAGE_SIZE);
+				cxt->nextpage * mtdoops_page_size);
 		i++;
-		cxt->nextpage = cxt->nextpage + (mtd->erasesize / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage +
+				(mtd->erasesize / mtdoops_page_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
-		if (i == (cxt->oops_pages / (mtd->erasesize / OOPS_PAGE_SIZE))) {
+		if (i == (cxt->oops_pages /
+				(mtd->erasesize / mtdoops_page_size))) {
 			printk(KERN_ERR "mtdoops: All blocks bad!\n");
 			return;
 		}
 	}
 
 	for (j = 0, ret = -1; (j < 3) && (ret < 0); j++)
-		ret = mtdoops_erase_block(cxt, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtdoops_erase_block(cxt,
+				cxt->nextpage * mtdoops_page_size);
 
 	if (ret >= 0) {
 		printk(KERN_DEBUG "mtdoops: Ready %d, %d \n", cxt->nextpage, cxt->nextcount);
@@ -172,7 +181,8 @@ badblock:
 	}
 
 	if (mtd->block_markbad && (ret == -EIO)) {
-		ret = mtd->block_markbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_markbad(mtd,
+				cxt->nextpage * mtdoops_page_size);
 		if (ret < 0) {
 			printk(KERN_ERR "mtdoops: block_markbad failed, aborting.\n");
 			return;
@@ -187,22 +197,25 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	size_t retlen;
 	int ret;
 
-	if (cxt->writecount < OOPS_PAGE_SIZE)
+	if (cxt->writecount < mtdoops_page_size)
 		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					OOPS_PAGE_SIZE - cxt->writecount);
+					mtdoops_page_size - cxt->writecount);
 
 	if (panic)
-		ret = mtd->panic_write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->panic_write(mtd, cxt->nextpage * mtdoops_page_size,
+					mtdoops_page_size, &retlen,
+					cxt->oops_buf);
 	else
-		ret = mtd->write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->write(mtd, cxt->nextpage * mtdoops_page_size,
+					mtdoops_page_size, &retlen,
+					cxt->oops_buf);
 
 	cxt->writecount = 0;
 
-	if ((retlen != OOPS_PAGE_SIZE) || (ret < 0))
+	if ((retlen != mtdoops_page_size) || (ret < 0))
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
-			cxt->nextpage * OOPS_PAGE_SIZE, retlen,	OOPS_PAGE_SIZE, ret);
+			cxt->nextpage * mtdoops_page_size, retlen,
+			mtdoops_page_size, ret);
 
 	mtdoops_inc_counter(cxt);
 }
@@ -226,11 +239,13 @@ static void find_next_position(struct mtdoops_context *cxt)
 	for (page = 0; page < cxt->oops_pages; page++) {
 		/* Assume it's dirty */
 		cxt->oops_page_dirty[page] = 1;
-		ret = mtd->read(mtd, page * OOPS_PAGE_SIZE, 8, &retlen, (u_char *) &count[0]);
+		ret = mtd->read(mtd, page * mtdoops_page_size, 8, &retlen,
+				(u_char *) &count[0]);
 
 		if ((retlen != 8) || ((ret < 0) && (ret != -EUCLEAN))) {
-			printk(KERN_ERR "mtdoops: Read failure at %d (%td of 8 read)"
-				", err %d.\n", page * OOPS_PAGE_SIZE, retlen, ret);
+			printk(KERN_ERR "mtdoops: Read failure at %d (%td of "
+				"8 read), err %d.\n", page * mtdoops_page_size,
+				retlen, ret);
 			continue;
 		}
 
@@ -274,7 +289,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 	struct mtdoops_context *cxt = &oops_cxt;
 	u64 mtdoops_pages = mtd->size;
 
-	do_div(mtdoops_pages, OOPS_PAGE_SIZE);
+	do_div(mtdoops_pages, mtdoops_page_size);
 
 	if (cxt->name && !strcmp(mtd->name, cxt->name))
 		cxt->mtd_index = mtd->index;
@@ -288,7 +303,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 		return;
 	}
 
-	if (mtd->erasesize < OOPS_PAGE_SIZE) {
+	if (mtd->erasesize < mtdoops_page_size) {
 		printk(KERN_ERR "Eraseblock size of MTD partition %d too small\n",
 				mtd->index);
 		return;
@@ -301,9 +316,9 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 	}
 	cxt->mtd = mtd;
 	if (mtd->size > INT_MAX)
-		cxt->oops_pages = INT_MAX / OOPS_PAGE_SIZE;
+		cxt->oops_pages = INT_MAX / mtdoops_page_size;
 	else
-		cxt->oops_pages = (int)mtd->size / OOPS_PAGE_SIZE;
+		cxt->oops_pages = (int)mtd->size / mtdoops_page_size;
 
 	find_next_position(cxt);
 
@@ -380,15 +395,15 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		cxt->writecount = 8;
 	}
 
-	if ((count + cxt->writecount) > OOPS_PAGE_SIZE)
-		count = OOPS_PAGE_SIZE - cxt->writecount;
+	if ((count + cxt->writecount) > mtdoops_page_size)
+		count = mtdoops_page_size - cxt->writecount;
 
 	memcpy(cxt->oops_buf + cxt->writecount, s, count);
 	cxt->writecount += count;
 
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == OOPS_PAGE_SIZE)
+	if (cxt->writecount == mtdoops_page_size)
 		mtdoops_console_sync();
 }
 
@@ -427,8 +442,16 @@ static int __init mtdoops_console_init(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
 
+	if (!is_power_of_2(mtdoops_page_size)) {
+		printk(KERN_ERR "Error: mtdoops_page_size not a power of two\n");
+		return -EINVAL;
+	}
+	if (mtdoops_page_size < 4096) {
+		printk(KERN_ERR "Error: mtdoops_page_size must be over 4096 bytes\n");
+		return -EINVAL;
+	}
 	cxt->mtd_index = -1;
-	cxt->oops_buf = vmalloc(OOPS_PAGE_SIZE);
+	cxt->oops_buf = vmalloc(mtdoops_page_size);
 	spin_lock_init(&cxt->writecount_lock);
 
 	if (!cxt->oops_buf) {
-- 
1.6.0.4

Re: [PATCH v3 2/3]: mtdoops: Make page size configurable

[Artem Bityutskiy]

On Thu, 2009-10-08 at 17:27 +0200, Simon Kagstrom wrote:
> The main justification for this is to allow catching long messages
> during a panic, where the top part might otherwise be lost since moving
> to the next block can require a flash erase.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  drivers/mtd/mtdoops.c |   83 +++++++++++++++++++++++++++++++-----------------
>  1 files changed, 53 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> index 435961e..7045578 100644
> --- a/drivers/mtd/mtdoops.c
> +++ b/drivers/mtd/mtdoops.c
> @@ -32,9 +32,14 @@
>  #include <linux/spinlock.h>
>  #include <linux/interrupt.h>
>  #include <linux/mtd/mtd.h>
> +#include <linux/log2.h>
>  
>  #define MTDOOPS_KERNMSG_MAGIC 0x5d005d00
> -#define OOPS_PAGE_SIZE 4096
> +
> +static int mtdoops_page_size = 4096;
> +module_param(mtdoops_page_size, int, 0);
> +MODULE_PARM_DESC(mtdoops_page_size,
> +		"page size for MTD OOPS pages in bytes (default 4096)");

Term "page" is so overloaded. I'd avoid exporting parameters with "page"
in the name. Could we please call them "records"? At least for the names
we export to users. Of course, ideally you would re-name all symbols in
mtdoops which use "page" term to use "record" term.

Also, the module is called "mtdoops", so it makes little sense to prefix
parameter names with "mtdoops". E.g., if you pass the parameter via the
kernel command line, with current naming you will have:

mtdoops.mtdoops_page_size

Too long, unreadable, confusing :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH v3 3/3]: mtdoops: store all kernel messages in a circular buffer

[Simon Kagstrom]

The last messages which happens before a crash might contain interesting
information about the crash. This patch reworks mtdoops to keep a
circular buffer of _all_ kernel messages, not just those that are
printed when an oops is initiated.

A handler that is called on panic is also added instead of
mtdoops_console_sync so that panic_on_oops and true panics are stored
(regular oopses are stored via a scheduled work).

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |  116 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 45 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 7045578..8134b0c 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -53,6 +53,7 @@ static struct mtdoops_context {
 	char *name;
 
 	void *oops_buf;
+	void *oops_buf_write;
 
 	/* writecount and disabling ready are spin lock protected */
 	spinlock_t writecount_lock;
@@ -195,22 +196,38 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	size_t retlen;
+	u32 *stamp;
+	int p;
 	int ret;
 
-	if (cxt->writecount < mtdoops_page_size)
-		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					mtdoops_page_size - cxt->writecount);
+	cxt->ready = 0;
+
+	BUG_ON(cxt->writecount < 8);
+	BUG_ON(cxt->writecount > mtdoops_page_size - 8);
+
+	/* oops_write_buf = [:8] + [writecount:] + [:writecount] */
+	stamp = cxt->oops_buf_write;
+	*stamp++ = cxt->nextcount;
+	*stamp = MTDOOPS_KERNMSG_MAGIC;
+
+	/* Find out the first non-0xff character */
+	for (p = cxt->writecount; p < mtdoops_page_size; p++) {
+		if (((u8 *)cxt->oops_buf)[p] != 0xff)
+			break;
+	}
+	memcpy(cxt->oops_buf_write + 8, cxt->oops_buf + p,
+			mtdoops_page_size - p);
+	memcpy(cxt->oops_buf_write + 8 + mtdoops_page_size - p,
+			cxt->oops_buf + 8, p - 8);
 
 	if (panic)
 		ret = mtd->panic_write(mtd, cxt->nextpage * mtdoops_page_size,
 					mtdoops_page_size, &retlen,
-					cxt->oops_buf);
+					cxt->oops_buf_write);
 	else
 		ret = mtd->write(mtd, cxt->nextpage * mtdoops_page_size,
 					mtdoops_page_size, &retlen,
-					cxt->oops_buf);
-
-	cxt->writecount = 0;
+					cxt->oops_buf_write);
 
 	if ((retlen != mtdoops_page_size) || (ret < 0))
 		printk(KERN_ERR "mtdoops: Write failure at %d (%td of %d written), err %d.\n",
@@ -220,6 +237,26 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	mtdoops_inc_counter(cxt);
 }
 
+static int mtdoops_panic(struct notifier_block *this, unsigned long event,
+		void *ptr)
+{
+	struct mtdoops_context *cxt = &oops_cxt;
+
+	cancel_work_sync(&cxt->work_write);
+	cxt->ready = 0;
+	if (cxt->mtd->panic_write)
+		mtdoops_write(cxt, 1);
+	else
+		printk(KERN_WARNING "mtdoops: panic_write is not defined, "
+					"cannot store dump from panic\n");
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_block = {
+	.notifier_call = mtdoops_panic,
+};
+
 
 static void mtdoops_workfunc_write(struct work_struct *work)
 {
@@ -322,6 +359,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 
 	find_next_position(cxt);
 
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 	printk(KERN_INFO "mtdoops: Attached to MTD device %d\n", mtd->index);
 }
 
@@ -339,28 +377,9 @@ static void mtdoops_notify_remove(struct mtd_info *mtd)
 static void mtdoops_console_sync(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
-	struct mtd_info *mtd = cxt->mtd;
-	unsigned long flags;
-
-	if (!cxt->ready || !mtd || cxt->writecount == 0)
-		return;
 
-	/* 
-	 *  Once ready is 0 and we've held the lock no further writes to the 
-	 *  buffer will happen
-	 */
-	spin_lock_irqsave(&cxt->writecount_lock, flags);
-	if (!cxt->ready) {
-		spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-		return;
-	}
-	cxt->ready = 0;
-	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-
-	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
-		/* Interrupt context, we're going to panic so try and log */
-		mtdoops_write(cxt, 1);
-	else
+	/* Write out the buffer if we are called during an oops */
+	if (oops_in_progress)
 		schedule_work(&cxt->work_write);
 }
 
@@ -369,13 +388,11 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct mtdoops_context *cxt = co->data;
 	struct mtd_info *mtd = cxt->mtd;
+	int copy_from;
+	int copy_wrap = 0;
+	int copy_wrap_diff = 0;
 	unsigned long flags;
 
-	if (!oops_in_progress) {
-		mtdoops_console_sync();
-		return;
-	}
-
 	if (!cxt->ready || !mtd)
 		return;
 
@@ -388,23 +405,21 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		return;
 	}
 
-	if (cxt->writecount == 0) {
-		u32 *stamp = cxt->oops_buf;
-		*stamp++ = cxt->nextcount;
-		*stamp = MTDOOPS_KERNMSG_MAGIC;
+	/* Handle wraps */
+	if ((count + cxt->writecount) >= mtdoops_page_size) {
+		copy_wrap_diff = mtdoops_page_size - cxt->writecount;
+		copy_wrap = cxt->writecount;
+
 		cxt->writecount = 8;
+		count -= copy_wrap_diff;
 	}
-
-	if ((count + cxt->writecount) > mtdoops_page_size)
-		count = mtdoops_page_size - cxt->writecount;
-
-	memcpy(cxt->oops_buf + cxt->writecount, s, count);
+	copy_from = cxt->writecount;
 	cxt->writecount += count;
-
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == mtdoops_page_size)
-		mtdoops_console_sync();
+	if (copy_wrap)
+		memcpy(cxt->oops_buf + copy_wrap, s, copy_wrap_diff);
+	memcpy(cxt->oops_buf + copy_from, s + copy_wrap_diff, count);
 }
 
 static int __init mtdoops_console_setup(struct console *co, char *options)
@@ -450,15 +465,25 @@ static int __init mtdoops_console_init(void)
 		printk(KERN_ERR "Error: mtdoops_page_size must be over 4096 bytes\n");
 		return -EINVAL;
 	}
+	cxt->writecount = 8; /* Start after the header */
 	cxt->mtd_index = -1;
 	cxt->oops_buf = vmalloc(mtdoops_page_size);
 	spin_lock_init(&cxt->writecount_lock);
 
+	cxt->oops_buf = vmalloc(mtdoops_page_size);
 	if (!cxt->oops_buf) {
 		printk(KERN_ERR "Failed to allocate mtdoops buffer workspace\n");
 		return -ENOMEM;
 	}
 	cxt->oops_page_dirty = NULL;
+	cxt->oops_buf_write = vmalloc(mtdoops_page_size);
+	if (!cxt->oops_buf_write) {
+		printk(KERN_ERR "Failed to allocate mtdoops write buffer workspace\n");
+		vfree(cxt->oops_buf);
+		return -ENOMEM;
+	}
+	memset(cxt->oops_buf_write, 0xff, mtdoops_page_size);
+	memset(cxt->oops_buf, 0xff, mtdoops_page_size);
 
 	INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase);
 	INIT_WORK(&cxt->work_write, mtdoops_workfunc_write);
@@ -478,6 +503,7 @@ static void __exit mtdoops_console_exit(void)
 	vfree(cxt->oops_buf);
 	if (cxt->oops_page_dirty)
 		vfree(cxt->oops_page_dirty);
+	vfree(cxt->oops_buf_write);
 }
 
 
-- 
1.6.0.4

Re: [PATCH v3 0/3]: mtdoops fixes and improvements

[Artem Bityutskiy]

On Thu, 2009-10-08 at 17:25 +0200, Simon Kagstrom wrote:
> Hi!
> 
> Here are a couple of patches to mtdoops which I've been working on for
> a while. They should be applied on top of Aaros fix for panic_on_oops.
> That one technically breaks building mtdoops as a module, so I sent a
> patch to LKML which exports panic_on_oops:
> 
>   http://patchwork.kernel.org/patch/52505/

Ah, you already sent it. Sorry, did not read this carefully.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v3 0/3]: mtdoops fixes and improvements

[Artem Bityutskiy]

On Thu, 2009-10-08 at 17:25 +0200, Simon Kagstrom wrote:
> Hi!
> 
> Here are a couple of patches to mtdoops which I've been working on for
> a while. They should be applied on top of Aaros fix for panic_on_oops.
> That one technically breaks building mtdoops as a module, so I sent a
> patch to LKML which exports panic_on_oops:
> 
>   http://patchwork.kernel.org/patch/52505/
> 
> The patches here are:

I've just pushed a small clean up patch for mtdoops to my l2 tree,
could you send patches on top of that, so that we would not conflict,
please?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH v4 0/3]: mtdoops fixes and improvements

[Simon Kagstrom]

Hi!

Here are a couple of patches to mtdoops which I've been working on for
a while. They should be applied on top of Aaros fix for panic_on_oops.
That one technically breaks building mtdoops as a module, so I sent a
patch to LKML which exports panic_on_oops:

  http://patchwork.kernel.org/patch/52505/

(Artem has also sent the same patch). The patches here are:

1. Avoid erasing already empty areas (the area would be erased at each
   module load otherwise)

2. Keep track of mtdoops page cleanliness in an array. This allows
   mtdoops_inc_counter to be called during panic (which fails in my
   case, although I believe this is MTD-driver dependent).

3. Make page size configurable to support longer messages. Mainly
   needed for patch 3, but also allows longer messages to be stored
   during panics (when the next oops area cannot be erased).

4. Store all kernel messages in a circular buffer. This changes the
   behavior of mtdoops a bit by constantly recording messages (not only
   on oopses), which are written out on oopses and panics.

   The code is changed to register a panic handler which calls the
   write function, and on normal oopses scheduling a write work to
   store the buffer.

ChangeLog:

v2:
  * Patches have been reordered to keep the least controversial (?) first.

  * Implement Artems suggestion to keep cleanliness in an array

  * Use Aaros patch to fix the panic_on_oops problem

v3:

  * Correct checkpatch issues

v4: Review corrections

  * Rebase on top of "[PATCH] mtd: mtdoops: several minor cleanups"

  * Patch 1 has been added

  * Use 1 bit per oops page, and rename clean/dirty unused/used

  * Don't initialize oops_page_dirty (it's a global structure anyway so
    it will be cleared together with the rest of BSS)

  * Rename mtdoops_page_size record_size (although only for the page
    size setting)

[PATCH v4 1/4]: mtdoops: avoid erasing already empty areas

[Simon Kagstrom]

After having scanned the entire mtdoops area, mtdoops will erase it if
there are no mtdoops headers in it. However, empty and already erased
areas (i.e., without mtdoops headers) will therefore also be erased at
each startup.

This patch counts the number of unclean pages (neither empty nor with
the mtdoops header) and only erases if no headers are found and the area
is still unclean.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 18c6c96..c785e1a 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -225,7 +225,7 @@ static void mtdoops_workfunc_write(struct work_struct *work)
 static void find_next_position(struct mtdoops_context *cxt)
 {
 	struct mtd_info *mtd = cxt->mtd;
-	int ret, page, maxpos = 0;
+	int ret, page, maxpos = 0, unclean_pages = 0;
 	u32 count[2], maxcount = 0xffffffff;
 	size_t retlen;
 
@@ -237,10 +237,13 @@ static void find_next_position(struct mtdoops_context *cxt)
 			continue;
 		}
 
-		if (count[1] != MTDOOPS_KERNMSG_MAGIC)
-			continue;
 		if (count[0] == 0xffffffff)
 			continue;
+		if (count[1] != MTDOOPS_KERNMSG_MAGIC) {
+			/* Page is neither clean nor empty */
+			unclean_pages++;
+			continue;
+		}
 		if (maxcount == 0xffffffff) {
 			maxcount = count[0];
 			maxpos = page;
@@ -259,7 +262,14 @@ static void find_next_position(struct mtdoops_context *cxt)
 	if (maxcount == 0xffffffff) {
 		cxt->nextpage = 0;
 		cxt->nextcount = 1;
-		schedule_work(&cxt->work_erase);
+		if (unclean_pages != 0) {
+			printk(KERN_INFO "mtdoops: cleaning area\n");
+			schedule_work(&cxt->work_erase);
+		} else {
+			printk(KERN_DEBUG "mtdoops: ready %d, %d (clean)\n",
+			       cxt->nextpage, cxt->nextcount);
+			cxt->ready = 1;
+		}
 		return;
 	}
 
-- 
1.6.0.4

[PATCH v4 2/4]: mtdoops: Keep track of used/unused mtdoops pages in an array

[Simon Kagstrom]

This patch makes mtdoops keep track of used/unused pages in an array
instead of scanning the flash after a write. The advantage with this
approach is that it avoids calling mtd->read on a panic, which is not
possible for all mtd drivers.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   72 ++++++++++++++++++++++++++++++++++++------------
 1 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index c785e1a..6af340e 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -44,6 +44,7 @@ static struct mtdoops_context {
 	int oops_pages;
 	int nextpage;
 	int nextcount;
+	u32 *oops_page_used;
 	char *name;
 
 	void *oops_buf;
@@ -54,18 +55,47 @@ static struct mtdoops_context {
 	int writecount;
 } oops_cxt;
 
+static void mark_page_used(struct mtdoops_context *cxt, int page)
+{
+	u32 *p = cxt->oops_page_used + page / (8 * sizeof(u32));
+	u32 idx = page % (8 * sizeof(u32));
+
+	*p = *p | (1 << idx);
+}
+
+static void mark_page_unused(struct mtdoops_context *cxt, int page)
+{
+	u32 *p = cxt->oops_page_used + page / (8 * sizeof(u32));
+	u32 idx = page % (8 * sizeof(u32));
+
+	*p = *p & ~(1 << idx);
+}
+
+static int page_is_used(struct mtdoops_context *cxt, int page)
+{
+	u32 *p = cxt->oops_page_used + page / (8 * sizeof(u32));
+	u32 idx = page % (8 * sizeof(u32));
+
+	return *p & (1 << idx);
+}
+
 static void mtdoops_erase_callback(struct erase_info *done)
 {
 	wait_queue_head_t *wait_q = (wait_queue_head_t *)done->priv;
 	wake_up(wait_q);
 }
 
-static int mtdoops_erase_block(struct mtd_info *mtd, int offset)
+static int mtdoops_erase_block(struct mtdoops_context *cxt, int offset)
 {
+	struct mtd_info *mtd = cxt->mtd;
+	u32 start_page_offset = mtd_div_by_eb(offset, mtd) * mtd->erasesize;
+	u32 start_page = start_page_offset / OOPS_PAGE_SIZE;
+	u32 erase_pages = mtd->erasesize / OOPS_PAGE_SIZE;
 	struct erase_info erase;
 	DECLARE_WAITQUEUE(wait, current);
 	wait_queue_head_t wait_q;
 	int ret;
+	int page;
 
 	init_waitqueue_head(&wait_q);
 	erase.mtd = mtd;
@@ -90,16 +120,15 @@ static int mtdoops_erase_block(struct mtd_info *mtd, int offset)
 	schedule();  /* Wait for erase to finish. */
 	remove_wait_queue(&wait_q, &wait);
 
+	/* Mark pages as unused */
+	for (page = start_page; page < start_page + erase_pages; page++)
+		mark_page_unused(cxt, page);
+
 	return 0;
 }
 
 static void mtdoops_inc_counter(struct mtdoops_context *cxt)
 {
-	struct mtd_info *mtd = cxt->mtd;
-	size_t retlen;
-	u32 count;
-	int ret;
-
 	cxt->nextpage++;
 	if (cxt->nextpage >= cxt->oops_pages)
 		cxt->nextpage = 0;
@@ -107,17 +136,7 @@ static void mtdoops_inc_counter(struct mtdoops_context *cxt)
 	if (cxt->nextcount == 0xffffffff)
 		cxt->nextcount = 0;
 
-	ret = mtd->read(mtd, cxt->nextpage * OOPS_PAGE_SIZE, 4,
-			&retlen, (u_char *) &count);
-	if (retlen != 4 || (ret < 0 && ret != -EUCLEAN)) {
-		printk(KERN_ERR "mtdoops: read failure at %d (%td of 4 read), err %d\n",
-		       cxt->nextpage * OOPS_PAGE_SIZE, retlen, ret);
-		schedule_work(&cxt->work_erase);
-		return;
-	}
-
-	/* See if we need to erase the next block */
-	if (count != 0xffffffff) {
+	if (page_is_used(cxt, cxt->nextpage)) {
 		schedule_work(&cxt->work_erase);
 		return;
 	}
@@ -168,7 +187,7 @@ badblock:
 	}
 
 	for (j = 0, ret = -1; (j < 3) && (ret < 0); j++)
-		ret = mtdoops_erase_block(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtdoops_erase_block(cxt, cxt->nextpage * OOPS_PAGE_SIZE);
 
 	if (ret >= 0) {
 		printk(KERN_DEBUG "mtdoops: ready %d, %d\n",
@@ -209,6 +228,7 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	if (retlen != OOPS_PAGE_SIZE || ret < 0)
 		printk(KERN_ERR "mtdoops: write failure at %d (%td of %d written), error %d\n",
 		       cxt->nextpage * OOPS_PAGE_SIZE, retlen, OOPS_PAGE_SIZE, ret);
+	mark_page_used(cxt, cxt->nextpage);
 
 	mtdoops_inc_counter(cxt);
 }
@@ -230,6 +250,8 @@ static void find_next_position(struct mtdoops_context *cxt)
 	size_t retlen;
 
 	for (page = 0; page < cxt->oops_pages; page++) {
+		/* Assume the page is used */
+		mark_page_used(cxt, page);
 		ret = mtd->read(mtd, page * OOPS_PAGE_SIZE, 8, &retlen, (u_char *) &count[0]);
 		if (retlen != 8 || (ret < 0 && ret != -EUCLEAN)) {
 			printk(KERN_ERR "mtdoops: read failure at %d (%td of 8 read), err %d\n",
@@ -237,6 +259,8 @@ static void find_next_position(struct mtdoops_context *cxt)
 			continue;
 		}
 
+		if (count[0] == 0xffffffff && count[1] == 0xffffffff)
+			mark_page_unused(cxt, page);
 		if (count[0] == 0xffffffff)
 			continue;
 		if (count[1] != MTDOOPS_KERNMSG_MAGIC) {
@@ -283,6 +307,9 @@ static void find_next_position(struct mtdoops_context *cxt)
 static void mtdoops_notify_add(struct mtd_info *mtd)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
+	u64 mtdoops_pages = mtd->size;
+
+	do_div(mtdoops_pages, OOPS_PAGE_SIZE);
 
 	if (cxt->name && !strcmp(mtd->name, cxt->name))
 		cxt->mtd_index = mtd->index;
@@ -302,6 +329,13 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 		return;
 	}
 
+	/* oops_page_used is a bit field */
+	cxt->oops_page_used = vmalloc(DIV_ROUND_UP(mtdoops_pages,
+			8 * sizeof(u32)));
+	if (!cxt->oops_page_used) {
+		printk(KERN_ERR "Could not allocate page array\n");
+		return;
+	}
 	cxt->mtd = mtd;
 	if (mtd->size > INT_MAX)
 		cxt->oops_pages = INT_MAX / OOPS_PAGE_SIZE;
@@ -454,6 +488,8 @@ static void __exit mtdoops_console_exit(void)
 	unregister_console(&mtdoops_console);
 	kfree(cxt->name);
 	vfree(cxt->oops_buf);
+	if (cxt->oops_page_used)
+		vfree(cxt->oops_page_used);
 }
 
 
-- 
1.6.0.4

[PATCH v4 3/4]: mtdoops: Make page (record) size configurable

[Simon Kagstrom]

The main justification for this is to allow catching long messages
during a panic, where the top part might otherwise be lost since moving
to the next block can require a flash erase.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   71 +++++++++++++++++++++++++++++--------------------
 1 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 6af340e..4f0a1fc 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -32,9 +32,14 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/mtd/mtd.h>
+#include <linux/log2.h>
 
 #define MTDOOPS_KERNMSG_MAGIC 0x5d005d00
-#define OOPS_PAGE_SIZE 4096
+
+static int record_size = 4096;
+module_param(record_size, int, 0);
+MODULE_PARM_DESC(record_size,
+		"record size for MTD OOPS pages in bytes (default 4096)");
 
 static struct mtdoops_context {
 	int mtd_index;
@@ -89,8 +94,8 @@ static int mtdoops_erase_block(struct mtdoops_context *cxt, int offset)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	u32 start_page_offset = mtd_div_by_eb(offset, mtd) * mtd->erasesize;
-	u32 start_page = start_page_offset / OOPS_PAGE_SIZE;
-	u32 erase_pages = mtd->erasesize / OOPS_PAGE_SIZE;
+	u32 start_page = start_page_offset / record_size;
+	u32 erase_pages = mtd->erasesize / record_size;
 	struct erase_info erase;
 	DECLARE_WAITQUEUE(wait, current);
 	wait_queue_head_t wait_q;
@@ -158,15 +163,15 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 	if (!mtd)
 		return;
 
-	mod = (cxt->nextpage * OOPS_PAGE_SIZE) % mtd->erasesize;
+	mod = (cxt->nextpage * record_size) % mtd->erasesize;
 	if (mod != 0) {
-		cxt->nextpage = cxt->nextpage + ((mtd->erasesize - mod) / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage + ((mtd->erasesize - mod) / record_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
 	}
 
 	while (mtd->block_isbad) {
-		ret = mtd->block_isbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_isbad(mtd, cxt->nextpage * record_size);
 		if (!ret)
 			break;
 		if (ret < 0) {
@@ -175,19 +180,19 @@ static void mtdoops_workfunc_erase(struct work_struct *work)
 		}
 badblock:
 		printk(KERN_WARNING "mtdoops: bad block at %08x\n",
-		       cxt->nextpage * OOPS_PAGE_SIZE);
+		       cxt->nextpage * record_size);
 		i++;
-		cxt->nextpage = cxt->nextpage + (mtd->erasesize / OOPS_PAGE_SIZE);
+		cxt->nextpage = cxt->nextpage + (mtd->erasesize / record_size);
 		if (cxt->nextpage >= cxt->oops_pages)
 			cxt->nextpage = 0;
-		if (i == cxt->oops_pages / (mtd->erasesize / OOPS_PAGE_SIZE)) {
+		if (i == cxt->oops_pages / (mtd->erasesize / record_size)) {
 			printk(KERN_ERR "mtdoops: all blocks bad!\n");
 			return;
 		}
 	}
 
 	for (j = 0, ret = -1; (j < 3) && (ret < 0); j++)
-		ret = mtdoops_erase_block(cxt, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtdoops_erase_block(cxt, cxt->nextpage * record_size);
 
 	if (ret >= 0) {
 		printk(KERN_DEBUG "mtdoops: ready %d, %d\n",
@@ -197,7 +202,7 @@ badblock:
 	}
 
 	if (mtd->block_markbad && ret == -EIO) {
-		ret = mtd->block_markbad(mtd, cxt->nextpage * OOPS_PAGE_SIZE);
+		ret = mtd->block_markbad(mtd, cxt->nextpage * record_size);
 		if (ret < 0) {
 			printk(KERN_ERR "mtdoops: block_markbad failed, aborting\n");
 			return;
@@ -212,22 +217,22 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	size_t retlen;
 	int ret;
 
-	if (cxt->writecount < OOPS_PAGE_SIZE)
+	if (cxt->writecount < record_size)
 		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					OOPS_PAGE_SIZE - cxt->writecount);
+					record_size - cxt->writecount);
 
 	if (panic)
-		ret = mtd->panic_write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->panic_write(mtd, cxt->nextpage * record_size,
+					record_size, &retlen, cxt->oops_buf);
 	else
-		ret = mtd->write(mtd, cxt->nextpage * OOPS_PAGE_SIZE,
-					OOPS_PAGE_SIZE, &retlen, cxt->oops_buf);
+		ret = mtd->write(mtd, cxt->nextpage * record_size,
+					record_size, &retlen, cxt->oops_buf);
 
 	cxt->writecount = 0;
 
-	if (retlen != OOPS_PAGE_SIZE || ret < 0)
+	if (retlen != record_size || ret < 0)
 		printk(KERN_ERR "mtdoops: write failure at %d (%td of %d written), error %d\n",
-		       cxt->nextpage * OOPS_PAGE_SIZE, retlen, OOPS_PAGE_SIZE, ret);
+		       cxt->nextpage * record_size, retlen, record_size, ret);
 	mark_page_used(cxt, cxt->nextpage);
 
 	mtdoops_inc_counter(cxt);
@@ -252,10 +257,10 @@ static void find_next_position(struct mtdoops_context *cxt)
 	for (page = 0; page < cxt->oops_pages; page++) {
 		/* Assume the page is used */
 		mark_page_used(cxt, page);
-		ret = mtd->read(mtd, page * OOPS_PAGE_SIZE, 8, &retlen, (u_char *) &count[0]);
+		ret = mtd->read(mtd, page * record_size, 8, &retlen, (u_char *) &count[0]);
 		if (retlen != 8 || (ret < 0 && ret != -EUCLEAN)) {
 			printk(KERN_ERR "mtdoops: read failure at %d (%td of 8 read), err %d\n",
-			       page * OOPS_PAGE_SIZE, retlen, ret);
+			       page * record_size, retlen, ret);
 			continue;
 		}
 
@@ -309,7 +314,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 	struct mtdoops_context *cxt = &oops_cxt;
 	u64 mtdoops_pages = mtd->size;
 
-	do_div(mtdoops_pages, OOPS_PAGE_SIZE);
+	do_div(mtdoops_pages, record_size);
 
 	if (cxt->name && !strcmp(mtd->name, cxt->name))
 		cxt->mtd_index = mtd->index;
@@ -323,7 +328,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 		return;
 	}
 
-	if (mtd->erasesize < OOPS_PAGE_SIZE) {
+	if (mtd->erasesize < record_size) {
 		printk(KERN_ERR "mtdoops: eraseblock size of MTD partition %d too small\n",
 		       mtd->index);
 		return;
@@ -338,9 +343,9 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 	}
 	cxt->mtd = mtd;
 	if (mtd->size > INT_MAX)
-		cxt->oops_pages = INT_MAX / OOPS_PAGE_SIZE;
+		cxt->oops_pages = INT_MAX / record_size;
 	else
-		cxt->oops_pages = (int)mtd->size / OOPS_PAGE_SIZE;
+		cxt->oops_pages = (int)mtd->size / record_size;
 
 	find_next_position(cxt);
 
@@ -417,15 +422,15 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		cxt->writecount = 8;
 	}
 
-	if (count + cxt->writecount > OOPS_PAGE_SIZE)
-		count = OOPS_PAGE_SIZE - cxt->writecount;
+	if (count + cxt->writecount > record_size)
+		count = record_size - cxt->writecount;
 
 	memcpy(cxt->oops_buf + cxt->writecount, s, count);
 	cxt->writecount += count;
 
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == OOPS_PAGE_SIZE)
+	if (cxt->writecount == record_size)
 		mtdoops_console_sync();
 }
 
@@ -464,8 +469,16 @@ static int __init mtdoops_console_init(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
 
+	if (!is_power_of_2(record_size)) {
+		printk(KERN_ERR "mtdoops: record_size must be a power of two\n");
+		return -EINVAL;
+	}
+	if (record_size < 4096) {
+		printk(KERN_ERR "mtdoops: record_size must be over 4096 bytes\n");
+		return -EINVAL;
+	}
 	cxt->mtd_index = -1;
-	cxt->oops_buf = vmalloc(OOPS_PAGE_SIZE);
+	cxt->oops_buf = vmalloc(record_size);
 	if (!cxt->oops_buf) {
 		printk(KERN_ERR "mtdoops: failed to allocate buffer workspace\n");
 		return -ENOMEM;
-- 
1.6.0.4

[PATCH v4 4/4]: mtdoops: store all kernel messages in a circular buffer

[Simon Kagstrom]

The last messages which happens before a crash might contain interesting
information about the crash. This patch reworks mtdoops to keep a
circular buffer of _all_ kernel messages, not just those that are
printed when an oops is initiated.

A handler that is called on panic is also added instead of
mtdoops_console_sync so that panic_on_oops and true panics are stored
(regular oopses are stored via a scheduled work).

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |  121 +++++++++++++++++++++++++++++-------------------
 1 files changed, 73 insertions(+), 48 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 4f0a1fc..b9dae40 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -53,6 +53,7 @@ static struct mtdoops_context {
 	char *name;
 
 	void *oops_buf;
+	void *oops_buf_write;
 
 	/* writecount and disabling ready are spin lock protected */
 	spinlock_t writecount_lock;
@@ -215,20 +216,36 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	size_t retlen;
+	u32 *stamp;
+	int p;
 	int ret;
 
-	if (cxt->writecount < record_size)
-		memset(cxt->oops_buf + cxt->writecount, 0xff,
-					record_size - cxt->writecount);
+	cxt->ready = 0;
+
+	BUG_ON(cxt->writecount < 8);
+	BUG_ON(cxt->writecount > record_size - 8);
+
+	/* oops_write_buf = [:8] + [writecount:] + [:writecount] */
+	stamp = cxt->oops_buf_write;
+	*stamp++ = cxt->nextcount;
+	*stamp = MTDOOPS_KERNMSG_MAGIC;
+
+	/* Find out the first non-0xff character */
+	for (p = cxt->writecount; p < record_size; p++) {
+		if (((u8 *)cxt->oops_buf)[p] != 0xff)
+			break;
+	}
+	memcpy(cxt->oops_buf_write + 8, cxt->oops_buf + p,
+			record_size - p);
+	memcpy(cxt->oops_buf_write + 8 + record_size - p,
+			cxt->oops_buf + 8, p - 8);
 
 	if (panic)
 		ret = mtd->panic_write(mtd, cxt->nextpage * record_size,
-					record_size, &retlen, cxt->oops_buf);
+					record_size, &retlen, cxt->oops_buf_write);
 	else
 		ret = mtd->write(mtd, cxt->nextpage * record_size,
-					record_size, &retlen, cxt->oops_buf);
-
-	cxt->writecount = 0;
+					record_size, &retlen, cxt->oops_buf_write);
 
 	if (retlen != record_size || ret < 0)
 		printk(KERN_ERR "mtdoops: write failure at %d (%td of %d written), error %d\n",
@@ -238,6 +255,26 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 	mtdoops_inc_counter(cxt);
 }
 
+static int mtdoops_panic(struct notifier_block *this, unsigned long event,
+		void *ptr)
+{
+	struct mtdoops_context *cxt = &oops_cxt;
+
+	cancel_work_sync(&cxt->work_write);
+	cxt->ready = 0;
+	if (cxt->mtd->panic_write)
+		mtdoops_write(cxt, 1);
+	else
+		printk(KERN_WARNING "mtdoops: panic_write is not defined, "
+					"cannot store dump from panic\n");
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block panic_block = {
+	.notifier_call = mtdoops_panic,
+};
+
 
 static void mtdoops_workfunc_write(struct work_struct *work)
 {
@@ -312,9 +349,9 @@ static void find_next_position(struct mtdoops_context *cxt)
 static void mtdoops_notify_add(struct mtd_info *mtd)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
-	u64 mtdoops_pages = mtd->size;
+	u64 mtdoops_records = mtd->size;
 
-	do_div(mtdoops_pages, record_size);
+	do_div(mtdoops_records, record_size);
 
 	if (cxt->name && !strcmp(mtd->name, cxt->name))
 		cxt->mtd_index = mtd->index;
@@ -335,7 +372,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 	}
 
 	/* oops_page_used is a bit field */
-	cxt->oops_page_used = vmalloc(DIV_ROUND_UP(mtdoops_pages,
+	cxt->oops_page_used = vmalloc(DIV_ROUND_UP(mtdoops_records,
 			8 * sizeof(u32)));
 	if (!cxt->oops_page_used) {
 		printk(KERN_ERR "Could not allocate page array\n");
@@ -349,6 +386,7 @@ static void mtdoops_notify_add(struct mtd_info *mtd)
 
 	find_next_position(cxt);
 
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 	printk(KERN_INFO "mtdoops: Attached to MTD device %d\n", mtd->index);
 }
 
@@ -366,28 +404,9 @@ static void mtdoops_notify_remove(struct mtd_info *mtd)
 static void mtdoops_console_sync(void)
 {
 	struct mtdoops_context *cxt = &oops_cxt;
-	struct mtd_info *mtd = cxt->mtd;
-	unsigned long flags;
 
-	if (!cxt->ready || !mtd || cxt->writecount == 0)
-		return;
-
-	/*
-	 *  Once ready is 0 and we've held the lock no further writes to the
-	 *  buffer will happen
-	 */
-	spin_lock_irqsave(&cxt->writecount_lock, flags);
-	if (!cxt->ready) {
-		spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-		return;
-	}
-	cxt->ready = 0;
-	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
-
-	if (mtd->panic_write && (in_interrupt() || panic_on_oops))
-		/* Interrupt context, we're going to panic so try and log */
-		mtdoops_write(cxt, 1);
-	else
+	/* Write out the buffer if we are called during an oops */
+	if (oops_in_progress)
 		schedule_work(&cxt->work_write);
 }
 
@@ -396,13 +415,11 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct mtdoops_context *cxt = co->data;
 	struct mtd_info *mtd = cxt->mtd;
+	int copy_from;
+	int copy_wrap = 0;
+	int copy_wrap_diff = 0;
 	unsigned long flags;
 
-	if (!oops_in_progress) {
-		mtdoops_console_sync();
-		return;
-	}
-
 	if (!cxt->ready || !mtd)
 		return;
 
@@ -415,23 +432,21 @@ mtdoops_console_write(struct console *co, const char *s, unsigned int count)
 		return;
 	}
 
-	if (cxt->writecount == 0) {
-		u32 *stamp = cxt->oops_buf;
-		*stamp++ = cxt->nextcount;
-		*stamp = MTDOOPS_KERNMSG_MAGIC;
+	/* Handle wraps */
+	if ((count + cxt->writecount) >= record_size) {
+		copy_wrap_diff = record_size - cxt->writecount;
+		copy_wrap = cxt->writecount;
+
 		cxt->writecount = 8;
+		count -= copy_wrap_diff;
 	}
-
-	if (count + cxt->writecount > record_size)
-		count = record_size - cxt->writecount;
-
-	memcpy(cxt->oops_buf + cxt->writecount, s, count);
+	copy_from = cxt->writecount;
 	cxt->writecount += count;
-
 	spin_unlock_irqrestore(&cxt->writecount_lock, flags);
 
-	if (cxt->writecount == record_size)
-		mtdoops_console_sync();
+	if (copy_wrap)
+		memcpy(cxt->oops_buf + copy_wrap, s, copy_wrap_diff);
+	memcpy(cxt->oops_buf + copy_from, s + copy_wrap_diff, count);
 }
 
 static int __init mtdoops_console_setup(struct console *co, char *options)
@@ -477,12 +492,21 @@ static int __init mtdoops_console_init(void)
 		printk(KERN_ERR "mtdoops: record_size must be over 4096 bytes\n");
 		return -EINVAL;
 	}
+	cxt->writecount = 8; /* Start after the header */
 	cxt->mtd_index = -1;
 	cxt->oops_buf = vmalloc(record_size);
 	if (!cxt->oops_buf) {
 		printk(KERN_ERR "mtdoops: failed to allocate buffer workspace\n");
 		return -ENOMEM;
 	}
+	cxt->oops_buf_write = vmalloc(record_size);
+	if (!cxt->oops_buf_write) {
+		printk(KERN_ERR "Failed to allocate mtdoops write buffer workspace\n");
+		vfree(cxt->oops_buf);
+		return -ENOMEM;
+	}
+	memset(cxt->oops_buf_write, 0xff, record_size);
+	memset(cxt->oops_buf, 0xff, record_size);
 
 	spin_lock_init(&cxt->writecount_lock);
 	INIT_WORK(&cxt->work_erase, mtdoops_workfunc_erase);
@@ -503,6 +527,7 @@ static void __exit mtdoops_console_exit(void)
 	vfree(cxt->oops_buf);
 	if (cxt->oops_page_used)
 		vfree(cxt->oops_page_used);
+	vfree(cxt->oops_buf_write);
 }
 
 
-- 
1.6.0.4