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

[Yazen Ghannam <yazen.ghannam@amd.com>]

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