Re: [RFC][PATCH -next] make pstore/kmsg_dump run after stopping other cpus in panic path

[Don Zickus <dzickus@redhat.com>]

On Mon, Oct 17, 2011 at 04:47:15PM -0700, Andrew Morton wrote:
> On Fri, 14 Oct 2011 16:53:05 -0400
> > @@ -131,11 +133,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> >  		total += l1_cpy + l2_cpy;
> >  		part++;
> >  	}
> > -	if (in_nmi()) {
> > -		if (is_locked)
> > -			spin_unlock(&psinfo->buf_lock);
> > -	} else
> > -		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> > +	spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> >  }
> 
> afacit this assumes that (reason == KMSG_DUMP_PANIC) if in_nmi().  Is
> that always the case, and will it always be the case in the future?

I see your point.  For now yes.  The common case for which I think pstore
was designed for was the APEI/GHES case.  Normally when GHES hits an NMI
it stays at the APEI/GHES layer and either uses an irq_workqueue for
recoverable errors or panics on non-recoverable errors.

So currently the only time it reaches the pstore layer is in the panic
case.  Unfortunately, I can't vouch for all the backends that can hook
into pstore.

Perhaps a 'BUG_ON(in_nmi() && reason != KMSG_DUMP_PANIC)'?


> 
> I felt that the spin_trylock() approach was less horrid than this.  I
> assume that the new approach will cause lockdep to go berzerk?

Heh.  Good point.  That is probably a good test case.  Though finding a
working GHES implementation in the firmware isn't easy these days, making
it hard to test. :-/

Cheers,
Don