[PATCH] x86/mce: Fix initialization error warning

[PATCH] x86/mce: Fix initialization error warning

[Prarit Bhargava]

When booting kernel with mce=off a loud warning from the mce code
is displayed.  This causes confusion for end users.

Add a check to see if MCE is available before outputting the warning
message.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-edac@vger.kernel.org
---
 arch/x86/kernel/cpu/mcheck/mce.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 00ef43233e03..943a0c440c55 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2662,7 +2662,8 @@ static __init int mcheck_init_device(void)
 	free_cpumask_var(mce_device_initialized);
 
 err_out:
-	pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
+	if (mce_available(&boot_cpu_data))
+		pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
 
 	return err;
 }
-- 
1.7.9.3

Re: [PATCH] x86/mce: Fix initialization error warning

[Thomas Gleixner]

On Mon, 16 Jan 2017, Prarit Bhargava wrote:
> When booting kernel with mce=off a loud warning from the mce code
> is displayed.  This causes confusion for end users.
> 
> Add a check to see if MCE is available before outputting the warning
> message.

Sigh.

> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: linux-edac@vger.kernel.org
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 00ef43233e03..943a0c440c55 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -2662,7 +2662,8 @@ static __init int mcheck_init_device(void)
>  	free_cpumask_var(mce_device_initialized);
>  
>  err_out:
> -	pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);
> +	if (mce_available(&boot_cpu_data))
> +		pr_err("Unable to init device /dev/mcelog (rc: %d)\n", err);

This is really crap. Why would you check the same condition twice? Just
because it's so much more complicated than doing the obvious:

--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2615,10 +2615,8 @@ static __init int mcheck_init_device(voi
 	enum cpuhp_state hp_online;
 	int err;
 
-	if (!mce_available(&boot_cpu_data)) {
-		err = -EIO;
-		goto err_out;
-	}
+	if (!mce_available(&boot_cpu_data))
+		return -EIO;
 
 	if (!zalloc_cpumask_var(&mce_device_initialized, GFP_KERNEL)) {
 		err = -ENOMEM;

Re: [PATCH] x86/mce: Fix initialization error warning

[Borislav Petkov]

On Mon, Jan 16, 2017 at 04:49:41PM -0500, Prarit Bhargava wrote:
> When booting kernel with mce=off a loud warning from the mce code
> is displayed.  This causes confusion for end users.

Is this what you call a loud warning?

[    1.885648] mce: Unable to init device /dev/mcelog (rc: -5)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mce: Fix initialization error warning

[Prarit Bhargava]

On 01/16/2017 04:56 PM, Borislav Petkov wrote:
> On Mon, Jan 16, 2017 at 04:49:41PM -0500, Prarit Bhargava wrote:
>> When booting kernel with mce=off a loud warning from the mce code
>> is displayed.  This causes confusion for end users.
> 
> Is this what you call a loud warning?
> 
> [    1.885648] mce: Unable to init device /dev/mcelog (rc: -5)

Yes, it was loud enough to generate a bug report from a user.

FWIW, I just found this from last year calling the same message "scary".  It
doesn't look like there was a follow up though.

https://lkml.org/lkml/2016/2/9/354

P.

> 

Re: [PATCH] x86/mce: Fix initialization error warning

[Borislav Petkov]

On Mon, Jan 16, 2017 at 05:06:02PM -0500, Prarit Bhargava wrote:
> Yes, it was loud enough to generate a bug report from a user.

Yeah, because all users are sane and we should do whatever they want -
no questions asked. Especially those who boot with "mce=off".

Did you actually ask that user why she/he is even booting with
"mce=off"?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mce: Fix initialization error warning

[Prarit Bhargava]

On 01/16/2017 05:43 PM, Borislav Petkov wrote:
> On Mon, Jan 16, 2017 at 05:06:02PM -0500, Prarit Bhargava wrote:
>> Yes, it was loud enough to generate a bug report from a user.
> 
> Yeah, because all users are sane and we should do whatever they want -
> no questions asked. Especially those who boot with "mce=off".
> 
> Did you actually ask that user why she/he is even booting with
> "mce=off"?

Yes, mce=off is the default for kdump:

KDUMP_COMMANDLINE_APPEND="irqpoll nr_cpus=1 reset_devices cgroup_disable=memory
mce=off numa=off udev.children-max=2 panic=10 rootflags=nofail
acpi_no_memhotplug transparent_hugepage=never"

There is a race condition between NMI completing on a CPU and the MCE
synchronization timing out that results in a kernel panic on the kdump kernel,
and a loss of the dump image.  There have been a few attempts to fix it over the
years.  It seems as simple as setting a flag in native_machine_crash_shutdown()
and querying it in do_machine_check() to avoid mce & nmi race.

P.

> 

Re: [PATCH] x86/mce: Fix initialization error warning

[Borislav Petkov]

On Mon, Jan 16, 2017 at 06:13:39PM -0500, Prarit Bhargava wrote:
> Yes, mce=off is the default for kdump:

So fix kdump to disable MCA properly instead of sending me brown paper
bags.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mce: Fix initialization error warning

[Prarit Bhargava]

On 01/16/2017 06:32 PM, Borislav Petkov wrote:
> On Mon, Jan 16, 2017 at 06:13:39PM -0500, Prarit Bhargava wrote:
>> Yes, mce=off is the default for kdump:
> 
> So fix kdump to disable MCA properly instead of sending me brown paper
> bags.

Sure, I'll take a shot at it.

Thanks,

P.

> 

Re: [PATCH] x86/mce: Fix initialization error warning

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> On Mon, Jan 16, 2017 at 06:13:39PM -0500, Prarit Bhargava wrote:
> > Yes, mce=off is the default for kdump:
> 
> So fix kdump to disable MCA properly instead of sending me brown paper
> bags.

That's one thing, but OTOH we really can avoid the KERN_ERR level print out
for the case where mce is not available (for whatever reasons: HW / Virt /
command line ....).

The simple 2 liner I posted earlier in this thread does it nicely.

Thanks,

	tglx

Re: [PATCH] x86/mce: Fix initialization error warning

[Borislav Petkov]

On Tue, Jan 17, 2017 at 09:34:18AM +0100, Thomas Gleixner wrote:
> That's one thing, but OTOH we really can avoid the KERN_ERR level print out
> for the case where mce is not available (for whatever reasons: HW / Virt /
> command line ....).
> 
> The simple 2 liner I posted earlier in this thread does it nicely.

Well, I seem to remember at the time of this:

  9c15a24b038f ("x86/mce: Improve mcheck_init_device() error handling")

that I requested to have that error message there so that we *know* when
that function fails instead of trying to decipher "Hey, mcelog doesn't
start here" and wonder why.

But we can kill it just as well, especially as there's patch deprecating
/dev/mcelog floating around. We can reorg error messages in that
function later, if deemed necessary.

So sure.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mce: Fix initialization error warning

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> On Tue, Jan 17, 2017 at 09:34:18AM +0100, Thomas Gleixner wrote:
> > That's one thing, but OTOH we really can avoid the KERN_ERR level print out
> > for the case where mce is not available (for whatever reasons: HW / Virt /
> > command line ....).
> > 
> > The simple 2 liner I posted earlier in this thread does it nicely.
> 
> Well, I seem to remember at the time of this:
> 
>   9c15a24b038f ("x86/mce: Improve mcheck_init_device() error handling")
> 
> that I requested to have that error message there so that we *know* when
> that function fails instead of trying to decipher "Hey, mcelog doesn't
> start here" and wonder why.
> 
> But we can kill it just as well, especially as there's patch deprecating
> /dev/mcelog floating around. We can reorg error messages in that
> function later, if deemed necessary.

Your decision.