On Sat, Oct 23, 2021 at 01:09:51AM +0800, Chen Yu wrote: > Introduce the pfru_update driver which can be used for Platform Firmware > Runtime code injection and driver update[1]. The user is expected to > provide the update firmware in the form of capsule file, and pass it to > the driver via ioctl. Then the driver would hand this capsule file to the > Platform Firmware Runtime Update via the ACPI device _DSM method. At last > the low level Management Mode would do the firmware update. > > The corresponding userspace tool and man page will be introduced at > tools/power/acpi/pfru. > > [1] https://uefi.org/sites/default/files/resources/Intel_MM_OS_Interface_Spec_Rev100.pdf Instead use Link: tag ? ... > +enum cap_index { > + CAP_STATUS_IDX, > + CAP_UPDATE_IDX, > + CAP_CODE_TYPE_IDX, > + CAP_FW_VER_IDX, > + CAP_CODE_RT_VER_IDX, > + CAP_DRV_TYPE_IDX, > + CAP_DRV_RT_VER_IDX, > + CAP_DRV_SVN_IDX, > + CAP_PLAT_ID_IDX, > + CAP_OEM_ID_IDX, > + CAP_OEM_INFO_IDX, > + CAP_NR_IDX, Is it terminator entry? Drop comma then. Same for all other similar cases. > +}; ... > +static int get_image_type(efi_manage_capsule_image_header_t *img_hdr, > + struct pfru_device *pfru_dev) > +{ > + guid_t *image_type_id = &img_hdr->image_type_id; > + > + /* check whether this is a code injection or driver update */ > + if (guid_equal(image_type_id, &pfru_dev->code_uuid)) > + return CODE_INJECT_TYPE; > + else if (guid_equal(image_type_id, &pfru_dev->drv_uuid)) > + return DRIVER_UPDATE_TYPE; > + else > + return -EINVAL; In both cases redundant 'else'. > +} ... > +static int adjust_efi_size(efi_manage_capsule_image_header_t *img_hdr, > + int size) > +{ > + /* > + * The (u64 hw_ins) was introduced in UEFI spec version 2, > + * and (u64 capsule_support) was introduced in version 3. > + * The size needs to be adjusted accordingly. That is to > + * say, version 1 should subtract the size of hw_ins+capsule_support, > + * and version 2 should sbstract the size of capsule_support. > + */ > + size += sizeof(efi_manage_capsule_image_header_t); > + switch (img_hdr->ver) { > + case 1: > + size -= 2 * sizeof(u64); > + break; > + case 2: > + size -= sizeof(u64); > + break; > + default: > + /* only support version 1 and 2 */ > + return -EINVAL; > + } > + return size; Perhaps directly return this from the cases? > +} ... > +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > +{ > + struct pfru_update_cap_info cap; > + struct pfru_device *pfru_dev; > + void __user *p; > + int ret = 0, rev; return 0; (see below)? > + pfru_dev = to_pfru_dev(file); > + p = (void __user *)arg; > + > + switch (cmd) { > + case PFRU_IOC_QUERY_CAP: > + ret = query_capability(&cap, pfru_dev); > + if (ret) > + return ret; > + > + if (copy_to_user(p, &cap, sizeof(cap))) > + return -EFAULT; > + > + break; > + case PFRU_IOC_SET_REV: > + if (copy_from_user(&rev, p, sizeof(unsigned int))) > + return -EFAULT; > + > + if (!pfru_valid_revid(rev)) > + return -EINVAL; > + > + pfru_dev->rev_id = rev; > + break; > + case PFRU_IOC_STAGE: > + ret = start_acpi_update(START_STAGE, pfru_dev); > + break; > + case PFRU_IOC_ACTIVATE: > + ret = start_acpi_update(START_ACTIVATE, pfru_dev); > + break; > + case PFRU_IOC_STAGE_ACTIVATE: > + ret = start_acpi_update(START_STAGE_ACTIVATE, pfru_dev); > + break; > + default: > + ret = -ENOTTY; > + break; > + } > + return ret; You may return 0 here and directly return from the cases above. > +} ... > + /* map the communication buffer */ Comment style inconsistency. Compare to... > + /* Check if the capsule header has a valid version number. */ ...this one. However, here, since it's one line, no period is needed. ... > +out: > + devm_kfree(&pdev->dev, pfru_dev); What is this? WHy?! > +} ... > +static int __init pfru_init(void) > +{ > + return platform_driver_register(&acpi_pfru_driver); > +} > + > +static void __exit pfru_exit(void) > +{ > + platform_driver_unregister(&acpi_pfru_driver); > +} > + > +module_init(pfru_init); > +module_exit(pfru_exit); NIH module_platform_driver(). ... > +#include <linux/uuid.h> This is wrong. Please, use raw buffers. ... Missed types.h. ... > +struct pfru_payload_hdr { > + __u32 sig; > + __u32 hdr_version; > + __u32 hdr_size; > + __u32 hw_ver; > + __u32 rt_ver; > + uuid_t platform_id; No way. Use __u8[16]. uuid_t is internal type for kernel. > +}; ... > +struct pfru_update_cap_info { > + enum pfru_dsm_status status; > + __u32 update_cap; > + > + uuid_t code_type; Ditto. > + __u32 fw_version; > + __u32 code_rt_version; > + uuid_t drv_type; Ditto. > + __u32 drv_rt_version; > + __u32 drv_svn; > + uuid_t platform_id; > + uuid_t oem_id; Ditto. > + char oem_info[]; > +}; ... > +struct pfru_updated_result { > + enum pfru_dsm_status status; > + enum pfru_dsm_status ext_status; What?! Are you sure enum is the same size on all possible architectures? > + __u64 low_auth_time; > + __u64 high_auth_time; > + __u64 low_exec_time; > + __u64 high_exec_time; > +}; -- With Best Regards, Andy Shevchenko
