lkml.org 
[lkml]   [2021]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
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


\
 
 \ /
  Last update: 2021-10-22 22:06    [W:1.327 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site