Skip to contentSkip to conversation

x86, MCE: Do not taint when handling correctable errors

PATCH In review Borislav Petkov 4 participants 9 messages Apr 15, 2011 – Apr 21, 2011 x86

lore
Share & tools
lore

Borislav Petkov

author
#0 lore raw

PATCH 1/2 x86, MCE: Do not taint when handling correctable errors

From: Borislav Petkov <[email protected]>

Correctable errors are considered something rather normal on modern hardware these days. Even more importantly, correctable errors mean exactly that - they've been corrected by the hardware - and there's no need to taint the kernel since execution hasn't been compromised so far.

Suggested-by Tony Luck <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
arch/x86/kernel/cpu/mcheck/mce.c +0 -1
index 3385ea2..68e2303 100644
@@ -590,7 +590,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		if (!(flags & MCP_DONTLOG) && !mce_dont_log_ce) {
 			mce_log(&m);
 			atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, &m);
-			add_taint(TAINT_MACHINE_CHECK);
 		}
 
 		/*
Signature · 1 line
1.7.4.rc2
#1 lore raw
Quoted 12 lines

-----Original Message-----
From: [email protected] [mailto:linux-edac- [email protected]] On Behalf Of Borislav Petkov
Sent: Friday, April 15, 2011 3:46 AM
To: H. Peter Anvin; Ingo Molnar; Thomas Gleixner
Cc: Prarit Bhargava; Tony Luck; Russ Anderson; LKML; EDAC devel; Borislav Petkov
Subject: [PATCH 1/2] x86, MCE: Do not taint when handling correctable errors

Quoted 7 lines

From: Borislav Petkov <[email protected]>

Correctable errors are considered something rather normal on modern hardware these days. Even more importantly, correctable errors mean exactly that - they've been corrected by the hardware - and there's no need to taint the kernel since execution hasn't been compromised so far.

Would it be okay to extend this reasoning and also remove the tainting caused by TM1/TM2 thermal events:

(quoted diff) +0 -0

b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 6f8c5e9..9f3b5ae 100644

arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -0
@@ -188,7 +188,6 @@ static int therm_throt_process(bool new_event, int event,

int level)

                                level == CORE_LEVEL ? "Core" : "Package",
                                state->count);
-               add_taint(TAINT_MACHINE_CHECK);
                return 1;
        }
        if (old_event) {

@@ -393,7 +392,6 @@ static void unexpected_thermal_interrupt(void) {
printk(KERN_ERR "CPU%d: Unexpected LVT thermal interrupt!\n",
smp_processor_id());
- add_taint(TAINT_MACHINE_CHECK);
}

static void (*smp_thermal_vector)(void) = unexpected_thermal_interrupt;

- naga -

Quoted 28 lines
Suggested-by Tony Luck <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>

---
arch/x86/kernel/cpu/mcheck/mce.c | 1 -

1 files changed, 0 insertions(+), 1 deletions(-)
arch/x86/kernel/cpu/mcheck/mce.c +0 -1
index 3385ea2..68e2303 100644
@@ -590,7 +590,6 @@ void machine_check_poll(enum mcp_flags flags,
 mce_banks_t *b)
 		if (!(flags & MCP_DONTLOG) && !mce_dont_log_ce) {
 			mce_log(&m);
 			atomic_notifier_call_chain(&x86_mce_decoder_chain, 0,
 &m);
-			add_taint(TAINT_MACHINE_CHECK);
 		}

		/*

--
1.7.4.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Acked-by
#2 lore raw
Quoted 2 lines

Would it be okay to extend this reasoning and also remove the tainting caused by TM1/TM2 thermal events:

Looks good to me - crossing a thermal threshold doesn't sound (by itself) worthy of a TAINT. Nothing has been corrupted.

Acked-by Tony Luck <[email protected]>
#3 lore raw
In reply to Chumbalkar, Nagananda
Quoted 18 lines · Chumbalkar, Nagananda

Would it be okay to extend this reasoning and also remove the tainting caused by TM1/TM2 thermal events:

arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -1
index 6f8c5e9..9f3b5ae 100644
@@ -188,7 +188,6 @@ static int therm_throt_process(bool new_event, int event,
 int level)
                                level == CORE_LEVEL ? "Core" : "Package",
                                state->count);
 
-               add_taint(TAINT_MACHINE_CHECK);
                return 1;
        }

        if (old_event) {

arch/x86/kernel/cpu/mcheck/therm_throt.c static void unexpected_thermal_interrupt(void) · line 395

Quoted 5 lines
arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -1
@@ -393,7 +392,6 @@ static void unexpected_thermal_interrupt(void)
 {
        printk(KERN_ERR "CPU%d: Unexpected LVT thermal interrupt!\n",
                        smp_processor_id());
-       add_taint(TAINT_MACHINE_CHECK);
 }

Mind sending a proper patch, with changelog, signoff, acks, etc?

Thanks,

Ingo

author
#4 lore raw
Quoted 16 lines · Ingo Molnar
Quoted 13 lines · Chumbalkar, Nagananda

Would it be okay to extend this reasoning and also remove the tainting caused by TM1/TM2 thermal events:

arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -0
index 6f8c5e9..9f3b5ae 100644
@@ -188,7 +188,6 @@ static int therm_throt_process(bool new_event, int event,
 int level)
                                level == CORE_LEVEL ? "Core" : "Package",
                                state->count);
Quoted 12 lines

arch/x86/kernel/cpu/mcheck/therm_throt.c static void unexpected_thermal_interrupt(void) · line 395

Quoted 10 lines
arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -1
-               add_taint(TAINT_MACHINE_CHECK);
                return 1;
        }

        if (old_event) {
arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -1
@@ -393,7 +392,6 @@ static void unexpected_thermal_interrupt(void)
 {
        printk(KERN_ERR "CPU%d: Unexpected LVT thermal interrupt!\n",
                        smp_processor_id());
-       add_taint(TAINT_MACHINE_CHECK);
 }

Mind sending a proper patch, with changelog, signoff, acks, etc?

No need since this is part of 7b70bd3441437b7bc04fc9d321e17c8ed0e8f958 now which is in tip/x86/mce.

Thanks.

Signature · 8 lines
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
#5 lore raw
Quoted 30 lines · Borislav Petkov
Quoted 16 lines · Ingo Molnar
Quoted 13 lines · Chumbalkar, Nagananda

Would it be okay to extend this reasoning and also remove the tainting caused by TM1/TM2 thermal events:

arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -0
index 6f8c5e9..9f3b5ae 100644
@@ -188,7 +188,6 @@ static int therm_throt_process(bool new_event, int event,
 int level)
                                level == CORE_LEVEL ? "Core" : "Package",
                                state->count);
Quoted 12 lines

arch/x86/kernel/cpu/mcheck/therm_throt.c static void unexpected_thermal_interrupt(void) · line 395

Quoted 10 lines
arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -1
-               add_taint(TAINT_MACHINE_CHECK);
                return 1;
        }

        if (old_event) {
arch/x86/kernel/cpu/mcheck/therm_throt.c +0 -1
@@ -393,7 +392,6 @@ static void unexpected_thermal_interrupt(void)
 {
        printk(KERN_ERR "CPU%d: Unexpected LVT thermal interrupt!\n",
                        smp_processor_id());
-       add_taint(TAINT_MACHINE_CHECK);
 }

Mind sending a proper patch, with changelog, signoff, acks, etc?

Quoted 2 lines

No need since this is part of 7b70bd3441437b7bc04fc9d321e17c8ed0e8f958 now which is in tip/x86/mce.

Ok, indeed. Also, in the future, if you take patches from others please also credit them in the changelog. Something like this would have been good in the current case:

Also, this patch includes a change from Nagananda Chumbalkar as well, which drops tainting in the therma throttling code for a similar reason: crossing a thermal threshold does not mean corruption.

Nagananda's Acked-by is there so there's at least partial credit - but we generally try to aim for at least 100% credit where credit is due :-)

Thanks,

Ingo

author
#6 lore raw
Quoted 10 lines · Ingo Molnar

Ok, indeed. Also, in the future, if you take patches from others please also credit them in the changelog. Something like this would have been good in the current case:

Also, this patch includes a change from Nagananda Chumbalkar as well, which drops tainting in the therma throttling code for a similar reason: crossing a thermal threshold does not mean corruption.

Nagananda's Acked-by is there so there's at least partial credit - but we generally try to aim for at least 100% credit where credit is due :-)

Absolutely, and in the light of recent events :) I'm still not sure how to do that though in a straight-forward manner so that it is visible at a first glance. Sure, adding freeform text to the commit message is one way. Using a SOB chain might work too - even the Acked-by tag - but all those have another main purpose and are being repurposed for annotating the fact that a patch is the result of more than one author's thought process.

IOW, in case I'm not missing anything, we don't really have a way to denote a multiple authorship, correct? And we should...

Thanks.

Signature · 8 lines
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
author
#7 lore raw
Quoted 20 lines · Borislav Petkov
Quoted 10 lines · Ingo Molnar

Ok, indeed. Also, in the future, if you take patches from others please also credit them in the changelog. Something like this would have been good in the current case:

Also, this patch includes a change from Nagananda Chumbalkar as well, which drops tainting in the therma throttling code for a similar reason: crossing a thermal threshold does not mean corruption.

Nagananda's Acked-by is there so there's at least partial credit - but we generally try to aim for at least 100% credit where credit is due :-)

Absolutely, and in the light of recent events :) I'm still not sure how to do that though in a straight-forward manner so that it is visible at a first glance. Sure, adding freeform text to the commit message is one way. Using a SOB chain might work too - even the Acked-by tag - but all those have another main purpose and are being repurposed for annotating the fact that a patch is the result of more than one author's thought process.

Quoted 2 lines

IOW, in case I'm not missing anything, we don't really have a way to denote a multiple authorship, correct? And we should...

Ok, after RTFMing <Documentation/SubmittingPatches> here's how:

From: Original Author <[email protected]>

^ this is the original author who sent the initial patch

Signed-off-by Original Author <[email protected]>
Signed-off-by Additional Author <[email protected]>
Signed-off-by Third Author <[email protected]>

and the SOBs following after the 1st one are denoting additional authors.

However, additional SOBs mean also subsystem maintainers on the delivery path of the patch. I guess this is ok though.

Thanks.

Signature · 8 lines
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
#8 lore raw
Quoted 26 lines · Borislav Petkov
Quoted 20 lines · Borislav Petkov
Quoted 10 lines · Ingo Molnar

Ok, indeed. Also, in the future, if you take patches from others please also credit them in the changelog. Something like this would have been good in the current case:

Also, this patch includes a change from Nagananda Chumbalkar as well, which drops tainting in the therma throttling code for a similar reason: crossing a thermal threshold does not mean corruption.

Nagananda's Acked-by is there so there's at least partial credit - but we generally try to aim for at least 100% credit where credit is due :-)

Absolutely, and in the light of recent events :) I'm still not sure how to do that though in a straight-forward manner so that it is visible at a first glance. Sure, adding freeform text to the commit message is one way. Using a SOB chain might work too - even the Acked-by tag - but all those have another main purpose and are being repurposed for annotating the fact that a patch is the result of more than one author's thought process.

Quoted 2 lines

IOW, in case I'm not missing anything, we don't really have a way to denote a multiple authorship, correct? And we should...

Ok, after RTFMing <Documentation/SubmittingPatches> here's how:

Quoted 7 lines

From: Original Author <[email protected]>

^ this is the original author who sent the initial patch

Signed-off-by Original Author <[email protected]>
Signed-off-by Additional Author <[email protected]>
Signed-off-by Third Author <[email protected]>

The easiest solution is to create a separate patch for the TM1/TM2 change and ask for a SOB :-)

Thanks,

Ingo