[v2] efi_loader: Make DisconnectController follow the EFI spec

commit 239d59a65e20 ("efi_loader: reconnect drivers on failure")
tried to fix the UninstallProtocol interface which must reconnect
any controllers it disconnected by calling ConnectController()
in case of failure. However, the reconnect functionality was wired in
efi_disconnect_all_drivers() instead of efi_uninstall_protocol().

As a result some SCT tests started failing.
Specifically, BBTestOpenProtocolInterfaceTest333CheckPoint3() test
 - Calls ConnectController for DriverImageHandle1
 - Calls DisconnectController for DriverImageHandle1 which will
   disconnect everything apart from TestProtocol4. That will remain
   open on purpose.
 - Calls ConnectController for DriverImageHandle2. TestProtocol4
   which was explicitly preserved was installed wth BY_DRIVER attributes.
   The new protocol will call DisconnectController since its attributes
   are BY_DRIVER|EXCLUSIVE, but TestProtocol4 will not be removed. The
   test expects EFI_ACCESS_DENIED which works fine.

   The problem is that DisconnectController, will eventually call
   EFI_DRIVER_BINDING_PROTOCOL.Stop(). But on the aforementioned test
   this will call CloseProtocol -- the binding protocol is defined in
   'DBindingDriver3.c' and the .Stop function uses CloseProtocol.
   If that close protocol call fails with EFI_NOT_FOUND, the current code
   will try to mistakenly reconnect all drivers and the subsequent tests
   that rely on the device being disconnected will fail.

Move the reconnection in efi_uninstall_protocol() were it belongs.

Fixes: commit 239d59a65e20 ("efi_loader: reconnect drivers on failure")
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Apologies for the fast resend, but since Heinrich reviewed it and
we want it in 2024.01 resending
Changes since v1:
- return ret instead of (ret != EFI_SUCCESS ? ret : EFI_SUCCESS) which does the
  same thing...
- Add a comment about reconnecting all controllers
 lib/efi_loader/efi_boottime.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

--
2.37.2

On 11/28/23 20:10, Ilias Apalodimas wrote:
> commit 239d59a65e20 ("efi_loader: reconnect drivers on failure")
> tried to fix the UninstallProtocol interface which must reconnect
> any controllers it disconnected by calling ConnectController()
> in case of failure. However, the reconnect functionality was wired in
> efi_disconnect_all_drivers() instead of efi_uninstall_protocol().
>
> As a result some SCT tests started failing.
> Specifically, BBTestOpenProtocolInterfaceTest333CheckPoint3() test
>   - Calls ConnectController for DriverImageHandle1
>   - Calls DisconnectController for DriverImageHandle1 which will
>     disconnect everything apart from TestProtocol4. That will remain
>     open on purpose.
>   - Calls ConnectController for DriverImageHandle2. TestProtocol4
>     which was explicitly preserved was installed wth BY_DRIVER attributes.
>     The new protocol will call DisconnectController since its attributes
>     are BY_DRIVER|EXCLUSIVE, but TestProtocol4 will not be removed. The
>     test expects EFI_ACCESS_DENIED which works fine.
>
>     The problem is that DisconnectController, will eventually call
>     EFI_DRIVER_BINDING_PROTOCOL.Stop(). But on the aforementioned test
>     this will call CloseProtocol -- the binding protocol is defined in
>     'DBindingDriver3.c' and the .Stop function uses CloseProtocol.
>     If that close protocol call fails with EFI_NOT_FOUND, the current code
>     will try to mistakenly reconnect all drivers and the subsequent tests
>     that rely on the device being disconnected will fail.
>
> Move the reconnection in efi_uninstall_protocol() were it belongs.
>
> Fixes: commit 239d59a65e20 ("efi_loader: reconnect drivers on failure")
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Apologies for the fast resend, but since Heinrich reviewed it and
> we want it in 2024.01 resending
> Changes since v1:
> - return ret instead of (ret != EFI_SUCCESS ? ret : EFI_SUCCESS) which does the
>    same thing...
> - Add a comment about reconnecting all controllers
>   lib/efi_loader/efi_boottime.c | 27 ++++++++++-----------------
>   1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 0b7579cb5af1..ea711919630b 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1339,7 +1339,7 @@ static efi_status_t efi_disconnect_all_drivers
>   				 const efi_guid_t *protocol,
>   				 efi_handle_t child_handle)
>   {
> -	efi_uintn_t number_of_drivers, tmp;
> +	efi_uintn_t number_of_drivers;
>   	efi_handle_t *driver_handle_buffer;
>   	efi_status_t r, ret;
>
> @@ -1350,27 +1350,13 @@ static efi_status_t efi_disconnect_all_drivers
>   	if (!number_of_drivers)
>   		return EFI_SUCCESS;
>
> -	tmp = number_of_drivers;
>   	while (number_of_drivers) {

You assume here that drivers can be removed in reverse order.

Please, have a look at CoreDisconnectControllersUsingProtocolInterface()
in EDK II.

Here the code continues iterating over all connected controllers while
at least one controller can be removed in each round.

Reverse order is more promising than forward order. But maybe we should
combine this with retrying like in EDK II.

Best regards

Heinrich

> -		ret = EFI_CALL(efi_disconnect_controller(
> +		r = EFI_CALL(efi_disconnect_controller(
>   				handle,
>   				driver_handle_buffer[--number_of_drivers],
>   				child_handle));
> -		if (ret != EFI_SUCCESS)
> -			goto reconnect;
> -	}
> -
> -	free(driver_handle_buffer);
> -	return ret;
> -
> -reconnect:
> -	/* Reconnect all disconnected drivers */
> -	for (; number_of_drivers < tmp; number_of_drivers++) {
> -		r = EFI_CALL(efi_connect_controller(handle,
> -						    &driver_handle_buffer[number_of_drivers],
> -						    NULL, true));
>   		if (r != EFI_SUCCESS)
> -			EFI_PRINT("Failed to reconnect controller\n");
> +			ret = r;
>   	}
>
>   	free(driver_handle_buffer);
> @@ -1409,6 +1395,13 @@ static efi_status_t efi_uninstall_protocol
>   	r = efi_disconnect_all_drivers(handle, protocol, NULL);
>   	if (r != EFI_SUCCESS) {
>   		r = EFI_ACCESS_DENIED;
> +		/*
> +		 * This will reconnect all controllers of the handle, even ones
> +		 * that were not connected before. This can be done better
> +		 * but we are following the EDKII implementation on this for
> +		 * now
> +		 */
> +		EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
>   		goto out;
>   	}
>   	/* Close protocol */
> --
> 2.37.2
>

Hi Heinrich,

On Wed, 29 Nov 2023 at 03:26, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/28/23 20:10, Ilias Apalodimas wrote:
> > commit 239d59a65e20 ("efi_loader: reconnect drivers on failure")
> > tried to fix the UninstallProtocol interface which must reconnect
> > any controllers it disconnected by calling ConnectController()
> > in case of failure. However, the reconnect functionality was wired in
> > efi_disconnect_all_drivers() instead of efi_uninstall_protocol().
> >
> > As a result some SCT tests started failing.
> > Specifically, BBTestOpenProtocolInterfaceTest333CheckPoint3() test
> >   - Calls ConnectController for DriverImageHandle1
> >   - Calls DisconnectController for DriverImageHandle1 which will
> >     disconnect everything apart from TestProtocol4. That will remain
> >     open on purpose.
> >   - Calls ConnectController for DriverImageHandle2. TestProtocol4
> >     which was explicitly preserved was installed wth BY_DRIVER attributes.
> >     The new protocol will call DisconnectController since its attributes
> >     are BY_DRIVER|EXCLUSIVE, but TestProtocol4 will not be removed. The
> >     test expects EFI_ACCESS_DENIED which works fine.
> >
> >     The problem is that DisconnectController, will eventually call
> >     EFI_DRIVER_BINDING_PROTOCOL.Stop(). But on the aforementioned test
> >     this will call CloseProtocol -- the binding protocol is defined in
> >     'DBindingDriver3.c' and the .Stop function uses CloseProtocol.
> >     If that close protocol call fails with EFI_NOT_FOUND, the current code
> >     will try to mistakenly reconnect all drivers and the subsequent tests
> >     that rely on the device being disconnected will fail.
> >
> > Move the reconnection in efi_uninstall_protocol() were it belongs.
> >
> > Fixes: commit 239d59a65e20 ("efi_loader: reconnect drivers on failure")
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Apologies for the fast resend, but since Heinrich reviewed it and
> > we want it in 2024.01 resending
> > Changes since v1:
> > - return ret instead of (ret != EFI_SUCCESS ? ret : EFI_SUCCESS) which does the
> >    same thing...
> > - Add a comment about reconnecting all controllers
> >   lib/efi_loader/efi_boottime.c | 27 ++++++++++-----------------
> >   1 file changed, 10 insertions(+), 17 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 0b7579cb5af1..ea711919630b 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1339,7 +1339,7 @@ static efi_status_t efi_disconnect_all_drivers
> >                                const efi_guid_t *protocol,
> >                                efi_handle_t child_handle)
> >   {
> > -     efi_uintn_t number_of_drivers, tmp;
> > +     efi_uintn_t number_of_drivers;
> >       efi_handle_t *driver_handle_buffer;
> >       efi_status_t r, ret;
> >
> > @@ -1350,27 +1350,13 @@ static efi_status_t efi_disconnect_all_drivers
> >       if (!number_of_drivers)
> >               return EFI_SUCCESS;
> >
> > -     tmp = number_of_drivers;
> >       while (number_of_drivers) {
>
> You assume here that drivers can be removed in reverse order.
>
> Please, have a look at CoreDisconnectControllersUsingProtocolInterface()
> in EDK II.
>
> Here the code continues iterating over all connected controllers while
> at least one controller can be removed in each round.
>
> Reverse order is more promising than forward order. But maybe we should
> combine this with retrying like in EDK II.

There are some SCT tests that fail on ConnectController and I was
going to have a look.
But the current patch basically reverts this function as it was prior
to commit 239d59a65e20.
Do we have time to rewrite this before the release? Or should we merge
this and plan to refactor it for 2024.04?

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> > -             ret = EFI_CALL(efi_disconnect_controller(
> > +             r = EFI_CALL(efi_disconnect_controller(
> >                               handle,
> >                               driver_handle_buffer[--number_of_drivers],
> >                               child_handle));
> > -             if (ret != EFI_SUCCESS)
> > -                     goto reconnect;
> > -     }
> > -
> > -     free(driver_handle_buffer);
> > -     return ret;
> > -
> > -reconnect:
> > -     /* Reconnect all disconnected drivers */
> > -     for (; number_of_drivers < tmp; number_of_drivers++) {
> > -             r = EFI_CALL(efi_connect_controller(handle,
> > -                                                 &driver_handle_buffer[number_of_drivers],
> > -                                                 NULL, true));
> >               if (r != EFI_SUCCESS)
> > -                     EFI_PRINT("Failed to reconnect controller\n");
> > +                     ret = r;
> >       }
> >
> >       free(driver_handle_buffer);
> > @@ -1409,6 +1395,13 @@ static efi_status_t efi_uninstall_protocol
> >       r = efi_disconnect_all_drivers(handle, protocol, NULL);
> >       if (r != EFI_SUCCESS) {
> >               r = EFI_ACCESS_DENIED;
> > +             /*
> > +              * This will reconnect all controllers of the handle, even ones
> > +              * that were not connected before. This can be done better
> > +              * but we are following the EDKII implementation on this for
> > +              * now
> > +              */
> > +             EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
> >               goto out;
> >       }
> >       /* Close protocol */
> > --
> > 2.37.2
> >
>