On 08/27/2012 08:06 PM, Borislav Petkov wrote:
On Mon, Aug 27, 2012 at 04:55:03PM +0530, Naveen N. Rao wrote:Many MCE boot flags are boolean in nature, but are declared as integers currently. We can pack these into a bitfield to save some space.Signed-off-by: Naveen N. Rao <naveen.n....@linux.vnet.ibm.com>Ok, looks good. A couple of issues though: You need to prepare your patches against tip/master (which contains tip/x86/mce) because there are MCE changes in tip and they conflict with your changes.
Sure, will do so for my next iteration.
--- arch/x86/include/asm/mce.h | 11 +++- arch/x86/kernel/cpu/mcheck/mce.c | 86 +++++++++++++++++++++----------- arch/x86/kernel/cpu/mcheck/mce_intel.c | 2 - 3 files changed, 66 insertions(+), 33 deletions(-) diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index a3ac52b..78caeb2 100644 --- a/arch/x86/include/asm/mce.h +++ b/arch/x86/include/asm/mce.h @@ -129,6 +129,15 @@ struct mce_log { #ifdef __KERNEL__ +struct mce_boot_flags { + __u32 cmci_disabled : 1, + ignore_ce : 1, + dont_log_ce : 1, + __pad : 29; +};This looks ok but I think it would be even better if it went into mce-internal.h where all the mce-only stuff can be collected in a private struct so that we don't pollute the global MCE namespace. Then you can call the struct simply struct boot_flags since it is private to mce code.
Good idea - will do.
+ +extern struct mce_boot_flags mce_boot_flags;Why do we need that extern thing?
So that this is visible across mce.c and mce_intel.c?
+ extern void mce_register_decode_chain(struct notifier_block *nb); extern void mce_unregister_decode_chain(struct notifier_block *nb); @@ -169,8 +178,6 @@ DECLARE_PER_CPU(struct device *, mce_device); #define MAX_NR_BANKS 32 #ifdef CONFIG_X86_MCE_INTEL -extern int mce_cmci_disabled; -extern int mce_ignore_ce; 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..5a0d399 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -79,11 +79,10 @@ static int rip_msr __read_mostly; static int mce_bootlog __read_mostly = -1; static int monarch_timeout __read_mostly = -1; 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_ser __read_mostly; +struct mce_boot_flags mce_boot_flags __read_mostly; + struct mce_bank *mce_banks __read_mostly; /* User mode helper program triggered by machine check event */ @@ -630,7 +629,7 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b) * Don't get the IP here because it's unlikely to * have anything to do with the actual error location. */ - if (!(flags & MCP_DONTLOG) && !mce_dont_log_ce) + if (!(flags & MCP_DONTLOG) && !mce_boot_flags.dont_log_ce) mce_log(&m); /* @@ -1601,7 +1600,7 @@ static void __mcheck_cpu_init_timer(void) setup_timer(t, mce_timer_fn, smp_processor_id()); - if (mce_ignore_ce) + if (mce_boot_flags.ignore_ce) return; __this_cpu_write(mce_next_interval, iv); @@ -1919,11 +1918,11 @@ static int __init mcheck_enable(char *str) if (!strcmp(str, "off")) mce_disabled = 1; else if (!strcmp(str, "no_cmci")) - mce_cmci_disabled = 1; + mce_boot_flags.cmci_disabled = 1; else if (!strcmp(str, "dont_log_ce")) - mce_dont_log_ce = 1; + mce_boot_flags.dont_log_ce = 1; else if (!strcmp(str, "ignore_ce")) - mce_ignore_ce = 1; + mce_boot_flags.ignore_ce = 1; else if (!strcmp(str, "bootlog") || !strcmp(str, "nobootlog")) mce_bootlog = (str[0] == 'b'); else if (isdigit(str[0])) { @@ -2076,7 +2075,7 @@ show_trigger(struct device *s, struct device_attribute *attr, char *buf) } static ssize_t set_trigger(struct device *s, struct device_attribute *attr, - const char *buf, size_t siz) + const char *buf, size_t size) { char *p;This is an unrelated change, pls drop it.
Ok.
@@ -2090,6 +2089,34 @@ static ssize_t set_trigger(struct device *s, struct device_attribute *attr, return strlen(mce_helper) + !!p; } +static ssize_t get_dont_log_ce(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return snprintf(buf, PAGE_SIZE, "%d\n", mce_boot_flags.dont_log_ce); +} + +static ssize_t set_dont_log_ce(struct device *s, + struct device_attribute *attr, + const char *buf, size_t size) +{ + u64 new; + + if (strict_strtoull(buf, 0, &new) < 0) + return -EINVAL; + + mce_boot_flags.dont_log_ce = !!new; + + return size; +} + +static ssize_t get_ignore_ce(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return snprintf(buf, PAGE_SIZE, "%d\n", mce_boot_flags.ignore_ce); +} +Those could be combined into a function similar to device_show/store_int pairs but working on a bitfield. Maybe later.
Yes, this looks like it needs some work and I wasn't sure it would be helpful at this time. So, yeah, maybe later.
Thanks! - Naveen
static ssize_t set_ignore_ce(struct device *s, struct device_attribute *attr, const char *buf, size_t size) @@ -2099,21 +2126,28 @@ static ssize_t set_ignore_ce(struct device *s, if (strict_strtoull(buf, 0, &new) < 0) return -EINVAL; - if (mce_ignore_ce ^ !!new) { + if (mce_boot_flags.ignore_ce ^ !!new) { if (new) { /* disable ce features */ mce_timer_delete_all(); on_each_cpu(mce_disable_cmci, NULL, 1); - mce_ignore_ce = 1; + mce_boot_flags.ignore_ce = 1; } else { /* enable ce features */ - mce_ignore_ce = 0; + mce_boot_flags.ignore_ce = 0; on_each_cpu(mce_enable_ce, (void *)1, 1); } } return size; } +static ssize_t get_cmci_disabled(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + return snprintf(buf, PAGE_SIZE, "%d\n", mce_boot_flags.cmci_disabled); +} + static ssize_t set_cmci_disabled(struct device *s, struct device_attribute *attr, const char *buf, size_t size) @@ -2123,14 +2157,14 @@ static ssize_t set_cmci_disabled(struct device *s, if (strict_strtoull(buf, 0, &new) < 0) return -EINVAL; - if (mce_cmci_disabled ^ !!new) { + if (mce_boot_flags.cmci_disabled ^ !!new) { if (new) { /* disable cmci */ on_each_cpu(mce_disable_cmci, NULL, 1); - mce_cmci_disabled = 1; + mce_boot_flags.cmci_disabled = 1; } else { /* enable cmci */ - mce_cmci_disabled = 0; + mce_boot_flags.cmci_disabled = 0; on_each_cpu(mce_enable_ce, NULL, 1); } } @@ -2149,31 +2183,23 @@ static ssize_t store_int_with_restart(struct device *s, static DEVICE_ATTR(trigger, 0644, show_trigger, set_trigger); static DEVICE_INT_ATTR(tolerant, 0644, tolerant); static DEVICE_INT_ATTR(monarch_timeout, 0644, monarch_timeout); -static DEVICE_INT_ATTR(dont_log_ce, 0644, mce_dont_log_ce); +static DEVICE_ATTR(dont_log_ce, 0644, get_dont_log_ce, set_dont_log_ce); +static DEVICE_ATTR(ignore_ce, 0644, get_ignore_ce, set_ignore_ce); +static DEVICE_ATTR(cmci_disabled, 0644, get_cmci_disabled, set_cmci_disabled); static struct dev_ext_attribute dev_attr_check_interval = { __ATTR(check_interval, 0644, device_show_int, store_int_with_restart), &check_interval }; -static struct dev_ext_attribute dev_attr_ignore_ce = { - __ATTR(ignore_ce, 0644, device_show_int, set_ignore_ce), - &mce_ignore_ce -}; - -static struct dev_ext_attribute dev_attr_cmci_disabled = { - __ATTR(cmci_disabled, 0644, device_show_int, set_cmci_disabled), - &mce_cmci_disabled -}; - static struct device_attribute *mce_device_attrs[] = { &dev_attr_tolerant.attr, &dev_attr_check_interval.attr, &dev_attr_trigger, &dev_attr_monarch_timeout.attr, - &dev_attr_dont_log_ce.attr, - &dev_attr_ignore_ce.attr, - &dev_attr_cmci_disabled.attr, + &dev_attr_dont_log_ce, + &dev_attr_ignore_ce, + &dev_attr_cmci_disabled, NULL }; @@ -2314,7 +2340,7 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu) break; case CPU_DOWN_FAILED: case CPU_DOWN_FAILED_FROZEN: - if (!mce_ignore_ce && check_interval) { + if (!mce_boot_flags.ignore_ce && check_interval) { t->expires = round_jiffies(jiffies + per_cpu(mce_next_interval, cpu)); add_timer_on(t, cpu); diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c index 38e49bc..aaf5c51 100644 --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c @@ -36,7 +36,7 @@ static int cmci_supported(int *banks) { u64 cap; - if (mce_cmci_disabled || mce_ignore_ce) + if (mce_boot_flags.cmci_disabled || mce_boot_flags.ignore_ce) return 0;Thanks.
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
