Bug 14102 - mtdoops panic dump to a device without panic_write() function doesn't work.
Summary: mtdoops panic dump to a device without panic_write() function doesn't work.
Status: CLOSED OBSOLETE
Alias: None
Product: Drivers
Classification: Unclassified
Component: Flash/Memory Technology Devices (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: David Woodhouse
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-01 10:38 UTC by Matthew Lear
Modified: 2012-06-13 16:14 UTC (History)
3 users (show)

See Also:
Kernel Version: 2.6.29
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Matthew Lear 2009-09-01 10:38:43 UTC
Hello - this issue has been discussed on the mtd mailing list but I wanted to raise it here as I believe it is the right thing to do, mostly because from what I can tell, the mtd oops code is not behaving correctly when executing on a non pre-emptive kernel (my system - linux 2.6.29 m68k coldfire with MMU, non preemptive kernel).

The full email thread can be found here:
http://lists.infradead.org/pipermail/linux-mtd/2009-August/027063.html

I'm not completely sure regarding the etiquette of pasting the email discussion into bugzilla, so if this is required I apologise. I appreciate that all the information should go into a bug entry so if lack of this is preventing the bug being addressed, please let me know.

Basically:

I'm trying to use the mtd oops feature on a non pre-emptible m68k
coldfire Linux kernel. Problem is, there is nothing being written to
flash upon a kernel panic. I'm passing the correct syntax on the command
line and I can see the kernel messages indicating that everything is ok, ie:

/ # dmesg | grep -i mtd
[    0.000000] Kernel command line: console=ttyS0,115200 ip=dhcp
root=/dev/nfs nfsroot=192.168.0.2:/home/matt/nfs/evb/rootfs/ rw
mtdparts=physmap-flash.0:4M@0x80000(kernel)ro,5M@0x480000(r
amdisk),-@0x980000(writeable) console=ttyMTD2
[    0.336920] console [ttyMTD2] enabled
[    0.933655] 3 cmdlinepart partitions found on MTD device physmap-flash.0
[    0.940745] Creating 3 MTD partitions on "physmap-flash.0":
[    0.951893] mtd: Giving out device 0 to kernel
[    0.965760] mtd: Giving out device 1 to ramdisk
[    0.979124] mtd: Giving out device 2 to writeable
[    0.993942] mtdoops: Ready 8, 9 (no erase)
[    0.993979] mtdoops: Attached to MTD device 2

The issue appears to be that the call to schedule_work() in
mtdoops_console_sync() does not result in mtdoops_workfunc_write() being
invoked to perform the flash write.

I understand that there is only a very small window in which to actually
flush the buffer to flash (involving a spinlock and testing
oops_in_progress). However, from what I can tell, on a non pre-emptible
kernel, after panic() does it's business of calling bust_spinlocks(),
which calls console_unblank(), which calls mtdoops_console_sync(), which
calls schedule_work(), there is no way [certainly that I can see] that
the system can context switch to the worker thread in order to invoke
the routine that actually writes to flash.

The only way I have been able to get the flash to be written to upon
panic is to make the following change:

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 1a6b3be..2d734e2 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -335,7 +335,7 @@ static void mtdoops_console_sync(void)
                /* Interrupt context, we're going to panic so try and log */
                mtdoops_write(cxt, 1);
        else
-               schedule_work(&cxt->work_write);
+               mtdoops_write(cxt, 0);
 }

 static void

I can pull out the logs just fine in user space using oopslog, courtesy
of Mr Purdie.

I can replicate the a similar situation on my system with a simple kernel
module. The module_init routine uses INIT_WORK() to prepare a routine for execution in the work thread context. The routine simply invokes panic(). Following INIT_WORK(), schedule_work() is used to set up the routine for execution. Immediately after the call to schedule_work() the code then sits in an endless for loop with an mdelay(1).

Running on my non-preemptive kernel, when the module is insmod, the system hangs and does not panic. If schedule() is called after schedule_work() and before entry to the for loop, the system panics. Running on a preemptive kernel, the call to schedule() is not required in order for the system to panic.

In mtdoops.c, I have tried invoking schedule() immediately after schedule_work() in mtdoops_console_sync(). However this still results in flash not being written to when there is a panic. I realise that now there are of course other factors at play which could explain why the flash never gets
written to, (specifically related to scheduling, eg thread time slices,
quantums etc), but I wanted to point out that there certainly appears to
be a couple of cases in mtdoops.c where there should be calls to
schedule() after calls to schedule_work() - only there aren't.

Obviously calling schedule() in the context of a panic is to be avoided.

Richard (Purdie) suggested the following change for mtdoops.c:

if (mtd->panic_write && in_interrupt())
	/* Interrupt context, we're going to panic so try and log */
	mtdoops_write(cxt, 1);
else if (in_interrupt())
	/* Interrupt context but with no panic write function */
        /* We're going to crash anyway so we may as well try and log */
	mtdoops_write(cxt, 0);
else
	schedule_work(&cxt->work_write);

The mtd driver I'm using does not have a panic_write() function and from what I can tell the execution is never in interrupt context, so the call to schedule_work() is always invoked.

The only way I can get 100% reliable writes to flash upon panic in a non-preemptive system is to apply my patch above. This obviously negates the requirement for the async flash write but this mechanism was clearly implemented for a reason.
Comment 1 Matthew Lear 2010-01-06 12:31:40 UTC
Just wondered if there were any thoughts on this? Don't mean to be rude but I expected to hear something within 3 months...
Comment 2 David Woodhouse 2010-01-06 13:11:10 UTC
We've revamped the mtdoops code recently; it's worth retesting with 2.6.33-rc2.

But...

> The mtd driver I'm using does not have a panic_write() function...
> The only way I can get 100% reliable writes to flash upon panic...

I suspect you'll just hit:
        /* Panics must be written immediately */
        if (reason != KMSG_DUMP_OOPS) {
                if (!cxt->mtd->panic_write)
                        printk(KERN_ERR "mtdoops: Cannot write from panic without panic_write\n");


Surely the way to get reliable writes to flash upon panic is to provide a panic_write() function? Perhaps I'm being dim, but I'm kind of missing the point about why the behaviour you describe is at all unexpected.
Comment 3 Matthew Lear 2010-01-06 13:34:31 UTC
(In reply to comment #2)
> Surely the way to get reliable writes to flash upon panic is to provide a
> panic_write() function? Perhaps I'm being dim, but I'm kind of missing the
> point about why the behaviour you describe is at all unexpected.

Yes, absolutely fair enough :-) Unfortunately it's not possible for me to test with 2.6.33-rc2 at the current time... That said, the code that I was running was written in a way which seemed that reliable writes to flash without a panic_write() function could still be performed. As I reported, this wasn't the case. The only way to absolutely guarantee that flash could be written to upon panic was to have a panic_write() function as you say. Given the fact that it seemed like it may or may not work even if there was no panic_write() function seemed wrong to me, though. Logically, I'd have thought that if you want to write to flash upon panic then you must install/have a panic_write() function. If not, then perhaps it would be better to indicate an error at mtdoops initialisation..? I suppose it keeps things flexible, though...

I've not looked at the current source so my comments may well be totally irrelevant and/or superfluous now...

Note You need to log in before you can comment on or make changes to this bug.