From: Chen Yu <yu.c.chen@intel.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Len Brown <len.brown@intel.com>,
Dan Williams <dan.j.williams@intel.com>,
Andy Shevchenko <andriy.shevchenko@intel.com>,
Aubrey Li <aubrey.li@intel.com>, Ashok Raj <ashok.raj@intel.com>,
Mike Rapoport <rppt@kernel.org>, Ard Biesheuvel <ardb@kernel.org>,
Jonathan Corbet <corbet@lwn.net>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Hans de Goede <hdegoede@redhat.com>,
Ben Widawsky <ben.widawsky@intel.com>,
"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
Date: Tue, 5 Oct 2021 12:24:53 +0800 [thread overview]
Message-ID: <20211005042453.GC7134@chenyu5-mobl1> (raw)
In-Reply-To: <CAJZ5v0i__g02rjQh5TuBQN2HejEbZUxiy-4=YSWDUEczrTui0Q@mail.gmail.com>
On Tue, Sep 28, 2021 at 02:22:28PM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 16, 2021 at 5:56 PM Chen Yu <yu.c.chen@intel.com> wrote:
[snip]
> > +static bool valid_cap_type(int idx, union acpi_object *obj)
> > +{
> > + acpi_object_type type = obj->type;
> > +
> > + if (idx == CAP_STATUS_IDX || idx == CAP_UPDATE_IDX ||
> > + idx == CAP_FW_VER_IDX || idx == CAP_CODE_RT_VER_IDX ||
> > + idx == CAP_DRV_RT_VER_IDX || idx == CAP_DRV_SVN_IDX)
> > + return type == ACPI_TYPE_INTEGER;
> > + else if (idx == CAP_CODE_TYPE_IDX || idx == CAP_DRV_TYPE_IDX ||
> > + idx == CAP_PLAT_ID_IDX || idx == CAP_OEM_ID_IDX ||
> > + idx == CAP_OEM_INFO_IDX)
> > + return type == ACPI_TYPE_BUFFER;
> > + else
> > + return false;
> > +}
>
> IMO, this is way overdesigned. It is not necessary to do all of these
> checks for every element of the package in query_capability().
>
[snip]
> It is not very useful to do a loop if there is a switch () on the
> control variable in every step.
>
> > + union acpi_object *obj = &out_obj->package.elements[i];
> > +
> > + if (!valid_cap_type(i, obj))
> > + goto free_acpi_buffer;
> > +
> > + switch (i) {
> > + case CAP_STATUS_IDX:
> > + cap->status = obj->integer.value;
> > + break;
>
> The above can be written as
>
> if (out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER)
> goto free_acpi_buffer;
>
> cap->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
>
> and analogously for all of the other index values.
>
> But check the number of elements upfront.
>
Ok, got it. The next version will combine above together, and eliminate
the loop, switch logic.
> > + case CAP_UPDATE_IDX:
> > + cap->update_cap = obj->integer.value;
> > + break;
> > + case CAP_CODE_TYPE_IDX:
> > + memcpy(&cap->code_type, obj->buffer.pointer,
> > + obj->buffer.length);
> > + break;
> > + case CAP_FW_VER_IDX:
> > + cap->fw_version = obj->integer.value;
> > + break;
> > + case CAP_CODE_RT_VER_IDX:
> > + cap->code_rt_version = obj->integer.value;
> > + break;
> > + case CAP_DRV_TYPE_IDX:
> > + memcpy(&cap->drv_type, obj->buffer.pointer,
> > + obj->buffer.length);
> > + break;
> > + case CAP_DRV_RT_VER_IDX:
> > + cap->drv_rt_version = obj->integer.value;
> > + break;
> > + case CAP_DRV_SVN_IDX:
> > + cap->drv_svn = obj->integer.value;
> > + break;
> > + case CAP_PLAT_ID_IDX:
> > + memcpy(&cap->platform_id, obj->buffer.pointer,
> > + obj->buffer.length);
> > + break;
> > + case CAP_OEM_ID_IDX:
> > + memcpy(&cap->oem_id, obj->buffer.pointer,
> > + obj->buffer.length);
> > + break;
> > + case CAP_OEM_INFO_IDX:
> > + /*vendor specific data*/
> > + break;
> > + default:
> > + pr_err("Incorrect format of Update Capability.\n");
>
> Why pr_err() and not dev_dbg()?
>
Will change to dev_dbg() in next version.
> Besides, it looks like you're going to fail if the package has more
> elements than expected, but is this really a big deal?
>
> Moreover, what if the number of package elements is too small?
>
Ok, will change it to check if the number of package elements is larger/equals to
expected in the spec.
> > + goto free_acpi_buffer;
> > + }
> > + }
> > + ret = 0;
> > +
> > +free_acpi_buffer:
> > + ACPI_FREE(out_obj);
> > +
> > + return ret;
> > +}
> > +
> > +static int query_buffer(struct pfru_com_buf_info *info)
> > +{
> > + union acpi_object *out_obj;
> > + acpi_handle handle;
> > + int i, ret = -EINVAL;
> > +
> > + handle = ACPI_HANDLE(pfru_dev->dev);
> > + out_obj = acpi_evaluate_dsm_typed(handle, &pfru_dev->uuid,
> > + pfru_dev->rev_id, FUNC_QUERY_BUF,
> > + NULL, ACPI_TYPE_PACKAGE);
> > + if (!out_obj)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < out_obj->package.count; i++) {
>
> Again, what is the benefit from doing this loop?
>
Will eliminate the loop in next version.
[snip]
> > +
> > +static int get_image_type(efi_manage_capsule_image_header_t *img_hdr,
> > + int *type)
> > +{
> > + guid_t *image_type_id;
> > +
> > + /* check whether this is a code injection or driver update */
> > + image_type_id = &img_hdr->image_type_id;
>
> Anything wrong with doing this assignment as initialization?
>
Ok, will change it in next version.
> > + if (guid_equal(image_type_id, &pfru_dev->code_uuid))
> > + *type = CODE_INJECT_TYPE;
> > + else if (guid_equal(image_type_id, &pfru_dev->drv_uuid))
> > + *type = DRIVER_UPDATE_TYPE;
>
> And what would be wrong with returning the type or a negative error code?
>
Will change it in next version.
> > + else
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * 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.
>
> Either turn this into a kerneldoc comment or put it inside the function.
>
Will put it inside the function.
> > + */
> > +static int adjust_efi_size(efi_manage_capsule_image_header_t *img_hdr,
> > + int *size)
> > +{
> > + int tmp_size = *size;
> > +
> > + tmp_size += sizeof(efi_manage_capsule_image_header_t);
> > + switch (img_hdr->ver) {
> > + case 1:
> > + tmp_size -= 2 * sizeof(u64);
> > + break;
> > + case 2:
> > + tmp_size -= sizeof(u64);
> > + break;
> > + default:
> > + /* only support version 1 and 2 */
> > + return -EINVAL;
> > + }
> > + *size = tmp_size;
>
> Why not simply return the size or a negative error code?
>
Will change it in next version.
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Sanity check if the capsule image has a newer version than current one.
> > + * Return: true if it is valid, false otherwise.
>
> Would it hurt if this was a proper kerneldoc comment?
>
Will put it inside the function.
[snip]
> > +
> > +static void dump_update_result(struct pfru_updated_result *result)
> > +{
> > + pr_debug("Update result:\n");
> > + pr_debug("Status:%d\n", result->status);
> > + pr_debug("Extended Status:%d\n", result->ext_status);
> > + pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
> > + pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
> > + pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
> > + pr_debug("Execution Time High:%lld\n", result->high_exec_time);
>
> All of these could be dev_dbg() I suppose?
>
Ok, will change it in next version.
> > +}
> > +
> > +
[snip]
> > + for (i = 0; i < out_obj->package.count; i++) {
>
> Same comment regarding the benefit of doing a loop: why is it needed?
>
Will remove the loop in next version.
[snip]
> > +
> > + switch (cmd) {
> > + case PFRU_IOC_SET_REV:
> > + if (copy_from_user(&rev, p, sizeof(unsigned int)))
> > + return -EFAULT;
> > + if (!pfru_valid_revid(rev))
> > + return -EFAULT;
>
> Why is this the right error code to return here?
>
Will change EFAULT to EINVAL in next version.
[snip]
> > + /* map the communication buffer */
> > + phy_addr = (phys_addr_t)(info.addr_lo | (info.addr_hi << 32));
> > + buf_ptr = memremap(phy_addr, info.buf_size, MEMREMAP_WB);
> > + if (IS_ERR(buf_ptr))
> > + return PTR_ERR(buf_ptr);
>
> Empty line here, please.
>
Ok, will do.
> > + if (!copy_from_iter_full(buf_ptr, len, &iter)) {
> > + pr_err("error! could not read capsule file\n");
>
> dev_dbg()?
>
Ok, will change it in next version.
> > + ret = -EINVAL;
> > + goto unmap;
> > + }
> > +
> > + /* Check if the capsule header has a valid version number. */
> > + ret = query_capability(&cap);
> > + if (ret)
> > + goto unmap;
>
> ret is guaranteed to be 0 here, so you can do
>
> if (cap.status != DSM_SUCCEED)
> ret = -EBUSY;
> else if (!valid_version(buf_ptr, &cap))
> ret = -EINVAL;
>
> and the gotos and the "ret = 0" statement below won't be necessary.
>
Ok, will do in next version.
[snip]
> > +static ssize_t pfru_read(struct file *filp, char __user *ubuf,
> > + size_t size, loff_t *off)
> > +{
> > + struct pfru_update_cap_info cap;
> > + int ret;
> > +
> > + ret = query_capability(&cap);
> > + if (ret)
> > + return ret;
> > +
> > + size = min_t(size_t, size, sizeof(cap));
> > +
> > + if (copy_to_user(ubuf, &cap, size))
> > + return -EFAULT;
>
> Well, if the read() is only needed for this, maybe consider
> implementing it as an ioctl() command and using read() for the
> telemetry retrieval? Then, you won't need the other special device
> file, the write() will be the code injection/update, the read() will
> be telemetry retrieval and all of the rest can be ioctl()s under one
> special device file.
>
Got it, will try to combine the two modules into one.
thanks,
Chenyu
next prev parent reply other threads:[~2021-10-05 4:25 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-16 15:53 [PATCH v3 0/5] Introduce Platform Firmware Runtime Update and Telemetry drivers Chen Yu
2021-09-16 15:58 ` [PATCH v3 1/5] Documentation: Introduce Platform Firmware Runtime Update documentation Chen Yu
2021-09-27 17:26 ` Rafael J. Wysocki
2021-09-16 16:00 ` [PATCH v3 2/5] efi: Introduce EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER and corresponding structures Chen Yu
2021-09-27 17:33 ` Rafael J. Wysocki
2021-09-16 16:02 ` [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver Chen Yu
2021-09-21 15:59 ` Greg Kroah-Hartman
2021-09-22 9:04 ` Chen Yu
2021-09-22 9:10 ` Greg Kroah-Hartman
2021-09-22 16:33 ` Chen Yu
2021-09-22 17:28 ` Greg Kroah-Hartman
2021-09-27 17:40 ` Rafael J. Wysocki
2021-10-05 4:08 ` Chen Yu
2021-10-05 4:12 ` Chen Yu
2021-09-28 12:22 ` Rafael J. Wysocki
2021-10-05 4:24 ` Chen Yu [this message]
2021-09-16 16:03 ` [PATCH v3 4/5] drivers/acpi: Introduce Platform Firmware Runtime Update Telemetry Chen Yu
2021-09-28 12:40 ` Rafael J. Wysocki
2021-09-16 16:04 ` [PATCH v3 5/5] selftests/pfru: add test for Platform Firmware Runtime Update and Telemetry Chen Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211005042453.GC7134@chenyu5-mobl1 \
--to=yu.c.chen@intel.com \
--cc=andriy.shevchenko@intel.com \
--cc=ardb@kernel.org \
--cc=ashok.raj@intel.com \
--cc=aubrey.li@intel.com \
--cc=ben.widawsky@intel.com \
--cc=corbet@lwn.net \
--cc=dan.j.williams@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=hdegoede@redhat.com \
--cc=len.brown@intel.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rppt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).