Hi Jonathan,
Apologies for the late reply
On Mon, 24 Feb 2025 at 04:38, Jon Humphreys <j-humphr...@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
