[PATCH] x86: mce: Honour bios-set CMCI threshold

[PATCH] x86: mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

The ACPI spec doesn't provide for a way for the bios to pass down
recommended thresholds to the OS on a _per-bank_ basis. This patch adds
a new boot option, which if passed, allows bios to initialize the CMCI
threshold. In such a case, we simply skip programming any threshold
value.

As fail-safe, we initialize threshold to 1 if some banks have not been
initialized by the bios and warn the user.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 Documentation/x86/x86_64/boot-options.txt |    5 ++++
 arch/x86/include/asm/mce.h                |    1 +
 arch/x86/kernel/cpu/mcheck/mce.c          |    4 +++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
 4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index c54b4f5..ec92540 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -50,6 +50,11 @@ Machine check
 		monarchtimeout:
 		Sets the time in us to wait for other CPUs on machine checks. 0
 		to disable.
+   mce=bios_cmci_threshold
+		Don't overwrite the bios-set CMCI threshold. This boot option
+		prevents Linux from overwriting the CMCI threshold set by the
+		bios. Without this option, Linux always sets the CMCI
+		threshold to 1.
 
    nomce (for compatibility with i386): same as mce=off
 
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index a3ac52b..8ad5078 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -171,6 +171,7 @@ DECLARE_PER_CPU(struct device *, mce_device);
 #ifdef CONFIG_X86_MCE_INTEL
 extern int mce_cmci_disabled;
 extern int mce_ignore_ce;
+extern int mce_bios_cmci_threshold;
 void mce_intel_feature_init(struct cpuinfo_x86 *c);
 void cmci_clear(void);
 void cmci_reenable(void);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 292d025..401359d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -82,6 +82,7 @@ static int			mce_panic_timeout	__read_mostly;
 static int			mce_dont_log_ce		__read_mostly;
 int				mce_cmci_disabled	__read_mostly;
 int				mce_ignore_ce		__read_mostly;
+int				mce_bios_cmci_threshold	__read_mostly;
 int				mce_ser			__read_mostly;
 
 struct mce_bank                *mce_banks		__read_mostly;
@@ -1907,6 +1908,7 @@ static struct miscdevice mce_chrdev_device = {
  *	check, or 0 to not wait
  * mce=bootlog Log MCEs from before booting. Disabled by default on AMD.
  * mce=nobootlog Don't log MCEs from before booting.
+ * mce=bios_cmci_threshold Don't program the CMCI threshold
  */
 static int __init mcheck_enable(char *str)
 {
@@ -1926,6 +1928,8 @@ static int __init mcheck_enable(char *str)
 		mce_ignore_ce = 1;
 	else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog"))
 		mce_bootlog = (str[0] == 'b');
+	else if (!strcmp(str, "bios_cmci_threshold"))
+		mce_bios_cmci_threshold = 1;
 	else if (isdigit(str[0])) {
 		get_option(&str, &tolerant);
 		if (*str == ',') {
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index 38e49bc..b869040 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -84,10 +84,16 @@ static void cmci_discover(int banks, int boot)
 	unsigned long flags;
 	int hdr = 0;
 	int i;
+	int bios_wrong_thresh = 0;
+
+	if (boot && mce_bios_cmci_threshold)
+		printk_once(KERN_INFO
+			"bios_cmci_threshold: Using bios-set threshold values for CMCI");
 
 	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
 	for (i = 0; i < banks; i++) {
 		u64 val;
+		int bios_zero_thresh = 0;
 
 		if (test_bit(i, owned))
 			continue;
@@ -102,8 +108,20 @@ static void cmci_discover(int banks, int boot)
 			continue;
 		}
 
-		val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
-		val |= MCI_CTL2_CMCI_EN | CMCI_THRESHOLD;
+		if (!mce_bios_cmci_threshold) {
+			val &= ~MCI_CTL2_CMCI_THRESHOLD_MASK;
+			val |= CMCI_THRESHOLD;
+		} else if (!(val & MCI_CTL2_CMCI_THRESHOLD_MASK)) {
+			/*
+			 * If bios_cmci_threshold boot option was specified
+			 * but the threshold is zero, we'll try to initialize
+			 * it to 1.
+			 */
+			bios_zero_thresh = 1;
+			val |= CMCI_THRESHOLD;
+		}
+
+		val |= MCI_CTL2_CMCI_EN;
 		wrmsrl(MSR_IA32_MCx_CTL2(i), val);
 		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
 
@@ -112,6 +130,15 @@ static void cmci_discover(int banks, int boot)
 			if (!test_and_set_bit(i, owned) && !boot)
 				print_update("CMCI", &hdr, i);
 			__clear_bit(i, __get_cpu_var(mce_poll_banks));
+			/*
+			 * We are able to set thresholds for some banks that
+			 * had a threshold of 0. This means the BIOS has not
+			 * set the thresholds properly or does not work with
+			 * this boot option. Note down now and report later.
+			 */
+			if (mce_bios_cmci_threshold && bios_zero_thresh &&
+					(val & MCI_CTL2_CMCI_THRESHOLD_MASK))
+				bios_wrong_thresh = 1;
 		} else {
 			WARN_ON(!test_bit(i, __get_cpu_var(mce_poll_banks)));
 		}
@@ -119,6 +146,12 @@ static void cmci_discover(int banks, int boot)
 	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
 	if (hdr)
 		printk(KERN_CONT "\n");
+	if (boot && mce_bios_cmci_threshold && bios_wrong_thresh) {
+		printk_once(KERN_INFO
+			"bios_cmci_threshold: Some banks do not have valid thresholds set");
+		printk_once(KERN_INFO
+			"bios_cmci_threshold: Make sure your BIOS supports this boot option");
+	}
 }
 
 /*
@@ -156,7 +189,7 @@ void cmci_clear(void)
 			continue;
 		/* Disable CMCI */
 		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
-		val &= ~(MCI_CTL2_CMCI_EN|MCI_CTL2_CMCI_THRESHOLD_MASK);
+		val &= ~MCI_CTL2_CMCI_EN;
 		wrmsrl(MSR_IA32_MCx_CTL2(i), val);
 		__clear_bit(i, __get_cpu_var(mce_banks_owned));
 	}

Re: [PATCH] x86: mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Wed, Aug 22, 2012 at 06:00:54PM +0530, Naveen N. Rao wrote:
> The ACPI spec doesn't provide for a way for the bios to pass down
> recommended thresholds to the OS on a _per-bank_ basis. This patch adds
> a new boot option, which if passed, allows bios to initialize the CMCI
> threshold. In such a case, we simply skip programming any threshold
> value.
> 
> As fail-safe, we initialize threshold to 1 if some banks have not been
> initialized by the bios and warn the user.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  Documentation/x86/x86_64/boot-options.txt |    5 ++++
>  arch/x86/include/asm/mce.h                |    1 +
>  arch/x86/kernel/cpu/mcheck/mce.c          |    4 +++
>  arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
>  4 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
> index c54b4f5..ec92540 100644
> --- a/Documentation/x86/x86_64/boot-options.txt
> +++ b/Documentation/x86/x86_64/boot-options.txt
> @@ -50,6 +50,11 @@ Machine check
>  		monarchtimeout:
>  		Sets the time in us to wait for other CPUs on machine checks. 0
>  		to disable.
> +   mce=bios_cmci_threshold
> +		Don't overwrite the bios-set CMCI threshold. This boot option
> +		prevents Linux from overwriting the CMCI threshold set by the
> +		bios. Without this option, Linux always sets the CMCI
> +		threshold to 1.
>  
>     nomce (for compatibility with i386): same as mce=off
>  
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index a3ac52b..8ad5078 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -171,6 +171,7 @@ DECLARE_PER_CPU(struct device *, mce_device);
>  #ifdef CONFIG_X86_MCE_INTEL
>  extern int mce_cmci_disabled;
>  extern int mce_ignore_ce;
> +extern int mce_bios_cmci_threshold;
>  void mce_intel_feature_init(struct cpuinfo_x86 *c);
>  void cmci_clear(void);
>  void cmci_reenable(void);
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 292d025..401359d 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -82,6 +82,7 @@ static int			mce_panic_timeout	__read_mostly;
>  static int			mce_dont_log_ce		__read_mostly;
>  int				mce_cmci_disabled	__read_mostly;
>  int				mce_ignore_ce		__read_mostly;
> +int				mce_bios_cmci_threshold	__read_mostly;
>  int				mce_ser			__read_mostly;

AFAICT, this is actually a single-bit flag but we're using a whole
integer for it and from looking at the other boot options a couple of
them are used as flags too.

Care to define a

struct boot_flags {
	__u64 	mce_bios_cmci_threshold : 1,
		__reserved		: 63;
};

and use

	boot_flags.mce_bios_cmci_threshold

in the conditionals below instead?

I'll try to convert the rest of them to that struct and thus save some
more space...

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86: mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

On 08/22/2012 06:16 PM, Borislav Petkov wrote:
> On Wed, Aug 22, 2012 at 06:00:54PM +0530, Naveen N. Rao wrote:
>> The ACPI spec doesn't provide for a way for the bios to pass down
>> recommended thresholds to the OS on a _per-bank_ basis. This patch adds
>> a new boot option, which if passed, allows bios to initialize the CMCI
>> threshold. In such a case, we simply skip programming any threshold
>> value.
>>
>> As fail-safe, we initialize threshold to 1 if some banks have not been
>> initialized by the bios and warn the user.
>>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   Documentation/x86/x86_64/boot-options.txt |    5 ++++
>>   arch/x86/include/asm/mce.h                |    1 +
>>   arch/x86/kernel/cpu/mcheck/mce.c          |    4 +++
>>   arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
>>   4 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
>> index c54b4f5..ec92540 100644
>> --- a/Documentation/x86/x86_64/boot-options.txt
>> +++ b/Documentation/x86/x86_64/boot-options.txt
>> @@ -50,6 +50,11 @@ Machine check
>>   		monarchtimeout:
>>   		Sets the time in us to wait for other CPUs on machine checks. 0
>>   		to disable.
>> +   mce=bios_cmci_threshold
>> +		Don't overwrite the bios-set CMCI threshold. This boot option
>> +		prevents Linux from overwriting the CMCI threshold set by the
>> +		bios. Without this option, Linux always sets the CMCI
>> +		threshold to 1.
>>
>>      nomce (for compatibility with i386): same as mce=off
>>
>> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
>> index a3ac52b..8ad5078 100644
>> --- a/arch/x86/include/asm/mce.h
>> +++ b/arch/x86/include/asm/mce.h
>> @@ -171,6 +171,7 @@ DECLARE_PER_CPU(struct device *, mce_device);
>>   #ifdef CONFIG_X86_MCE_INTEL
>>   extern int mce_cmci_disabled;
>>   extern int mce_ignore_ce;
>> +extern int mce_bios_cmci_threshold;
>>   void mce_intel_feature_init(struct cpuinfo_x86 *c);
>>   void cmci_clear(void);
>>   void cmci_reenable(void);
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> index 292d025..401359d 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -82,6 +82,7 @@ static int			mce_panic_timeout	__read_mostly;
>>   static int			mce_dont_log_ce		__read_mostly;
>>   int				mce_cmci_disabled	__read_mostly;
>>   int				mce_ignore_ce		__read_mostly;
>> +int				mce_bios_cmci_threshold	__read_mostly;
>>   int				mce_ser			__read_mostly;
>
> AFAICT, this is actually a single-bit flag but we're using a whole
> integer for it and from looking at the other boot options a couple of
> them are used as flags too.
>
> Care to define a
>
> struct boot_flags {
> 	__u64 	mce_bios_cmci_threshold : 1,
> 		__reserved		: 63;
> };
>
> and use
>
> 	boot_flags.mce_bios_cmci_threshold
>
> in the conditionals below instead?

Sure - sounds like a good idea. Further, a #define could eliminate the 
need to change other references, but I'm not sure that's GENERALLacceptable

#define mce_bios_cmci_threshold boot_flags.mce_bios_cmci_threshold

could eliminate the need to change other references, but I'm not sure 
that's acceptable

But, I just had a quick look and it seems to me that these were defined 
as integers since they are exposed via sysfs. For instance:

static struct dev_ext_attribute dev_attr_cmci_disabled = {
         __ATTR(cmci_disabled, 0644, device_show_int, set_cmci_disabled),
         &mce_cmci_disabled
};

Converting mce_cmci_disabled to a bit-field doesn't work since we take 
its address above. We could ignore and not set the second field at all 
(dev_ext_attribute->var) and define our own callbacks, but that'll be 
more work and I'm not sure if we work fine without


>
> I'll try to convert the rest of them to that struct and thus save some
> more space...
>
> Thanks.
>

Re: [PATCH] x86: mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

On 08/22/2012 06:16 PM, Borislav Petkov wrote:
> On Wed, Aug 22, 2012 at 06:00:54PM +0530, Naveen N. Rao wrote:
>> The ACPI spec doesn't provide for a way for the bios to pass down
>> recommended thresholds to the OS on a _per-bank_ basis. This patch adds
>> a new boot option, which if passed, allows bios to initialize the CMCI
>> threshold. In such a case, we simply skip programming any threshold
>> value.
>>
>> As fail-safe, we initialize threshold to 1 if some banks have not been
>> initialized by the bios and warn the user.
>>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   Documentation/x86/x86_64/boot-options.txt |    5 ++++
>>   arch/x86/include/asm/mce.h                |    1 +
>>   arch/x86/kernel/cpu/mcheck/mce.c          |    4 +++
>>   arch/x86/kernel/cpu/mcheck/mce_intel.c    |   39 +++++++++++++++++++++++++++--
>>   4 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
>> index c54b4f5..ec92540 100644
>> --- a/Documentation/x86/x86_64/boot-options.txt
>> +++ b/Documentation/x86/x86_64/boot-options.txt
>> @@ -50,6 +50,11 @@ Machine check
>>   		monarchtimeout:
>>   		Sets the time in us to wait for other CPUs on machine checks. 0
>>   		to disable.
>> +   mce=bios_cmci_threshold
>> +		Don't overwrite the bios-set CMCI threshold. This boot option
>> +		prevents Linux from overwriting the CMCI threshold set by the
>> +		bios. Without this option, Linux always sets the CMCI
>> +		threshold to 1.
>>
>>      nomce (for compatibility with i386): same as mce=off
>>
>> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
>> index a3ac52b..8ad5078 100644
>> --- a/arch/x86/include/asm/mce.h
>> +++ b/arch/x86/include/asm/mce.h
>> @@ -171,6 +171,7 @@ DECLARE_PER_CPU(struct device *, mce_device);
>>   #ifdef CONFIG_X86_MCE_INTEL
>>   extern int mce_cmci_disabled;
>>   extern int mce_ignore_ce;
>> +extern int mce_bios_cmci_threshold;
>>   void mce_intel_feature_init(struct cpuinfo_x86 *c);
>>   void cmci_clear(void);
>>   void cmci_reenable(void);
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> index 292d025..401359d 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -82,6 +82,7 @@ static int			mce_panic_timeout	__read_mostly;
>>   static int			mce_dont_log_ce		__read_mostly;
>>   int				mce_cmci_disabled	__read_mostly;
>>   int				mce_ignore_ce		__read_mostly;
>> +int				mce_bios_cmci_threshold	__read_mostly;
>>   int				mce_ser			__read_mostly;
>
> AFAICT, this is actually a single-bit flag but we're using a whole
> integer for it and from looking at the other boot options a couple of
> them are used as flags too.
>
> Care to define a
>
> struct boot_flags {
> 	__u64 	mce_bios_cmci_threshold : 1,
> 		__reserved		: 63;
> };
>
> and use
>
> 	boot_flags.mce_bios_cmci_threshold
>
> in the conditionals below instead?

Sure - that sounds like a good idea. Further, a #define can be used to 
avoid the need to change other references, though I'm not sure it's 
considered good practice:

#define mce_bios_cmci_threshold   boot_flags.mce_bios_cmci_threshold


Thanks!
- Naveen

>
> I'll try to convert the rest of them to that struct and thus save some
> more space...
>
> Thanks.
>

Re: [PATCH] x86: mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Thu, Aug 23, 2012 at 05:38:05PM +0530, Naveen N. Rao wrote:
> Sure - that sounds like a good idea. Further, a #define can be used to
> avoid the need to change other references, though I'm not sure it's
> considered good practice:
>
> #define mce_bios_cmci_threshold boot_flags.mce_bios_cmci_threshold

That would hide the fact that this flag is part of the boot_flags struct
and would obfuscate the code. We can always add it later though, if it
makes sense.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86: mce: Honour bios-set CMCI threshold

[Borislav Petkov]

On Thu, Aug 23, 2012 at 05:26:09PM +0530, Naveen N. Rao wrote:
> Sure - sounds like a good idea. Further, a #define could eliminate
> the need to change other references, but I'm not sure that's
> GENERALLacceptable
>
> #define mce_bios_cmci_threshold boot_flags.mce_bios_cmci_threshold
>
> could eliminate the need to change other references, but I'm not sure
> that's acceptable

Yeah, that's kinda obfuscating it for no reason. As I said before, we
can always add it later if it makes sense.

> But, I just had a quick look and it seems to me that these were
> defined as integers since they are exposed via sysfs. For instance:
> 
> static struct dev_ext_attribute dev_attr_cmci_disabled = {
>         __ATTR(cmci_disabled, 0644, device_show_int, set_cmci_disabled),
>         &mce_cmci_disabled
> };
> 
> Converting mce_cmci_disabled to a bit-field doesn't work since we
> take its address above. We could ignore and not set the second field
> at all (dev_ext_attribute->var) and define our own callbacks, but
> that'll be more work and I'm not sure if we work fine without

Right,

but take a look at set_cmci_disabled(): it converts the newly read value
to bool anyway:

        if (mce_cmci_disabled ^ !!new) {

so we can do later

	flags.mce_cmci_disabled = true;

or
	flags.mce_cmci_disabled = false;

instead of assigning 0 or 1 to it.

And, about showing it with device_show_int, a simple test works:

---
#include <stdio.h>
#include <stdbool.h>

int main()
{
        bool a = true;
        printf("%d\n", a);
        return 0;
}
--

but even if there are troubles with that, we can change device_show_int
to a locally defined function.

But, anyway, long story short: I wasn't suggesting you go and change all
of them - simply start by adding your flag mce_bios_cmci_threshold to a
struct <something>_flags and I'll take care of the rest.

Unless you really want to do it, of course :-)

Oh, and the more important thing is, Tony would need to review your
Intel-specific changes so pls keep him CCed on your next iteration too.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86: mce: Honour bios-set CMCI threshold

[Naveen N. Rao]

On 08/27/2012 02:42 PM, Borislav Petkov wrote:
> On Thu, Aug 23, 2012 at 05:26:09PM +0530, Naveen N. Rao wrote:
>> Sure - sounds like a good idea. Further, a #define could eliminate
>> the need to change other references, but I'm not sure that's
>> GENERALLacceptable
>>
>> #define mce_bios_cmci_threshold boot_flags.mce_bios_cmci_threshold
>>
>> could eliminate the need to change other references, but I'm not sure
>> that's acceptable
>
> Yeah, that's kinda obfuscating it for no reason. As I said before, we
> can always add it later if it makes sense.

Ouch!
This was an old draft and I'm not sure how this ended up on the list!
Sorry for all the trouble.

>
>> But, I just had a quick look and it seems to me that these were
>> defined as integers since they are exposed via sysfs. For instance:
>>
>> static struct dev_ext_attribute dev_attr_cmci_disabled = {
>>          __ATTR(cmci_disabled, 0644, device_show_int, set_cmci_disabled),
>>          &mce_cmci_disabled
>> };
>>
>> Converting mce_cmci_disabled to a bit-field doesn't work since we
>> take its address above. We could ignore and not set the second field
>> at all (dev_ext_attribute->var) and define our own callbacks, but
>> that'll be more work and I'm not sure if we work fine without
>
> Right,
>
> but take a look at set_cmci_disabled(): it converts the newly read value
> to bool anyway:
>
>          if (mce_cmci_disabled ^ !!new) {
>
> so we can do later
>
> 	flags.mce_cmci_disabled = true;
>
> or
> 	flags.mce_cmci_disabled = false;
>
> instead of assigning 0 or 1 to it.
>
> And, about showing it with device_show_int, a simple test works:
>
> ---
> #include <stdio.h>
> #include <stdbool.h>
>
> int main()
> {
>          bool a = true;
>          printf("%d\n", a);
>          return 0;
> }
> --
>
> but even if there are troubles with that, we can change device_show_int
> to a locally defined function.
>
> But, anyway, long story short: I wasn't suggesting you go and change all
> of them - simply start by adding your flag mce_bios_cmci_threshold to a
> struct <something>_flags and I'll take care of the rest.
>
> Unless you really want to do it, of course :-)

Sure, I'll send a patch for this soon.


Regards,
Naveen

>
> Oh, and the more important thing is, Tony would need to review your
> Intel-specific changes so pls keep him CCed on your next iteration too.
>
> Thanks.
>