Re: [PATCH 2/2] trace, RAS: Add eMCA trace event interface

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

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

On Fri, Mar 07, 2014 at 12:44:16PM +0100, Borislav Petkov wrote:
[...]
> > +static void mem_err_location(struct cper_sec_mem_err *mem)
> > +{
> > +	char *p;
> > +	u32 n = 0;
> > +
> > +	memset(mem_location, 0, LOC_LEN);
> > +	p = mem_location;
> > +	if (mem->validation_bits & CPER_MEM_VALID_NODE)
> > +		n += sprintf(p + n, " node: %d", mem->node);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_CARD)
> > +		n += sprintf(p + n, " card: %d", mem->card);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
> > +		n += sprintf(p + n, " module: %d", mem->module);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> > +		n += sprintf(p + n, " rank: %d", mem->rank);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_BANK)
> > +		n += sprintf(p + n, " bank: %d", mem->bank);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
> > +		n += sprintf(p + n, " device: %d", mem->device);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_ROW)
> > +		n += sprintf(p + n, " row: %d", mem->row);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
> > +		n += sprintf(p + n, " column: %d", mem->column);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
> > +		n += sprintf(p + n, " bit_position: %d", mem->bit_pos);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
> > +		n += sprintf(p + n, " requestor_id: 0x%016llx",
> > +				mem->requestor_id);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
> > +		n += sprintf(p + n, " responder_id: 0x%016llx",
> > +				mem->responder_id);
> > +	if (n >= LOC_LEN)
> > +		goto end;
> > +	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
> > +		n += sprintf(p + n, " target_id: 0x%016llx", mem->target_id);
> > +end:
> > +	return;
> > +}
> 
> Looks like this wants to share with cper_print_mem() - definitely a lot
> of duplication there.
> 
> > +
> > +static void dimm_err_location(struct cper_sec_mem_err *mem)
> > +{
> > +	const char *bank = NULL, *device = NULL;
> > +
> > +	memset(dimm_location, 0, LOC_LEN);
> > +	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
> > +		return;
> > +
> > +	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> > +	if (bank != NULL && device != NULL)
> > +		snprintf(dimm_location, LOC_LEN - 1, "%s %s", bank, device);
> > +	else
> > +		snprintf(dimm_location, LOC_LEN - 1, "DMI handle: 0x%.4x",
> > +			 mem->mem_dev_handle);
> > +}
> 
> This one too.
> 
Not really. Firstly they service for different purpose. Secondly the
format here can be changed/updated depending on further requirment.
I can't assume they always keep the same format.

> > +
> > +static void trace_mem_error(const uuid_le *fru_id, char *fru_text,
> > +			    u64 err_count, u32 severity,
> > +			    struct cper_sec_mem_err *mem)
> > +{
> > +	u32 etype = ~0U;
> > +	u64 phy_addr = ~0ull;
> 
> I'm assuming userspace knows that all 1s means field value is invalid?
Yep, I suppose so.

> 
> > +	unsigned long flags;
> > +
> > +	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
> > +		etype = mem->error_type;
> 
> newline.
Sure.

[...]
> We probably need a mechanism to disable printking to dmesg once
> userspace has opened the tracepoint.
Do we really need to do that? IMHO, I think they are used for two different
usages, just like dmesg & mcelog.

[...]
> >  static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> >  {
> >  	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
> > @@ -233,8 +241,7 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> >  	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");
> > +			cper_mem_err_type_str(etype));
> >  	}
> >  	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
> >  		const char *bank = NULL, *device = NULL;
> 
> Ditto.
I know you hope the print function in CPER & trace for cpi_extlog can be
merged into one. I just have one concern about it. Can we ensure these
two functions keeping align all the time? IOW, merge them for now until
change happens one day?

[...]
> > +#define LOC_LEN		512
> > +
> > +TRACE_EVENT(extlog_mem_event,
> 
> So this is a mem thing so we're defining a tracepoint for memory events,
> specifically.
> 
> However, if extlog carries all kinds of errors outside, not only DRAM
> errors, we should do a TRACE_EVENT_CLASS which contains the shared args
> to every error type and then make a mem event ontop of it.
I agree.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]