* [PATCH] efi_loader: Make DisconnectController follow the EFI spec
@ 2023-11-28 15:45 Ilias Apalodimas
2023-11-28 16:30 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: Ilias Apalodimas @ 2023-11-28 15:45 UTC (permalink / raw)
To: xypron.glpk; +Cc: masahisa.kojima, ryosuke.saito, Ilias Apalodimas, u-boot
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>
---
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
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);
}
/**
@@ -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));
goto out;
}
/* Close protocol */
--
2.37.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: Make DisconnectController follow the EFI spec
2023-11-28 15:45 [PATCH] efi_loader: Make DisconnectController follow the EFI spec Ilias Apalodimas
@ 2023-11-28 16:30 ` Heinrich Schuchardt
2023-11-28 17:40 ` Ilias Apalodimas
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2023-11-28 16:30 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: masahisa.kojima, ryosuke.saito, u-boot
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@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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: Make DisconnectController follow the EFI spec
2023-11-28 16:30 ` Heinrich Schuchardt
@ 2023-11-28 17:40 ` Ilias Apalodimas
2023-11-28 17:57 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: Ilias Apalodimas @ 2023-11-28 17:40 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: masahisa.kojima, ryosuke.saito, u-boot
Hi Heinrich
On Tue, 28 Nov 2023 at 18:30, Heinrich Schuchardt <xypron.glpk@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@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
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: Make DisconnectController follow the EFI spec
2023-11-28 17:40 ` Ilias Apalodimas
@ 2023-11-28 17:57 ` Heinrich Schuchardt
0 siblings, 0 replies; 4+ messages in thread
From: Heinrich Schuchardt @ 2023-11-28 17:57 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: masahisa.kojima, ryosuke.saito, u-boot
On 28.11.23 18:40, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Tue, 28 Nov 2023 at 18:30, Heinrich Schuchardt <xypron.glpk@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@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
>>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-28 17:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-28 15:45 [PATCH] efi_loader: Make DisconnectController follow the EFI spec Ilias Apalodimas
2023-11-28 16:30 ` Heinrich Schuchardt
2023-11-28 17:40 ` Ilias Apalodimas
2023-11-28 17:57 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.