Received-SPF: pass (google.com: domain of linux-kernel+bounces-208773-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Subject: Re: [PATCH 2/4] acpi/ghes, efi/cper: Recognize and process CXL
 Protocol Errors.
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: Alison Schofield <alison.schofield@intel.com>, linux-efi@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org,
 Ard Biesheuvel <ardb@kernel.org>, Vishal Verma <vishal.l.verma@intel.com>,
 Ira Weiny <ira.weiny@intel.com>, Dan Williams <dan.j.williams@intel.com>,
 Yazen Ghannam <yazen.ghannam@amd.com>, Bowman Terry <terry.bowman@amd.com>
References: <20240522150839.27578-1-Smita.KoralahalliChannabasappa@amd.com>
 <20240522150839.27578-3-Smita.KoralahalliChannabasappa@amd.com>
 <Zk6H5jTdVfN4aFhF@aschofie-mobl2>
 <09e0d961-e19f-30d6-5306-1b35609b7d79@amd.com>
 <20240607162622.00000819@Huawei.com>
From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Message-ID: <7fe041ed-9ee3-7cbe-ddce-fb727fba12c7@amd.com>
Date: Mon, 10 Jun 2024 12:07:48 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
In-Reply-To: <20240607162622.00000819@Huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
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?aUtUYXRYbkw1ZEI1bVB3TGVFTGZtTzNQZ09idnNkSmRiTUE0bGhnTStCcWRD?=
 =?utf-8?B?MDVkcUFtVHd4WllwZWlYNEFjN3FsbFFOdGdSNFVFNXRidnpIa2tLVEhUZHZR?=
 =?utf-8?B?UTBoWmN3YmVMZDVLYnRxdC91bEFDVG11ZnNaQ2Y0NlN1ci9qenhSTGpxOWcy?=
 =?utf-8?B?UklucUoyb2VwdWtCSnNodkdFSDVsTG9TOXNhNkc0MlBaUWh4alBoUlM5STBh?=
 =?utf-8?B?bmNrN1V6NjhmUmE1dVQwRDgwamM4R0F4T1BzN1EyNmhTYXNnVkJucU02VTVr?=
 =?utf-8?B?bVIvOGJTSEpHeHFTY0NpVVV3Z2FleUlLK2xOZGVEYm5DZGYwRUFDa1czRThh?=
 =?utf-8?B?TXU1WGxFQWlaOWprVnVqSEVORFBlNWJEQkdmSFYzOXphRjRTL2trM2JKbnor?=
 =?utf-8?B?VDBwU1JhN0lNMDhxOTBzNi9oNDd5S0RBSUtlbkFiVmp6MmtEZGd2YisycnF0?=
 =?utf-8?B?QmpWTGZEVHZUV0wvaTM4WDIvcjRUTlU0Q3BweTYyb3E5MlRIdFdLWXNmeVJu?=
 =?utf-8?B?cHAvbVhjS0x2cmpvVWxISVN5NXZkWGQzZ3p0cmU2R1Q0VG1JZlFuYmQ2ZXlR?=
 =?utf-8?B?OWRiWXVCdmVCSGR2VW5vc0hsUHp6ci9OYlhyNVVDdXRObG10OHc2dUoyNW9O?=
 =?utf-8?B?bmRkaGtVQnp1NVZJczJ3OFRQdGh4UFhyTFp3TGFjUUdiMVh2MHJ5OVRZVGdm?=
 =?utf-8?B?L3pCR0RCbVFyK0RwYjlNR2VJTHZYOThjVnBDNTVLOW9rTG5tMzJDNnYrZVFk?=
 =?utf-8?B?aWhXR1NnWlFNQVBhNktHMTVHWTBFWm1IVzBHMVRldHNFMEV5MWpsZnlsVkw1?=
 =?utf-8?B?a0tEVU9QWHZBQ1N2OGJEL1c5NmgxTDJNQ1M5K0VaR05FbHZ5c3ZSRTBTR1lv?=
 =?utf-8?B?M2pJejEzRGhWVFVlTEp2WmZVd0k4d2I3NHUvWmt0MGRYakV0MDBzV1RXSjBo?=
 =?utf-8?B?dW1DTEo5dGIvbXo3eUV0N0NDdWdmblFuTk9UUlZldTJWQktEaUVla2tJTWVN?=
 =?utf-8?B?elY5a2V6OVVTTFJzT2pTVExRZmsxaTV3WGVTU1pRVE0yVHhTaVZPUWxKVjVo?=
 =?utf-8?B?dW5wWm44MW4vQlVkQndITnorNWJqWjIyOGlOZWlLRDdGYkcraEg3M2xZRlFZ?=
 =?utf-8?B?SGtDcEpmVzVaVkdvdVdyRmx1cFE5bHZEOHUvbytiQ0NKTXJVK293aGdsbk93?=
 =?utf-8?B?UVYyRERlOEt6cDJxNlppMHFjU3BUanJqS2xIK21Jd0dMcThVU2RDU3Z3ZDZD?=
 =?utf-8?B?aXBXei8vb3ZKT25yRDAwUmZvTEZmNjYyZ2YweWMvTDFSK2ZwYkN4S2ptWmpN?=
 =?utf-8?B?NkV3TDBjM0Y5UFVvRVNldTFWcmw4NHVaUGhwTmpwaEd3OXNzbmJ0WW1UdFdo?=
 =?utf-8?B?d2JRV2d3aWI2YzkwNnlXUzFTOVZyTmFGU2FCWldJbjNBM3E4aVVKUWxDUTFG?=
 =?utf-8?B?Ym1TYXI2NDIyR08yZVZWYjJMV0xqd3ppbDUzWjduU1NZSVlKQ2xKb3kvQmhQ?=
 =?utf-8?B?Y0FyUHlmRkpKR0p5OHovNzhxa0gyOXl1OUZ1K3ZESzFTSExUZE13dDNoaTB5?=
 =?utf-8?B?a0YzU2lUVE1NVnZEaDJkQnAxNE9jRWtGWC80ZUErRUZtYWVEanhUQUFML1I3?=
 =?utf-8?B?ODZyclNOK3hocGpwUG9NODI2dVFjZGJPaGtrcUQ5Y3lZWnBZQWhlZGhWcGd1?=
 =?utf-8?B?bnY5NXp5TGNoL05yMER3R0tPY1lEMmhUdE0xZ0QvOEdKTDJDV3dSaUN6NFZH?=
 =?utf-8?B?MjNuZDhEYVJrY1YyNG4wKzE4V2VoV2JxdC9QWjBmckdoSFZSbDFCSlNCSGcr?=
 =?utf-8?B?S01EaWZIL0dBNXNxVmQ4a1VmOVlScm5LVUJCWUZHWXNGQW1jRjJacGUrTnZJ?=
 =?utf-8?B?U3o2VUE5WCs0YXlwWXdNckx3SXJvRWM3bVNmK1hjOUhjbkxFQUM2ZEcxVFJr?=
 =?utf-8?B?a3QwaDd5SkdrOUx2WnR5dk1kVzBlSDBsZWVIQkhWczd0SWRsUlA3QjdxZWNo?=
 =?utf-8?B?TTNHT21OTjdZdStrWXFrWGw4d0ZDYTk4eFowZllVYTdsdTNXWGZKMG01OVE4?=
 =?utf-8?B?NTRYc3JyWDI4aUErKzhsYlE2MS9UWGVNS3hDemttQkNQVmc2NmIreG51L2gx?=
 =?utf-8?Q?Bbzbx1yDBfsE2Rx4UpVQ/qk+b?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bd280052-4ebc-4ed7-b5fa-08dc89809f1a
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2024 19:07:49.9099
 (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: gp0KZHHsMl4Gkg0yCTKDQW3hSWEgY2SWnL/534JOStseKg5QWdl7+dJsxvQVuw8y6ucO6agwqctRWSGFy2XdTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR12MB9391

On 6/7/2024 8:26 AM, Jonathan Cameron wrote:
> On Thu, 23 May 2024 14:21:40 -0700
> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote:
> 
>> Hi Alison,
>>
>> On 5/22/2024 5:03 PM, Alison Schofield wrote:
>>> On Wed, May 22, 2024 at 03:08:37PM +0000, Smita Koralahalli wrote:
>>>> UEFI v2.10 section N.2.13 defines a CPER record for CXL Protocol errors.
>>>>
>>>> Add GHES support to detect CXL CPER Protocol Error Record and Cache Error
>>>> Severity, Device ID, Device Serial number and CXL RAS capability struct in
>>>> struct cxl_cper_prot_err. Include this struct as a member of struct
>>>> cxl_cper_work_data.
>>>>
>>>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>

[snip]


>>>> +	 * The device ID or agent address is required for CXL RCD, CXL
>>>> +	 * SLD, CXL LD, CXL Fabric Manager Managed LD, CXL Root Port,
>>>> +	 * CXL Downstream Switch Port and CXL Upstream Switch Port.
>>>> +	 */
>>>> +	if (prot_err->agent_type <= 0x7 && prot_err->agent_type != RCH_DP) {
>>>
>>> For this check against agent_type, and the similar one below, would a boolean
>>> array indexed by the agent type work? That would avoid the <= 0x7 and > 0x4
>>> below. It seems one array would suffice for this case, but naming it isn't obvious
>>> to me. Maybe it'll be to you.
>>>
>>> Something similar to what is done for prot_err_agent_type_strs[]
>>>
>>> static const bool agent_requires_id_address_serial[] = {
>>> 	true,	/* RDC */ 	
>>> 	false,	/* RCH_DP */
> 	[RCD] = false,
> 
> etc rather than comments would be neater.
> Given two similar things already. Maybe time for a little structure.
> 
> //with better name than this
> struct agent_reqs {
> 	bool sn;
> 	bool sbdf;
> };
> 
> static const agent_reqs agent_reqs[] = {
> 	[RCD] = { .sn = false, .sbdf = true, },
> };
> 
> etc.
> 
> Maybe just bring the the string in as well
> 
> struct agent_info {
> 	const char *string;
> 	bool req_sn;
> 	bool req_sbdf;
> };
> 
> static const agent_info agent_info[] = {
> 	[RD] = {
> 		.string = "Restricted CXL Device",
> 		.req_sn = false,
> 		.req_sbdf = true,
> 	},
> };
> 
> Values made up, but hopefully conveys that moving to having
> all the data in one place makes it harder to forget stuff
> for new entries etc.
> 

Okay. I was considering 2D array without naming before. Something like:

static const bool agent_requires_id_address[][] = {
        /* Device_ID, Serial_num */
        {true, true},   /* RCD */
        {false, false}, /* RCH_DP */

Let me change to array of structures if naming helps.

Thanks,
Smita

[snip]