hi Simon, On Thu, 18 Aug 2022 at 23:20, Simon Glass <s...@chromium.org> wrote: > > pHi Sughosh, > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > hi Simon, > > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 17 Aug 2022 at 06:44, Sughosh Ganu <sughosh.g...@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.g...@linaro.org> > > > > Reviewed-by: Patrick Delaunay <patrick.delau...@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