On Wed, Dec 22, 2021 at 06:16:00PM +0100, Rafael J. Wysocki wrote: > On Wed, Dec 22, 2021 at 5:27 AM Chen Yu <yu.c.chen@xxxxxxxxx> wrote: > > > > The PFRUT(Platform Firmware Runtime Update and Telemetry) kernel interface > > is designed to interact with the platform firmware interface defined in the > > `Management Mode Firmware Runtime Update > > <https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf>` > > specification. The primary function of PFRUT is to carry out runtime > > updates of the platform firmware, which doesn't require the system to > > be restarted. It also allows telemetry data to be retrieved from the > > platform firmware. > > > > ============= > > - Change from v12 to v13: > > - Print the _DSM failure result if invalid EFI capsule is provided. > > (Hongyu Ning) > > - Change from v11 to v12: > > - Rename the driver from pfru_update to pfr_update, and the > > telemetry part to pfr_telementry for symmetry. > > (Rafael J. Wysocki) > > - Rename the "capsule file" to "EFI capsule". > > (Rafael J. Wysocki) > > - Revise the commit log. (Rafael J. Wysocki) > > - Add the reason in the commit log that explains why it is > > a good idea to add this driver to the kernel. > > (Rafael J. Wysocki) > > - Rename pfru.h to pfrut.h (Rafael J. Wysocki) > > - Rename the CONFIG_ACPI_PFRU to CONFIG_ACPI_PFRUT > > (Rafael J. Wysocki) > > - Rename the Kconfig name to "ACPI Platform Firmware Runtime > > Update and Telemetry" > > (Rafael J. Wysocki) > > - Revise the Kconfig help message (Rafael J. Wysocki) > > - Describe what the driver is for briefly in the beginning of > > the source code file. > > (Rafael J. Wysocki) > > - Add a comment pointing to the definitions of the GUIDs and > > explain briefly what they are for. > > (Rafael J. Wysocki) > > - Reduce redundant memory updates by doing all of the checks > > upfront. (Rafael J. Wysocki) > > - Rename valid_version() to applicable_image(), and revise the > > comment.(Rafael J. Wysocki) > > - Remove Redundant else in applicable_image().(Rafael J. Wysocki) > > - Rename dump_update_result() to print_ipdate_debug_info(). > > (Rafael J. Wysocki) > > - Rename start_acpi_update() to start_update()(Rafael J. Wysocki) > > Add a blank line before each "case xxx:"(Rafael J. Wysocki) > > - Move status check into query_capability() and returns -EBUSY if > > fails(Rafael J. Wysocki) > > - Rename the device node name from "acpi_pfru%d" to "acpi_pfr_update%d" > > (Rafael J. Wysocki) > > - Rename the drive name from "pfru_update" to "pfr_update" > > (Rafael J. Wysocki) > > - Rename PFRU_MAGIC_FOR_IOCTL to PFRUT_IOCTL_MAGIC(Rafael J. Wysocki) > > Fix grammar errors in the comments.(Rafael J. Wysocki) > > - Change from v10 to v11: > > - Revise the commit log to explain why version check is introduced > > in kernel rather than letting Management Mode to do it. > > (Rafael J. Wysocki) > > - Revise the commit log to better describe the pack attribute. > > (Rafael J. Wysocki) > > - Refine the comment for hw_ins and capsule_support. > > (Rafael J. Wysocki) > > - Change from v9 to v10: > > - Remove the explicit assignment of the last item of enum. > > (Andy Shevchenko) > > - Change from v8 to v9: > > - Use GUID_INIT() instead of guid_parse() during boot up. > > (Andy Shevchenko) > > - Drop uuid, code_uuid, drv_uuid in struct pfru_device as they > > are not needed. (Andy Shevchenko) > > - Drop type casting from void * in valid_version(). > > (Andy Shevchenko) > > - Use kfree() instead of ACPI_FREE() in non-ACPICA usage. > > (Andy Shevchenko) > > - Use sizeof(rev) instead of sizeof(u32) in copy_from_user(). > > (Andy Shevchenko) > > - Generate physical address from MSB part to LSB. > > (Andy Shevchenko) > > - Use devm_add_action_or_reset() to add ida release into dev resource > > management. (Andy Shevchenko) > > - Use devm_kasprintf() instead of kasprintf() to format the > > pfru_dev name.(Andy Shevchenko) > > - Remove redundant 0 in acpi_pfru_ids. (Andy Shevchenko) > > - Adjust the order of included headers in pfru.h. > > (Andy Shevchenko) > > - Replace PFRU_MAGIC with PFRU_MAGIC_FOR_IOCTL in uapi file. > > (Andy Shevchenko) > > Use devm_kasprintf() instead of kasprintf() to format the > > pfru_log_dev name.(Andy Shevchenko) > > Remove redundant 0 in acpi_pfru_log_ids. (Andy Shevchenko) > > - Change from v7 to v8: > > - Remove the variable-length array in struct pfru_update_cap_info, and > > copy the non-variable-length struct pfru_update_cap_info to userspace > > directly. (Greg Kroah-Hartman) > > - Use efi_guid_t instead of guid_t when parsing capsule file. > > (Andy Shevchenko) > > - Change the type of rev_id from int to u32, because this data will > > be copied between kernel and userspace. (Greg Kroah-Hartman) > > - Add a prefix for dev in struct pfru_device to parent_dev, so as > > to indicate that this filed is the parent of the created miscdev. > > (Greg Kroah-Hartman) > > - Use blank lines between different macro sections. (Greg Kroah-Hartman) > > Illusatrate the possible errno for each ioctl interface. > > (Greg Kroah-Hartman) > > - Remove pfru_valid_revid() from uapi header to avoid poluting the global > > namespace.(Greg Kroah-Hartman) > > - Assign the value to the enum type explicitly.(Greg Kroah-Hartman) > > - Change the guid_t to efi_guid_t when parsing image header in get_image_type() > > (Greg Kroah-Hartman) > > - Remove the void * to other type casting in valid_version(). (Andy Shevchenko) > > - Combined the assignment of variables with definitions. (Andy Shevchenko) > > - Define this magic for revision ID. (Andy Shevchenko) > > - Make the labeling consistent for error handling. (Andy Shevchenko) > > - Replace the UUID_SIZE in uapi with 16 directly. (Andy Shevchenko) > > - Add blank line between generic include header and uapi header. > > (Andy Shevchenko) > > - Arrange the order between devm_kzalloc() and normal allocation in > > acpi_pfru_probe() that, the former should always be ahead of the > > latter. (Andy Shevchenko) > > - Change from v6 to v7: > > - Use __packed instead of pragma pack(1). > > (Greg Kroah-Hartman, Ard Biesheuve) > > - Use ida_alloc() to allocate a ID, and release the ID when > > device is removed. (Greg Kroah-Hartman) > > - Check the _DSM method at early stage, before allocate or parse > > anything in acpi_pfru_[log_]probe(). (Greg Kroah-Hartman) > > - Set the parent of the misc device. (Greg Kroah-Hartman) > > - Use module_platform_driver() instead of platform_driver_register() > > in module_init(). Separate pfru driver and pfru_telemetry driver > > to two files. (Greg Kroah-Hartman) > > - Change from v5 to v6: > > - Use Link: tag to add the specification download address. > > (Andy Shevchenko) > > - Drop comma for each terminator entry in the enum structure. > > (Andy Shevchenko) > > - Remove redundant 'else' in get_image_type(). > > (Andy Shevchenko) > > - Directly return results from the switch cases in adjust_efi_size() > > and pfru_ioctl().(Andy Shevchenko) > > - Keep comment style consistency by removing the period for > > one line comment. > > (Andy Shevchenko) > > - Remove devm_kfree() if .probe() failed. > > (Andy Shevchenko) > > - Remove linux/uuid.h and use raw buffers to contain uuid. > > (Andy Shevchenko) > > - Include types.h in pfru.h. (Andy Shevchenko) > > - Use __u8[16] instead of uuid_t. (Andy Shevchenko) > > - Replace enum in pfru.h with __u32 as enum size is not the > > same size on all possible architectures. > > (Andy Shevchenko) > > - Simplify the userspace tool to use while loop for getopt_long(). > > (Andy Shevchenko) > > - Change from v4 to v5: > > - Remove Documentation/ABI/pfru, and move the content to kernel doc > > in include/uapi/linux/pfru.h (Greg Kroah-Hartman) > > - Shrink the range of ioctl numbers declared in > > Documentation/userspace-api/ioctl/ioctl-number.rst > > from 16 to 8. (Greg Kroah-Hartman) > > - Change global variable struct pfru_device *pfru_dev to > > per PFRU device. (Greg Kroah-Hartman) > > - Unregister the misc device in acpi_pfru_remove(). > > (Greg Kroah-Hartman) > > - Convert the kzalloc() to devm_kzalloc() in the driver so > > as to avoid freeing the memory. (Greg Kroah-Hartman) > > - Fix the compile warning by declaring the pfru_log_ioctl() as > > static. (kernel test robot LKP) > > - Change to global variable misc_device to per PFRU device. > > (Greg Kroah-Hartman) > > - Remove the telemetry output in commit log. (Greg Kroah-Hartman) > > - Add link for corresponding userspace tool in the commit log. > > (Greg Kroah-Hartman) > > - Replace the telemetry .read() with .mmap() so that the userspace > > could mmap once, and read multiple times. (Greg Kroah-Hartman) > > - Change from v3 to v4: > > - Add Documentation/ABI/testing/pfru to document the ABI and > > remove Documentation/x86/pfru.rst (Rafael J. Wysocki) > > - Replace all pr_err() with dev_dbg() (Greg Kroah-Hartman, > > Rafael J. Wysocki) > > - returns ENOTTY rather than ENOIOCTLCMD if invalid ioctl command > > is provided. (Greg Kroah-Hartman) > > - Remove compat ioctl. (Greg Kroah-Hartman) > > - Rename /dev/pfru/pfru_update to /dev/acpi_pfru (Greg Kroah-Hartman) > > - Simplify the check for element of the package in query_capability() > > (Rafael J. Wysocki) > > - Remove the loop in query_capability(), query_buffer() and query > > the package elemenet directly. (Rafael J. Wysocki) > > - Check the number of elements in case the number of package > > elements is too small. (Rafael J. Wysocki) > > - Doing the assignment as initialization in get_image_type(). > > Meanwhile, returns the type or a negative error code in > > get_image_type(). (Rafael J. Wysocki) > > - Put the comments inside the function. (Rafael J. Wysocki) > > - Returns the size or a negative error code in adjust_efi_size() > > (Rafael J. Wysocki) > > - Fix the return value from EFAULT to EINVAL if pfru_valid_revid() > > does not pass. (Rafael J. Wysocki) > > - Change the write() to be the code injection/update, the read() to > > be telemetry retrieval and all of the rest to be ioctl()s under > > one special device file.(Rafael J. Wysocki) > > - Remove redundant parens. (Rafael J. Wysocki) > > - Putting empty code lines after an if () statement that is not > > followed by a block. (Rafael J. Wysocki) > > - Remove "goto" tags to make the code more readable. (Rafael J. Wysocki) > > - Change from v2 to v3: > > - Use valid types for structures that cross the user/kernel boundary > > in the uapi header. (Greg Kroah-Hartman) > > - Rename the structure in uapi to start with a prefix pfru so as > > to avoid confusing in the global namespace. (Greg Kroah-Hartman) > > - Change from v1 to v2: > > - Add a spot in index.rst so it becomes part of the docs build > > (Jonathan Corbet). > > - Sticking to the 80-column limit(Jonathan Corbet). > > - Underline lengths should match the title text(Jonathan Corbet). > > - Use literal blocks for the code samples(Jonathan Corbet). > > - Add sanity check for duplicated instance of ACPI device. > > - Update the driver to work with allocated pfru_device objects. > > (Mike Rapoport) > > - For each switch case pair, get rid of the magic case numbers > > and add a default clause with the error handling.(Mike Rapoport) > > - Move the obj->type checks outside the switch to reduce redundancy. > > (Mike Rapoport) > > - Parse the code_inj_id and drv_update_id at driver initialization time > > to reduce the re-parsing at runtime. (Mike Rapoport) > > - Explain in detail how the size needs to be adjusted when doing > > version check. (Mike Rapoport) > > - Rename parse_update_result() to dump_update_result() > > (Mike Rapoport) > > - Remove redundant return.(Mike Rapoport) > > - Do not expose struct capsulate_buf_info to uapi, since it is > > not needed in userspace. (Mike Rapoport) > > - Do not allow non-root user to run this test.(Shuah Khan) > > - Test runs on platform without pfru_telemetry should skip > > instead of reporting failure/error.(Shuah Khan) > > - Reuse uapi/linux/pfru.h instead of copying it into the test > > directory. (Mike Rapoport) > > > > Chen Yu (4): > > efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and > > corresponding structures > > drivers/acpi: Introduce Platform Firmware Runtime Update device driver > > drivers/acpi: Introduce Platform Firmware Runtime Telemetry > > tools: Introduce power/acpi/tools/pfrut > > > > .../userspace-api/ioctl/ioctl-number.rst | 1 + > > drivers/acpi/Kconfig | 22 + > > drivers/acpi/Makefile | 1 + > > drivers/acpi/pfr_telemetry.c | 434 +++++++++++++ > > drivers/acpi/pfr_update.c | 575 ++++++++++++++++++ > > include/linux/efi.h | 46 ++ > > include/uapi/linux/pfrut.h | 262 ++++++++ > > tools/power/acpi/.gitignore | 1 + > > tools/power/acpi/Makefile | 16 +- > > tools/power/acpi/Makefile.rules | 2 +- > > tools/power/acpi/man/pfrut.8 | 137 +++++ > > tools/power/acpi/tools/pfrut/Makefile | 23 + > > tools/power/acpi/tools/pfrut/pfrut.c | 424 +++++++++++++ > > 13 files changed, 1935 insertions(+), 9 deletions(-) > > create mode 100644 drivers/acpi/pfr_telemetry.c > > create mode 100644 drivers/acpi/pfr_update.c > > create mode 100644 include/uapi/linux/pfrut.h > > create mode 100644 tools/power/acpi/man/pfrut.8 > > create mode 100644 tools/power/acpi/tools/pfrut/Makefile > > create mode 100644 tools/power/acpi/tools/pfrut/pfrut.c > > > > -- > > I have one comment regarding the second patch (I'll send it > separately), but generally I'm inclined to pick up the patches in > their current form, barring any objections from Greg. > Ok, thanks! Thanks, Chenyu > Thanks!
