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

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

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.
>