[PATCH v3 2/3] efi_firmware: set EFI capsule dfu_alt_info env explicitly

[Ilias Apalodimas]

Hi Jonathan,

Apologies for the late reply

On Mon, 24 Feb 2025 at 04:38, Jon Humphreys <j-humphreys at ti.com> wrote:

[...]
> >> +
> >> +    ret = fit_update(image);
> >
> >> +
> >> +    if (env_set("dfu_alt_info", orig_dfu_env)) {
> >> +            log_err("unable to set env variable \"dfu_alt_info\"!\n");
> >> +            ret = 1;
> >
> > This will work but for consistency just use an EFI defined error e.g
> > ret = EFI_DEVICE_ERROR;
> >
> >> +    }
> >> +    free(orig_dfu_env);
> >> +
> >> +    if (ret)
> >
> > I am not 100% sure this exiting here is useful. If fit_update has succeeded
> > we have updated our firmware. So I think we should just add a warning here,
> > that resetting the dfu to its original value failed, and any further dfu
> > ops will fail.
> >
>
> Hi Ilias, thanks for the review.
>
> The exit here is for either fit_update() failing OR we cannot restore the
> original value for the dfu_alt_info variable, so it is needed, at least in
> the fit_update() failure case.
>
> I was trying to keep the control flow simpler by handling the 2 failure
> events together. Are you wanting to separate the 2 failure conditions, and
> return EFI_DEVICE_ERROR if fit_update() fails, and emit a warning and
> return EFI_SUCCESS if restoring the env variable fails?

Yes, I think it's better

Thanks
/Ilias
>
> Jon
>
> >>              return EFI_EXIT(EFI_DEVICE_ERROR);
> >>
> >>      efi_firmware_set_fmp_state_var(&state, image_index);
> >> @@ -717,6 +734,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >>      u8 dfu_alt_num;
> >>      efi_status_t status;
> >>      struct fmp_state state = { 0 };
> >> +    char *orig_dfu_env;
> >>
> >>      EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >>                image_size, vendor_code, progress, abort_reason);
> >> @@ -747,8 +765,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >>              }
> >>      }
> >>
> >> -    if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size,
> >> -                         NULL, NULL))
> >> +    orig_dfu_env = strdup(env_get("dfu_alt_info"));
> >> +    if (env_set("dfu_alt_info", update_info.dfu_string)) {
> >> +            log_err("unable to set env variable \"dfu_alt_info\"!\n");
> >> +            free(orig_dfu_env);
> >> +            return EFI_EXIT(EFI_DEVICE_ERROR);
> >> +    }
> >> +
> >> +    ret = dfu_write_by_alt(dfu_alt_num, (void *)image, image_size,
> >> +                           NULL, NULL);
> >> +
> >> +    if (env_set("dfu_alt_info", orig_dfu_env)) {
> >> +            log_err("unable to set env variable \"dfu_alt_info\"!\n");
> >> +            ret = 1;
> >> +    }
> >> +    free(orig_dfu_env);
> >> +
> >> +    if (ret)
> >>              return EFI_EXIT(EFI_DEVICE_ERROR);
> >>
> >>      efi_firmware_set_fmp_state_var(&state, image_index);
> >> --
> >> 2.34.1
> >>
> >
> > Thanks
> > /Ilias