| Message ID | 1395985981-20476-3-git-send-email-gong.chen@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Mar 28, 2014 at 01:52:58AM -0400, Chen, Gong wrote: > Some codes can be reorganzied as a common function for > other usages. No functional changes. > > Signed-off-by: Chen, Gong <gong.chen@linux.intel.com> > --- > drivers/firmware/efi/cper.c | 134 +++++++++++++++++++++++++++++++++----------- > include/linux/cper.h | 7 +++ > 2 files changed, 108 insertions(+), 33 deletions(-) > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > index 1491dd4..4e88885 100644 > --- a/drivers/firmware/efi/cper.c > +++ b/drivers/firmware/efi/cper.c > @@ -34,6 +34,12 @@ > #include <linux/aer.h> > > #define INDENT_SP " " > +DEFINE_RAW_SPINLOCK(cper_loc_lock); > +EXPORT_SYMBOL_GPL(cper_loc_lock); 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]; > + > /* > * CPER record ID need to be unique even after reboot, because record > * ID is used as index for ERST storage, while CPER records from > @@ -57,11 +63,12 @@ static const char *cper_severity_strs[] = { > "info", > }; > > -static const char *cper_severity_str(unsigned int severity) > +const char *cper_severity_str(unsigned int severity) > { > return severity < ARRAY_SIZE(cper_severity_strs) ? > cper_severity_strs[severity] : "unknown"; > } > +EXPORT_SYMBOL_GPL(cper_severity_str); > > * cper_print_bits - print strings for set bits > @@ -196,55 +203,116 @@ static const char *cper_mem_err_type_strs[] = { > "physical memory map-out event", > }; > > -static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) > +const char *cper_mem_err_type_str(unsigned int etype) > { > - if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS) > - printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status); > - if (mem->validation_bits & CPER_MEM_VALID_PA) > - printk("%s""physical_address: 0x%016llx\n", > - pfx, mem->physical_addr); > - if (mem->validation_bits & CPER_MEM_VALID_PA_MASK) > - printk("%s""physical_address_mask: 0x%016llx\n", > - pfx, mem->physical_addr_mask); > + return etype < ARRAY_SIZE(cper_mem_err_type_strs) ? > + cper_mem_err_type_strs[etype] : "unknown"; > +} > +EXPORT_SYMBOL_GPL(cper_mem_err_type_str); > + > +char *cper_mem_err_location(const struct cper_sec_mem_err *mem) > +{ > + char *p; > + u32 n = 0; > + > + memset(mem_location, 0, CPER_REC_LEN); > + p = mem_location; > if (mem->validation_bits & CPER_MEM_VALID_NODE) > - pr_debug("node: %d\n", mem->node); > + n += sprintf(p + n, " node: %d", mem->node); > + if (n >= CPER_REC_LEN) > + goto end; n += vscnprintf(p + n, CPER_REC_LEN - n - 1, "... ", ...); and you can drop those if (n >= CPER_REC_LEN) tests. vscnprintf() because it returns the actual number of characters written and not what snprintf does and return the chars count it would've written. (Thx Richard for the hint!). > if (mem->validation_bits & CPER_MEM_VALID_CARD) > - pr_debug("card: %d\n", mem->card); > + n += sprintf(p + n, " card: %d", mem->card); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_MODULE) > - pr_debug("module: %d\n", mem->module); > + n += sprintf(p + n, " module: %d", mem->module); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER) > - pr_debug("rank: %d\n", mem->rank); > + n += sprintf(p + n, " rank: %d", mem->rank); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_BANK) > - pr_debug("bank: %d\n", mem->bank); > + n += sprintf(p + n, " bank: %d", mem->bank); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_DEVICE) > - pr_debug("device: %d\n", mem->device); > + n += sprintf(p + n, " device: %d", mem->device); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_ROW) > - pr_debug("row: %d\n", mem->row); > + n += sprintf(p + n, " row: %d", mem->row); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_COLUMN) > - pr_debug("column: %d\n", mem->column); > + n += sprintf(p + n, " column: %d", mem->column); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION) > - pr_debug("bit_position: %d\n", mem->bit_pos); > + n += sprintf(p + n, " bit_position: %d", mem->bit_pos); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID) > - pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id); > + n += sprintf(p + n, " requestor_id: 0x%016llx", > + mem->requestor_id); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID) > - pr_debug("responder_id: 0x%016llx\n", mem->responder_id); > + n += sprintf(p + n, " responder_id: 0x%016llx", > + mem->responder_id); > + if (n >= CPER_REC_LEN) > + goto end; > if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID) > - pr_debug("target_id: 0x%016llx\n", mem->target_id); > + n += sprintf(p + n, " target_id: 0x%016llx", mem->target_id); > +end: > + return mem_location; > +} > +EXPORT_SYMBOL_GPL(cper_mem_err_location); > + > +char *cper_dimm_err_location(const struct cper_sec_mem_err *mem) > +{ > + const char *bank = NULL, *device = NULL; > + > + memset(dimm_location, 0, CPER_REC_LEN); > + if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)) > + goto end; a goto label which is used only once?? What's wrong with return dimm_location; ? > + > + dmi_memdev_name(mem->mem_dev_handle, &bank, &device); > + if (bank != NULL && device != NULL) if (bank && device) > + snprintf(dimm_location, CPER_REC_LEN - 1, > + "DIMM location: %s %s", bank, device); > + else > + snprintf(dimm_location, CPER_REC_LEN - 1, "DMI handle: 0x%.4x", Why the DMI handle? Why not say: "unable to find location" or "location not present in DMI" or something more user-friendly. > + mem->mem_dev_handle); > +end: > + return dimm_location; > +} > +EXPORT_SYMBOL_GPL(cper_dimm_err_location); > + > +static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) > +{ > + unsigned long flags; > + > + if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS) > + printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status); > + if (mem->validation_bits & CPER_MEM_VALID_PA) > + printk("%s""physical_address: 0x%016llx\n", > + pfx, mem->physical_addr); > + if (mem->validation_bits & CPER_MEM_VALID_PA_MASK) > + printk("%s""physical_address_mask: 0x%016llx\n", > + pfx, mem->physical_addr_mask); > + raw_spin_lock_irqsave(&cper_loc_lock, flags); > + pr_debug("%s", cper_mem_err_location(mem)); > + raw_spin_unlock_irqrestore(&cper_loc_lock, flags); > if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) { > u8 etype = mem->error_type; > printk("%s""error_type: %d, %s\n", pfx, etype, > - etype < ARRAY_SIZE(cper_mem_err_type_strs) ? > - cper_mem_err_type_strs[etype] : "unknown"); > - } > - if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) { > - const char *bank = NULL, *device = NULL; > - dmi_memdev_name(mem->mem_dev_handle, &bank, &device); > - if (bank != NULL && device != NULL) > - printk("%s""DIMM location: %s %s", pfx, bank, device); > - else > - printk("%s""DIMM DMI handle: 0x%.4x", > - pfx, mem->mem_dev_handle); > + cper_mem_err_type_str(etype)); > } > + raw_spin_lock_irqsave(&cper_loc_lock, flags); > + printk("%s%s", pfx, cper_dimm_err_location(mem)); > + raw_spin_unlock_irqrestore(&cper_loc_lock, flags); > } > > static const char *cper_pcie_port_type_strs[] = { > diff --git a/include/linux/cper.h b/include/linux/cper.h > index 2fc0ec3..55c10db 100644 > --- a/include/linux/cper.h > +++ b/include/linux/cper.h > @@ -23,6 +23,8 @@ > > #include <linux/uuid.h> > > +extern struct raw_spinlock cper_loc_lock; > + > /* CPER record signature and the size */ > #define CPER_SIG_RECORD "CPER" > #define CPER_SIG_SIZE 4 > @@ -35,6 +37,7 @@ > */ > #define CPER_RECORD_REV 0x0100 > > +#define CPER_REC_LEN 512 How did we come up with this? Some spec? 512 chars for an error record? That's a bit too much in my book. Thanks.
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.
On Mon, Apr 14, 2014 at 03:39:24PM +0200, Borislav Petkov wrote: > > +char *cper_mem_err_location(const struct cper_sec_mem_err *mem) > > +{ > > + char *p; > > + u32 n = 0; > > + > > + memset(mem_location, 0, CPER_REC_LEN); > > + p = mem_location; > > if (mem->validation_bits & CPER_MEM_VALID_NODE) > > - pr_debug("node: %d\n", mem->node); > > + n += sprintf(p + n, " node: %d", mem->node); > > + if (n >= CPER_REC_LEN) > > + goto end; > > n += vscnprintf(p + n, CPER_REC_LEN - n - 1, "... ", ...); > > and you can drop those > > if (n >= CPER_REC_LEN) > > tests. > > vscnprintf() because it returns the actual number of characters written > and not what snprintf does and return the chars count it would've > written. (Thx Richard for the hint!). You are absolutely right. This function is perfect for this usage. > > +char *cper_dimm_err_location(const struct cper_sec_mem_err *mem) > > +{ > > + const char *bank = NULL, *device = NULL; > > + > > + memset(dimm_location, 0, CPER_REC_LEN); > > + if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)) > > + goto end; > > a goto label which is used only once?? What's wrong with > > return dimm_location; > I just feel repeating the same words looks ugly. OK, it is fine to me to adopt your suggestion. > > + dmi_memdev_name(mem->mem_dev_handle, &bank, &device); > > + if (bank != NULL && device != NULL) > > if (bank && device) > > > + snprintf(dimm_location, CPER_REC_LEN - 1, > > + "DIMM location: %s %s", bank, device); > > + else > > + snprintf(dimm_location, CPER_REC_LEN - 1, "DMI handle: 0x%.4x", > > Why the DMI handle? Why not say: "unable to find location" or "location > not present in DMI" or something more user-friendly. In essential, I use the suggestion from Tony Luck. DMI handle can't be found so I just list the original info for it. Maybe I can merge them into one. > > +#define CPER_REC_LEN 512 > > How did we come up with this? Some spec? 512 chars for an error record? > That's a bit too much in my book. > Because 128 is not enough once all fields in error record exist, and 256 looks a little bit tough so I choose a bigger value. No spec for it. I just hope it can contain everything.
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.
On Tue, Apr 15, 2014 at 05:24:42AM -0400, Chen, Gong wrote: > 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. You still haven't answered... > > So you have to think about all possible call paths ending here and > > *then* introduce proper sync. ^^^^^^^^^^^^^ this question. Do we even *need* the locking? If so, why? What paths are going to end there? The answers to those questions will give you the correct synchronization. Thanks.
On Tue, Apr 15, 2014 at 05:19:44AM -0400, Chen, Gong wrote: > > Why the DMI handle? Why not say: "unable to find location" or "location > > not present in DMI" or something more user-friendly. > In essential, I use the suggestion from Tony Luck. DMI handle can't be found > so I just list the original info for it. Maybe I can merge them into one. Yep. Make it user-friendly while showing the original info pls. > > > +#define CPER_REC_LEN 512 > > > > How did we come up with this? Some spec? 512 chars for an error record? > > That's a bit too much in my book. > > > Because 128 is not enough once all fields in error record exist, and 256 looks > a little bit tough so I choose a bigger value. No spec for it. I just hope Tough? How? Not enough? > it can contain everything. Pls add a comment over this definition to state why we've chosen this number exactly. Thanks.
On Tue, Apr 15, 2014 at 08:02:59PM +0200, Borislav Petkov wrote: > this question. Do we even *need* the locking? If so, why? What paths are > going to end there? > Yes, The sync can be avoided via separating buffers. I will update my patch in the next version.
On Tue, Apr 15, 2014 at 08:05:54PM +0200, Borislav Petkov wrote: > > Because 128 is not enough once all fields in error record exist, and 256 looks > > a little bit tough so I choose a bigger value. No spec for it. I just hope > > Tough? How? Not enough? > We have different CPER error type. For example, for processor error type, it has following definition: struct cper_sec_proc_generic { __u64 validation_bits; __u8 proc_type; __u8 proc_isa; __u8 proc_error_type; __u8 operation; __u8 flags; __u8 level; __u16 reserved; __u64 cpu_version; char cpu_brand[128]; __u64 proc_id; __u64 target_addr; __u64 requestor_id; __u64 responder_id; __u64 ip; }; If we want to show it in string format, 256 bytes should be not enough. But by now we don't use this macro for processor error type so I will shrink it to 256 bytes and add a comment for it.
On Wed, Apr 16, 2014 at 01:01:58AM -0400, Chen, Gong wrote:
> Yes, The sync can be avoided via separating buffers.
But why do we even need to sync? IOW, we can land here from GHES and
EXTLOG, right? So we have two different paths, which means, we need
synchronization.
And yes, separating the buffers by having GHES and EXTLOG hand down
their own char buffers would make the whole thing very clean IMHO and
lockless. So definitely worth a try.
:-)
Thanks.
On Wed, Apr 16, 2014 at 02:23:23AM -0400, Chen, Gong wrote: > We have different CPER error type. For example, for processor error type, > it has following definition: > > struct cper_sec_proc_generic { > __u64 validation_bits; > __u8 proc_type; > __u8 proc_isa; > __u8 proc_error_type; > __u8 operation; > __u8 flags; > __u8 level; > __u16 reserved; > __u64 cpu_version; > char cpu_brand[128]; > __u64 proc_id; > __u64 target_addr; > __u64 requestor_id; > __u64 responder_id; > __u64 ip; > }; > > If we want to show it in string format, 256 bytes should be not enough. But > by now we don't use this macro for processor error type so I will shrink it > to 256 bytes and add a comment for it. Yeah, we don't have to always adhere to the spec if we feel it doesn't make any sense. A lot of those fields above are purely useless and we shouldn't carry them blindly to the outside. For example, we don't need to carry cpu_brand[128] to the outside for *every* error. Who even came up with this crap, is beyond me??? It's like the cpu changes brand on every other error or what? You harvest this info only *once* from /proc/cpuinfo. cpu_version too. And so on and so on... Please sanity-check stuff like that before hardcoding it into the tracepoint. If it is in the spec it doesn't always mean it makes sense. We need to carry out only the minimum amount of information of each error which is actually getting used in userspace, for additional RAS actions. Carrying fat blobs just because the spec says so is simply wrong. Thanks.
On Wed, Apr 16, 2014 at 03:28:18PM +0200, Borislav Petkov wrote: > Yeah, we don't have to always adhere to the spec if we feel it doesn't > make any sense. A lot of those fields above are purely useless and we > shouldn't carry them blindly to the outside. > > For example, we don't need to carry cpu_brand[128] to the outside for > *every* error. Who even came up with this crap, is beyond me??? It's > like the cpu changes brand on every other error or what? You harvest > this info only *once* from /proc/cpuinfo. cpu_version too. And so on and > so on... > > Please sanity-check stuff like that before hardcoding it into the > tracepoint. If it is in the spec it doesn't always mean it makes sense. > We need to carry out only the minimum amount of information of each > error which is actually getting used in userspace, for additional RAS > actions. Carrying fat blobs just because the spec says so is simply > wrong. > I agree. But at least for memory, I want to show them all. Maybe requestor_id/responder_id/target_id are not so important. But in reality, I've never seen they are valid. So just leave them there.
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c index 1491dd4..4e88885 100644 --- a/drivers/firmware/efi/cper.c +++ b/drivers/firmware/efi/cper.c @@ -34,6 +34,12 @@ #include <linux/aer.h> #define INDENT_SP " " +DEFINE_RAW_SPINLOCK(cper_loc_lock); +EXPORT_SYMBOL_GPL(cper_loc_lock); + +static char mem_location[CPER_REC_LEN]; +static char dimm_location[CPER_REC_LEN]; + /* * CPER record ID need to be unique even after reboot, because record * ID is used as index for ERST storage, while CPER records from @@ -57,11 +63,12 @@ static const char *cper_severity_strs[] = { "info", }; -static const char *cper_severity_str(unsigned int severity) +const char *cper_severity_str(unsigned int severity) { return severity < ARRAY_SIZE(cper_severity_strs) ? cper_severity_strs[severity] : "unknown"; } +EXPORT_SYMBOL_GPL(cper_severity_str); /* * cper_print_bits - print strings for set bits @@ -196,55 +203,116 @@ static const char *cper_mem_err_type_strs[] = { "physical memory map-out event", }; -static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) +const char *cper_mem_err_type_str(unsigned int etype) { - if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS) - printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status); - if (mem->validation_bits & CPER_MEM_VALID_PA) - printk("%s""physical_address: 0x%016llx\n", - pfx, mem->physical_addr); - if (mem->validation_bits & CPER_MEM_VALID_PA_MASK) - printk("%s""physical_address_mask: 0x%016llx\n", - pfx, mem->physical_addr_mask); + return etype < ARRAY_SIZE(cper_mem_err_type_strs) ? + cper_mem_err_type_strs[etype] : "unknown"; +} +EXPORT_SYMBOL_GPL(cper_mem_err_type_str); + +char *cper_mem_err_location(const struct cper_sec_mem_err *mem) +{ + char *p; + u32 n = 0; + + memset(mem_location, 0, CPER_REC_LEN); + p = mem_location; if (mem->validation_bits & CPER_MEM_VALID_NODE) - pr_debug("node: %d\n", mem->node); + n += sprintf(p + n, " node: %d", mem->node); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_CARD) - pr_debug("card: %d\n", mem->card); + n += sprintf(p + n, " card: %d", mem->card); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_MODULE) - pr_debug("module: %d\n", mem->module); + n += sprintf(p + n, " module: %d", mem->module); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER) - pr_debug("rank: %d\n", mem->rank); + n += sprintf(p + n, " rank: %d", mem->rank); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_BANK) - pr_debug("bank: %d\n", mem->bank); + n += sprintf(p + n, " bank: %d", mem->bank); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_DEVICE) - pr_debug("device: %d\n", mem->device); + n += sprintf(p + n, " device: %d", mem->device); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_ROW) - pr_debug("row: %d\n", mem->row); + n += sprintf(p + n, " row: %d", mem->row); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_COLUMN) - pr_debug("column: %d\n", mem->column); + n += sprintf(p + n, " column: %d", mem->column); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION) - pr_debug("bit_position: %d\n", mem->bit_pos); + n += sprintf(p + n, " bit_position: %d", mem->bit_pos); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID) - pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id); + n += sprintf(p + n, " requestor_id: 0x%016llx", + mem->requestor_id); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID) - pr_debug("responder_id: 0x%016llx\n", mem->responder_id); + n += sprintf(p + n, " responder_id: 0x%016llx", + mem->responder_id); + if (n >= CPER_REC_LEN) + goto end; if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID) - pr_debug("target_id: 0x%016llx\n", mem->target_id); + n += sprintf(p + n, " target_id: 0x%016llx", mem->target_id); +end: + return mem_location; +} +EXPORT_SYMBOL_GPL(cper_mem_err_location); + +char *cper_dimm_err_location(const struct cper_sec_mem_err *mem) +{ + const char *bank = NULL, *device = NULL; + + memset(dimm_location, 0, CPER_REC_LEN); + if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)) + goto end; + + dmi_memdev_name(mem->mem_dev_handle, &bank, &device); + if (bank != NULL && device != NULL) + snprintf(dimm_location, CPER_REC_LEN - 1, + "DIMM location: %s %s", bank, device); + else + snprintf(dimm_location, CPER_REC_LEN - 1, "DMI handle: 0x%.4x", + mem->mem_dev_handle); +end: + return dimm_location; +} +EXPORT_SYMBOL_GPL(cper_dimm_err_location); + +static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem) +{ + unsigned long flags; + + if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS) + printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status); + if (mem->validation_bits & CPER_MEM_VALID_PA) + printk("%s""physical_address: 0x%016llx\n", + pfx, mem->physical_addr); + if (mem->validation_bits & CPER_MEM_VALID_PA_MASK) + printk("%s""physical_address_mask: 0x%016llx\n", + pfx, mem->physical_addr_mask); + raw_spin_lock_irqsave(&cper_loc_lock, flags); + pr_debug("%s", cper_mem_err_location(mem)); + raw_spin_unlock_irqrestore(&cper_loc_lock, flags); if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) { u8 etype = mem->error_type; printk("%s""error_type: %d, %s\n", pfx, etype, - etype < ARRAY_SIZE(cper_mem_err_type_strs) ? - cper_mem_err_type_strs[etype] : "unknown"); - } - if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) { - const char *bank = NULL, *device = NULL; - dmi_memdev_name(mem->mem_dev_handle, &bank, &device); - if (bank != NULL && device != NULL) - printk("%s""DIMM location: %s %s", pfx, bank, device); - else - printk("%s""DIMM DMI handle: 0x%.4x", - pfx, mem->mem_dev_handle); + cper_mem_err_type_str(etype)); } + raw_spin_lock_irqsave(&cper_loc_lock, flags); + printk("%s%s", pfx, cper_dimm_err_location(mem)); + raw_spin_unlock_irqrestore(&cper_loc_lock, flags); } static const char *cper_pcie_port_type_strs[] = { diff --git a/include/linux/cper.h b/include/linux/cper.h index 2fc0ec3..55c10db 100644 --- a/include/linux/cper.h +++ b/include/linux/cper.h @@ -23,6 +23,8 @@ #include <linux/uuid.h> +extern struct raw_spinlock cper_loc_lock; + /* CPER record signature and the size */ #define CPER_SIG_RECORD "CPER" #define CPER_SIG_SIZE 4 @@ -35,6 +37,7 @@ */ #define CPER_RECORD_REV 0x0100 +#define CPER_REC_LEN 512 /* * Severity difinition for error_severity in struct cper_record_header * and section_severity in struct cper_section_descriptor @@ -395,7 +398,11 @@ struct cper_sec_pcie { #pragma pack() u64 cper_next_record_id(void); +const char *cper_severity_str(unsigned int); +const char *cper_mem_err_type_str(unsigned int); void cper_print_bits(const char *prefix, unsigned int bits, const char * const strs[], unsigned int strs_size); +char *cper_mem_err_location(const struct cper_sec_mem_err *mem); +char *cper_dimm_err_location(const struct cper_sec_mem_err *mem); #endif
Some codes can be reorganzied as a common function for other usages. No functional changes. Signed-off-by: Chen, Gong <gong.chen@linux.intel.com> --- drivers/firmware/efi/cper.c | 134 +++++++++++++++++++++++++++++++++----------- include/linux/cper.h | 7 +++ 2 files changed, 108 insertions(+), 33 deletions(-)