Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 13:46:24 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Yazen Ghannam <yazen.ghannam@amd.com>
Cc:     linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        tony.luck@intel.com, x86@kernel.org,
        Smita.KoralahalliChannabasappa@amd.com
Subject: Re: [PATCH] x86/MCE/AMD: Clear DFR errors found in THR handler
Message-ID: <Y1pvkIJ/Uipxolqy@zn.tnic>
References: <20220621155943.33623-1-yazen.ghannam@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220621155943.33623-1-yazen.ghannam@amd.com>
Precedence: bulk

On Tue, Jun 21, 2022 at 03:59:43PM +0000, Yazen Ghannam wrote:
> AMD's MCA Thresholding feature counts errors of all severites not just
> correctable errors. If a deferred error causes the threshold limit to be
> reached (it was the error that caused the overflow), then both a
> deferred error interrupt and a thresholding interrupt will be triggered.
> 
> The order of the interrupts is not guaranteed. If the threshold
> interrupt handler is executed first, then it will clear MCA_STATUS for
> the error. It will not check or clear MCA_DESTAT which also holds a copy
> of the deferred error. When the deferred error interrupt handler runs it
> will not find an error in MCA_STATUS, but it will find the error in
> MCA_DESTAT. This will cause two errors to be logged.
> 
> Check for deferred errors when handling a threshold interrupt. If a bank
> contains a deferred error, then clear the bank's MCA_DESTAT register.
> 
> Define a new helper function to do the deferred error check and clearing
> of MCA_DESTAT.
> 
> Fixes: 37d43acfd79f ("x86/mce/AMD: Redo error logging from APIC LVT interrupt handlers")
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/kernel/cpu/mce/amd.c | 37 +++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index 1c87501e0fa3..ab1145cf8328 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -788,6 +788,28 @@ _log_error_bank(unsigned int bank, u32 msr_stat, u32 msr_addr, u64 misc)
>  	return status & MCI_STATUS_DEFERRED;
>  }
>  
> +static bool _log_error_deferred(unsigned int bank, u32 misc)
> +{
> +	bool defrd;
> +
> +	defrd = _log_error_bank(bank, mca_msr_reg(bank, MCA_STATUS),
> +				mca_msr_reg(bank, MCA_ADDR), misc);
> +
> +	if (!defrd)
> +		return false;

I've zapped that defrd variable:

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index ab1145cf8328..6ae7edea3270 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -790,12 +790,8 @@ _log_error_bank(unsigned int bank, u32 msr_stat, u32 msr_addr, u64 misc)
 
 static bool _log_error_deferred(unsigned int bank, u32 misc)
 {
-	bool defrd;
-
-	defrd = _log_error_bank(bank, mca_msr_reg(bank, MCA_STATUS),
-				mca_msr_reg(bank, MCA_ADDR), misc);
-
-	if (!defrd)
+	if (!_log_error_bank(bank, mca_msr_reg(bank, MCA_STATUS),
+			     mca_msr_reg(bank, MCA_ADDR), misc))
 		return false;
 
 	/*

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette