Received-SPF: pass (google.com: domain of linux-kernel+bounces-179076-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 14 May 2024 13:44:06 -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 3/5] ACPI: APEI: EINJ: Add debugfs files for EINJv2
 support
Message-ID: <ZkOw9nPxf4xoAXXd@AUS-L1-JOHALLEN.amd.com>
References: <20240312212626.29007-1-zaidal@os.amperecomputing.com>
 <20240312212626.29007-4-zaidal@os.amperecomputing.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240312212626.29007-4-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?Z74gnJYH4ONbJtOVJ1HqaqCwz9Fufwph0xG6d2ouYh9kRY+BENRXVz5suREm?=
 =?us-ascii?Q?vZ61qTtg5KxXd4bLLp0MyZPmqOXYSD/wFYG96rRbpravAxBcJMAHm9sejp5H?=
 =?us-ascii?Q?DssnP8+i63REZqyAgkcQa1sGgugxR3UVOcZ2kC9iCfPZaouB+3NEwFhxOvVF?=
 =?us-ascii?Q?WVfCN2w1gGSqQL6PbNH0nMVyA+5+McL1LDHIGBl1vaQReZ7N7uY4sSTw5+MN?=
 =?us-ascii?Q?AazbdeYgvz8E7GnpJFXMwbZhGfTRoRl5cMkGrgDpbvI1iC+Bo0ACIZnkqf66?=
 =?us-ascii?Q?6A+KsO/rqiAJUVf7tBD7uwOGx3ds3p2K8VxTASPcXSh7Nwy3bLVMReLSJ4ia?=
 =?us-ascii?Q?3kDAXjv7ZPzGsvA+x5c6RUt0plArmuGa9Ms/HQA9Acm3Pd4dccbXGLdOnjya?=
 =?us-ascii?Q?Nmck3xxFXp91BwFFPqh6uYhvF4jzZ5I0zOXbBEUZvbUm1oaiTgSfVwsqZJjt?=
 =?us-ascii?Q?Nn6gbf85xVxXtIj5ZP7R1HWslP6WktS+IDYc3LQfSn6e6xWAMjcX1t/khqpt?=
 =?us-ascii?Q?BB2uP+pkkemhZJJfy+zygF1n7oq5YQA2CehMI2Whfqnra86Uv1dkhBtB/tm0?=
 =?us-ascii?Q?wIdU4C3rKtxhXoWF29PDTQ6XD3xROvXXfiAUsNfEwGD205rLYzrDKAioLg+b?=
 =?us-ascii?Q?Xg7O/uC5MiPXL+T846SOU5PPzzDizvEtRnksAdhf0zSTi7IwzvfoQnIi0MM4?=
 =?us-ascii?Q?AlGiofwUxckeKd7XdV/5zOxt4NfZaSo81imw7hmynN23WBfdcvurNR08KzDf?=
 =?us-ascii?Q?UDxXNclrnqfRRlYNNHZ5uB09bEJtlGX4V1TOx6xaJfRUVe7VCUpiGgoXNgET?=
 =?us-ascii?Q?21ywqpkkLrc14r0LexJLXGbliR0CVycjUBNqlvi/azqMboVkoYmy3KhYOqEo?=
 =?us-ascii?Q?9meYFt46wqp3k/9hakS76zMvOcNr3YpkX4BJ/k88GBLAbEm49eovnM7QEw23?=
 =?us-ascii?Q?6lNOdvpwfpX2d15bit1vy8Jh5y7CkDBPot97LWZZalGHjGCSsPxR+e331JwU?=
 =?us-ascii?Q?mzZzUAV2VBE9IMZ2Uz1WKwdkIZmhFaENreU1lFsc6w5nT2Qbec4rFSxY9gVj?=
 =?us-ascii?Q?pR8geVHOZMEj3LrY+uCBxd4JBPPNvIBhgOMEmIjIVQbfsGbesY4uhJddwYAV?=
 =?us-ascii?Q?51uzKbAyKFeNtRZOZ5QWsjo/TCcbC7cOemgPO2cyi9y+cB5smJiqqW7iAa/m?=
 =?us-ascii?Q?s8y7G8iaBNPGgGzFDBwOuusWF03E1REKbv57RXheel6qjKy4dhTx4aC5IGzH?=
 =?us-ascii?Q?t9M68Bk4qvbQRcUNgcp9qQsMuZEPHSve96+EnEQ7bBPNLx9S3UPrL2nF38Nn?=
 =?us-ascii?Q?a7U+a7zs5HcUU+f6KErbkpjfITPgz+u5QudRdq3jO4vN2azJ7Mb5eEytk8IA?=
 =?us-ascii?Q?+1apQUB3JC15p1gnG/XMH5PHRyynjJHHQQ6TbS5I+SwqUr27us330ies3s9R?=
 =?us-ascii?Q?fuPnbiviYxsiEiJSqwMXTIJ4r76NuOR8Mvhi7aGUMrXdV8/WfLPK+4nRfoA2?=
 =?us-ascii?Q?3GqZpSLx6Ft7EeeSElwztYoma8tsi591CIOA/9KiBAa6ZGlZJv4h0Ud15Ysw?=
 =?us-ascii?Q?00HWkVwRtoxCPLpPK68YLTrXTWOQ6fFb2HEIiNhX?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 35752a39-5ff3-404f-4146-08dc7445da7b
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5995.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2024 18:44:15.9336
 (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: 5vyPFzji1G8MesJFB4NA1RHFXlnHHMpXELLq9lcwJVyCt8HPg1rdMRGvuZ1ww7L234vaK6ua1r0sI8cujH512A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6259

On Tue, Mar 12, 2024 at 02:26:24PM -0700, Zaid Alali wrote:
> EINJv2 enables users to inject errors to multiple components/
> devices at the same time. This commit creates a debugfs blob

Drop "This commit" and write this using imperative mood (as a command).
For example, "Create a debugfs blob file to be used for reading the user
input for the component array".

> file to be used for reading the user input for component array.
> 
> Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com>
> ---
>  drivers/acpi/apei/einj.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 119f7accd1c9..ceac53aa0d3f 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -101,6 +101,10 @@ static struct debugfs_blob_wrapper vendor_blob;
>  static struct debugfs_blob_wrapper vendor_errors;
>  static char vendor_dev[64];
>  
> +static struct debugfs_blob_wrapper einjv2_component_arr;
> +static u64 component_count;
> +static void *user_input;
> +
>  /*
>   * Some BIOSes allow parameters to the SET_ERROR_TYPE entries in the
>   * EINJ table through an unpublished extension. Use with caution as
> @@ -810,6 +814,8 @@ static int __init einj_init(void)
>  
>  	einj_param = einj_get_parameter_address();
>  	if ((param_extension || acpi5) && einj_param) {
> +		u32 error_type;
> +
>  		debugfs_create_x32("flags", S_IRUSR | S_IWUSR, einj_debug_dir,
>  				   &error_flags);
>  		debugfs_create_x64("param1", S_IRUSR | S_IWUSR, einj_debug_dir,
> @@ -820,6 +826,25 @@ static int __init einj_init(void)
>  				   &error_param3);
>  		debugfs_create_x64("param4", S_IRUSR | S_IWUSR, einj_debug_dir,
>  				   &error_param4);
> +
> +		rc = einj_get_available_error_type(&error_type, ACPI_EINJ_GET_ERROR_TYPE);
> +		if (rc)
> +			return rc;
> +
> +		if (error_type & ACPI65_EINJV2_SUPP) {
> +			debugfs_create_x64("einjv2_component_count", S_IRUSR | S_IWUSR,
> +					einj_debug_dir,	&component_count);
> +			user_input = kzalloc(PAGE_SIZE, GFP_KERNEL);

What is the reason for a PAGE_SIZE allocation here?

I would guess that a typical user will only supply a couple entries in
the component array. If this is x86 and PAGE_SIZE is 4k, that's probably
fine, but IIUC, ARM can have up to 64k pages which seems like a lot more
than is needed here. I would think that since this is architecture
independent ACPI code, we want to avoid using something architecture
dependent like page size here anyway.

I also think that while 4k may be fine (and usually overkill) in most
cases, it's much smaller than the maximum possible number of entries.
While uncommon, we might want to allow for a larger allocation while
still keeping the default allocation small. Maybe a module parameter
could be used to allocate a bigger file if needed?

Thanks,
John

> +			if (!user_input) {
> +				rc = -ENOMEM;
> +				goto err_release;
> +			}
> +			einjv2_component_arr.data = user_input;
> +			einjv2_component_arr.size = PAGE_SIZE;
> +			debugfs_create_blob("einjv2_component_array", S_IRUSR | S_IWUSR,
> +					einj_debug_dir, &einjv2_component_arr);
> +		}
> +
>  		debugfs_create_x32("notrigger", S_IRUSR | S_IWUSR,
>  				   einj_debug_dir, &notrigger);
>  	}
> @@ -871,6 +896,7 @@ static void __exit einj_exit(void)
>  	apei_resources_fini(&einj_resources);
>  	debugfs_remove_recursive(einj_debug_dir);
>  	acpi_put_table((struct acpi_table_header *)einj_tab);
> +	kfree(user_input);
>  }
>  
>  module_init(einj_init);
> -- 
> 2.34.1
>