Hi Sughosh,
- */
+#define LOG_CATEGORY UCLASS_FWU_MDATA
[...]
+/**
- fwu_verify_mdata() - Verify the FWU metadata
- @mdata: FWU metadata structure
- @pri_part: FWU metadata partition is primary or secondary
- Verify the FWU metadata by computing the CRC32 for the metadata
- structure and comparing it against the CRC32 value stored as part
- of the structure.
+/**
- fwu_update_active_index() - Update active_index from the FWU metadata
- @active_idx: active_index value to be updated
+int fwu_update_active_index(u32 active_idx) +{
int ret;struct fwu_mdata *mdata = NULL;if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {log_err("Active index value to be updated is incorrect\n");
Nit but "Invalid active index value" is makes more sense
return -1;}
[...]
+}
+/**
- 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)+{
int ret, i;efi_guid_t *image_guid;struct udevice *dev = NULL;struct fwu_mdata *mdata = NULL;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);if (ret)return ret;ret = fwu_get_mdata(&mdata);if (ret) {log_err("Unable to get valid FWU metadata\n");goto out;}/** The FWU metadata has been read. Now get the image_uuid for the* image with the update_bank.*/for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {if (!guidcmp(image_type_id,&mdata->img_entry[i].image_type_uuid)) {img_entry = &mdata->img_entry[i];img_bank_info = &img_entry->img_bank_info[update_bank];image_guid = &img_bank_info->image_uuid;ret = fwu_plat_get_alt_num(dev, image_guid, alt_num);break;}}
I think if you set ret = -EINVAL on top of the for loop you can mostly get rid of the code below. ret = -EINVAL for () { ..... ret = fwu_plat_get_alt_num(dev, image_guid, alt_num); if (ret) log_err("alt_num not found for partition with GUID %pUs\n", image_guid); goto out; //on success instead of break } log_err("Partition with the image type %pUs not found\n", image_type_id);
out: free(mdata);
return ret;
Isn't that more readable?
if (i == CONFIG_FWU_NUM_IMAGES_PER_BANK) {log_err("Partition with the image type %pUs not found\n",image_type_id);ret = -EINVAL;goto out;}if (!ret) {log_debug("alt_num %d for partition %pUs\n",*alt_num, image_guid);} else {log_err("alt_num not found for partition with GUID %pUs\n",image_guid);ret = -EINVAL;}+out:
free(mdata);return ret;+}
+/**
[...]
u32 bank, u8 action)
+{
int ret, i;u32 nimages;
Get rid of nimages when they are only used as CONFIG_FWU_NUM_IMAGES_PER_BANK (there's more than one)
[...]
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index a432e43871..598a8c10a0 100644 @@ -0,0 +1,67 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/*
- Copyright (c) 2022, Linaro Limited
- */
+#if !defined _FWU_MDATA_H_ +#define _FWU_MDATA_H_
+#include <efi.h>
+/**
- 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__));
The spec doesn't require those to be packed. If there's a reason we should have it in comments, otherwise just drop the packed attribute
+/**
- struct fwu_image_entry - information for a particular type of image
- @image_type_uuid: Guid value for identifying the image type
- @location_uuid: Guid of the storage volume where the image is located
- @img_bank_info: Array containing properties of images
- This structure contains information on various types of updatable
- firmware images. Each image type then contains an array of image
- information per bank.
- */
+struct fwu_image_entry {
efi_guid_t image_type_uuid;efi_guid_t location_uuid;struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];+} __attribute__((__packed__));
+/**
- struct fwu_mdata - FWU metadata structure for multi-bank updates
- @crc32: crc32 value for the FWU metadata
- @version: FWU metadata version
- @active_index: Index of the bank currently used for booting images
- @previous_active_inde: Index of the bank used before the current bank
being used for booting
- @img_entry: Array of information on various firmware images that can
be updated
- This structure is used to store all the needed information for performing
- multi bank updates on the platform. This contains info on the bank being
- used to boot along with the information needed for identification of
- individual images
- */
+struct fwu_mdata {
uint32_t crc32;uint32_t version;uint32_t active_index;uint32_t previous_active_index;struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];+} __attribute__((__packed__));
+#endif /* _FWU_MDATA_H_ */
2.34.1
Thanks! /Ilias