Received-SPF: pass (google.com: domain of linux-kernel+bounces-179094-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 14 May 2024 14:10:45 -0500
From: John Allen <john.allen@amd.com>
To: Zaid Alali <zaidal@os.amperecomputing.com>
Cc: lenb@kernel.org, james.morse@arm.com, tony.luck@intel.com, bp@alien8.de,
	robert.moore@intel.com, Avadhut.Naik@amd.com,
	xueshuai@linux.alibaba.com, linux-acpi@vger.kernel.org,
	linux-kernel@vger.kernel.org, acpica-devel@lists.linux.dev
Subject: Re: [RFC PATCH 1/5] ACPI: APEI: EINJ: Enable the discovery of EINJv2
 capabilities
Message-ID: <ZkO3NRUV7Kjrf/VS@AUS-L1-JOHALLEN.amd.com>
References: <20240312212626.29007-1-zaidal@os.amperecomputing.com>
 <20240312212626.29007-2-zaidal@os.amperecomputing.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240312212626.29007-2-zaidal@os.amperecomputing.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?oWtdVcncTRFkbHgiQEcM4qraj66N0PqJHEgtC7lSiWPSPwVa0HpXEtSq0x/U?=
 =?us-ascii?Q?+FvhsfcoM8+6BNlmJlQCOV6wD1kyQoSUAsTpcO2fcgJGT8X8JYaBzJWTV8H7?=
 =?us-ascii?Q?2XQMsxyAcKmKBp3AcmI18J0YWjAVgRRNiKHem3zRg2Bq85aZDKZW0Ca9rypE?=
 =?us-ascii?Q?l/6OIuWLiUu+cH8+gUoL0RCT8CaJiHAH5xVqBT9v8fBBqF0zyvhRrzzgRhxi?=
 =?us-ascii?Q?6PeewR7yR0Rt8185iUYLB/EtfBkgW27lEg3ZKpTSuyhygqOLNA/NqhDZYINs?=
 =?us-ascii?Q?9qPswOeM0pci1Ne4pKONtLZg5yfpLL7qVOB7Koxa5y1Tfc16Y5belufgsLJQ?=
 =?us-ascii?Q?4BdW4QOZDcay4zMsYl2jCUZVRsHQGtxMZBiqrhe3gABpwU0W7XydG7qIXUyw?=
 =?us-ascii?Q?i2kb9T4gnIx3M80RKLJYtRUR4CYcjQUF47sJ5/BXQAUMWTY7beHCR/iXvZcQ?=
 =?us-ascii?Q?JfhGGUluykAExHaznVeO1doUfSQPTneCpovXVzvWr1bKVh0Q8qk83L9a+y5D?=
 =?us-ascii?Q?uGXzTYDfT31geOpGodWJ4urVOclYFVVeWClsaogV2AIJL1A7t7u3DY1NgSWH?=
 =?us-ascii?Q?7DXMKSAIfxVAIgOYg8M7a5ezFd9tXJUFoUf7dvZFTBhClzsfZ0G90jYE057T?=
 =?us-ascii?Q?mITRCXLUzdUI3XoLd+BMsnI11BiwuQzLOw9aO0xQtC6P5wLiAxjnJqxFKFa2?=
 =?us-ascii?Q?hofdM8fL7G0Ua3NW70oUR8YfiLHmdWUL7x3DQ2bHBrhWZNN1lO8teMI9ZHGQ?=
 =?us-ascii?Q?iTPDdh/GOCZOTT6IqCzE/sGeeCQnxPDyCPMvZawgpX8MHaxRKAOYFZy+UiIy?=
 =?us-ascii?Q?9gtsOrL9NOSMSm67ci0N5oHINr9tQ7m3XH2Neju9vkN0Bc5nBBoEzNn/fiC+?=
 =?us-ascii?Q?LftqcqfT4eISHsogxm54fUBOaxoEE4x4d57Qk5hhDKb57RqawJ4eLq7mpkmf?=
 =?us-ascii?Q?rFc0BQjDPI0DFXbBDm8f2aYa+DNwNdseWDowtHSuTtm5P1orqfE5LYuJ6YhE?=
 =?us-ascii?Q?iT+4ZJC6GZyuDkS3IGtxjftaKoXaoWxAeNxy/4rlE8N14ff7FO1aiP6v22EE?=
 =?us-ascii?Q?4Af3bWPudb0pDhXmfec8mRh5nWltXdTJfwrvutOGa85bMsIHsSOiikQa6Brh?=
 =?us-ascii?Q?Muk5STTL7SeCzX+8skLiL1G7bEH+hXWF5viJ6NUb5JmRMjNxgkQjwojDu6/t?=
 =?us-ascii?Q?PKer5NEQ6TTsZDUu3klGg0xJP4X5bZScNkn6hKfzurPhuTuteG5ofZ2/CraZ?=
 =?us-ascii?Q?0I9lNEj2gIXWQ+OJLM+pTS6qFDnhs/SyVp8skQApWmPr/d5NuefeHi1NQkOj?=
 =?us-ascii?Q?giy8QibcC0RnFUpRLB8nDUUqA8Tx2sza9cNnhoE9E4+EJqf0v30LJh+WaDMb?=
 =?us-ascii?Q?+crvcOkA//KjCBQLw3W5ytzNpzFZFDeytZVwnSKnXKHsHga5VbhFKGVfJTD7?=
 =?us-ascii?Q?L902SBCuDpx/+PAjUIdV2t1ZVbhS28DqGMZ6xPZgermgT3JOHrDXIEyKnJ/6?=
 =?us-ascii?Q?b+EmI8IztHHZCjS+UlkbxYVHKkGvQCjq5OdpI+GCHK13q+DsblOkzfspfIie?=
 =?us-ascii?Q?1jo61Hdo02Rkd69TiLUBkiKL06GFhZQn01AtdoPE?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b7ab5589-2234-4098-b881-08dc74499441
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5995.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2024 19:10:55.0648
 (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: QrZ8KAOQ02l9j6b4nLFbFX8DH8qliGrd45r+Iznh9Zt9F4/yEDkSYRTwZJ+Eyp8rzJmjykPqlUutC+PGYScyAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB8738

On Tue, Mar 12, 2024 at 02:26:22PM -0700, Zaid Alali wrote:
> EINJv2 capabilities can be discovered by checking the return value
> of get_error_type, where bit 30 set indicates EINJv2 support. This
> commit enables the driver to show all supported error injections

Drop "This commit" and write this using imperative mood (as a command).
For this one, "Enable the driver to show all supported error injections
for EINJ and EINJv2 at the same time".

> for EINJ and EINJv2 at the same time.
> 
> Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com>
> ---
>  drivers/acpi/apei/einj.c | 37 ++++++++++++++++++++++++++++++-------
>  include/acpi/actbl1.h    |  1 +
>  2 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 89fb9331c611..90efbcbf6b54 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -32,6 +32,7 @@
>  #define SLEEP_UNIT_MAX		5000			/* 5ms */
>  /* Firmware should respond within 1 seconds */
>  #define FIRMWARE_TIMEOUT	(1 * USEC_PER_SEC)
> +#define ACPI65_EINJV2_SUPP	BIT(30)
>  #define ACPI5_VENDOR_BIT	BIT(31)
>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
> @@ -145,13 +146,13 @@ static void einj_exec_ctx_init(struct apei_exec_context *ctx)
>  			   EINJ_TAB_ENTRY(einj_tab), einj_tab->entries);
>  }
>  
> -static int __einj_get_available_error_type(u32 *type)
> +static int __einj_get_available_error_type(u32 *type, int version)
>  {
>  	struct apei_exec_context ctx;
>  	int rc;
>  
>  	einj_exec_ctx_init(&ctx);
> -	rc = apei_exec_run(&ctx, ACPI_EINJ_GET_ERROR_TYPE);
> +	rc = apei_exec_run(&ctx, version);
>  	if (rc)
>  		return rc;
>  	*type = apei_exec_ctx_get_output(&ctx);
> @@ -160,12 +161,12 @@ static int __einj_get_available_error_type(u32 *type)
>  }
>  
>  /* Get error injection capabilities of the platform */
> -static int einj_get_available_error_type(u32 *type)
> +static int einj_get_available_error_type(u32 *type, int version)
>  {
>  	int rc;
>  
>  	mutex_lock(&einj_mutex);
> -	rc = __einj_get_available_error_type(type);
> +	rc = __einj_get_available_error_type(type, version);
>  	mutex_unlock(&einj_mutex);
>  
>  	return rc;
> @@ -221,9 +222,14 @@ static void check_vendor_extension(u64 paddr,
>  
>  static void *einj_get_parameter_address(void)
>  {
> -	int i;
> +	int i, rc;
>  	u64 pa_v4 = 0, pa_v5 = 0;
>  	struct acpi_whea_header *entry;
> +	u32 error_type = 0;
> +
> +	rc = einj_get_available_error_type(&error_type, ACPI_EINJ_GET_ERROR_TYPE);
> +	if (rc)
> +		return NULL;
>  
>  	entry = EINJ_TAB_ENTRY(einj_tab);
>  	for (i = 0; i < einj_tab->entries; i++) {
> @@ -615,19 +621,35 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>  	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
>  	{ BIT(31), "Vendor Defined Error Types" },
>  };
> +static struct { u32 mask; const char *str; } const einjv2_error_type_string[] = {
> +	{ BIT(0), "EINJV2 Processor Error" },
> +	{ BIT(1), "EINJV2 Memory Error" },
> +	{ BIT(2), "EINJV2 PCI Express Error" },
> +};
>  
>  static int available_error_type_show(struct seq_file *m, void *v)
>  {
>  	int rc;
>  	u32 error_type = 0;
>  
> -	rc = einj_get_available_error_type(&error_type);
> +	rc = einj_get_available_error_type(&error_type, ACPI_EINJ_GET_ERROR_TYPE);
>  	if (rc)
>  		return rc;
>  	for (int pos = 0; pos < ARRAY_SIZE(einj_error_type_string); pos++)
>  		if (error_type & einj_error_type_string[pos].mask)
>  			seq_printf(m, "0x%08x\t%s\n", einj_error_type_string[pos].mask,
>  				   einj_error_type_string[pos].str);
> +	if (error_type & ACPI65_EINJV2_SUPP) {
> +		rc = einj_get_available_error_type(&error_type, ACPI_EINJV2_GET_ERROR_TYPE);
> +		if (rc)
> +			return rc;
> +		seq_printf(m, "====================\n");

Seems like if we're going to visually split the EINJ and EINJv2 cases,
rather than just splitting them with the above line, it might be better
to be more descriptive. For example:

# cat available_error_type
EINJ error types:
0x00000002    Processor Uncorrectable non-fatal
0x00000008    Memory Correctable
0x00000010    Memory Uncorrectable non-fatal
EINJv2 error types:
0x00000001    EINJV2 Processor Error
0x00000002    EINJV2 Memory Error

> +		for (int pos = 0; pos < ARRAY_SIZE(einjv2_error_type_string); pos++)
> +			if (error_type & einjv2_error_type_string[pos].mask)
> +				seq_printf(m, "0x%08x\t%s\n", einjv2_error_type_string[pos].mask,
> +					einjv2_error_type_string[pos].str);

When you break a long function call like this, the second line should
align below the first character after the parenthesis. For example:

	seq_printf(m, "0x%08x\t%s\n", einjv2_error_type_string[pos].mask,
		   einjv2_error_type_string[pos].str);

There are a few other places in the series where alignment should be
fixed in this way as well.

Thanks,
John

> +
> +	}
>  
>  	return 0;
>  }
> @@ -662,7 +684,8 @@ static int error_type_set(void *data, u64 val)
>  	if (tval & (tval - 1))
>  		return -EINVAL;
>  	if (!vendor) {
> -		rc = einj_get_available_error_type(&available_error_type);
> +		rc = einj_get_available_error_type(&available_error_type,
> +				ACPI_EINJ_GET_ERROR_TYPE);
>  		if (rc)
>  			return rc;
>  		if (!(val & available_error_type))
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index a33375e055ad..a07d564b0590 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -1030,6 +1030,7 @@ enum acpi_einj_actions {
>  	ACPI_EINJ_SET_ERROR_TYPE_WITH_ADDRESS = 8,
>  	ACPI_EINJ_GET_EXECUTE_TIMINGS = 9,
>  	ACPI_EINJ_ACTION_RESERVED = 10,	/* 10 and greater are reserved */
> +	ACPI_EINJV2_GET_ERROR_TYPE = 0x11,
>  	ACPI_EINJ_TRIGGER_ERROR = 0xFF	/* Except for this value */
>  };
>  
> -- 
> 2.34.1
>