Re: [PATCH v11 3/4] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry

On Tue, Dec 07, 2021 at 08:44:52PM +0100, Rafael J. Wysocki wrote:

On Sun, Nov 21, 2021 at 4:17 PM Chen Yu [off-list ref] wrote:

quoted

First of all, the subject of the patch should be "Introduce Platform
Firmware Runtime Telemetry", because there is no "update" part in it.

Ok, will change it. 

quoted

Platform Firmware Runtime Update(PFRU) Telemetry Service is part of RoT
(Root of Trust), which allows PFRU handler and other PFRU drivers to
produce telemetry data to upper layer OS consumer at runtime.

The above paragraph is redundant IMO.  It provides a little useful
information IMV.

Ok, will remove it 

quoted

The Linux kernel provides interfaces for the user to query the parameters
of telemetry data, and the user could read out the telemetry data
accordingly.

In fact, this driver allows user space to fetch telemetry data from
the firmware with the help of the Platform Firmware Runtime Telemetry
interface.

Ok. 

quoted

PFRU and PFRU Telemetry both invoke _DSM to trigger the low level actions.

"PFRU and PFRT " or "PFRU and PFR Telemetry ", please.

Ok will use the former one.

quoted

However the input parameters and ACPI package result as well as the
functions are different from each other. It is hard to extract the common
code between them, so introduce separated files for them.

I would write the above in the following way:

"Both PFRU and PFRT are based on ACPI _DSM interfaces located under
special device objects in the ACPI Namespace, but these interfaces are
different from each other, so it is better to provide a separate
driver from each of them.  However, they share some common definitions
and naming conventions."

Ok, will change it. 

quoted

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---

[cut the history]

quoted

---
 drivers/acpi/pfru/Makefile         |   2 +-
 drivers/acpi/pfru/pfru_telemetry.c | 457 +++++++++++++++++++++++++++++
 include/uapi/linux/pfru.h          |  88 ++++++
 3 files changed, 546 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/pfru/pfru_telemetry.c

diff --git a/drivers/acpi/pfru/Makefile b/drivers/acpi/pfru/Makefile
index 098cbe80cf3d..9ba11efe10b3 100644
--- a/drivers/acpi/pfru/Makefile
+++ b/drivers/acpi/pfru/Makefile

Actually, I'm not sure if a separate directory for this is needed.

The files (pfrut.h, pfr_update.c and pfr_telemetry.c) can be located
directly in drivers/acpi/ as far as I'm concerned.

pfr_update.c and pfr_telemetry.c could be put in drivers/acpi, but pfrut.h
is used by user space tool for ioctl interface and shares some data structures
between the driver and user space tool, so the pfrut.h might need to be put
to include/uapi/linux/pfrut.h 

quoted

@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_ACPI_PFRU) += pfru_update.o
+obj-$(CONFIG_ACPI_PFRU) += pfru_update.o pfru_telemetry.o

Please also update Kconfig with the information regarding PFRT (which
is not going to be added by the previous patch I suppose).

Ok, will do. 

quoted

diff --git a/drivers/acpi/pfru/pfru_telemetry.c b/drivers/acpi/pfru/pfru_telemetry.c
new file mode 100644
index 000000000000..5140a4591c9e
--- /dev/null
+++ b/drivers/acpi/pfru/pfru_telemetry.c

@@ -0,0 +1,457 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ACPI Platform Firmware Runtime Update Telemetry Driver

"ACPI Platform Runtime Telemetry driver"

Ok.

quoted

+ *
+ * Copyright (C) 2021 Intel Corporation
+ * Author: Chen Yu [off-list ref]

Please describe the driver here at least briefly (what it is for etc.).

Ok, will do. 

quoted

+ */
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mm.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+#include <linux/uio.h>
+#include <linux/uuid.h>
+
+#include <uapi/linux/pfru.h>
+
+#define PFRU_LOG_EXEC_IDX      0
+#define PFRU_LOG_HISTORY_IDX   1
+
+#define PFRU_LOG_ERR           0
+#define PFRU_LOG_WARN  1
+#define PFRU_LOG_INFO  2
+#define PFRU_LOG_VERB  4
+
+#define PFRU_FUNC_SET_LEV              1
+#define PFRU_FUNC_GET_LEV              2
+#define PFRU_FUNC_GET_DATA             3
+
+#define PFRU_REVID_1           1
+#define PFRU_REVID_2           2
+#define PFRU_DEFAULT_REV_ID    PFRU_REVID_1

I would use the PFRT_ prefix in the above symbols.

Ok, will change them accordingly. 

quoted

+
+enum log_index {
+       LOG_STATUS_IDX = 0,
+       LOG_EXT_STATUS_IDX = 1,
+       LOG_MAX_SZ_IDX = 2,
+       LOG_CHUNK1_LO_IDX = 3,
+       LOG_CHUNK1_HI_IDX = 4,
+       LOG_CHUNK1_SZ_IDX = 5,
+       LOG_CHUNK2_LO_IDX = 6,
+       LOG_CHUNK2_HI_IDX = 7,
+       LOG_CHUNK2_SZ_IDX = 8,
+       LOG_ROLLOVER_CNT_IDX = 9,
+       LOG_RESET_CNT_IDX = 10,
+       LOG_NR_IDX
+};
+
+struct pfru_log_device {

'pfrt_log_device"

Ok. 

quoted

+       int index;
+       struct pfru_log_info info;
+       struct device *parent_dev;
+       struct miscdevice miscdev;
+};
+
+static const guid_t pfru_log_guid =
+       GUID_INIT(0x75191659, 0x8178, 0x4D9D, 0xB8, 0x8F, 0xAC, 0x5E,
+                 0x5E, 0x93, 0xE8, 0xBF);

The UUID needs to be documented.

Ok.

quoted

+
+static DEFINE_IDA(pfru_log_ida);

pfrt_log_ida

Generally, I would use the pfrt_ prefix in the names below instead of pfru_.

Ok, will change them accordingly.

quoted

+
+static inline struct pfru_log_device *to_pfru_log_dev(struct file *file)
+{
+       return container_of(file->private_data, struct pfru_log_device, miscdev);
+}
+
+static int get_pfru_log_data_info(struct pfru_log_data_info *data_info,
+                                 struct pfru_log_device *pfru_log_dev)
+{
+       acpi_handle handle = ACPI_HANDLE(pfru_log_dev->parent_dev);
+       union acpi_object *out_obj, in_obj, in_buf;
+       int ret = -EINVAL;
+
+       memset(&in_obj, 0, sizeof(in_obj));
+       memset(&in_buf, 0, sizeof(in_buf));
+       in_obj.type = ACPI_TYPE_PACKAGE;
+       in_obj.package.count = 1;
+       in_obj.package.elements = &in_buf;
+       in_buf.type = ACPI_TYPE_INTEGER;
+       in_buf.integer.value = pfru_log_dev->info.log_type;
+
+       out_obj = acpi_evaluate_dsm_typed(handle, &pfru_log_guid,
+                                         pfru_log_dev->info.log_revid, PFRU_FUNC_GET_DATA,
+                                         &in_obj, ACPI_TYPE_PACKAGE);
+       if (!out_obj)
+               return ret;
+
+       if (out_obj->package.count < LOG_NR_IDX)
+               goto free_acpi_buffer;
+
+       if (out_obj->package.elements[LOG_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->status = out_obj->package.elements[LOG_STATUS_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->ext_status =
+               out_obj->package.elements[LOG_EXT_STATUS_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_MAX_SZ_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->max_data_size =
+               out_obj->package.elements[LOG_MAX_SZ_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_CHUNK1_LO_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->chunk1_addr_lo =
+               out_obj->package.elements[LOG_CHUNK1_LO_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_CHUNK1_HI_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->chunk1_addr_hi =
+               out_obj->package.elements[LOG_CHUNK1_HI_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_CHUNK1_SZ_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->chunk1_size =
+               out_obj->package.elements[LOG_CHUNK1_SZ_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_CHUNK2_LO_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->chunk2_addr_lo =
+               out_obj->package.elements[LOG_CHUNK2_LO_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_CHUNK2_HI_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->chunk2_addr_hi =
+               out_obj->package.elements[LOG_CHUNK2_HI_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_CHUNK2_SZ_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->chunk2_size =
+               out_obj->package.elements[LOG_CHUNK2_SZ_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->rollover_cnt =
+               out_obj->package.elements[LOG_ROLLOVER_CNT_IDX].integer.value;
+
+       if (out_obj->package.elements[LOG_RESET_CNT_IDX].type != ACPI_TYPE_INTEGER)
+               goto free_acpi_buffer;
+
+       data_info->reset_cnt =
+               out_obj->package.elements[LOG_RESET_CNT_IDX].integer.value;

Like in some functions in the other patch, it would be good to reduce
the number of goto statements and possibly redundant updates by
grouping the checks.

Ok, will do. 

quoted

+
+       ret = 0;
+
+free_acpi_buffer:
+       kfree(out_obj);
+
+       return ret;
+}
+

[cut]

quoted

+static long pfru_log_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+       struct pfru_log_device *pfru_log_dev = to_pfru_log_dev(file);
+       struct pfru_log_data_info data_info;
+       struct pfru_log_info info;
+       void __user *p;
+       int ret = 0;
+
+       p = (void __user *)arg;
+
+       switch (cmd) {
+       case PFRU_LOG_IOC_SET_INFO:
+               if (copy_from_user(&info, p, sizeof(info)))
+                       return -EFAULT;
+
+               if (valid_log_revid(info.log_revid))
+                       pfru_log_dev->info.log_revid = info.log_revid;
+
+               if (valid_log_level(info.log_level)) {
+                       ret = set_pfru_log_level(info.log_level, pfru_log_dev);
+                       if (ret < 0)
+                               return ret;
+
+                       pfru_log_dev->info.log_level = info.log_level;
+               }
+
+               if (valid_log_type(info.log_type))
+                       pfru_log_dev->info.log_type = info.log_type;
+
+               return 0;

Please use blank lines to separate the cases visually from each other.

Ok.

quoted

+       case PFRU_LOG_IOC_GET_INFO:
+               info.log_level = get_pfru_log_level(pfru_log_dev);
+               if (ret < 0)
+                       return ret;
+

[cut]

quoted

+       pfru_log_dev->info.log_revid = PFRU_DEFAULT_REV_ID;
+       pfru_log_dev->parent_dev = &pdev->dev;
+
+       pfru_log_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
+       pfru_log_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                                                   "pfru_telemetry%d",

I would call it "pfr_telemetry" (without the "u" in the prefix).

Ok.

quoted

+                                                   pfru_log_dev->index);
+       if (!pfru_log_dev->miscdev.name)
+               return -ENOMEM;
+
+       pfru_log_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                                                       "acpi_pfru_telemetry%d",

And this one (analogously) "acpi_pfr_telemetry".

Ok, will do.

quoted

+                                                       pfru_log_dev->index);
+       if (!pfru_log_dev->miscdev.nodename)
+               return -ENOMEM;
+

[cut]

And of course it needs to be rebased to take the changes in the
previous patch into account.

Ok, will do. 

Overall, it's almost there IMO.

thanks,
Chenyu