arch/x86/include/asm/mce.h | 2 -- arch/x86/kernel/cpu/mce/core.c | 44 +++++++++++++++++--------------- arch/x86/kernel/cpu/mce/inject.c | 1 - 3 files changed, 23 insertions(+), 24 deletions(-)
Here are 3 small patches that result in mce_notify_irq becoming private to core.c and unexporting it. I have one pending question that I've put under the scissor line in Patch 3. But overall they are pretty self-explanatory and obvious. Nikolay Borisov (3): x86/mce/inject: Remova call to mce_notify_irq() x86/mce: Make mce_notify_irq() static x86/mce: Make mce_notify_irq() depend on CONFIG_X86_MCELOG_LEGACY arch/x86/include/asm/mce.h | 2 -- arch/x86/kernel/cpu/mce/core.c | 44 +++++++++++++++++--------------- arch/x86/kernel/cpu/mce/inject.c | 1 - 3 files changed, 23 insertions(+), 24 deletions(-) -- 2.43.0
Patches now rebased on core/ras Nikolay Borisov (3): x86/mce/inject: Remova call to mce_notify_irq() x86/mce: Make mce_notify_irq() static x86/mce: Make mce_notify_irq() depend on CONFIG_X86_MCELOG_LEGACY arch/x86/include/asm/mce.h | 2 -- arch/x86/kernel/cpu/mce/core.c | 45 ++++++++++++++++---------------- arch/x86/kernel/cpu/mce/inject.c | 1 - 3 files changed, 23 insertions(+), 25 deletions(-) -- 2.43.0
The call is actually a noop because when the MCE is raised the early
notifier is the only call site that correctly calls mce_notify_irq()
because it also sets mce_need_notify. So let's just remove this call,
which allows to unexport mce_notify_irq.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
arch/x86/kernel/cpu/mce/core.c | 1 -
arch/x86/kernel/cpu/mce/inject.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 0dc00c9894c7..23e5e7f7c554 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1793,7 +1793,6 @@ bool mce_notify_irq(void)
}
return false;
}
-EXPORT_SYMBOL_GPL(mce_notify_irq);
static void __mcheck_cpu_mce_banks_init(void)
{
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 313fe682db33..06e3cf7229ce 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -229,7 +229,6 @@ static int raise_local(void)
} else if (m->status) {
pr_info("Starting machine check poll CPU %d\n", cpu);
raise_poll(m);
- mce_notify_irq();
pr_info("Machine check poll done on CPU %d\n", cpu);
} else
m->finished = 0;
--
2.43.0On Wed, Jan 15, 2025 at 09:36:38AM +0200, Nikolay Borisov wrote:
Hi Nikolay,
There is a typo in the $SUBJECT.
> The call is actually a noop because when the MCE is raised the early
> notifier is the only call site that correctly calls mce_notify_irq()
> because it also sets mce_need_notify. So let's just remove this call,
> which allows to unexport mce_notify_irq.
>
The commit message should be in passive and imperative voice.
"So let's just remove this..." -> "Remove this..."
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> ---
> arch/x86/kernel/cpu/mce/core.c | 1 -
> arch/x86/kernel/cpu/mce/inject.c | 1 -
> 2 files changed, 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 0dc00c9894c7..23e5e7f7c554 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1793,7 +1793,6 @@ bool mce_notify_irq(void)
> }
> return false;
> }
> -EXPORT_SYMBOL_GPL(mce_notify_irq);
>
> static void __mcheck_cpu_mce_banks_init(void)
> {
> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
> index 313fe682db33..06e3cf7229ce 100644
> --- a/arch/x86/kernel/cpu/mce/inject.c
> +++ b/arch/x86/kernel/cpu/mce/inject.c
> @@ -229,7 +229,6 @@ static int raise_local(void)
> } else if (m->status) {
> pr_info("Starting machine check poll CPU %d\n", cpu);
> raise_poll(m);
> - mce_notify_irq();
With this change, there are no users of mce_notify_irq() outside of
mce/core.c. So you could go further and make the function static to
core.c.
In other words, you could squash the second patch into this one.
Thanks,
Yazen
On 22.01.25 г. 20:24 ч., Yazen Ghannam wrote:
> On Wed, Jan 15, 2025 at 09:36:38AM +0200, Nikolay Borisov wrote:
>
> Hi Nikolay,
>
> There is a typo in the $SUBJECT.
>
>> The call is actually a noop because when the MCE is raised the early
>> notifier is the only call site that correctly calls mce_notify_irq()
>> because it also sets mce_need_notify. So let's just remove this call,
>> which allows to unexport mce_notify_irq.
>>
>
> The commit message should be in passive and imperative voice.
>
> "So let's just remove this..." -> "Remove this..."
>
>> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
>> ---
>> arch/x86/kernel/cpu/mce/core.c | 1 -
>> arch/x86/kernel/cpu/mce/inject.c | 1 -
>> 2 files changed, 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
>> index 0dc00c9894c7..23e5e7f7c554 100644
>> --- a/arch/x86/kernel/cpu/mce/core.c
>> +++ b/arch/x86/kernel/cpu/mce/core.c
>> @@ -1793,7 +1793,6 @@ bool mce_notify_irq(void)
>> }
>> return false;
>> }
>> -EXPORT_SYMBOL_GPL(mce_notify_irq);
>>
>> static void __mcheck_cpu_mce_banks_init(void)
>> {
>> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
>> index 313fe682db33..06e3cf7229ce 100644
>> --- a/arch/x86/kernel/cpu/mce/inject.c
>> +++ b/arch/x86/kernel/cpu/mce/inject.c
>> @@ -229,7 +229,6 @@ static int raise_local(void)
>> } else if (m->status) {
>> pr_info("Starting machine check poll CPU %d\n", cpu);
>> raise_poll(m);
>> - mce_notify_irq();
>
> With this change, there are no users of mce_notify_irq() outside of
> mce/core.c. So you could go further and make the function static to
> core.c.
>
> In other words, you could squash the second patch into this one.
Thanks, I've incorporated those in V2 that I sent.
>
> Thanks,
> Yazen
It's no longer used outside of core.c so let's make it static. No
functional changes.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
arch/x86/include/asm/mce.h | 2 --
arch/x86/kernel/cpu/mce/core.c | 43 +++++++++++++++++-----------------
2 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index eb2db07ef39c..6c77c03139f7 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -296,8 +296,6 @@ enum mcp_flags {
void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
-bool mce_notify_irq(void);
-
DECLARE_PER_CPU(struct mce, injectm);
/* Disable CMCI/polling for MCA bank claimed by firmware */
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 23e5e7f7c554..89625ff79c3b 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -584,6 +584,28 @@ bool mce_is_correctable(struct mce *m)
}
EXPORT_SYMBOL_GPL(mce_is_correctable);
+/*
+ * Notify the user(s) about new machine check events.
+ * Can be called from interrupt context, but not from machine check/NMI
+ * context.
+ */
+static int mce_notify_irq(void)
+{
+ /* Not more than two messages every minute */
+ static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
+
+ if (test_and_clear_bit(0, &mce_need_notify)) {
+ mce_work_trigger();
+
+ if (__ratelimit(&ratelimit))
+ pr_info(HW_ERR "Machine check events logged\n");
+
+ return 1;
+ }
+
+ return 0;
+}
+
static int mce_early_notifier(struct notifier_block *nb, unsigned long val,
void *data)
{
@@ -1773,27 +1795,6 @@ static void mce_timer_delete_all(void)
del_timer_sync(&per_cpu(mce_timer, cpu));
}
-/*
- * Notify the user(s) about new machine check events.
- * Can be called from interrupt context, but not from machine check/NMI
- * context.
- */
-bool mce_notify_irq(void)
-{
- /* Not more than two messages every minute */
- static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
-
- if (test_and_clear_bit(0, &mce_need_notify)) {
- mce_work_trigger();
-
- if (__ratelimit(&ratelimit))
- pr_info(HW_ERR "Machine check events logged\n");
-
- return true;
- }
- return false;
-}
-
static void __mcheck_cpu_mce_banks_init(void)
{
struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
--
2.43.0> From: Nikolay Borisov <nik.borisov@suse.com>
> [...]
> Subject: [RESEND PATCH 2/3] x86/mce: Make mce_notify_irq() static
>
> It's no longer used outside of core.c so let's make it static. No functional
> changes.
>
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> ---
> arch/x86/include/asm/mce.h | 2 --
> arch/x86/kernel/cpu/mce/core.c | 43 +++++++++++++++++-----------------
> 2 files changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index
> eb2db07ef39c..6c77c03139f7 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -296,8 +296,6 @@ enum mcp_flags {
>
> void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
>
> -bool mce_notify_irq(void);
> -
> DECLARE_PER_CPU(struct mce, injectm);
>
> /* Disable CMCI/polling for MCA bank claimed by firmware */ diff --git
> a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index
> 23e5e7f7c554..89625ff79c3b 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -584,6 +584,28 @@ bool mce_is_correctable(struct mce *m) }
> EXPORT_SYMBOL_GPL(mce_is_correctable);
>
> +/*
> + * Notify the user(s) about new machine check events.
> + * Can be called from interrupt context, but not from machine check/NMI
> + * context.
> + */
> +static int mce_notify_irq(void)
> +{
> + /* Not more than two messages every minute */
> + static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
> +
> + if (test_and_clear_bit(0, &mce_need_notify)) {
> + mce_work_trigger();
> +
> + if (__ratelimit(&ratelimit))
> + pr_info(HW_ERR "Machine check events logged\n");
> +
> + return 1;
> + }
> +
> + return 0;
> +}
> +
Did you mistakenly change the return value type to int?
On 15.01.25 г. 15:37 ч., Zhuo, Qiuxu wrote:
>> From: Nikolay Borisov <nik.borisov@suse.com>
>> [...]
>> Subject: [RESEND PATCH 2/3] x86/mce: Make mce_notify_irq() static
>>
>> It's no longer used outside of core.c so let's make it static. No functional
>> changes.
>>
>> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
>> ---
>> arch/x86/include/asm/mce.h | 2 --
>> arch/x86/kernel/cpu/mce/core.c | 43 +++++++++++++++++-----------------
>> 2 files changed, 22 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index
>> eb2db07ef39c..6c77c03139f7 100644
>> --- a/arch/x86/include/asm/mce.h
>> +++ b/arch/x86/include/asm/mce.h
>> @@ -296,8 +296,6 @@ enum mcp_flags {
>>
>> void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
>>
>> -bool mce_notify_irq(void);
>> -
>> DECLARE_PER_CPU(struct mce, injectm);
>>
>> /* Disable CMCI/polling for MCA bank claimed by firmware */ diff --git
>> a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index
>> 23e5e7f7c554..89625ff79c3b 100644
>> --- a/arch/x86/kernel/cpu/mce/core.c
>> +++ b/arch/x86/kernel/cpu/mce/core.c
>> @@ -584,6 +584,28 @@ bool mce_is_correctable(struct mce *m) }
>> EXPORT_SYMBOL_GPL(mce_is_correctable);
>>
>> +/*
>> + * Notify the user(s) about new machine check events.
>> + * Can be called from interrupt context, but not from machine check/NMI
>> + * context.
>> + */
>> +static int mce_notify_irq(void)
>> +{
>> + /* Not more than two messages every minute */
>> + static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
>> +
>> + if (test_and_clear_bit(0, &mce_need_notify)) {
>> + mce_work_trigger();
>> +
>> + if (__ratelimit(&ratelimit))
>> + pr_info(HW_ERR "Machine check events logged\n");
>> +
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> Did you mistakenly change the return value type to int?
It was int on master, where this patch originated from. Whereas on
core/ras there's c845cb8dbd2e1a804babfd13648026c3a7cfbc0b which changes
the function to bool. So I guess it's a rebase artifact, will fix it on
next submission, but I will wait for more feedback.
mce_notify_irq() really depends on the legacy mcelog being enabled as
otherwise mce_work_trigger() will never schedule the trigger work as
mce_helper can't be set unless CONFIG_X86_MCELOG_LEGACY is defined.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
arch/x86/kernel/cpu/mce/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 89625ff79c3b..b21aa1494da0 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -591,6 +591,7 @@ EXPORT_SYMBOL_GPL(mce_is_correctable);
*/
static int mce_notify_irq(void)
{
+#ifdef CONFIG_X86_MCELOG_LEGACY
/* Not more than two messages every minute */
static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
@@ -602,7 +603,7 @@ static int mce_notify_irq(void)
return 1;
}
-
+#endif
return 0;
}
--
2.43.0> From: Nikolay Borisov <nik.borisov@suse.com>
> Sent: Wednesday, January 15, 2025 3:37 PM
> To: linux-edac@vger.kernel.org
> Cc: x86@kernel.org; linux-kernel@vger.kernel.org; bp@alien8.de; Nikolay
> Borisov <nik.borisov@suse.com>
> Subject: [RESEND PATCH 3/3] x86/mce: Make mce_notify_irq() depend on
> CONFIG_X86_MCELOG_LEGACY
>
> mce_notify_irq() really depends on the legacy mcelog being enabled as
> otherwise mce_work_trigger() will never schedule the trigger work as
> mce_helper can't be set unless CONFIG_X86_MCELOG_LEGACY is defined.
>
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> ---
> arch/x86/kernel/cpu/mce/core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 89625ff79c3b..b21aa1494da0 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -591,6 +591,7 @@ EXPORT_SYMBOL_GPL(mce_is_correctable);
> */
> static int mce_notify_irq(void)
> {
> +#ifdef CONFIG_X86_MCELOG_LEGACY
> /* Not more than two messages every minute */
> static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
>
> @@ -602,7 +603,7 @@ static int mce_notify_irq(void)
>
The message printed inside this function should not depend on
CONFIG_X86_MCELOG_LEGACY. User-space tools/scripts might look for this
message to detect machine events. It is also useful for debugging purposes.
if (__ratelimit(&ratelimit))
pr_info(HW_ERR "Machine check events logged\n");
> return 1;
> }
> -
> +#endif
> return 0;
> }
>
> --
> 2.43.0
>
On 15.01.25 г. 15:45 ч., Zhuo, Qiuxu wrote:
>> From: Nikolay Borisov <nik.borisov@suse.com>
>> Sent: Wednesday, January 15, 2025 3:37 PM
>> To: linux-edac@vger.kernel.org
>> Cc: x86@kernel.org; linux-kernel@vger.kernel.org; bp@alien8.de; Nikolay
>> Borisov <nik.borisov@suse.com>
>> Subject: [RESEND PATCH 3/3] x86/mce: Make mce_notify_irq() depend on
>> CONFIG_X86_MCELOG_LEGACY
>>
>> mce_notify_irq() really depends on the legacy mcelog being enabled as
>> otherwise mce_work_trigger() will never schedule the trigger work as
>> mce_helper can't be set unless CONFIG_X86_MCELOG_LEGACY is defined.
>>
>> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
>> ---
>> arch/x86/kernel/cpu/mce/core.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
>> index 89625ff79c3b..b21aa1494da0 100644
>> --- a/arch/x86/kernel/cpu/mce/core.c
>> +++ b/arch/x86/kernel/cpu/mce/core.c
>> @@ -591,6 +591,7 @@ EXPORT_SYMBOL_GPL(mce_is_correctable);
>> */
>> static int mce_notify_irq(void)
>> {
>> +#ifdef CONFIG_X86_MCELOG_LEGACY
>> /* Not more than two messages every minute */
>> static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
>>
>> @@ -602,7 +603,7 @@ static int mce_notify_irq(void)
>>
>
> The message printed inside this function should not depend on
> CONFIG_X86_MCELOG_LEGACY. User-space tools/scripts might look for this
> message to detect machine events. It is also useful for debugging purposes.
The thing is if MCELOG_LEGACY is turned off then mce_work_trigger is a
noop, hence nothing is really logged which makes this message somewhat
bogus. After all the early handler's job is to log to userspace, if we
don't log anything no need to spam the kernel log.
>
> if (__ratelimit(&ratelimit))
> pr_info(HW_ERR "Machine check events logged\n");
>
>> return 1;
>> }
>> -
>> +#endif
>> return 0;
>> }
>>
>> --
>> 2.43.0
>>
>
> From: Nikolay Borisov <nik.borisov@suse.com>
> [...]
> >> --- a/arch/x86/kernel/cpu/mce/core.c
> >> +++ b/arch/x86/kernel/cpu/mce/core.c
> >> @@ -591,6 +591,7 @@ EXPORT_SYMBOL_GPL(mce_is_correctable);
> >> */
> >> static int mce_notify_irq(void)
> >> {
> >> +#ifdef CONFIG_X86_MCELOG_LEGACY
> >> /* Not more than two messages every minute */
> >> static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
> >>
> >> @@ -602,7 +603,7 @@ static int mce_notify_irq(void)
> >>
> >
> > The message printed inside this function should not depend on
> > CONFIG_X86_MCELOG_LEGACY. User-space tools/scripts might look for
> > this message to detect machine events. It is also useful for debugging
> purposes.
>
> The thing is if MCELOG_LEGACY is turned off then mce_work_trigger is a
> noop, hence nothing is really logged which makes this message somewhat
> bogus. After all the early handler's job is to log to userspace, if we don't log
> anything no need to spam the kernel log.
Currently, some customers have reported that the Intel EDAC driver didn't
report errors on some memory DIMMs. The print message here helped
me confirm whether the MCE event originated from the x86/mce code or
if the MCE event was lost somewhere in the EDAC driver.
IMHO, it would be better to keep this print message here, or update it a bit like below
if !CONFIG_X86_MCELOG_LEGACY:
pr_info(HW_ERR "Machine check events generated\n");
Thanks!
-Qiuxu
> From: Nikolay Borisov <nik.borisov@suse.com> > Sent: Wednesday, January 15, 2025 12:37 AM > To: linux-edac@vger.kernel.org > Cc: x86@kernel.org; linux-kernel@vger.kernel.org; bp@alien8.de; Nikolay > Borisov <nik.borisov@suse.com> > Subject: [PATCH 0/3] Cleanup mce_notify_irq usage I tried to apply this series on top of the ras/core branch of the TIP tree, but there were some conflicts. Would it be better to rebase this series on top of the ras/core branch of the TIP tree? https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git ras/core
On 15.01.25 г. 8:37 ч., Zhuo, Qiuxu wrote: >> From: Nikolay Borisov <nik.borisov@suse.com> >> Sent: Wednesday, January 15, 2025 12:37 AM >> To: linux-edac@vger.kernel.org >> Cc: x86@kernel.org; linux-kernel@vger.kernel.org; bp@alien8.de; Nikolay >> Borisov <nik.borisov@suse.com> >> Subject: [PATCH 0/3] Cleanup mce_notify_irq usage > > I tried to apply this series on top of the ras/core branch of the TIP tree, but there were some conflicts. > Would it be better to rebase this series on top of the ras/core branch of the TIP tree? > > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git ras/core Sure, will do. It's based off master curently
© 2016 - 2025 Red Hat, Inc.