Received-SPF: pass (google.com: domain of linux-kernel+bounces-65826-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 14 Feb 2024 19:44:23 +0100
From: Borislav Petkov <bp@alien8.de>
To: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: tony.luck@intel.com, linux-edac@vger.kernel.org,
	linux-kernel@vger.kernel.org, avadhut.naik@amd.com,
	john.allen@amd.com, muralidhara.mk@amd.com,
	naveenkrishna.chatradhi@amd.com, sathyapriya.k@amd.com
Subject: Re: [PATCH 2/2] RAS: Introduce the FRU Memory Poison Manager
Message-ID: <20240214184423.GNZc0KB_8va7gUyk_m@fat_crate.local>
References: <20240214033516.1344948-1-yazen.ghannam@amd.com>
 <20240214033516.1344948-3-yazen.ghannam@amd.com>
 <20240214110541.GFZcyehY44eSSYsW2l@fat_crate.local>
 <3f793ed7-b65c-48d0-a556-f51bdf4f5add@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <3f793ed7-b65c-48d0-a556-f51bdf4f5add@amd.com>

On Wed, Feb 14, 2024 at 09:56:14AM -0500, Yazen Ghannam wrote:
> > This one needs to go too.
> > 
> 
> Ack.

Gone:

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index a67a4b67cf9d..643c36b6dc9c 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -146,11 +146,6 @@ static DEFINE_MUTEX(fmpm_update_mutex);
 #define for_each_fru(i, rec) \
 	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
 
-static inline struct cper_fru_poison_desc *get_fpd(struct fru_rec *rec, u32 entry)
-{
-	return &rec->entries[entry];
-}
-
 static inline u32 get_fmp_len(struct fru_rec *rec)
 {
 	return rec->sec_desc.section_length - sizeof(struct cper_section_descriptor);
@@ -253,7 +248,9 @@ static bool rec_has_fpd(struct fru_rec *rec, struct cper_fru_poison_desc *fpd)
 	unsigned int i;
 
 	for (i = 0; i < rec->fmp.nr_entries; i++) {
-		if (same_fpd(get_fpd(rec, i), fpd)) {
+		struct cper_fru_poison_desc *fpd_i = &rec->entries[i];
+
+		if (same_fpd(fpd_i, fpd)) {
 			pr_debug("Found duplicate record");
 			return true;
 		}
@@ -265,7 +262,7 @@ static bool rec_has_fpd(struct fru_rec *rec, struct cper_fru_poison_desc *fpd)
 static void update_fru_record(struct fru_rec *rec, struct mce *m)
 {
 	struct cper_sec_fru_mem_poison *fmp = &rec->fmp;
-	struct cper_fru_poison_desc fpd;
+	struct cper_fru_poison_desc fpd, *fpd_dest;
 	u32 entry = 0;
 
 	mutex_lock(&fmpm_update_mutex);
@@ -287,9 +284,10 @@ static void update_fru_record(struct fru_rec *rec, struct mce *m)
 		goto out_unlock;
 	}
 
-	entry = fmp->nr_entries;
+	entry	  = fmp->nr_entries;
+	fpd_dest  = &rec->entries[entry];
 
-	memcpy(get_fpd(rec, entry), &fpd, sizeof(struct cper_fru_poison_desc));
+	memcpy(fpd_dest, &fpd, sizeof(struct cper_fru_poison_desc));
 
 	fmp->nr_entries		 = entry + 1;
 	fmp->validation_bits	|= FMP_VALID_LIST_ENTRIES;
@@ -359,11 +357,10 @@ static u32 get_cpu_from_fru_id(u64 fru_id)
 
 static void retire_mem_fmp(struct fru_rec *rec, u32 nr_entries, u32 cpu)
 {
-	struct cper_fru_poison_desc *fpd;
 	unsigned int i;
 
 	for (i = 0; i < nr_entries; i++) {
-		fpd = get_fpd(rec, i);
+		struct cper_fru_poison_desc *fpd = &rec->entries[i];
 
 		if (fpd->hw_id_type != FPD_HW_ID_TYPE_MCA_IPID)
 			continue;


> >          /* Use the complement value. */
> >          rec->fmp.checksum = -checksum;
> > 
> > I'd say.
> > 
> 
> This was my first thought. Other checksum code in the kernel does
> the (0-X) thing. So I wasn't sure if there's any odd side effects
> of one over the other. And I didn't take the time to dig into it.

I guess to probably be more expressive? I don't see how

0 - X

and 

-X

differ.

And you can always do a before-after and look at the asm:

before:
# drivers/ras/amd/fmpm.c:202:   rec->fmp.checksum = 0 - checksum;
#NO_APP
        subl    %edx, %eax      # checksum, tmp100
        movl    %eax, 200(%rbx) # tmp100, rec_9(D)->fmp.checksum

after:
# drivers/ras/amd/fmpm.c:202:   rec->fmp.checksum = -checksum;
#NO_APP
        subl    %edx, %eax      # checksum, tmp100
        movl    %eax, 200(%rbx) # tmp100, rec_9(D)->fmp.checksum


> > -/* Calculate a new checksum. */
> > -static u32 get_fmp_checksum(struct fru_rec *rec)
> 
> I made this a helper because we need to validate the checksum when
> reading records from storage too.

It has a single user that's why I whacked it. If a new one materializes,
sure, you can carve it out.

Thx.

-- 
Regards/Gruss,
    Boris.

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