Re: [PATCH v10 06/15] FWU: Add helper functions for accessing FWU metadata

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

[...]

> > > +static int fwu_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,
> > > +                            u8 *alt_num, unsigned char dfu_dev)
> > > +{
> > > +     int ret = -1;
> > > +     int i, part, dev_num;
> > > +     int nalt;
> > > +     struct dfu_entity *dfu;
> > > +
> > > +     dev_num = desc->devnum;
> > > +     part = get_gpt_dfu_identifier(desc, image_guid);
> > > +     if (part < 0)
> > > +             return -ENOENT;
> > > +
> > > +     dfu_init_env_entities(NULL, NULL);
> > > +
> > > +     nalt = 0;
> > > +     list_for_each_entry(dfu, &dfu_list, list)
> > > +             nalt++;
> > > +
> > > +     if (!nalt) {
> > > +             log_warning("No entities in dfu_alt_info\n");
> > > +             dfu_free_entities();
> > > +             return -ENOENT;
> > > +     }
> > > +
> > > +     for (i = 0; i < nalt; i++) {
> > > +             dfu = dfu_get_entity(i);
> > > +
> > > +             if (!dfu)
> > > +                     continue;
> > > +
> > > +             /*
> > > +              * Currently, Multi Bank update
> > > +              * feature is being supported
> > > +              * only on GPT partitioned
> > > +              * MMC/SD devices.
> > > +              */
> > > +             if (dfu->dev_type != dfu_dev)
> > > +                     continue;
> > > +
> > > +             if (dfu->layout == DFU_RAW_ADDR &&
> > > +                 dfu->data.mmc.dev_num == dev_num &&
> > > +                 dfu->data.mmc.part == part) {
> > > +                     *alt_num = dfu->alt;
> > > +                     ret = 0;
> > > +                     break;
> >
> > I get that we only currently support it on mmc, but the if above is not
> > going to scale as we add devices.  Is there something better we  can come
> > up with? Probably a helper in the dfu layer?
> 
> Currently, the DFU supports only the mmc which will be a GPT
> partitioned device. But yes, yours is a valid point in terms of
> scalability. However, I am wondering if this check should instead be
> in the board file. Every platform would know which is the device being
> used for storing the firmware images. Will this check not be more apt
> in a board function?

Isn't this supposed to come from the dfu_alt_info in the command line?
IOW What you are trying to prevent here is a user misconfiguration?

Thanks
/Ilias
> 
> -sughosh
> 
> >
> > Cheers
> > /Ilias
> >
> >
> > > +             }
> > > +     }
> > > +
> > > +     dfu_free_entities();
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +/**
> > > + * fwu_plat_get_alt_num() - Get the DFU alt number
> > > + * @dev: FWU metadata device
> > > + * @image_guid: GUID value of the image for which the alt num is to
> > > + *              be obtained
> > > + * @alt_num: The DFU alt number for the image that is to be updated
> > > + *
> > > + * Get the DFU alt number for the image that is to be updated. The
> > > + * image is identified with the image_guid parameter that is passed
> > > + * to the function.
> > > + *
> > > + * Note: This is a weak function and platforms can override this with
> > > + * their own implementation for obtaining the alt number value.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + *
> > > + */
> > > +__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> > > +                             u8 *alt_num)
> > > +{
> > > +     struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> > > +
> > > +     return fwu_gpt_get_alt_num(dev_get_uclass_plat(priv->blk_dev),
> > > +                                image_guid, alt_num, DFU_DEV_MMC);
> > > +}
> > > --
> > > 2.34.1
> > >