[PATCH v4 2/6] FWU: mtd: Add helper functions for accessing FWU metadata

[Jassi Brar]

On Wed, 29 Mar 2023 at 06:55, Michal Simek <michal.simek at amd.com> wrote:
> On 3/27/23 23:16, jassisinghbrar at gmail.com wrote:


> > +/**
> > + * fwu_gen_alt_info_from_mtd() - Parse dfu_alt_info from metadata in mtd
> > + * @buf: Buffer into which the dfu_alt_info is filled
> > + * @len: Maximum characters that can be written in buf
> > + * @mtd: Pointer to underlying MTD device
> > + *
> > + * Parse dfu_alt_info from metadata in mtd. Used for setting the env.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
>
> nit: remove this line above.
>
ok

> > + */
> > +int fwu_gen_alt_info_from_mtd(char *buf, size_t len, struct mtd_info *mtd);
> > +
> > +/**
> > + * fwu_mtd_get_alt_num() - Mapping of fwu_plat_get_alt_num for MTD device
> > + * @image_guid: Image GUID for which DFU alt number needs to be retrieved
>
> it doesn't match with parameter below.
>
ok

> > + * @alt_num: Pointer to the alt_num
> > + * @mtd_dev: Name of mtd device instance
> > + *
> > + * To map fwu_plat_get_alt_num onto mtd based metadata implementation.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
>
> nit: remove this line above
>
ok

> > + */
> > +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num, const char *mtd_dev);
>
>
> ./scripts/kernel-doc -v -man include/fwu.h 1>/dev/null
>
> include/fwu.h:276: info: Scanning doc for fwu_mtd_get_alt_num
> include/fwu.h:286: warning: Function parameter or member 'image_id' not
> described in 'fwu_mtd_get_alt_num'
> include/fwu.h:286: warning: Excess function parameter 'image_guid' description
> in 'fwu_mtd_get_alt_num'
>
ok


> > +
> > +static struct fwu_mtd_image_info *mtd_img_by_uuid(const char *uuidbuf)
> > +{
> > +     int num_images = sizeof(fwu_mtd_images) / sizeof(struct fwu_mtd_image_info);
>
> include/linux/kernel.h:55:#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
of course. thanks.


> > +int fwu_mtd_get_alt_num(efi_guid_t *image_id, u8 *alt_num,
> > +                     const char *mtd_dev)
> > +{
> > +     int i, nalt;
> > +     int ret = -1;
>
> nit: can be on the same line.
>
ok

> > +     struct mtd_info *mtd;
> > +     struct dfu_entity *dfu;
> > +     fdt_addr_t offset, size = 0;
> > +     char uuidbuf[UUID_STR_LEN + 1];
> > +     struct fwu_mtd_image_info *mtd_img_info;
>
> nit: reverse christmas tree order
>
ok

> > +
> > +     mtd_probe_devices();
> > +     mtd = get_mtd_device_nm(mtd_dev);
>
> you are getting link but you are not using it anywhere.
> You should check return value or remove this call completely.
>
This should go. thanks.


> > +
> > +     uuid_bin_to_str(image_id->b, uuidbuf, UUID_STR_FORMAT_STD);
> > +
> > +     mtd_img_info = mtd_img_by_uuid(uuidbuf);
> > +     if (!mtd_img_info) {
> > +             log_err("%s: Not found partition for image %s\n", __func__, uuidbuf);
> > +             return -ENOENT;
> > +     }
>
> nit: newline
>
ok

> > +     offset = mtd_img_info->start;
> > +     size = mtd_img_info->size;
> > +
> > +     dfu_init_env_entities(NULL, NULL);
>
> worth to check return value here.
>
ok, though it would also cleanly fail immediately after this call when
it find empty dfu_list.

Thanks.