[PATCH v5 03/23] FWU: Add FWU metadata access driver for GPT partitioned block devices
Sughosh Ganu
sughosh.ganu at linaro.org
Tue Jun 28 12:01:44 CEST 2022
hi Patrick,
Apologies for the late reply. I had missed out replying to the review
comments on this patch. There are some review comments on the
Synquacer patches which need to be taken care of by another engineer.
Once those review comments are taken care of, I will be sending the
next version.
On Tue, 21 Jun 2022 at 15:04, Patrick DELAUNAY
<patrick.delaunay at foss.st.com> wrote:
>
> Hi,
>
> On 6/9/22 14:29, Sughosh Ganu wrote:
> > In the FWU Multi Bank Update feature, the information about the
> > updatable images is stored as part of the metadata, on a separate
> > partition. Add a driver for reading from and writing to the metadata
> > when the updatable images and the metadata are stored on a block
> > device which is formated with GPT based partition scheme.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > drivers/fwu-mdata/Kconfig | 9 +
> > drivers/fwu-mdata/Makefile | 1 +
> > drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++
> > include/fwu.h | 2 +
> > 4 files changed, 416 insertions(+)
> > create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> >
<snip>
> > diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile
> > index 7fec7171f4..12a5b4fe04 100644
> > --- a/drivers/fwu-mdata/Makefile
> > +++ b/drivers/fwu-mdata/Makefile
> > @@ -4,3 +4,4 @@
> > #
> >
> > obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o
> > +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o
>
>
> It is strange to have '_' and '-' in file name for the same directory
>
> => to be coherent = fwu-mdata-gpt-blk.c
I see this kind of naming style in many other directories under
drivers/. The uclass file is named using the foo-uclass.c, while the
other driver files are named bar_driver.c
>
>
> > diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > new file mode 100644
> > index 0000000000..329bd3779b
> > --- /dev/null
> > +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
> > @@ -0,0 +1,404 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <blk.h>
> > +#include <dm.h>
> > +#include <efi_loader.h>
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <memalign.h>
> > +#include <part.h>
> > +#include <part_efi.h>
> > +
> > +#include <dm/device-internal.h>
> > +#include <linux/errno.h>
> > +#include <linux/types.h>
> > +#include <u-boot/crc.h>
> > +
> > +#define PRIMARY_PART BIT(0)
> > +#define SECONDARY_PART BIT(1)
> > +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)
> > +
> > +#define MDATA_READ BIT(0)
> > +#define MDATA_WRITE BIT(1)
> > +
> > +
>
>
> [...]
>
> > +
> > +int fwu_gpt_mdata_check(struct udevice *dev)
> > +{
> > + /*
> > + * Check if both the copies of the FWU metadata are
> > + * valid. If one has gone bad, restore it from the
> > + * other good copy.
> > + */
> > + return gpt_check_mdata_validity(dev);
> > +}
> > +
> > +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata)
> > +{
> > + struct blk_desc *desc;
> > +
> > + desc = dev_get_uclass_plat(dev_get_priv(dev));
>
> as dev = fwu_mdata_gpt_blk(UCLASS_FWU_MDATA)
>
> dev_get_priv(dev) => get value saved in dev_set_priv(dev, mdata_dev);
>
> even if it is OK, it not clear here.
>
> can you add a struct to prepare addition of other elements in privdata:
>
> struct fwu_mdata_gpt_blk_priv {
> struct udevice *blk_dev;
> }
>
>
> + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
>
> + struct blk_desc *desc;
>
> + desc = dev_get_uclass_plat(priv->blk_dev);
Okay. Will add a priv structure as you suggest.
>
>
> > + if (!desc) {
> > + log_err("Block device not found\n");
> > + return -ENODEV;
> > + }
> > +
> > + return gpt_get_mdata(desc, mdata);
> > +}
> > +
> > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev)
> > +{
> > + u32 phandle;
> > + int ret, size;
> > + struct udevice *parent, *child;
> > + const fdt32_t *phandle_p = NULL;
> > +
> > + phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store",
> > + &size);
>
> phandle_p = dev_read_prop(dev, "fwu-mdata-store", &size);
>
>
> it is more simple
Okay
>
>
> > + if (!phandle_p) {
> > + log_err("fwu-mdata-store property not found\n");
> > + return -ENOENT;
> > + }
> > +
> > + phandle = fdt32_to_cpu(*phandle_p);
>
>
> or phandle can be read directly by
>
> + ret =dev_read_phandle_with_args(dev, "fwu-mdata-store", NULL, 0, 0,
> phandle_p)
I did not understand this review comment properly. In any case, I am
using the API, dev_read_prop that you suggested above to read the
phandle pointer.
>
> + if (ret) {
> + log_err("fwu-mdata-store property not found\n");
> + return ret;
> + }
>
> > +
> > + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> > + &parent);
> > + if (ret)
> > + return ret;
> > +
> > + ret = -ENODEV;
> > + for (device_find_first_child(parent, &child); child;
> > + device_find_next_child(&child)) {
> > + if (device_get_uclass_id(child) == UCLASS_BLK) {
> > + *mdata_dev = child;
> > + ret = 0;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
> > +{
> > + int ret;
> > + struct udevice *mdata_dev = NULL;
> > +
> > + ret = fwu_get_mdata_device(dev, &mdata_dev);
> > + if (ret)
> > + return ret;
> > +
> > + dev_set_priv(dev, mdata_dev);
>
> Avoid to use dev_set_priv() in driver :
>
> /**
> * dev_set_priv() - Set the private data for a device
> *
> * This is normally handled by driver model, which automatically allocates
> * private data when an 'auto' size if provided by the driver.
> *
> * Use this function to override normal operation for special
> situations, such
> * as needing to allocate a variable amount of data.
> *
>
> ...
>
> + struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
>
> + priv->blk_dev = mdata_dev;
Okay
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> > + .mdata_check = fwu_gpt_mdata_check,
> > + .get_mdata = fwu_gpt_get_mdata,
> > + .update_mdata = fwu_gpt_update_mdata,
> > +};
> > +
> > +static const struct udevice_id fwu_mdata_ids[] = {
> > + { .compatible = "u-boot,fwu-mdata-gpt" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {
> > + .name = "fwu-mdata-gpt-blk",
> > + .id = UCLASS_FWU_MDATA,
> > + .of_match = fwu_mdata_ids,
> > + .ops = &fwu_gpt_blk_ops,
> > + .probe = fwu_mdata_gpt_blk_probe,
>
>
> + .priv_auto = sizeof(struct fwu_mdata_gpt_blk_priv),
Okay. I have incorporated these review comments. Thanks.
-sughosh
>
>
> > +};
> > diff --git a/include/fwu.h b/include/fwu.h
> > index f9e44e7b39..3b1ee4e83e 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -39,6 +39,8 @@ int fwu_get_active_index(u32 *active_idx);
> > int fwu_update_active_index(u32 active_idx);
> > int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> > int *alt_num);
> > +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev);
> > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
> > int fwu_mdata_check(void);
> > int fwu_revert_boot_index(void);
> > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
>
>
> Regards
>
> Patrick
>
More information about the U-Boot
mailing list