Message-ID: <20211005042453.GC7134@chenyu5-mobl1>
Date:   Tue, 5 Oct 2021 12:24:53 +0800
From:   Chen Yu <yu.c.chen@...el.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Len Brown <len.brown@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Aubrey Li <aubrey.li@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Mike Rapoport <rppt@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Ben Widawsky <ben.widawsky@...el.com>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime
 Update device driver

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@...el.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

Powered by blists - more mailing lists