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

["Chen, Gong" <gong.chen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

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@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, m.chehab@samsung.com, rostedt@goodmis.org,
>  linux-acpi@vger.kernel.org, arozansk@redhat.com
> 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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]