[PATCH v8 02/13] FWU: Add FWU metadata structure and driver for accessing metadata
Sughosh Ganu
sughosh.ganu at linaro.org
Fri Aug 19 15:36:15 CEST 2022
hi Simon,
On Thu, 18 Aug 2022 at 23:20, Simon Glass <sjg at chromium.org> wrote:
>
> pHi Sughosh,
>
> On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Wed, 17 Aug 2022 at 06:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > In the FWU Multi Bank Update feature, the information about the
> > > > updatable images is stored as part of the metadata, which is stored on
> > > > a dedicated partition. Add the metadata structure, and a driver model
> > > > uclass which provides functions to access the metadata. These are
> > > > generic API's, and implementations can be added based on parameters
> > > > like how the metadata partition is accessed and what type of storage
> > > > device houses the metadata.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > > > ---
> > > > Changes since V7:
> > > > * Rephrased the error message in fwu_update_active_index as per
> > > > suggestion from Ilias.
> > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion
> > > > from Ilias.
> > > >
> > > > drivers/Kconfig | 2 +
> > > > drivers/Makefile | 1 +
> > > > drivers/fwu-mdata/Kconfig | 7 +
> > > > drivers/fwu-mdata/Makefile | 6 +
> > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++
> > > > include/dm/uclass-id.h | 1 +
> > > > include/fwu.h | 49 +++
> > > > include/fwu_mdata.h | 67 ++++
> > > > 8 files changed, 596 insertions(+)
> > > > create mode 100644 drivers/fwu-mdata/Kconfig
> > > > create mode 100644 drivers/fwu-mdata/Makefile
> > > > create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
> > > > create mode 100644 include/fwu.h
> > > > create mode 100644 include/fwu_mdata.h
> > > >
> > > > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > > > index 8b6fead351..75ac149d31 100644
> > > > --- a/drivers/Kconfig
> > > > +++ b/drivers/Kconfig
> > > > @@ -44,6 +44,8 @@ source "drivers/fuzz/Kconfig"
> > > >
> > > > source "drivers/fpga/Kconfig"
> > > >
> > > > +source "drivers/fwu-mdata/Kconfig"
> > > > +
> > > > source "drivers/gpio/Kconfig"
> > > >
> > > > source "drivers/hwspinlock/Kconfig"
>
> [..]
>
> > > > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
> > > > +{
> > > > + u32 calc_crc32;
> > > > + void *buf;
> > > > +
> > > > + buf = &mdata->version;
> > > > + calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > > > +
> > > > + if (calc_crc32 != mdata->crc32) {
> > > > + log_err("crc32 check failed for %s FWU metadata partition\n",
> > > > + pri_part ? "primary" : "secondary");
> > > > + return -1;
> > >
> > > Please use an -Exxx value like -EPERM
> >
> > I don't think there is any value which relates to a crc mismatch. If
> > you insist, I can think of -EIO, but I don't think -EPERM is a good
> > match in this case.
>
> See my comment in the other patch. You should not be printing messages in a uclass or driver, except in extreme situations.
>
> Choose a value that is somewhat meaningful so you can report the error sensibly in top-level command code.
>
> >
> > >
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > > > + * @active_idx: active_index value to be read
> > > > + *
> > > > + * Read the active_index field from the FWU metadata and place it in
> > > > + * the variable pointed to be the function argument.
> > > > + *
> > > > + * Return: 0 if OK, -ve on error
> > > > + *
> > > > + */
> > > > +int fwu_get_active_index(u32 *active_idx)
> > >
> > > Return the active index rather than having an arg?
> >
> > I would prefer to keep it the way it is. That allows returning an
> > error value and keep the active_index separate. Moreover it is just a
> > single parameter.
>
> OK, then can you use active_idxp for the name, as is generally done in driver model?
>
> >
> > >
> > > > +{
> > > > + int ret;
> > > > + struct fwu_mdata *mdata = NULL;
> > > > +
> > > > + ret = fwu_get_mdata(&mdata);
> > >
> > > Can you use a local var and avoid the malloc() / free() ?
> >
> > Do you see any disadvantages of using space on the heap? If you don't
> > have a strong opinion on this, I would prefer to keep it as is.
>
> Yes, this is not UEFI and we try to avoid allocating memory to no purpose. It takes time and fragments the heap.
>
> >
> > >
> > > > + if (ret < 0) {
> > > > + log_err("Unable to get valid FWU metadata\n");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Found the FWU metadata partition, now read the active_index
> > > > + * value
> > > > + */
> > > > + *active_idx = mdata->active_index;
> > > > + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) {
> > > > + log_err("Active index value read is incorrect\n");
> > > > + ret = -EINVAL;
> > > > + }
> > > > +
> > > > +out:
> > > > + free(mdata);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * fwu_update_active_index() - Update active_index from the FWU metadata
> > > > + * @active_idx: active_index value to be updated
> > > > + *
> > > > + * Update the active_index field in the FWU metadata
> > > > + *
> > > > + * Return: 0 if OK, -ve on error
> > > > + *
> > > > + */
> > > > +int fwu_update_active_index(u32 active_idx)
> > >
> > > uint
> >
> > Are you referring to the function parameter? If so, it is a u32 since
> > the active_index field in the metadata structure defined in the spec
> > is a 32 bit value.
>
> Sure, but how does that affect the function parameter? Generally we should use natural types in function parameters so we don't do silly things like force a 32-bit arg into a 64-bit register, causing the compiler to have to mask the value, etc.
>
> [..]
>
> > > > +/**
> > > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update
> > > > + * @image_type_id: pointer to the image GUID as passed in the capsule
> > > > + * @update_bank: Bank to which the update is to be made
> > > > + * @alt_num: The alt_num for the image
> > > > + *
> > > > + * Based on the GUID value passed in the capsule, along with the bank to which the
> > > > + * image needs to be updated, get the dfu alt number which will be used for the
> > > > + * capsule update
> > > > + *
> > > > + * Return: 0 if OK, -ve on error
> > > > + *
> > > > + */
> > > > +int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> > > > + int *alt_num)
> > >
> > > alt_nump is often used with driver model to indicate a return
> > > value.Again I wonder if the return value could be used?
> >
> > Can you point me to an example? I could not find any instance of this.
>
> Where did you look? Just to take one example, returning a device:
>
> git grep -w devp drivers/ |wc -l
> 316
>
> > Again, I believe this is a matter of taste, so I would prefer the
> > current method.
>
> Please fit into the uclass / driver model code style.
>
> >
> > >
> > > > +{
> > > > + int ret, i;
> > > > + efi_guid_t *image_guid;
> > > > + struct udevice *dev = NULL;
> > > > + struct fwu_mdata *mdata = NULL;
> > >
> > > Please don't set things to NULL for no reason, it cluttlers the code.
> >
> > This is being set to NULL in case the called function that is supposed
> > to populate the metadata structure fails to do so. We can pass NULL to
> > free, else it could result in freeing up of unrelated memory.
>
> If the function fails to set it, then the compiler will complain.
>
> I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering?
>
> >
> > >
> > > > + struct fwu_image_entry *img_entry;
> > > > + const struct fwu_mdata_ops *ops = NULL;
> > > > + struct fwu_image_bank_info *img_bank_info;
> > > > +
> > > > + ret = fwu_get_dev_ops(&dev, &ops);
> > >
> > > Rather than this, see the pattern used by other uclasses
> >
> > Can you point me to some example code. Also, not sure what the issue
> > is with using this function.
>
> It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch.
>
> int clk_request(struct udevice *dev, struct clk *clk)
> {
> const struct clk_ops *ops;
>
> ...
> ops = clk_dev_ops(dev);
>
> (where that is defined in the header file)
If you mean clk_dev_ops, it is defined in clk-uclass.c, and IMO that
makes sense, since it is being called multiple times in the driver.
Similarly, the fwu_get_dev_ops is being called multiple times in the
uclass driver, so I think it makes sense to have it as a function. If
you want me to move it to a header file or rename it, I can do that.
-sughosh
>
> [..]
>
> > > > +/**
> > > > + * struct fwu_image_bank_info - firmware image information
> > > > + * @image_uuid: Guid value of the image in this bank
> > > > + * @accepted: Acceptance status of the image
> > > > + * @reserved: Reserved
> > > > + *
> > > > + * The structure contains image specific fields which are
> > > > + * used to identify the image and to specify the image's
> > > > + * acceptance status
> > > > + */
> > > > +struct fwu_image_bank_info {
> > > > + efi_guid_t image_uuid;
> > > > + uint32_t accepted;
> > > > + uint32_t reserved;
> > > > +} __attribute__((__packed__));
> > >
> > > Why is this packed?
> >
> > This was based on a review comment from Masami [1], as he wanted to
> > use the same structure in the low level bootloader as well.
>
> It doesn't actually make any sense though. The packed struct is the same as the normal struct, so far as I can tell. What am I missing?
>
> It does have a cost for the compiler. For example I tried adding __packed to struct image_header (which has the same issue mentioned by Masami in that it is a file header) and it added 400 bytes to snow.
>
> [..]
>
> >
> > -sughosh
> >
> > [1] - https://lists.denx.de/pipermail/u-boot/2021-December/470118.html
More information about the U-Boot
mailing list