Re: [PATCH v8 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
From: Andy Shevchenko
Date: Wed Nov 03 2021 - 13:32:30 EST
On Wed, Nov 03, 2021 at 11:43:50PM +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.
...
> +#define PFRU_UUID "ECF9533B-4A3C-4E89-939E-C77112601C6D"
> +#define PFRU_CODE_INJ_UUID "B2F84B79-7B6E-4E45-885F-3FB9BB185402"
> +#define PFRU_DRV_UPDATE_UUID "4569DD8C-75F1-429A-A3D6-24DE8097A0DF"
What stops you to have these being binaries?
GUID_INIT() / EFI_GUID_INIT()
...
> +enum cap_index {
> + CAP_STATUS_IDX = 0,
> + CAP_UPDATE_IDX = 1,
> + CAP_CODE_TYPE_IDX = 2,
> + CAP_FW_VER_IDX = 3,
> + CAP_CODE_RT_VER_IDX = 4,
> + CAP_DRV_TYPE_IDX = 5,
> + CAP_DRV_RT_VER_IDX = 6,
> + CAP_DRV_SVN_IDX = 7,
> + CAP_PLAT_ID_IDX = 8,
> + CAP_OEM_ID_IDX = 9,
> + CAP_OEM_INFO_IDX = 10,
> + CAP_NR_IDX = 11
Assignment here doesn't make any sense (it just adds unneeded churn and
burden). Same to the rest of similar cases below.
> +};
...
> +struct pfru_device {
> + guid_t uuid, code_uuid, drv_uuid;
You don't need these. At least for now.
> + u32 rev_id, index;
> + struct device *parent_dev;
> + struct miscdevice miscdev;
> +};
...
> + m_hdr = (struct efi_manage_capsule_header *)(data + size);
Do you need this casting?
...
> + m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);
Ditto.
...
> + auth = (struct efi_image_auth *)(data + size);
Ditto.
...
> + ACPI_FREE(out_obj);
Recently with Hans we realised that this (ACPI_FREE() API) is mostly
for ACPICA use. We may use simple kfree(). Sorry for getting back and
forward.
...
> +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct pfru_update_cap_info cap_hdr;
> + struct pfru_device *pfru_dev = to_pfru_dev(file);
> + void __user *p = (void __user *)arg;
> + u32 rev;
> + int ret;
> +
> + switch (cmd) {
> + case PFRU_IOC_QUERY_CAP:
> + ret = query_capability(&cap_hdr, pfru_dev);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(p, &cap_hdr, sizeof(cap_hdr)))
I'm wondering what will happen if p has less _real data_ than sizeof(cap_hdr)?
> + return -EFAULT;
> +
> + return 0;
> + case PFRU_IOC_SET_REV:
> + if (copy_from_user(&rev, p, sizeof(u32)))
sizeof(rev)
> + return -EFAULT;
> +
> + if (!pfru_valid_revid(rev))
> + return -EINVAL;
> +
> + pfru_dev->rev_id = rev;
> +
> + return 0;
> + case PFRU_IOC_STAGE:
> + return start_acpi_update(START_STAGE, pfru_dev);
> + case PFRU_IOC_ACTIVATE:
> + return start_acpi_update(START_ACTIVATE, pfru_dev);
> + case PFRU_IOC_STAGE_ACTIVATE:
> + return start_acpi_update(START_STAGE_ACTIVATE, pfru_dev);
> + default:
> + return -ENOTTY;
> + }
> +}
...
> + /* map the communication buffer */
> + phy_addr = (phys_addr_t)(buf_info.addr_lo | (buf_info.addr_hi << 32));
It's better to read if you start from MSB part to LSB.
...
> + ret = ida_alloc(&pfru_ida, GFP_KERNEL);
> + if (ret < 0)
> + return ret;
(1)
...
> + pfru_dev->miscdev.name = kasprintf(GFP_KERNEL,
> + "pfru%d", pfru_dev->index);
devm_kasprinf()
...
> + pfru_dev->miscdev.nodename = kasprintf(GFP_KERNEL,
> + "acpi_pfru%d", pfru_dev->index);
Ditto.
Yep, I know about (1), but do your homework and see how you can satisfy both
comments.
...
> +static const struct acpi_device_id acpi_pfru_ids[] = {
> + {"INTC1080", 0},
0 is redundant.
> + {}
> +};
...
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
Order?
...
> +#define PFRU_MAGIC 0xEE
Perhaps PFRU_MAGIC_FOR_IOCTL.
--
With Best Regards,
Andy Shevchenko