Re: [PATCH 2/2] RAS: Introduce the FRU Memory Poison Manager

[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