Re: [PATCH v7 09/13] FWU: Add support for the FWU Multi Bank Update feature

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Sughosh,

[...]

>  #endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
>
> +static bool fwu_empty_capsule(struct efi_capsule_header *capsule)
> +{
> +       return !guidcmp(&capsule->capsule_guid,
> +                       &fwu_guid_os_request_fw_revert) ||
> +               !guidcmp(&capsule->capsule_guid,
> +                        &fwu_guid_os_request_fw_accept);
> +}
> +
> +static efi_status_t fwu_empty_capsule_process(
> +       struct efi_capsule_header *capsule)
> +{
> +       int status;
> +       u32 active_idx;
> +       efi_status_t ret;
> +       efi_guid_t *image_guid;
> +
> +       if (!guidcmp(&capsule->capsule_guid,
> +                    &fwu_guid_os_request_fw_revert)) {
> +               /*
> +                * One of the previously updated image has
> +                * failed the OS acceptance test. OS has
> +                * requested to revert back to the earlier
> +                * boot index
> +                */
> +               status = fwu_revert_boot_index();
> +               if (status < 0) {
> +                       log_err("Failed to revert the FWU boot index\n");
> +                       if (status == -ENODEV ||
> +                           status == -ERANGE ||
> +                           status == -EIO)
> +                               ret = EFI_DEVICE_ERROR;
> +                       else if (status == -EINVAL)
> +                               ret = EFI_INVALID_PARAMETER;
> +                       else
> +                               ret = EFI_OUT_OF_RESOURCES;

In all the case you carry those if statements,  define a function like

static efi_status_t fwu_to_efi_error (int err)
{
    switch(err) {
    case -ENODEV:
    case -ERANGE:
    case -EIO:
        return EFI_DEVICE_ERROR;
    }
    .....
}
and use it in the error handling below.
That should make the weird looking error handling go away and adding
more cases in the future a lot easier.

> +               } else {
> +                       ret = EFI_SUCCESS;
> +                       log_err("Reverted the FWU active_index. Recommend rebooting the system\n");

Does it really need log_err?

[...]

Regards
/Ilias