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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

Am 1. Juli 2021 02:49:09 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>NAK.
>
>On Wed, Jun 30, 2021 at 05:31:15PM +0200, Heinrich Schuchardt wrote:
>> The log category must be LOG_CATEGORY LOGC_EFI.
>> 
>> efi_set_variable() should be called with EFI_CALL(). Use
>> efi_set_variable_int() instead.
>> 
>> A log text "Updating ..." if SetVariable() fails does not make sense
>for a
>> variable that is not required to be preexisting.
>> 
>> 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?

>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
>>