[PATCH] efi_loader: Make DisconnectController follow the EFI spec

[Ilias Apalodimas]

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],
> > -                                                 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
> >
>