hi Ilias,
On Wed, 20 Jul 2022 at 13:06, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Sughosh,
nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;active_bank = mdata->active_index;img_entry = &mdata->img_entry[0];for (i = 0; i < nimages; i++) {img_bank_info = &img_entry[i].img_bank_info[active_bank];if (!img_bank_info->accepted) {trial_state = 1;break;}}Is this used elsewhere in the patchset? The function is starting to be big, so perhaps moving this in a static bool "in_trial_state()" or similar would make it more readable.
There was a discussion about this on the synquacer thread for A/B updates. Once you split those in a function, it's better to extend the bootcount API with an EFI backed storage. The reasoning that a user might disable editing env variables for security reasons and that device might not be able to preserve RAM or store the counter in CPU registers across reboots.
If we extend the bootcount API with this code we can plug in the functionality seamlessly based on the hardware capabilities.
I have incorporated the rest of your comments on this patch. I moved the access to the TrialStateCtr EFI variable to the bootcount API like you suggested. However, I face an issue, in that on every boot, the bootdelay_process function gets called which increments the bootcount variable. This does not map with the requirement of the FWU feature since the TrialStateCtr variable needs to be updated only when the platform transitions to the Trial State, after a successful update. Once the platform transitions from the Trial State to the Regular State, the variable gets deleted. Hence I think it would be better to use the current logic of the handling of the TrialStateCtr variable. Thanks.
-sughosh
[...]
Regards
/Ilias