Re: [PATCH] panic.c: export panic_on_oops

Ingo Molnar
2009-10-12T12:10:06Z

* Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> (Risking that Artem also replies, I'll bite on this one! Let's hope we
> agree at least :-))
>
> On Mon, 12 Oct 2009 13:37:58 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
>
> > > - 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.
>
> Well, the main reason is to get the write done directly if we know
> we're going to crash. The rest of the code around the patch looks like
> this:
>
> 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
> schedule_work(&cxt->work_write);
>
> so if we're oopsing in interrupt context or are going to panic, we
> just write directly. mtdoops_write will then use mtd->panic_write if
> it's available to get the write done immediately without sleeping.

but i'm not sure that code achieves your intention.

in_interrupt() is a generic test. It will be true whenever you printk in
irq context - be that a panic or not a panic.

Also, the panic_on_oops usage looks wrong as well: it is set on a system
that wants a panic on oops - but the flag will be set all the time, even
when we are not oopsing.

I suppose the intention is to add a logic like this:

- buffer writes to the MTD async writeout thread for regular printks

- if we are in some sort of emergency, write to the MTD device directly
as we cannot buffer anymore.

Correct?

> [...]
>
> To handle the panic case, I've simply added a panic notifier which
> does
>
> static int mtdoops_panic(struct notifier_block *this, unsigned long event,
> void *ptr)
> {
> struct mtdoops_context *cxt = &oops_cxt;
>
> cancel_work_sync(&cxt->work_write);
> cxt->ready = 0;
> if (cxt->mtd->panic_write)
> mtdoops_write(cxt, 1);
> else
> printk(KERN_WARNING "mtdoops: panic_write is not defined, "
> "cannot store dump from panic\n");
>
> return NOTIFY_DONE;
> }
>
> So with this one, the exported panic_on_oops is no longer needed, and
> normal oopses are handled by the scheduled work while panic_on_oopses
> are handled by the panic handler.

Yes, that looks like the better direction - but 'panic' is still the
wrong trigger condition i think. We generally just crash and dont panic.
Often we'll display a kernel warning and then hang. Etc.

Also, would it be possible to just simplify the thing and not do any
buffering at all? Extra buffering complexity in a console driver is only
asking for trouble. Or is flash storage write cycles optimization that
important in this case?

Ingo