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

From: Yazen Ghannam
Date: Sun Apr 07 2024 - 09:43:38 EST




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@xxxxxxx>
---
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@xxxxxxx>

Thanks,
Yazen