Re: [PATCH 1/2] ACPI: PRM: Add PRM handler direct call support

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

On 3/26/24 17:26, John Allen wrote:
> Platform Runtime Mechanism (PRM) handlers can be invoked from either the
> AML interpreter or directly by an OS driver. Implement the direct call
> method.
> 
> Export the symbol as this will be used by modules such as the AMD
> Address Translation Library and likely others in the future.
> 
> Signed-off-by: John Allen <john.allen@amd.com>
> ---
>   drivers/acpi/prmt.c  | 24 ++++++++++++++++++++++++
>   include/linux/prmt.h |  5 +++++
>   2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> index c78453c74ef5..9e548426cc22 100644
> --- a/drivers/acpi/prmt.c
> +++ b/drivers/acpi/prmt.c
> @@ -214,6 +214,30 @@ static struct prm_handler_info *find_prm_handler(const guid_t *guid)
>   #define UPDATE_LOCK_ALREADY_HELD 	4
>   #define UPDATE_UNLOCK_WITHOUT_LOCK 	5
>   
> +int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
> +{
> +	struct prm_handler_info *handler = find_prm_handler(&handler_guid);
> +	struct prm_module_info *module = find_prm_module(&handler_guid);

I wonder if the module revision should be checked. Maybe this is a
future problem to address if/when the need arises?

It seems like versioning can be done a few ways for a semi-stable
interface.

1) Keep the module GUID the same and change the module major/minor
revisions as needed.
2) Give the module a new GUID every time there's a change.
3) Keep the module GUID the same, but change a handler GUID if the
handler's inputs/outputs change.

I think #3 would be the way to go for most cases. So the onus is on the
caller to have the correct handler GUID. And no changes needed here.

Just wanted to write out some thoughts in case others have feedback.

> +	struct prm_context_buffer context;
> +	efi_status_t status;
> +
> +	if (!module || !handler)
> +		return -ENODEV;
> +
> +	memset(&context, 0, sizeof(context));
> +	ACPI_COPY_NAMESEG(context.signature, "PRMC");
> +	context.identifier = handler->guid;
> +	context.static_data_buffer = handler->static_data_buffer_addr;
> +	context.mmio_ranges = module->mmio_info;

Minor nit: I suggest aligning these lines on the '=' for easier reading.

> +
> +	status = efi_call_acpi_prm_handler(handler->handler_addr,
> +					   (u64)param_buffer,
> +					   &context);
> +
> +	return efi_status_to_err(status);
> +}
> +EXPORT_SYMBOL_GPL(acpi_call_prm_handler);
> +
>   /*
>    * This is the PlatformRtMechanism opregion space handler.
>    * @function: indicates the read/write. In fact as the PlatformRtMechanism
> diff --git a/include/linux/prmt.h b/include/linux/prmt.h
> index 24da8364b919..9c094294403f 100644
> --- a/include/linux/prmt.h
> +++ b/include/linux/prmt.h
> @@ -2,6 +2,11 @@
>   
>   #ifdef CONFIG_ACPI_PRMT
>   void init_prmt(void);
> +int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer);
>   #else
>   static inline void init_prmt(void) { }
> +static inline int acpi_call_prm_handler(guid_t handler_guid, void *param_buffer)
> +{
> +	return -EOPNOTSUPP;
> +}
>   #endif

Overall, looks good to me.

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen