[PATCH] efi_loader: Make DisconnectController follow the EFI spec

[Heinrich Schuchardt]

On 28.11.23 16:45, 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 at linaro.org>
> ---
> Heinrich this is critical. Although it doesn't break anything on our normal
> use cases it does break the ACS testing for SystemReady-IR and I prefer
> landing it in -master before the 2024.01 release

Ok

>
>   lib/efi_loader/efi_boottime.c | 23 +++++------------------
>   1 file changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 0b7579cb5af1..370eaee8ce1a 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,31 +1350,17 @@ static efi_status_t efi_disconnect_all_drivers
>   	if (!number_of_drivers)
>   		return EFI_SUCCESS;
>
> -	tmp = number_of_drivers;
>   	while (number_of_drivers) {
> -		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);
> -	return ret;
> +	return (ret != EFI_SUCCESS ? ret : EFI_SUCCESS);

This will always return ret. Did you want to return another value?

>   }
>
>   /**
> @@ -1409,6 +1395,7 @@ static efi_status_t efi_uninstall_protocol
>   	r = efi_disconnect_all_drivers(handle, protocol, NULL);
>   	if (r != EFI_SUCCESS) {
>   		r = EFI_ACCESS_DENIED;
> +		EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));

Chapter "EFI_BOOT_SERVICES.UninstallProtocolInterface()" of the EFI
specification says: " In addition, all of the drivers that were
disconnected with the boot service DisconnectController() earlier, are
reconnected with the boot service
EFI_BOOT_SERVICES.ConnectController()." What makes you think that
ConnectController() should be called with DriverImageHandle = NULL? I
cannot derive this from the specification.

EDK2's CoreDisconnectControllersUsingProtocolInterface() does so. But
shouldn't such behavior be defined in the specification?

We have 4 places where we use EFI_CALL to call efi_connect_controller.
Should we provide an internal function?

Best regards

Heinrich

>   		goto out;
>   	}
>   	/* Close protocol */
> --
> 2.37.2
>