Hi Andy, On Fri, Oct 22, 2021 at 11:05:55PM +0300, Andy Shevchenko wrote: > 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 ? > Yes, will fix it. > > +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. > Ok, will remove the comma. > > +}; > > ... > > > +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'. > Ok. > > +} > > ... > > > +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? > Ok, will do. > > +} > > ... > > > +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. > I think we can remove the 'return ret' completely, by returning 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. > > Ok, will fix it. > ... > > > +out: > > + devm_kfree(&pdev->dev, pfru_dev); > > What is this? WHy?! > The devm resource is expected to be released during driver de-attach. devm_kfree() is used to free the resource explicitly at the earliest time. But it is ok to remove the devm_kfree() and leverage the devm to deal with it. > > +} > > ... > > > > +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(). > There would be more than one platform driver to be loaded in this file, so module_platform_driver() might not work here. > ... > > > +#include <linux/uuid.h> > > This is wrong. Please, use raw buffers. > Ok, will remove this. > ... > > Missed types.h. > Ok, will add it. > ... > > > +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. > Ok, will change it. > > +}; > > ... > > > +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? > Ok, changed then to __u32 in next version. thanks, Chenyu
