[PATCH] efi_loader: Make DisconnectController follow the EFI spec

[Heinrich Schuchardt]

On 28.11.23 18:40, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Tue, 28 Nov 2023 at 18:30, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> 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],

DriverHandleBuffer must be a NULL terminated array of handles.

Our implementation of ConnectController() is wrong as it does not
implement this.

Best regards

Heinrich

>>> -                                                 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?
>
> No just ret
>
>>>    }
>>>
>>>    /**
>>> @@ -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 does this as well to reconnect all controllers. Don't  we have
> the same functionality?
>
>>
>> EDK2's CoreDisconnectControllersUsingProtocolInterface() does so. But
>> shouldn't such behavior be defined in the specification?
>
> This is what confused me in the first place and I added the reconnect
> code in disconnect_controller. But if you look more closely they also
> have CoreDisconnectController() which *is* the EFI protocol they
> expose.
>
> CoreDisconnectControllersUsingProtocolInterface is just a wrapper they
> use on UninstallProtocol()
>
>>
>> We have 4 places where we use EFI_CALL to call efi_connect_controller.
>> Should we provide an internal function?
>
> Sure, but not for 2024.01
>
>>
>> Best regards
>>
>> Heinrich
>
> Thanks
> /Ilias
>>
>>>                goto out;
>>>        }
>>>        /* Close protocol */
>>> --
>>> 2.37.2
>>>
>>