On Wed, Oct 27, 2021 at 03:07:51PM +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.
...
> +static int get_image_type(struct efi_manage_capsule_image_header *img_hdr,
> + struct pfru_device *pfru_dev)
> +{
> + guid_t *image_type_id = &img_hdr->image_type_id;
efi_guid_t ?
> + /* check whether this is a code injection or driver update */
> + if (guid_equal(image_type_id, &pfru_dev->code_uuid))
> + return CODE_INJECT_TYPE;
> +
> + if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
> + return DRIVER_UPDATE_TYPE;
> +
> + return -EINVAL;
> +}
...
> +static bool valid_version(const void *data, struct pfru_update_cap_info *cap,
> + struct pfru_device *pfru_dev)
> +{
> + struct pfru_payload_hdr *payload_hdr;
> + efi_capsule_header_t *cap_hdr;
> + struct efi_manage_capsule_header *m_hdr;
> + struct efi_manage_capsule_image_header *m_img_hdr;
> + struct efi_image_auth *auth;
> + int type, size;
> +
> + /*
> + * Sanity check if the capsule image has a newer version
> + * than current one.
> + */
> + cap_hdr = (efi_capsule_header_t *)data;
Why casting?
> + size = cap_hdr->headersize;
> + m_hdr = (struct efi_manage_capsule_header *)(data + size);
> + /*
> + * Current data structure size plus variable array indicated
> + * by number of (emb_drv_cnt + payload_cnt)
> + */
> + size += sizeof(struct efi_manage_capsule_header) +
> + (m_hdr->emb_drv_cnt + m_hdr->payload_cnt) * sizeof(u64);
> + m_img_hdr = (struct efi_manage_capsule_image_header *)(data + size);
> +
> + type = get_image_type(m_img_hdr, pfru_dev);
> + if (type < 0)
> + return false;
> +
> + size = adjust_efi_size(m_img_hdr, size);
> + if (size < 0)
> + return false;
> +
> + auth = (struct efi_image_auth *)(data + size);
> + size += sizeof(u64) + auth->auth_info.hdr.len;
> + payload_hdr = (struct pfru_payload_hdr *)(data + size);
> +
> + /* finally compare the version */
> + if (type == CODE_INJECT_TYPE)
> + return payload_hdr->rt_ver >= cap->code_rt_version;
> + else
> + return payload_hdr->rt_ver >= cap->drv_rt_version;
> +}
...
> +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, rev;
> + pfru_dev = to_pfru_dev(file);
> + p = (void __user *)arg;
Can be combined with definitions above. Ditto for the rest cases in the code.
> +}
...
> + phy_addr = (phys_addr_t)(info.addr_lo | (info.addr_hi << 32));
Does it compile without warnings for 32-bit target?
...
> + ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
> + if (ret)
> + return ret;
> +
> + ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
> + if (ret)
> + return ret;
> +
> + ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);
> + if (ret)
> + return ret;
Why do you need to keep zillions of copies of the data which seems
is not going to be changed? Three global variables should be enough,
no?
> + ret = ida_alloc(&pfru_ida, GFP_KERNEL);
> + if (ret < 0)
> + return ret;
> +
> + pfru_dev->index = ret;
...
> + /* default rev id is 1 */
Shouldn't you rather define this magic and drop this doubtful comment?
> + pfru_dev->rev_id = 1;
...
> +failed:
Make you labeling consistent. The usual pattern is to explain what will be
happened when goto to the certain label, for example, here is 'err_free_ida'.
Also, add an empty line everywhere before labels.
> + ida_free(&pfru_ida, pfru_dev->index);
> +
> + return ret;
> +}
...
> +#define UUID_SIZE 16
It must not be here at all.
Or it should be properly namespaced.
--
With Best Regards,
Andy Shevchenko
