diff mbox series

efi_selftests: fix controllers repeated selftesting

Message ID 20230613132306.83940-1-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series efi_selftests: fix controllers repeated selftesting | expand

Commit Message

Ilias Apalodimas June 13, 2023, 1:23 p.m. UTC
Running the controller selftest more than one times fails with

=> setenv efi_selftest 'controllers' && bootefi selftest
Testing EFI API implementation
Selected test: 'controllers'
Setting up 'controllers'
Setting up 'controllers' succeeded
Executing 'controllers'
Executing 'controllers' succeeded
Summary: 0 failures

=> bootefi selftest
Testing EFI API implementation
Selected test: 'controllers'
Setting up 'controllers'
lib/efi_selftest/efi_selftest_controllers.c(280):
ERROR: InstallProtocolInterface failed
lib/efi_selftest/efi_selftest.c(89):
ERROR: Setting up 'controllers' failed
Summary: 1 failures

There are multiple reason for this.  We don't uninstall the binding
interface from the controller handle and we don't reset the handle
pointers either.  So let's uninstall all the protocols properly and
reset the handles to NULL on setup().

While at it add a forgotten check when uninstalling protocols from the
handle_controller and make sure the number of child controllers is 0

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_selftest/efi_selftest_controllers.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Heinrich Schuchardt June 13, 2023, 3 p.m. UTC | #1
On 6/13/23 15:23, Ilias Apalodimas wrote:
> Running the controller selftest more than one times fails with
>
> => setenv efi_selftest 'controllers' && bootefi selftest
> Testing EFI API implementation
> Selected test: 'controllers'
> Setting up 'controllers'
> Setting up 'controllers' succeeded
> Executing 'controllers'
> Executing 'controllers' succeeded
> Summary: 0 failures
>
> => bootefi selftest
> Testing EFI API implementation
> Selected test: 'controllers'
> Setting up 'controllers'
> lib/efi_selftest/efi_selftest_controllers.c(280):
> ERROR: InstallProtocolInterface failed
> lib/efi_selftest/efi_selftest.c(89):
> ERROR: Setting up 'controllers' failed
> Summary: 1 failures
>
> There are multiple reason for this.  We don't uninstall the binding
> interface from the controller handle and we don't reset the handle
> pointers either.  So let's uninstall all the protocols properly and
> reset the handles to NULL on setup().
>
> While at it add a forgotten check when uninstalling protocols from the
> handle_controller and make sure the number of child controllers is 0
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_selftest/efi_selftest_controllers.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_selftest/efi_selftest_controllers.c b/lib/efi_selftest/efi_selftest_controllers.c
> index d2bbd1c4f65b..07a17ed866a9 100644
> --- a/lib/efi_selftest/efi_selftest_controllers.c
> +++ b/lib/efi_selftest/efi_selftest_controllers.c
> @@ -271,6 +271,8 @@ static int setup(const efi_handle_t img_handle,
>   	efi_status_t ret;
>
>   	boottime = systable->boottime;
> +	handle_controller =  NULL;
> +	handle_driver = NULL;

Other instances of the same problem could be in

lib/efi_selftest/efi_selftest_open_protocol.c
lib/efi_selftest/efi_selftest_loadimage.c
lib/efi_selftest/efi_selftest_manageprotocols.c

We should probably try for each test if it can be repeated.

>
>   	/* Create controller handle */
>   	ret = boottime->install_protocol_interface(
> @@ -402,8 +404,18 @@ static int execute(void)
>   	/* Check number of child controllers */
>   	ret = count_child_controllers(handle_controller, &guid_controller,
>   				      &count);
> -	if (ret == EFI_SUCCESS)
> +	if (ret == EFI_SUCCESS || count != 0)
>   		efi_st_error("Uninstall failed\n");
> +
> +	/* Uninstall binding protocol */
> +	ret = boottime->uninstall_protocol_interface(handle_driver,
> +						     &guid_driver_binding_protocol,
> +						     &binding_interface);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Failed to uninstall protocols\n");
> +		return EFI_ST_FAILURE;
> +	}
> +

Could this easily be moved to teardown()?

Best regards

Heinrich

>   	return EFI_ST_SUCCESS;
>   }
>
Ilias Apalodimas June 13, 2023, 3:08 p.m. UTC | #2
Hi Heinrich,

On Tue, 13 Jun 2023 at 18:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/13/23 15:23, Ilias Apalodimas wrote:
> > Running the controller selftest more than one times fails with
> >
> > => setenv efi_selftest 'controllers' && bootefi selftest
> > Testing EFI API implementation
> > Selected test: 'controllers'
> > Setting up 'controllers'
> > Setting up 'controllers' succeeded
> > Executing 'controllers'
> > Executing 'controllers' succeeded
> > Summary: 0 failures
> >
> > => bootefi selftest
> > Testing EFI API implementation
> > Selected test: 'controllers'
> > Setting up 'controllers'
> > lib/efi_selftest/efi_selftest_controllers.c(280):
> > ERROR: InstallProtocolInterface failed
> > lib/efi_selftest/efi_selftest.c(89):
> > ERROR: Setting up 'controllers' failed
> > Summary: 1 failures
> >
> > There are multiple reason for this.  We don't uninstall the binding
> > interface from the controller handle and we don't reset the handle
> > pointers either.  So let's uninstall all the protocols properly and
> > reset the handles to NULL on setup().
> >
> > While at it add a forgotten check when uninstalling protocols from the
> > handle_controller and make sure the number of child controllers is 0
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_selftest/efi_selftest_controllers.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_selftest/efi_selftest_controllers.c b/lib/efi_selftest/efi_selftest_controllers.c
> > index d2bbd1c4f65b..07a17ed866a9 100644
> > --- a/lib/efi_selftest/efi_selftest_controllers.c
> > +++ b/lib/efi_selftest/efi_selftest_controllers.c
> > @@ -271,6 +271,8 @@ static int setup(const efi_handle_t img_handle,
> >       efi_status_t ret;
> >
> >       boottime = systable->boottime;
> > +     handle_controller =  NULL;
> > +     handle_driver = NULL;
>
> Other instances of the same problem could be in
>
> lib/efi_selftest/efi_selftest_open_protocol.c
> lib/efi_selftest/efi_selftest_loadimage.c
> lib/efi_selftest/efi_selftest_manageprotocols.c
>
> We should probably try for each test if it can be repeated.

I've fixed the manageprotocols in a subsequent patch.  The rest are
probably problematic as well, but I was planning to deal with them
after I am done with the protocol refactoring.

>
> >
> >       /* Create controller handle */
> >       ret = boottime->install_protocol_interface(
> > @@ -402,8 +404,18 @@ static int execute(void)
> >       /* Check number of child controllers */
> >       ret = count_child_controllers(handle_controller, &guid_controller,
> >                                     &count);
> > -     if (ret == EFI_SUCCESS)
> > +     if (ret == EFI_SUCCESS || count != 0)
> >               efi_st_error("Uninstall failed\n");
> > +
> > +     /* Uninstall binding protocol */
> > +     ret = boottime->uninstall_protocol_interface(handle_driver,
> > +                                                  &guid_driver_binding_protocol,
> > +                                                  &binding_interface);
> > +     if (ret != EFI_SUCCESS) {
> > +             efi_st_error("Failed to uninstall protocols\n");
> > +             return EFI_ST_FAILURE;
> > +     }
> > +
>
> Could this easily be moved to teardown()?

Yea, I don't think this will be a problem

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >       return EFI_ST_SUCCESS;
> >   }
> >
>
diff mbox series

Patch

diff --git a/lib/efi_selftest/efi_selftest_controllers.c b/lib/efi_selftest/efi_selftest_controllers.c
index d2bbd1c4f65b..07a17ed866a9 100644
--- a/lib/efi_selftest/efi_selftest_controllers.c
+++ b/lib/efi_selftest/efi_selftest_controllers.c
@@ -271,6 +271,8 @@  static int setup(const efi_handle_t img_handle,
 	efi_status_t ret;
 
 	boottime = systable->boottime;
+	handle_controller =  NULL;
+	handle_driver = NULL;
 
 	/* Create controller handle */
 	ret = boottime->install_protocol_interface(
@@ -402,8 +404,18 @@  static int execute(void)
 	/* Check number of child controllers */
 	ret = count_child_controllers(handle_controller, &guid_controller,
 				      &count);
-	if (ret == EFI_SUCCESS)
+	if (ret == EFI_SUCCESS || count != 0)
 		efi_st_error("Uninstall failed\n");
+
+	/* Uninstall binding protocol */
+	ret = boottime->uninstall_protocol_interface(handle_driver,
+						     &guid_driver_binding_protocol,
+						     &binding_interface);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Failed to uninstall protocols\n");
+		return EFI_ST_FAILURE;
+	}
+
 	return EFI_ST_SUCCESS;
 }