Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov <bp@alien8.de>]

On Mon, Apr 14, 2014 at 03:39:24PM +0200, Borislav Petkov wrote:
> Definitely a bad idea to export a spinlock. If all you need is to sync
> against multiple callers of cper_mem_err_location(), simply grab that
> spinlock in the function itself, without exporting it.
> 
> > +
> > +static char mem_location[CPER_REC_LEN];
> > +static char dimm_location[CPER_REC_LEN];

In thinking about this more, even with the proper synchronization,
cper_dimm_err_location() returns a pointer to this dimm_location string.
Now, imagine what happens if another caller grabs the lock and enters
cper_dimm_err_location() while dimm_location is still being accessed by
the tracepoint or the previous caller of cper_dimm_err_location...

IOW, you either need a synchronization at a higher level so that dumping
of dimm locations can be serialized or the higher callers (interrupt
handlers, etc) already give you that synchronization.

So you have to think about all possible call paths ending here and
*then* introduce proper sync.

Oh, and saying "No functional changes." in the commit message is a bit
misleading, don't you think?

:-)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--