Messages in this thread | | | Date | Wed, 14 Feb 2024 09:56:14 -0500 | Subject | Re: [PATCH 2/2] RAS: Introduce the FRU Memory Poison Manager | From | Yazen Ghannam <> |
| |
On 2/14/2024 6:05 AM, Borislav Petkov wrote: > On Tue, Feb 13, 2024 at 09:35:16PM -0600, Yazen Ghannam wrote: >> +static inline struct cper_fru_poison_desc *get_fpd(struct fru_rec *rec, u32 entry) >> +{ >> + return &rec->entries[entry]; >> +} > > This one needs to go too. >
Ack.
>> +static inline u32 get_fmp_len(struct fru_rec *rec) >> +{ >> + return rec->sec_desc.section_length - sizeof(struct cper_section_descriptor); >> +} > > Oh well, I guess we can keep that one. >
Okay.
>> +/* Calculate a new checksum. */ >> +static u32 get_fmp_checksum(struct fru_rec *rec) >> +{ >> + struct cper_sec_fru_mem_poison *fmp = get_fmp(rec); >> + u32 len, checksum; >> + >> + len = get_fmp_len(rec); >> + >> + /* Get the current total. */ >> + checksum = do_fmp_checksum(fmp, len); >> + >> + /* Subtract the current checksum from total. */ >> + checksum -= fmp->checksum; >> + >> + /* Return the compliment value. */ >> + return 0 - checksum; >> +} > > Let's get rid of that one. > > Also, I think it is called *complement* value and you simply do >
Yep, good catch! (compliment)
> /* 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.
> --- > > diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c > index 9eaf892e35b9..f8799beddcc4 100644 > --- a/drivers/ras/amd/fmpm.c > +++ b/drivers/ras/amd/fmpm.c > @@ -195,11 +195,12 @@ static u32 do_fmp_checksum(struct cper_sec_fru_mem_poison *fmp, u32 len) > return 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.
> +static int update_record_on_storage(struct fru_rec *rec) > { > u32 len, checksum; > + int ret; > > + /* Calculate a new checksum. */ > len = get_fmp_len(rec); > > /* Get the current total. */ > @@ -208,15 +209,8 @@ static u32 get_fmp_checksum(struct fru_rec *rec) > /* Subtract the current checksum from total. */ > checksum -= rec->fmp.checksum; > > - /* Return the compliment value. */ > - return 0 - checksum; > -} > - > -static int update_record_on_storage(struct fru_rec *rec) > -{ > - int ret; > - > - rec->fmp.checksum = get_fmp_checksum(rec); > + /* Use the complement value. */ > + rec->fmp.checksum = -checksum; > > pr_debug("Writing to storage"); > >
Thanks, Yazen
| |