Hi Sughosh,
Thanks for updating the series. I have some comment on this patch.
2022年2月8日(火) 3:21 Sughosh Ganu sughosh.ganu@linaro.org: [snip]
+/**
- 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) +{
int ret;struct fwu_mdata *mdata = NULL;ret = fwu_get_mdata(&mdata);if (ret < 0) {log_err("Unable to get valid FWU metadata\n");goto out;
Again, as we discussed previous series, please don't return unused allocated memory if the function returns an error. That is something like putting a burden on the callers. They always needs to initialize the pointer before call and free it even if the function is failed.
}/** 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) +{
int ret;void *buf;struct fwu_mdata *mdata = NULL;if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {log_err("Active index value to be updated is incorrect\n");return -1;}ret = fwu_get_mdata(&mdata);if (ret < 0) {log_err("Unable to get valid FWU metadata\n");goto out;}/** Update the active index and previous_active_index fields* in the FWU metadata*/mdata->previous_active_index = mdata->active_index;mdata->active_index = active_idx;/** Calculate the crc32 for the updated FWU metadata* and put the updated value in the FWU metadata crc32* field*/buf = &mdata->version;mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
I would like to suggest moving this crc32 calculation in the fwu_update_mdata(). If the crc32 is for detecting a broken metadata on the "storage media", I think it should be calculated in the fwu_update_mdata() to simplify the code and avoid unexpected bugs from mistakes. If the crc32 is calculated right before updating metadata, we can ensure that the crc32 is always sane except for someone breaking it on the storage media.
Thank you,