[PATCH 0/3] Cleanup mce_notify_irq usage

[PATCH 0/3] Cleanup mce_notify_irq usage

[Nikolay Borisov]

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

[PATCH 1/3] x86/mce/inject: Remova call to mce_notify_irq()

[Nikolay Borisov]

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 7fb5556a0b53..e87676d86a85 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1793,7 +1793,6 @@ int mce_notify_irq(void)
 	}
 	return 0;
 }
-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.0

[PATCH 2/3] x86/mce: Make mce_notify_irq() static

[Nikolay Borisov]

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 | 42 ++++++++++++++++++----------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 4543cf2eb5e8..a665cd464889 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);
 
-int 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 e87676d86a85..2544e9ae7449 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,26 +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.
- */
-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 void __mcheck_cpu_mce_banks_init(void)
 {
-- 
2.43.0

[PATCH 3/3] x86/mce: Make mce_notify_irq() depend on CONFIG_X86_MCELOG_LEGACY

[Nikolay Borisov]

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>
---

In addition to making the body of this function depend on MCELOG, mce_need_notify
also depends entirely on MCELOG, however I haven't added it since I don't want to
proliferate ifdefs, this begs the qustion whether this patch is acceptable ?

 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 2544e9ae7449..4e2f6c162b43 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

RE: [PATCH 0/3] Cleanup mce_notify_irq usage

[Zhuo, 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

Re: [PATCH 0/3] Cleanup mce_notify_irq usage

[Nikolay Borisov]

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

[RESEND PATCH 0/3] Make mce_notify_irq() static

[Nikolay Borisov]

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

[RESEND PATCH 1/3] x86/mce/inject: Remova call to mce_notify_irq()

[Nikolay Borisov]

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.0

[RESEND PATCH 2/3] x86/mce: Make mce_notify_irq() static

[Nikolay Borisov]

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

[RESEND PATCH 3/3] x86/mce: Make mce_notify_irq() depend on CONFIG_X86_MCELOG_LEGACY

[Nikolay Borisov]

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

RE: [RESEND PATCH 2/3] x86/mce: Make mce_notify_irq() static

[Zhuo, Qiuxu]

> 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? 

Re: [RESEND PATCH 2/3] x86/mce: Make mce_notify_irq() static

[Nikolay Borisov]

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.

RE: [RESEND PATCH 3/3] x86/mce: Make mce_notify_irq() depend on CONFIG_X86_MCELOG_LEGACY

[Zhuo, Qiuxu]

> 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
> 

Re: [RESEND PATCH 3/3] x86/mce: Make mce_notify_irq() depend on CONFIG_X86_MCELOG_LEGACY

[Nikolay Borisov]

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
>>
> 

Re: [RESEND PATCH 1/3] x86/mce/inject: Remova call to mce_notify_irq()

[Yazen Ghannam]

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,
Yazen

Re: [RESEND PATCH 1/3] x86/mce/inject: Remova call to mce_notify_irq()

[Nikolay Borisov]

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

RE: [RESEND PATCH 3/3] x86/mce: Make mce_notify_irq() depend on CONFIG_X86_MCELOG_LEGACY

[Zhuo, Qiuxu]

> 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