On Mon, Apr 14, 2014 at 04:05:14PM +0200, Borislav Petkov wrote: > Date: Mon, 14 Apr 2014 16:05:14 +0200 > From: Borislav Petkov <bp@xxxxxxxxx> > To: "Chen, Gong" <gong.chen@xxxxxxxxxxxxxxx> > Cc: tony.luck@xxxxxxxxx, m.chehab@xxxxxxxxxxx, rostedt@xxxxxxxxxxx, > linux-acpi@xxxxxxxxxxxxxxx, arozansk@xxxxxxxxxx > Subject: Re: [PATCH 2/5] CPER: Adjust code flow of some functions > User-Agent: Mutt/1.5.21 (2010-09-15) > > 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. That's why I export this spinlock because in another patch(3/5) I use this spinlock to surround whole handling procedure of tracepoint. If exporting this spinlock directly is too ugly, I can use an inline function to get the same purpose. > > 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? OK. Looks like this is not the 1st time you remind me not using so-called "No functional change" :-). I have a different definition for functional change ;-). It seems that I need to align my standard with you guys.
Attachment:
signature.asc
Description: Digital signature
