Re: [PATCH v10 09/15] FWU: Add boot time checks as highlighted by the FWU specification

On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu sughosh.ganu@linaro.org wrote:

....

diff --git a/include/fwu.h b/include/fwu.h index 484289ed4f..d5f77ce83c 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -256,4 +256,17 @@ int fwu_plat_get_update_index(uint *update_idx);

*/ void fwu_plat_get_bootidx(uint *boot_idx);

Or simply return the boot_idx instead of modifying the pointed variable

.....

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index c633fcd91e..557e97de4a 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -199,6 +199,7 @@ static efi_status_t __efi_init_early(void) goto out;

    ret = efi_disk_init();

out: return ret; }

We can do without this change in this patchset :)

.....

+static int fwu_trial_state_check(struct udevice *dev) +{

•   int ret;
  

•   efi_status_t status;
  

•   efi_uintn_t var_size;
  

•   u16 trial_state_ctr;
  

•   u32 var_attributes;
  

•   struct fwu_mdata mdata = { 0 };
  

•   ret = fwu_get_mdata(dev, &mdata);
  

•   if (ret)
  

•           return ret;
  

•   if ((trial_state = in_trial_state(&mdata))) {
  
  

This may raise warnings on code checkers. So maybe move the assignment out of the check.

.....

+static int fwu_boottime_checks(void *ctx, struct event *event) +{

•   int ret;
  

•   struct udevice *dev;
  

•   u32 boot_idx, active_idx;
  

•   ret = fwu_get_dev_mdata(&dev, NULL);
  

•   if (ret)
  

•           return ret;
  

•   ret = fwu_mdata_check(dev);
  

•   if (ret) {
  

•           return 0;
  

•   }
  

•   /*
  

•    * Get the Boot Index, i.e. the bank from
  

•    * which the platform has booted. This value
  

•    * gets passed from the ealier stage bootloader
  

•    * which booted u-boot, e.g. tf-a. If the
  

•    * boot index is not the same as the
  

•    * active_index read from the FWU metadata,
  

•    * update the active_index.
  

•    */
  

•   fwu_plat_get_bootidx(&boot_idx);
  

•   if (boot_idx >= CONFIG_FWU_NUM_BANKS) {
  

•           log_err("Received incorrect value of boot_index\n");
  

•           return 0;
  

•   }
  

•   ret = fwu_get_active_index(&active_idx);
  

•   if (ret) {
  

•           log_err("Unable to read active_index\n");
  

•           return 0;
  

•   }
  

•   if (boot_idx != active_idx) {
  

•           log_info("Boot idx %u is not matching active idx %u, changing active_idx\n",
  

•                    boot_idx, active_idx);
  

•           ret = fwu_update_active_index(boot_idx);
  

•           if (!ret)
  

•                   boottime_check = 1;
  
  

We may not want to do anything FWU (accept, reject, modify mdata) until we reboot, if we are recovering from last bad upgrade. So maybe not set boottime_check

cheers