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

[Artem Bityutskiy <dedekind1@gmail.com>]

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 (Артём Битюцкий)