Messages in this thread | | | Date | Mon, 12 Oct 2009 13:37:58 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] panic.c: export panic_on_oops |
| |
* Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2009-10-12 at 13:15 +0200, ext Ingo Molnar wrote: > > > We use the mtdoops module which stores oopses on the Flash partition, > > > in order to make it possible to analyze them later. And mtdoops needs > > > to know whether 'panic_on_oops' is on or off. Thus, we need this > > > variable to be exported. > > > > hm, why does it need it? Could you send the patch please that makes use > > of it (as an easy way to explain the need for the export) - current > > upstream drivers/mtd/mtdoops.c doesnt need it so it must be some pending > > change. > > Yes, current drivers/mtd/mtdoops.c does not need it, but we have this > patch: > > http://lists.infradead.org/pipermail/linux-mtd/2009-October/027450.html > > which makes sure mtdoops writes the oops log immediately, because we > have 'panic_on_oops' set so this is our last chance to save the oops. > > Here is the patch inlined as well: > > Author: Aaro Koskinen <aaro.koskinen@nokia.com> > Date: Thu Oct 1 18:16:55 2009 +0300 > > mtd: mtdoops: do not schedule work if we are going to die > > If panic_on_oops is set, the log should be written right away > because this is not going to be a second chance. > > Signed-off-by: Aaro Koskinen <aaro.koskinen@nokia.com> > Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com> > > diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c > index 1060337..ac67833 100644 > --- a/drivers/mtd/mtdoops.c > +++ b/drivers/mtd/mtdoops.c > @@ -335,7 +335,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);
Hm, the code seems to be somewhat confused about this. It tries to guess when it's panic-ing, right? in_interrupt() is the wrong test for that.
mtdoops_console_sync() gets called by regular printk() as well via:
static void mtdoops_console_write(struct console *co, const char *s, unsigned int count) { struct mtdoops_context *cxt = co->data; struct mtd_info *mtd = cxt->mtd; unsigned long flags;
if (!oops_in_progress) { mtdoops_console_sync(); return;
I think this is all layered incorrectly - because mtdoops (which is a _VERY_ cool debugging facility btw - i wish generic x86 had something like that) tries to be a 'driver' and tries to achieve these things without modifying the core kernel.
But it really should do that. We can certainly enhance the core kernel logic to tell the console driver more clearly when printk should go out immediately.
Instead of using oops_in_progress it might be better to go into 'sync immeditately' mode if the kernel gets tainted. Add a callback for that to struct console (in include/linux/console.h). The ->unblank() callback is already such a "user attention needed immediately" callback. We could add a ->kernel_bug() callback to that.
Ingo
| |