Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340586 > unrolled thread

Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable  MCA errors Borislav Petkov <bp@alien8.de> - 2016-02-23 13:40 +0100
    Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA  errors Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> - 2016-02-24 00:30 +0100
      Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable  MCA errors Borislav Petkov <bp@alien8.de> - 2016-02-24 12:30 +0100

#1340586 — Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

On Tue, Feb 16, 2016 at 03:45:08PM -0600, Aravind Gopalakrishnan wrote:
> For Scalable MCA enabled processors, errors are listed
> per IP block. And since it is not required for an IP to
> map to a particular bank, we need to use HWID and McaType
> values from the MCx_IPID register to figure out which IP
> a given bank represents.
> 
> We also have a new bit (TCC) in the MCx_STATUS register
> to indicate Task context is corrupt.
> 
> Add logic here to decode errors from all known IP
> blocks for Fam17h Model 00-0fh and to print TCC errors.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>  arch/x86/include/asm/mce.h           |  50 ++++++
>  arch/x86/include/asm/msr-index.h     |   2 +
>  arch/x86/kernel/cpu/mcheck/mce_amd.c |  11 ++
>  drivers/edac/mce_amd.c               | 327 ++++++++++++++++++++++++++++++++++-
>  4 files changed, 389 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 2ea4527..2ec67ac 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -42,6 +42,17 @@
>  /* AMD-specific bits */
>  #define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
>  #define MCI_STATUS_POISON	(1ULL<<43)  /* access poisonous data */
> +#define MCI_STATUS_TCC		(1ULL<<55)  /* Task context corrupt */

\n

> +/*
> + * McaX field if set indicates a given bank supports MCA extensions:
> + *  - Deferred error interrupt type is specifiable by bank
> + *  - BlkPtr field indicates prescence of extended MISC registers.
				^^^^^^^^^

Btw, that's MCi_MISC[BlkPtr] ?

Also, please integrate a spell checker into your patch creation
workflow.


> + *    But should not be used to determine MSR numbers
> + *  - TCC bit is present in MCx_STATUS

All sentences end with a "."

> + */
> +#define MCI_CONFIG_MCAX		0x1
> +#define MCI_IPID_MCATYPE	0xFFFF0000
> +#define MCI_IPID_HWID		0xFFF
>  
>  /*
>   * Note that the full MCACOD field of IA32_MCi_STATUS MSR is
> @@ -287,4 +298,43 @@ struct cper_sec_mem_err;
>  extern void apei_mce_report_mem_error(int corrected,
>  				      struct cper_sec_mem_err *mem_err);
>  
> +/*
> + * Enumerating new IP types and HWID values

Please stop using gerund, i.e., -ing, in comments and commit messages.

"Enumerate new IP ..." is just fine.

> + * in ScalableMCA enabled AMD processors
> + */
> +#ifdef CONFIG_X86_MCE_AMD
> +enum ip_types {

AMD-specific so "amd_ip_types"

> +	F17H_CORE = 0,	/* Core errors */
> +	DF,		/* Data Fabric */
> +	UMC,		/* Unified Memory Controller */
> +	FUSE,		/* FUSE subsystem */

What's FUSE subsystem?

In any case, this could use a different name in order not to confuse
with Linux's filesystem in userspace.

> +	PSP,		/* Platform Security Processor */
> +	SMU,		/* System Management Unit */
> +	N_IP_TYPES
> +};
> +
> +struct hwid {

amd_hwid and so on. All below should have the "amd_" prefix so that it
is obvious.

> +	const char *ipname;
> +	unsigned int hwid_value;
> +};
> +
> +extern struct hwid hwid_mappings[N_IP_TYPES];


> +
> +enum core_mcatypes {
> +	LS = 0,		/* Load Store */
> +	IF,		/* Instruction Fetch */
> +	L2_CACHE,	/* L2 cache */
> +	DE,		/* Decoder unit */
> +	RES,		/* Reserved */
> +	EX,		/* Execution unit */
> +	FP,		/* Floating Point */
> +	L3_CACHE	/* L3 cache */
> +};
> +
> +enum df_mcatypes {
> +	CS = 0,		/* Coherent Slave */
> +	PIE		/* Power management, Interrupts, etc */
> +};
> +#endif

So all those are defined here but we have a header for exactly that
drivers/edac/mce_amd.h. And then you define and export hwid_mappings in
arch/x86/kernel/cpu/mcheck/mce_amd.c to not use it there.

Why isn't all this in drivers/edac/mce_amd.[ch] ?

And if it is there, then you obviously don't need the "amd_" prefix.

> +
>  #endif /* _ASM_X86_MCE_H */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 5523465..93bccbc 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -266,7 +266,9 @@
>  
>  /* 'SMCA': AMD64 Scalable MCA */
>  #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
> +#define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
>  #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
> +#define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))

Are those MSRs used in multiple files? If not, -> mce.h.

>  #define MSR_P6_PERFCTR0			0x000000c1
>  #define MSR_P6_PERFCTR1			0x000000c2
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 88de27b..8169103 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -71,6 +71,17 @@ static const char * const th_names[] = {
>  	"execution_unit",
>  };
>  
> +/* Defining HWID to IP type mappings for Scalable MCA */

" Define ..."

> +struct hwid hwid_mappings[] = {
> +	[F17H_CORE]	= { "f17h_core", 0xB0 },
> +	[DF]		= { "df", 0x2E },
> +	[UMC]		= { "umc", 0x96 },
> +	[FUSE]		= { "fuse", 0x5 },
> +	[PSP]		= { "psp", 0xFF },
> +	[SMU]		= { "smu", 0x1 },
> +};
> +EXPORT_SYMBOL_GPL(hwid_mappings);
> +
>  static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
>  static DEFINE_PER_CPU(unsigned char, bank_map);	/* see which banks are on */
>  
> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> index e3a945c..6e6b327 100644
> --- a/drivers/edac/mce_amd.c
> +++ b/drivers/edac/mce_amd.c
> @@ -147,6 +147,136 @@ static const char * const mc6_mce_desc[] = {
>  	"Status Register File",

...

> +static void decode_f17hcore_errors(u8 xec, unsigned int mca_type)
> +{
> +	switch (mca_type) {
> +	case LS:
> +		if (xec == 0x4 || xec > (ARRAY_SIZE(f17h_ls_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_ls_mce_desc[xec]);
> +		break;
> +
> +	case IF:
> +		if (xec > (ARRAY_SIZE(f17h_if_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_if_mce_desc[xec]);
> +		break;
> +
> +	case L2_CACHE:
> +		if (xec > (ARRAY_SIZE(f17h_l2_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_l2_mce_desc[xec]);
> +		break;
> +
> +	case DE:
> +		if (xec > (ARRAY_SIZE(f17h_de_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_de_mce_desc[xec]);
> +		break;
> +
> +	case EX:
> +		if (xec > (ARRAY_SIZE(f17h_ex_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_ex_mce_desc[xec]);
> +		break;
> +
> +	case FP:
> +		if (xec > (ARRAY_SIZE(f17h_fp_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_fp_mce_desc[xec]);
> +		break;
> +
> +	case L3_CACHE:
> +		if (xec > (ARRAY_SIZE(f17h_l3_mce_desc) - 1))
> +			goto wrong_f17hcore_error;
> +
> +		pr_cont("%s.\n", f17h_l3_mce_desc[xec]);
> +		break;
> +
> +	default:
> +		goto wrong_f17hcore_error;

That's a lot of repeated code. You can assign the desc array to a temp
variable depending on mca_type and do the if and pr_cont only once using
that temp variable.

> +	}
> +
> +	return;
> +
> +wrong_f17hcore_error:
> +	pr_cont("Unrecognized error code from %s MCA bank\n",
> +		(mca_type == L3_CACHE) ? "L3 Cache" : "F17h Core");
> +}
> +
> +static void decode_df_errors(u8 xec, unsigned int mca_type)
> +{
> +	switch (mca_type) {
> +	case  CS:
> +		if (xec > (ARRAY_SIZE(f17h_cs_mce_desc) - 1))
> +			goto wrong_df_error;
> +
> +		pr_cont("%s.\n", f17h_cs_mce_desc[xec]);
> +		break;
> +
> +	case PIE:
> +		if (xec > (ARRAY_SIZE(f17h_pie_mce_desc) - 1))
> +			goto wrong_df_error;
> +
> +		pr_cont("%s.\n", f17h_pie_mce_desc[xec]);
> +		break;

Ditto.

> +
> +	default:
> +		goto wrong_df_error;
> +	}
> +
> +	return;
> +
> +wrong_df_error:
> +	pr_cont("Unrecognized error code from DF MCA bank\n");
> +}
> +
> +/* Decode errors according to Scalable MCA specification */
> +static void decode_smca_errors(struct mce *m)
> +{
> +	u32 low, high;
> +	u32 addr = MSR_AMD64_SMCA_MCx_IPID(m->bank);
> +	unsigned int hwid, mca_type, i;
> +	u8 xec = XEC(m->status, xec_mask);
> +
> +	if (rdmsr_safe(addr, &low, &high)) {
> +		pr_emerg("Unable to decode errors from banks\n");

That statement is not very useful in its current form.

> +		return;
> +	}
> +
> +	hwid = high & MCI_IPID_HWID;
> +	mca_type = (high & MCI_IPID_MCATYPE) >> 16;
> +
> +	pr_emerg(HW_ERR "MC%d IPID value: 0x%08x%08x\n",
> +		 m->bank, high, low);
> +
> +	/*
> +	 * Based on hwid and mca_type values,
> +	 * decode errors from respective IPs.
> +	 * Note: mca_type values make sense only
> +	 * in the context of an hwid
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(hwid_mappings); i++)

So you use those hwid_mappings here. Why aren't they defined here too?

> +		if (hwid_mappings[i].hwid_value == hwid)
> +			break;
> +
> +	switch (i) {
> +	case F17H_CORE:
> +		pr_emerg(HW_ERR "%s Error: ",
> +			 (mca_type == L3_CACHE) ? "L3 Cache" : "F17h Core");
> +		decode_f17hcore_errors(xec, mca_type);
> +		break;
> +
> +	case DF:
> +		pr_emerg(HW_ERR "DF Error: ");
> +		decode_df_errors(xec, mca_type);
> +		break;
> +
> +	case UMC:
> +		pr_emerg(HW_ERR "UMC Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_umc_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from UMC MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_umc_mce_desc[xec]);
> +		break;
> +
> +	case FUSE:
> +		pr_emerg(HW_ERR "FUSE Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_fuse_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from FUSE MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_fuse_mce_desc[xec]);
> +		break;
> +
> +	case PSP:
> +		pr_emerg(HW_ERR "PSP Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_psp_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from PSP MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_psp_mce_desc[xec]);
> +		break;
> +
> +	case SMU:
> +		pr_emerg(HW_ERR "SMU Error: ");
> +		if (xec > (ARRAY_SIZE(f17h_smu_mce_desc) - 1)) {
> +			pr_cont("Unrecognized error code from SMU MCA bank\n");
> +			return;
> +		}
> +		pr_cont("%s.\n", f17h_smu_mce_desc[xec]);
> +		break;

Also a lot of repeated code which could be simplified.

> +
> +	default:
> +		pr_emerg(HW_ERR "HWID:%d does not match any existing IPs\n", hwid);
> +	}
> +}
> +
>  static const char *decode_error_status(struct mce *m)
>  {
>  	if (m->status & MCI_STATUS_UC) {
> @@ -769,11 +1071,21 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>  		((m->status & MCI_STATUS_PCC)	? "PCC"	  : "-"),
>  		((m->status & MCI_STATUS_ADDRV)	? "AddrV" : "-"));
>  
> -	if (c->x86 == 0x15 || c->x86 == 0x16)
> +	if (c->x86 >= 0x15)
>  		pr_cont("|%s|%s",
>  			((m->status & MCI_STATUS_DEFERRED) ? "Deferred" : "-"),
>  			((m->status & MCI_STATUS_POISON)   ? "Poison"   : "-"));
>  
> +	if (mce_flags.smca) {

ERROR: "mce_flags" [drivers/edac/edac_mce_amd.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

Just read CPUID again here instead of exporting mce_flags.

> +		u32 smca_low, smca_high;


> +		u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(m->bank);

s/smca_//

Also, all those MSRs should be defined in drivers/edac/mce_amd.h if not
used outside.

> +
> +		if (!rdmsr_safe(smca_addr, &smca_low, &smca_high) &&
> +		    (smca_low & MCI_CONFIG_MCAX))
> +			pr_cont("|%s",

No need for that break here.

> +				((m->status & MCI_STATUS_TCC) ? "TCC" : "-"));
> +	}
> +
>  	/* do the two bits[14:13] together */
>  	ecc = (m->status >> 45) & 0x3;
>  	if (ecc)
> @@ -784,6 +1096,11 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
>  	if (m->status & MCI_STATUS_ADDRV)
>  		pr_emerg(HW_ERR "MC%d Error Address: 0x%016llx\n", m->bank, m->addr);
>  
> +	if (mce_flags.smca) {
> +		decode_smca_errors(m);
> +		goto err_code;
> +	}
> +
>  	if (!fam_ops)
>  		goto err_code;
>  
> @@ -888,6 +1205,14 @@ static int __init mce_amd_init(void)
>  		fam_ops->mc2_mce = f16h_mc2_mce;
>  		break;
>  
> +	case 0x17:
> +		xec_mask = 0x3f;
> +		if (!mce_flags.smca) {
> +			printk(KERN_WARNING "Decoding supported only on Scalable MCA enabled processors\n");

What is that supposed to do? I thought all new ones will have SMCA...

> +			return 0;
> +		}
> +		break;
> +
>  	default:
>  		printk(KERN_WARNING "Huh? What family is it: 0x%x?!\n", c->x86);
>  		kfree(fam_ops);
> -- 
> 2.7.0
> 
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [next] | [standalone]

#1341165 — Re: [PATCH 1/4] EDAC, MCE, AMD: Enable error decoding of Scalable MCA errors

On 2/23/16 6:37 AM, Borislav Petkov wrote:
> On Tue, Feb 16, 2016 at 03:45:08PM -0600, Aravind Gopalakrishnan wrote:
>>   /* AMD-specific bits */
>>   #define MCI_STATUS_DEFERRED	(1ULL<<44)  /* declare an uncorrected error */
>>   #define MCI_STATUS_POISON	(1ULL<<43)  /* access poisonous data */
>> +#define MCI_STATUS_TCC		(1ULL<<55)  /* Task context corrupt */
> \n
>

Ack.

>> +/*
>> + * McaX field if set indicates a given bank supports MCA extensions:
>> + *  - Deferred error interrupt type is specifiable by bank
>> + *  - BlkPtr field indicates prescence of extended MISC registers.
> 				^^^^^^^^^
>
> Btw, that's MCi_MISC[BlkPtr] ?

MCi_MISC0[BlkPtr] specifically. Will update the comments about this.

> Also, please integrate a spell checker into your patch creation
> workflow.

Sorry about that. Looks like this pair is not defined in spelling.txt. 
So, might be worth adding there as well?

>> + *    But should not be used to determine MSR numbers
>> + *  - TCC bit is present in MCx_STATUS
> All sentences end with a "."

Will fix.

>
>> +/*
>> + * Enumerating new IP types and HWID values
> Please stop using gerund, i.e., -ing, in comments and commit messages.
>
> "Enumerate new IP ..." is just fine.

Ack.

>
>> + * in ScalableMCA enabled AMD processors
>> + */
>> +#ifdef CONFIG_X86_MCE_AMD
>> +enum ip_types {
> AMD-specific so "amd_ip_types"

Ok, will fix.

>
>> +	F17H_CORE = 0,	/* Core errors */
>> +	DF,		/* Data Fabric */
>> +	UMC,		/* Unified Memory Controller */
>> +	FUSE,		/* FUSE subsystem */
> What's FUSE subsystem?

It's the block for programming FUSE registers.

>
> In any case, this could use a different name in order not to confuse
> with Linux's filesystem in userspace.

Ok, will ask internally as well as to what name suits here.

>> +
>> +struct hwid {
> amd_hwid and so on. All below should have the "amd_" prefix so that it
> is obvious.

Will fix.

>
>> +	const char *ipname;
>> +	unsigned int hwid_value;
>> +};
>> +
>> +extern struct hwid hwid_mappings[N_IP_TYPES];
>> +
>> +enum core_mcatypes {
>> +	LS = 0,		/* Load Store */
>> +	IF,		/* Instruction Fetch */
>> +	L2_CACHE,	/* L2 cache */
>> +	DE,		/* Decoder unit */
>> +	RES,		/* Reserved */
>> +	EX,		/* Execution unit */
>> +	FP,		/* Floating Point */
>> +	L3_CACHE	/* L3 cache */
>> +};
>> +
>> +enum df_mcatypes {
>> +	CS = 0,		/* Coherent Slave */
>> +	PIE		/* Power management, Interrupts, etc */
>> +};
>> +#endif
> So all those are defined here but we have a header for exactly that
> drivers/edac/mce_amd.h. And then you define and export hwid_mappings in
> arch/x86/kernel/cpu/mcheck/mce_amd.c to not use it there.
>
> Why isn't all this in drivers/edac/mce_amd.[ch] ?
>
> And if it is there, then you obviously don't need the "amd_" prefix.

I have a patch that uses these enums here. But I didn't send it out 
along with this patchset as I was testing the patch.
So yes, I need it here and in the EDAC driver.

>
>> +
>>   #endif /* _ASM_X86_MCE_H */
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 5523465..93bccbc 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -266,7 +266,9 @@
>>   
>>   /* 'SMCA': AMD64 Scalable MCA */
>>   #define MSR_AMD64_SMCA_MC0_CONFIG	0xc0002004
>> +#define MSR_AMD64_SMCA_MC0_IPID		0xc0002005
>>   #define MSR_AMD64_SMCA_MCx_CONFIG(x)	(MSR_AMD64_SMCA_MC0_CONFIG + 0x10*(x))
>> +#define MSR_AMD64_SMCA_MCx_IPID(x)	(MSR_AMD64_SMCA_MC0_IPID + 0x10*(x))
> Are those MSRs used in multiple files? If not, -> mce.h.

Yes, I'll need them in arch/x86/.../mce_amd.c as well.
A later patch will be using it there.

>>   
>>   
>> +/* Defining HWID to IP type mappings for Scalable MCA */
> " Define ..."

Ack

>
>> +	case L3_CACHE:
>> +		if (xec > (ARRAY_SIZE(f17h_l3_mce_desc) - 1))
>> +			goto wrong_f17hcore_error;
>> +
>> +		pr_cont("%s.\n", f17h_l3_mce_desc[xec]);
>> +		break;
>> +
>> +	default:
>> +		goto wrong_f17hcore_error;
> That's a lot of repeated code. You can assign the desc array to a temp
> variable depending on mca_type and do the if and pr_cont only once using
> that temp variable.

Ok, will simplify.

>
>> +
>> +	case PIE:
>> +		if (xec > (ARRAY_SIZE(f17h_pie_mce_desc) - 1))
>> +			goto wrong_df_error;
>> +
>> +		pr_cont("%s.\n", f17h_pie_mce_desc[xec]);
>> +		break;
> Ditto.
>

Will fix.

>> +
>> +/* Decode errors according to Scalable MCA specification */
>> +static void decode_smca_errors(struct mce *m)
>> +{
>> +	u32 low, high;
>> +	u32 addr = MSR_AMD64_SMCA_MCx_IPID(m->bank);
>> +	unsigned int hwid, mca_type, i;
>> +	u8 xec = XEC(m->status, xec_mask);
>> +
>> +	if (rdmsr_safe(addr, &low, &high)) {
>> +		pr_emerg("Unable to decode errors from banks\n");
> That statement is not very useful in its current form.

How about "Unable to gather IP block that threw the error. Therefore 
cannot decode errors further.\n"

Or should I just remove the pr_emerg()?

>
>> +	for (i = 0; i < ARRAY_SIZE(hwid_mappings); i++)
> So you use those hwid_mappings here. Why aren't they defined here too?

Same reason as the enums-
In a subsequent patch, I'll need it in arch/x86/../mce_amd.c
(I should have probably indicated this in the cover letter so you were 
aware of it. Sorry about that)

>
>> +	case SMU:
>> +		pr_emerg(HW_ERR "SMU Error: ");
>> +		if (xec > (ARRAY_SIZE(f17h_smu_mce_desc) - 1)) {
>> +			pr_cont("Unrecognized error code from SMU MCA bank\n");
>> +			return;
>> +		}
>> +		pr_cont("%s.\n", f17h_smu_mce_desc[xec]);
>> +		break;
> Also a lot of repeated code which could be simplified.

Hmm, will try to simplify it in V2.

>
>>   
>> +	if (mce_flags.smca) {
> ERROR: "mce_flags" [drivers/edac/edac_mce_amd.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> make: *** Waiting for unfinished jobs....
>
> Just read CPUID again here instead of exporting mce_flags.

Right. Will fix this.

>
>> +		u32 smca_low, smca_high;
>> +		u32 smca_addr = MSR_AMD64_SMCA_MCx_CONFIG(m->bank);
> s/smca_//

Will fix.

>
> Also, all those MSRs should be defined in drivers/edac/mce_amd.h if not
> used outside.

It will be used in arch/x86/../mce_amd.c

>
>> +
>> +		if (!rdmsr_safe(smca_addr, &smca_low, &smca_high) &&
>> +		    (smca_low & MCI_CONFIG_MCAX))
>> +			pr_cont("|%s",
> No need for that break here.

Ok, Will fix.

>
>>   
>> +	case 0x17:
>> +		xec_mask = 0x3f;
>> +		if (!mce_flags.smca) {
>> +			printk(KERN_WARNING "Decoding supported only on Scalable MCA enabled processors\n");
> What is that supposed to do? I thought all new ones will have SMCA...
>
>
If for some reason the CPUID bit is not set, then we should not assume 
the processor supports the features right?

Thanks,
-Aravind.

[toc] | [prev] | [next] | [standalone]

#1341917

On Tue, Feb 23, 2016 at 04:50:37PM -0600, Aravind Gopalakrishnan wrote:
> Sorry about that. Looks like this pair is not defined in spelling.txt. So,
> might be worth adding there as well?

Oh geez, we have a spelling.txt! I think we can declare the kernel as
done and go do something else with our lives...

> It's the block for programming FUSE registers.

Oh, that's what it is.

So maybe "fuses block" or "fuses" or ... just the capitalized "FUSE" is
kinda misleading.

> How about "Unable to gather IP block that threw the error. Therefore cannot
> decode errors further.\n"

Or simply "Invalid IP block specified, error information is unreliable."
and still continue decoding. It might still be recognizable from the
signature, methinks.

> If for some reason the CPUID bit is not set, then we should not assume the
> processor supports the features right?

Is that even remotely possible? If yes, then we should keep the warning,
otherwise it is useless.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web