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

[Artem Bityutskiy <dedekind1@gmail.com>]

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