Hi Heinrich On Tue, 28 Nov 2023 at 18:30, Heinrich Schuchardt <xypron.g...@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.apalodi...@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 > > >