From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.nokia.com ([192.100.105.134] helo=mgw-mx09.nokia.com) by bombadil.infradead.org with esmtps (Exim 4.69 #1 (Red Hat Linux)) id 1MvlY7-0000cJ-RC for linux-mtd@lists.infradead.org; Thu, 08 Oct 2009 05:28:27 +0000 Subject: Re: [PATCH 2/3]: mtdoops: Use panic_write if panic_on_oops is set From: Artem Bityutskiy To: Simon Kagstrom In-Reply-To: <20091002160652.5581c13c@marrow.netinsight.se> References: <20091002160510.191ef5a4@marrow.netinsight.se> <20091002160652.5581c13c@marrow.netinsight.se> Content-Type: text/plain; charset="UTF-8" Date: Thu, 08 Oct 2009 08:28:12 +0300 Message-Id: <1254979692.3359.32.camel@localhost> Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Cc: Artem.Bityutskiy@nokia.com, linux-mtd , Aaro Koskinen Reply-To: dedekind1@gmail.com List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 > --- > 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 (Артём Битюцкий)