[PATCH v8 02/13] FWU: Add FWU metadata structure and driver for accessing metadata

[Simon Glass]

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)

[..]

> > > +/**
> > > + * 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