[U-Boot,v11,2/6] efi_loader: Drop setup_ok

This value is stored in data which appears to be read-only with sandbox on
my Ubuntu 18.04 machine. In any case it is not good practice to store
run-time data in a build-time linker list.

The value does not seem to be that useful, since tests that fail to setup
are likely to fail to run also. Let's drop it for now.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v11:
- Add a new patch to drop setup_ok

Changes in v9: None
Changes in v7: None
Changes in v5: None
Changes in v4: None
Changes in v3: None

 include/efi_selftest.h          |  2 --
 lib/efi_selftest/efi_selftest.c | 14 +++++++-------
 2 files changed, 7 insertions(+), 9 deletions(-)

On 10/15/2018 04:17 PM, Simon Glass wrote:
> This value is stored in data which appears to be read-only with sandbox on
> my Ubuntu 18.04 machine. In any case it is not good practice to store
> run-time data in a build-time linker list.
Yes this should be changed. Otherwise a reset of the board will not put
us back into the initial status.

> 
> The value does not seem to be that useful, since tests that fail to setup
> are likely to fail to run also. Let's drop it for now.

With your change we will run execute() even if setup() fails. This
contradicts the commit message. Please, find a solution that avoids
calling execute() after a failed setup().

Best regards

Heinrich


> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v11:
> - Add a new patch to drop setup_ok
> 
> Changes in v9: None
> Changes in v7: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> 
>  include/efi_selftest.h          |  2 --
>  lib/efi_selftest/efi_selftest.c | 14 +++++++-------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/include/efi_selftest.h b/include/efi_selftest.h
> index 56beac305ec..49d3d6d0b47 100644
> --- a/include/efi_selftest.h
> +++ b/include/efi_selftest.h
> @@ -129,7 +129,6 @@ u16 efi_st_get_key(void);
>   * @setup:	set up the unit test
>   * @teardown:	tear down the unit test
>   * @execute:	execute the unit test
> - * @setup_ok:	setup was successful (set at runtime)
>   * @on_request:	test is only executed on request
>   */
>  struct efi_unit_test {
> @@ -139,7 +138,6 @@ struct efi_unit_test {
>  		     const struct efi_system_table *systable);
>  	int (*execute)(void);
>  	int (*teardown)(void);
> -	int setup_ok;
>  	bool on_request;
>  };
>  
> diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
> index dd338db687e..dfd11be2302 100644
> --- a/lib/efi_selftest/efi_selftest.c
> +++ b/lib/efi_selftest/efi_selftest.c
> @@ -74,20 +74,20 @@ void efi_st_exit_boot_services(void)
>   */
>  static int setup(struct efi_unit_test *test, unsigned int *failures)
>  {
> -	if (!test->setup) {
> -		test->setup_ok = EFI_ST_SUCCESS;
> +	int ret;
> +
> +	if (!test->setup)
>  		return EFI_ST_SUCCESS;
> -	}
>  	efi_st_printc(EFI_LIGHTBLUE, "\nSetting up '%s'\n", test->name);
> -	test->setup_ok = test->setup(handle, systable);
> -	if (test->setup_ok != EFI_ST_SUCCESS) {
> +	ret = test->setup(handle, systable);
> +	if (ret) {
>  		efi_st_error("Setting up '%s' failed\n", test->name);
>  		++*failures;
>  	} else {
>  		efi_st_printc(EFI_LIGHTGREEN,
>  			      "Setting up '%s' succeeded\n", test->name);
>  	}
> -	return test->setup_ok;
> +	return ret;
>  }
>  
>  /*
> @@ -197,7 +197,7 @@ void efi_st_do_tests(const u16 *testname, unsigned int phase,
>  			continue;
>  		if (steps & EFI_ST_SETUP)
>  			setup(test, failures);
> -		if (steps & EFI_ST_EXECUTE && test->setup_ok == EFI_ST_SUCCESS)
> +		if (steps & EFI_ST_EXECUTE)
>  			execute(test, failures);
>  		if (steps & EFI_ST_TEARDOWN)
>  			teardown(test, failures);
>

Hi,

On 15 October 2018 at 11:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 10/15/2018 04:17 PM, Simon Glass wrote:
>> This value is stored in data which appears to be read-only with sandbox on
>> my Ubuntu 18.04 machine. In any case it is not good practice to store
>> run-time data in a build-time linker list.
> Yes this should be changed. Otherwise a reset of the board will not put
> us back into the initial status.
>
>>
>> The value does not seem to be that useful, since tests that fail to setup
>> are likely to fail to run also. Let's drop it for now.
>
> With your change we will run execute() even if setup() fails. This
> contradicts the commit message. Please, find a solution that avoids
> calling execute() after a failed setup().

How about we just exit if setup() fails? It should not fail. The test
can fail, but not the setup.

Regards,
Simon

On 10/19/2018 05:25 AM, Simon Glass wrote:
> Hi,
> 
> On 15 October 2018 at 11:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 10/15/2018 04:17 PM, Simon Glass wrote:
>>> This value is stored in data which appears to be read-only with sandbox on
>>> my Ubuntu 18.04 machine. In any case it is not good practice to store
>>> run-time data in a build-time linker list.
>> Yes this should be changed. Otherwise a reset of the board will not put
>> us back into the initial status.
>>
>>>
>>> The value does not seem to be that useful, since tests that fail to setup
>>> are likely to fail to run also. Let's drop it for now.
>>
>> With your change we will run execute() even if setup() fails. This
>> contradicts the commit message. Please, find a solution that avoids
>> calling execute() after a failed setup().
> 
> How about we just exit if setup() fails? It should not fail. The test
> can fail, but not the setup.
> 
> Regards,
> Simon
> 
I just have sent an alternative patch.

Thanks for reporting the issue.

Best regards

Heinrich