Re: [PATCH v2 1/2] efi_loader: fix set_capsule_result()

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Thu, Jul 01, 2021 at 03:20:48AM +0200, Heinrich Schuchardt wrote:
> Am 1. Juli 2021 02:49:09 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >NAK.

For example,

> >On Wed, Jun 30, 2021 at 05:31:15PM +0200, Heinrich Schuchardt wrote:
> >> The log category must be LOG_CATEGORY LOGC_EFI.

This one above and


> >> efi_set_variable() should be called with EFI_CALL(). Use
> >> efi_set_variable_int() instead.

and this are mutually irrelevant. 

> >> A log text "Updating ..." if SetVariable() fails does not make sense
> >for a
> >> variable that is not required to be preexisting.

This change is also irrelevant.

> >> CapsuleLast should always be immediately updated.
> >
> >As I said to your v1 in [1],
> >
> >You are trying to fix several irrelevant issues here.
> 
> Why do you think the changes are irrelevant?

No question.

-Takahiro Akashi


> >Please split them into separate patches as you have always
> >asked me before.
> 
> Splitting does not change relevance.
> 
> Do you want to play tit for tat?
> 
> Best regards
> 
> Heinrich
> 
> >
> >[1] https://lists.denx.de/pipermail/u-boot/2021-June/453148.html
> >
> >-Takahiro Akashi
> >
> >
> >
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >> v2:
> >> 	don't update OsIndications in set_capsule_result()
> >> 	update CapsuleLast immediately
> >> ---
> >>  lib/efi_loader/efi_capsule.c | 39
> >+++++++++++++++++++-----------------
> >>  1 file changed, 21 insertions(+), 18 deletions(-)
> >> 
> >> diff --git a/lib/efi_loader/efi_capsule.c
> >b/lib/efi_loader/efi_capsule.c
> >> index 2c37a0d97b..f87ef2a514 100644
> >> --- a/lib/efi_loader/efi_capsule.c
> >> +++ b/lib/efi_loader/efi_capsule.c
> >> @@ -6,6 +6,8 @@
> >>   *			Author: AKASHI Takahiro
> >>   */
> >> 
> >> +#define LOG_CATEGORY LOGC_EFI
> >> +
> >>  #include <common.h>
> >>  #include <efi_loader.h>
> >>  #include <efi_variable.h>
> >> @@ -95,13 +97,25 @@ void set_capsule_result(int index, struct
> >efi_capsule_header *capsule,
> >>  	else
> >>  		memset(&result.capsule_processed, 0, sizeof(time));
> >>  	result.capsule_status = return_status;
> >> -	ret = efi_set_variable(variable_name16, &efi_guid_capsule_report,
> >> -			       EFI_VARIABLE_NON_VOLATILE |
> >> -			       EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> -			       EFI_VARIABLE_RUNTIME_ACCESS,
> >> -			       sizeof(result), &result);
> >> -	if (ret)
> >> -		log_err("EFI: creating %ls failed\n", variable_name16);
> >> +	ret = efi_set_variable_int(variable_name16,
> >&efi_guid_capsule_report,
> >> +				   EFI_VARIABLE_NON_VOLATILE |
> >> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +				   EFI_VARIABLE_RUNTIME_ACCESS,
> >> +				   sizeof(result), &result, false);
> >> +	if (ret != EFI_SUCCESS) {
> >> +		log_err("Setting %ls failed\n", variable_name16);
> >> +		return;
> >> +	}
> >> +
> >> +	/* Variable CapsuleLast must not include terminating 0x0000 */
> >> +	ret = efi_set_variable_int(L"CapsuleLast",
> >&efi_guid_capsule_report,
> >> +				   EFI_VARIABLE_READ_ONLY |
> >> +				   EFI_VARIABLE_NON_VOLATILE |
> >> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +				   EFI_VARIABLE_RUNTIME_ACCESS,
> >> +				   22, variable_name16, false);
> >> +	if (ret != EFI_SUCCESS)
> >> +		log_err("Setting %ls failed\n", L"CapsuleLast");
> >>  }
> >> 
> >>  #ifdef CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT
> >> @@ -988,7 +1002,6 @@ efi_status_t efi_launch_capsules(void)
> >>  	struct efi_capsule_header *capsule = NULL;
> >>  	u16 **files;
> >>  	unsigned int nfiles, index, i;
> >> -	u16 variable_name16[12];
> >>  	efi_status_t ret;
> >> 
> >>  	if (!check_run_capsules())
> >> @@ -1045,16 +1058,6 @@ efi_status_t efi_launch_capsules(void)
> >>  		free(files[i]);
> >>  	free(files);
> >> 
> >> -	/* CapsuleLast */
> >> -	efi_create_indexed_name(variable_name16, sizeof(variable_name16),
> >> -				"Capsule", index - 1);
> >> -	efi_set_variable_int(L"CapsuleLast", &efi_guid_capsule_report,
> >> -			     EFI_VARIABLE_READ_ONLY |
> >> -			     EFI_VARIABLE_NON_VOLATILE |
> >> -			     EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> -			     EFI_VARIABLE_RUNTIME_ACCESS,
> >> -			     22, variable_name16, false);
> >> -
> >>  	return ret;
> >>  }
> >>  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> >> --
> >> 2.30.2
> >> 
>