Re: [PATCH] panic.c: export panic_on_oops

[Ingo Molnar <mingo@elte.hu>]

* 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