Received-SPF: pass (google.com: domain of linux-kernel+bounces-65369-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <3f793ed7-b65c-48d0-a556-f51bdf4f5add@amd.com>
Date: Wed, 14 Feb 2024 09:56:14 -0500
User-Agent: Mozilla Thunderbird
Cc: yazen.ghannam@amd.com, 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
Content-Language: en-US
To: Borislav Petkov <bp@alien8.de>
References: <20240214033516.1344948-1-yazen.ghannam@amd.com>
 <20240214033516.1344948-3-yazen.ghannam@amd.com>
 <20240214110541.GFZcyehY44eSSYsW2l@fat_crate.local>
From: Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <20240214110541.GFZcyehY44eSSYsW2l@fat_crate.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?V0NsT05GVWN5Y3dVTGIvS0ZvV2xYTWM5WFFWUUN5Y0hVUzFsOUZEdnJjc3Nt?=
 =?utf-8?B?Ti9jMkYwT0d1dmNFZjhuSEhFQ2ZieUIxZDlWVEw0eXFENDBwUUhGcmpKN3hq?=
 =?utf-8?B?d2owTDlnSTNudTB6aGlYNjgvbUliUityQTkwWDIvcVc0OUV1RjFOWXBwTGR0?=
 =?utf-8?B?Sys0KzFQU1ZYMzJ0VW1SRkgrZG9BTXEyWHJuUndLdkV4UUtvRHVjanhGbDJl?=
 =?utf-8?B?K09reTZUbnl1eTgyRFBCbjVYY2g4WEF0ckxFV1lCQzhkc0JwTHZKT3R2QUlv?=
 =?utf-8?B?QWl2aStCWGx4dXVNUUlqVWFLKzhpOFdMaEpFV3VTQ084Rm1ySDVUZW4rS0Nu?=
 =?utf-8?B?cDJMNXAyb3dEN0xtbXMxeGMvL0l1WU42N0E4NHgwSDFyV21rdXV6QWc5OHdU?=
 =?utf-8?B?ODhxVUJjcGZHcGNjTFpVemVOUDJoVVY4UkFaNldBRlhjRnBIQzFkRllNTWw1?=
 =?utf-8?B?eEt5dEhTalY0TnJoaS9LcHB2OWRRcTFGa3NnT1lSMHV0QVFYN1dPWUtLY0g1?=
 =?utf-8?B?dE1nV3lRV1JMU1dBREc4OVVaZndaWmFWemgyUjF5Y2h2RHJQSXdSMzJSbUha?=
 =?utf-8?B?ZzJPd2NFVC9QRFdQalJRR3E4WGU5bm1mOHQ3MXFaYzc1YzBQVFJRdW4xeG9M?=
 =?utf-8?B?RjFJcE9YRVJ5V2hPblBsUDFpd1VQWjdRNm91MGFQQ0sxUjZWM2l2cXZucmtG?=
 =?utf-8?B?QnlEdktlYVJrU2dyRFUyYlduUUFqWHI1RVdaWVdJMWV1V3hyS3NjcWhNM01l?=
 =?utf-8?B?aFVVNGtNbllYb1NXWk5CQWdRdUlnU3g0Z28wdzhlcHRkUWZsYTBCUVZTZWow?=
 =?utf-8?B?blo1d3U5MHV0ZTlWU2Q5L1cwZG1ZUWpDRCt4WkYzSk96VkdzdDZBQjFETW9W?=
 =?utf-8?B?QXA1algyTldSUHVSMllQMHZMMUFNZHRtUzB4S0dTWkw2anVWSVVpY2s2ai8v?=
 =?utf-8?B?UTlaZGtVZFBSVnBQY05nUmtaNGUyOWdudEtLWlg1WFlLOWxTS05nUDBMODBK?=
 =?utf-8?B?MmNpcHNrT1gwbFpKZWwxY1YzRTRoakRNc1JOdFNuamd4ZGh4eTBBemNwT0hm?=
 =?utf-8?B?Q0ltQ0pFYkJSNGVSbDl5b3VQZDU2NUthTmNCSjN5TFRtN3dFN1gvTjNZK1M0?=
 =?utf-8?B?ZDlRVyttck1rL3hLK1p2OW5ocmxmTmtTSUc2WlJZSVhMaFZnUTNrNnpxQmxS?=
 =?utf-8?B?djJ4cHVvY0NsK1hxU29FZkpBa0RJcWhONTBHMys5K2pacXI5bGpIOHRDMzNu?=
 =?utf-8?B?KzJyazJHVnJZTWhLN1BIWFI4V1RoWU5pSml0em5kY0UwcnNZVjA3L3hyeEs3?=
 =?utf-8?B?N0FwUEEyUG1DNG8wdWVKekRUR2VFUnFhMUJldm96RGdleVE3b010aUVXUlR6?=
 =?utf-8?B?Y3RXTkFKRGtWM1lIRS9vem50QThUMDgxZ1hYQjBtdDkwaWdyRHhMRGd2Q1Iw?=
 =?utf-8?B?eVZOVjI4V1c3U1RVZ2YrTEh2WGo1N2NLallUY0VJazIxVyt6d1dNa01hTlZY?=
 =?utf-8?B?bWdha2hFR0JtRENGdzJLZlFVUVBHOEV2UERQNWFNMEFaWXhBZndLVVhtY0VD?=
 =?utf-8?B?LzA4TWFTVDRMME1KQVZZYmhMUmN5M0tIR3l2Y0YvWkR4Y24rZzFxMjZFWWZK?=
 =?utf-8?B?WEhJczdndDI5NjU5YVhKUmRUaExDRTFLMGxWcTY1N2xjSWdxSXp5N1dDSTk5?=
 =?utf-8?B?amJ1b0V1ZEozQUV5RDdLREIyS3EvOXVXYXgvUFFtWVp4TEw3cGhlaUhkSm1v?=
 =?utf-8?B?MC9QZGpsaDBSamlpb2xtQWthem1vNS84QVhmM1BYMUtBN0xuZ3VsaHpoN0hG?=
 =?utf-8?B?WDEwOHRRQ2Q3UGsvbkFrdFFFMkNvZmdCRU0yT2pvcUpLZ3d4S3FBUWFBMUIx?=
 =?utf-8?B?T0VXNzJMdG9Dam1XTUd1S3hzTU1VdVFQbXBHb0FhOTFnNmhsQUZCaVI1UUtD?=
 =?utf-8?B?bkRYdi9hUHE5RGx0V0lWaC9ZeldUY2JRNVlMR2p2d0lySTlKcThrbm9rTW9G?=
 =?utf-8?B?eXlJSlA4WXY2NmxlOVA5ZVI2amZnek5WZFJSOGJWaVNLdEtQT3FwWmp4RllB?=
 =?utf-8?B?enlHUzA1d2g2cmUrMHU1bGRCRVB6SWxyN3pveVZpTHZJSjRXVjYxa2ZyQ0FW?=
 =?utf-8?Q?+oxsf8N7AQdYHi3POcM/7gbhY?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 904cfc52-24fd-47e3-160d-08dc2d6d1835
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2024 14:56:16.2561
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2zt3jtkUWdnwNMxWpy9TpY4sp8sCmuDV6DNc9weJ6o1k52SZdR6/O6DGXiBwm5jqZ3ZOS8M9KdYOTs/118c0tQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4924

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