| Message ID | 1397056476-9183-2-git-send-email-tomasz.nowicki@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 09, 2014 at 05:14:29PM +0200, Tomasz Nowicki wrote: > This commit is dealing with MCE code in: > - hest.c > Move acpi_disable_cmcff flag to hest_parse_cmc() and makes > that depend on CONFIG_X86_MCE so that we do not have to maintain > acpi_disable_cmcff for architectures which do not support MCE. > Also, wrap architectural MCE header inside #ifdef CONFIG_X86_MCE. > > - ghes.c > Wrap architectural MCE header inside #ifdef CONFIG_X86_MCE similar to rest > of the MCE code in this file. > > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > --- > drivers/acpi/apei/ghes.c | 2 ++ > drivers/acpi/apei/hest.c | 8 ++++++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index dab7cb7..f7edffc 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -49,7 +49,9 @@ > #include <linux/aer.h> > > #include <acpi/ghes.h> > +#ifdef CONFIG_X86_MCE > #include <asm/mce.h> > +#endif > #include <asm/tlbflush.h> > #include <asm/nmi.h> > > diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c > index f5e37f3..98db702 100644 > --- a/drivers/acpi/apei/hest.c > +++ b/drivers/acpi/apei/hest.c > @@ -36,7 +36,9 @@ > #include <linux/io.h> > #include <linux/platform_device.h> > #include <acpi/apei.h> > +#ifdef CONFIG_X86_MCE > #include <asm/mce.h> > +#endif Actually, I would prefer if you wrapped all the arch-specific calls into arch-specific functions, say, convert apei_mce_report_mem_error -> apei_arch_report_mem_error and have default empty functions for arches which don't use that functionality. This way you can save yourself the ugly ifdeffery around the place. > > #include "apei-internal.h" > > @@ -133,6 +135,9 @@ static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data) > struct acpi_hest_ia_corrected *cmc; > struct acpi_hest_ia_error_bank *mc_bank; > > + if (acpi_disable_cmcff) > + return 1; This could be if (arch_disable_cmcff()) return 1; with the default stub being static inline bool arch_disable_cmcff(void) { return false; } and so on, like it is done in many other places in the kernel. Thanks.
On 05.05.2014 13:44, Borislav Petkov wrote: > On Wed, Apr 09, 2014 at 05:14:29PM +0200, Tomasz Nowicki wrote: >> This commit is dealing with MCE code in: >> - hest.c >> Move acpi_disable_cmcff flag to hest_parse_cmc() and makes >> that depend on CONFIG_X86_MCE so that we do not have to maintain >> acpi_disable_cmcff for architectures which do not support MCE. >> Also, wrap architectural MCE header inside #ifdef CONFIG_X86_MCE. >> >> - ghes.c >> Wrap architectural MCE header inside #ifdef CONFIG_X86_MCE similar to rest >> of the MCE code in this file. >> >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> --- >> drivers/acpi/apei/ghes.c | 2 ++ >> drivers/acpi/apei/hest.c | 8 ++++++-- >> 2 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index dab7cb7..f7edffc 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -49,7 +49,9 @@ >> #include <linux/aer.h> >> >> #include <acpi/ghes.h> >> +#ifdef CONFIG_X86_MCE >> #include <asm/mce.h> >> +#endif >> #include <asm/tlbflush.h> >> #include <asm/nmi.h> >> >> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c >> index f5e37f3..98db702 100644 >> --- a/drivers/acpi/apei/hest.c >> +++ b/drivers/acpi/apei/hest.c >> @@ -36,7 +36,9 @@ >> #include <linux/io.h> >> #include <linux/platform_device.h> >> #include <acpi/apei.h> >> +#ifdef CONFIG_X86_MCE >> #include <asm/mce.h> >> +#endif > > Actually, I would prefer if you wrapped all the arch-specific calls into > arch-specific functions, say, convert > > apei_mce_report_mem_error -> apei_arch_report_mem_error > > and have default empty functions for arches which don't use that > functionality. > > This way you can save yourself the ugly ifdeffery around the place. > True, this can be improved as you suggested. >> >> #include "apei-internal.h" >> >> @@ -133,6 +135,9 @@ static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data) >> struct acpi_hest_ia_corrected *cmc; >> struct acpi_hest_ia_error_bank *mc_bank; >> >> + if (acpi_disable_cmcff) >> + return 1; > > This could be > > if (arch_disable_cmcff()) > return 1; > > with the default stub being > > static inline bool arch_disable_cmcff(void) > { > return false; > } > > and so on, like it is done in many other places in the kernel. acpi_disable_cmcff as global value can switch off/on MC entries analysing via kernel args. This glob value resides in x86 ACPI code and has meaning only for MCE related mechanism, that is why I have moved it under hest_parse_cmc. Thanks. Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 05, 2014 at 04:34:41PM +0200, Tomasz Nowicki wrote: > acpi_disable_cmcff as global value can switch off/on MC entries > analysing via kernel args. No, it switches off firmware first mode for correctable errors because of buggy BIOSes - 9ad95879cd1b2 (what else...) > This glob value resides in x86 ACPI code and has meaning only for MCE > related mechanism, Of course it doesn't! > that is why I have moved it under hest_parse_cmc. See APEI section in the ACPI spec "18.4 Firmware First Error Handling." Regardless of what your version of APEI does, you actually shouldn't need to touch acpi_disable_cmcff at all as it not arch-specific.
On 05.05.2014 16:53, Borislav Petkov wrote: > On Mon, May 05, 2014 at 04:34:41PM +0200, Tomasz Nowicki wrote: >> acpi_disable_cmcff as global value can switch off/on MC entries >> analysing via kernel args. > > No, it switches off firmware first mode for correctable errors because > of buggy BIOSes - 9ad95879cd1b2 (what else...) > >> This glob value resides in x86 ACPI code and has meaning only for MCE >> related mechanism, > > Of course it doesn't! > >> that is why I have moved it under hest_parse_cmc. > > See APEI section in the ACPI spec "18.4 Firmware First Error Handling." > > Regardless of what your version of APEI does, you actually shouldn't > need to touch acpi_disable_cmcff at all as it not arch-specific. > You are right! I will fix that big misunderstanding from my side in next patch version. Thanks, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, May 05, 2014 at 05:32:07PM +0200, Tomasz Nowicki wrote: > You are right! I will fix that big misunderstanding from my side in > next patch version. Ok, but pls wait with resubmitting until I take a look at the rest of your patches. Thanks.
On 05.05.2014 17:33, Borislav Petkov wrote: > On Mon, May 05, 2014 at 05:32:07PM +0200, Tomasz Nowicki wrote: >> You are right! I will fix that big misunderstanding from my side in >> next patch version. > > Ok, but pls wait with resubmitting until I take a look at the rest of > your patches. Sure, I will. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index dab7cb7..f7edffc 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -49,7 +49,9 @@ #include <linux/aer.h> #include <acpi/ghes.h> +#ifdef CONFIG_X86_MCE #include <asm/mce.h> +#endif #include <asm/tlbflush.h> #include <asm/nmi.h> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c index f5e37f3..98db702 100644 --- a/drivers/acpi/apei/hest.c +++ b/drivers/acpi/apei/hest.c @@ -36,7 +36,9 @@ #include <linux/io.h> #include <linux/platform_device.h> #include <acpi/apei.h> +#ifdef CONFIG_X86_MCE #include <asm/mce.h> +#endif #include "apei-internal.h" @@ -133,6 +135,9 @@ static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data) struct acpi_hest_ia_corrected *cmc; struct acpi_hest_ia_error_bank *mc_bank; + if (acpi_disable_cmcff) + return 1; + if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK) return 0; @@ -263,8 +268,7 @@ void __init acpi_hest_init(void) goto err; } - if (!acpi_disable_cmcff) - apei_hest_parse(hest_parse_cmc, NULL); + apei_hest_parse(hest_parse_cmc, NULL); if (!ghes_disable) { rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
This commit is dealing with MCE code in: - hest.c Move acpi_disable_cmcff flag to hest_parse_cmc() and makes that depend on CONFIG_X86_MCE so that we do not have to maintain acpi_disable_cmcff for architectures which do not support MCE. Also, wrap architectural MCE header inside #ifdef CONFIG_X86_MCE. - ghes.c Wrap architectural MCE header inside #ifdef CONFIG_X86_MCE similar to rest of the MCE code in this file. Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> --- drivers/acpi/apei/ghes.c | 2 ++ drivers/acpi/apei/hest.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)