[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240614214736.GA726880@yaz-khff2.amd.com>
Date: Fri, 14 Jun 2024 17:47:36 -0400
From: Yazen Ghannam <yazen.ghannam@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
tony.luck@...el.com, x86@...nel.org, avadhut.naik@....com,
john.allen@....com
Subject: Re: [PATCH 3/3] x86/mce: Use mce_prep_record() helpers for
apei_smca_report_x86_error()
On Mon, Jun 03, 2024 at 06:55:30PM +0200, Borislav Petkov wrote:
> On Mon, Jun 03, 2024 at 10:34:10AM -0400, Yazen Ghannam wrote:
[...]
> > This is to catch the case where there was no break from the loop.
>
> If the CPU is possible != whether there was a apicid match.
>
> Here's how you do that and I'd let you figure out why yours doesn't
> always work:
>
I don't see why it won't work. If there is no break, then the iterator
ends by setting the variable past the last valid value.
For example, I ran this on a system with 512 CPUs:
unsigned int cpu;
/* Loops over CPUs 0-511. */
for_each_possible_cpu(cpu)
pr_info("loop: cpu=%d\n", cpu);
/* CPU is now set to 512. */
pr_info("final: cpu=%d\n", cpu);
/* CPU 512 is not possible. */
pr_info("CPU %d is %s possible\n", cpu, cpu_possible(cpu) ? "" : "not");
But...I like your suggestion as it is much more explicit. And I might be
missing something. :/
> diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
> index 0cbadfaf2400..3885fe05f01e 100644
> --- a/arch/x86/kernel/cpu/mce/apei.c
> +++ b/arch/x86/kernel/cpu/mce/apei.c
> @@ -66,6 +66,7 @@ EXPORT_SYMBOL_GPL(apei_mce_report_mem_error);
> int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
> {
> const u64 *i_mce = ((const u64 *) (ctx_info + 1));
> + bool apicid_found = false;
> unsigned int cpu;
> struct mce m;
>
> @@ -98,11 +99,13 @@ int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
> return -EINVAL;
>
> for_each_possible_cpu(cpu) {
> - if (cpu_data(cpu).topo.initial_apicid == lapic_id)
> + if (cpu_data(cpu).topo.initial_apicid == lapic_id) {
> + apicid_found = true;
> break;
> + }
> }
>
> - if (!cpu_possible(cpu))
> + if (!apicid_found)
> return -EINVAL;
>
> mce_prep_record_common(&m);
>
>
Would you like me to send another revision with this change? Do you have
any other comments?
Thanks,
Yazen
Powered by blists - more mailing lists