[PATCH 1/5] efi_loader: check the status of disconnected drivers

[PATCH 1/5] efi_loader: check the status of disconnected drivers

[Ilias Apalodimas]

efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never
checks the return value.  Honor that and return an appropriate error
if the associated controllers failed to disconnect

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_boottime.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 5006c0e1e4af..68198e6b5ff6 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1353,7 +1353,11 @@ static efi_status_t efi_uninstall_protocol
 	if (r != EFI_SUCCESS)
 		goto out;
 	/* Disconnect controllers */
-	efi_disconnect_all_drivers(efiobj, protocol, NULL);
+	r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
+	if (r != EFI_SUCCESS) {
+		r = EFI_DEVICE_ERROR;
+		goto out;
+	}
 	/* Close protocol */
 	list_for_each_entry_safe(item, pos, &handler->open_infos, link) {
 		if (item->info.attributes ==
-- 
2.39.2

[PATCH 2/5] efi_loader: reconnect drivers on failure

[Ilias Apalodimas]

efi_disconnect_controller() doesn't reconnect drivers in case of
failure.  Reconnect the disconnected drivers properly

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_boottime.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 68198e6b5ff6..df675d0ad488 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -97,6 +97,12 @@ static efi_status_t EFIAPI efi_disconnect_controller(
 					efi_handle_t driver_image_handle,
 					efi_handle_t child_handle);
 
+static efi_status_t EFIAPI efi_connect_controller(
+			efi_handle_t controller_handle,
+			efi_handle_t *driver_image_handle,
+			struct efi_device_path *remain_device_path,
+			bool recursive);
+
 /* Called on every callback entry */
 int __efi_entry_check(void)
 {
@@ -1298,7 +1304,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;
+	efi_uintn_t number_of_drivers, tmp;
 	efi_handle_t *driver_handle_buffer;
 	efi_status_t r, ret;
 
@@ -1308,15 +1314,30 @@ static efi_status_t efi_disconnect_all_drivers
 		return ret;
 	if (!number_of_drivers)
 		return EFI_SUCCESS;
-	ret = EFI_NOT_FOUND;
+
+	tmp = number_of_drivers;
 	while (number_of_drivers) {
-		r = EFI_CALL(efi_disconnect_controller(
+		ret = EFI_CALL(efi_disconnect_controller(
 				handle,
 				driver_handle_buffer[--number_of_drivers],
 				child_handle));
-		if (r == EFI_SUCCESS)
-			ret = r;
+		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");
+	}
+
 	free(driver_handle_buffer);
 	return ret;
 }
-- 
2.39.2

[PATCH 3/5] efi_loader: disconnect all controllers when uninstalling a protocol

[Ilias Apalodimas]

When we are trying to uninstall a protocol interface from a controller
handle we are trying to disconnect drivers related to that protocol.
However, when we call efi_disconnect_all_drivers() we pass the protocol
GUID.  If 2 different drivers are using the same protocol interface and
one of them can't be stopped (e.g by returning EFI_DEVICE_ERROR) we
should stop uninstalling it.

Instead of explicitly passing the protocol GUID, pass NULL as an argument.
That will force efi_get_drivers() to return all drivers consuming the
interface regardless of the protocol GUID.
While at it call efi_disconnect_all_drivers() with a handle instead of
the efiobj

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_boottime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index df675d0ad488..b148824c7ec5 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1374,7 +1374,7 @@ static efi_status_t efi_uninstall_protocol
 	if (r != EFI_SUCCESS)
 		goto out;
 	/* Disconnect controllers */
-	r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
+	r = efi_disconnect_all_drivers(handle, NULL, NULL);
 	if (r != EFI_SUCCESS) {
 		r = EFI_DEVICE_ERROR;
 		goto out;
--
2.39.2

[PATCH 4/5] efi_loader: fix the return codes of UninstallProtocol

[Ilias Apalodimas]

Up to now we did not check the return value of DisconnectController.
A previous patch is fixing that taking into account what happened during
the controller disconnect.  But that check takes place before our code
is trying to figure out if the interface exists to begin with.  In case a
driver is not allowed to unbind -- e.g returning EFI_DEVICE_ERROR, we
will end up returning that error instead of EFI_NOT_FOUND.

Add an extra check on the top of the function to make sure the protocol
interface exists before trying to disconnect any drivers

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_boottime.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index b148824c7ec5..d6d52d4bbac8 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1373,6 +1373,8 @@ static efi_status_t efi_uninstall_protocol
 	r = efi_search_protocol(handle, protocol, &handler);
 	if (r != EFI_SUCCESS)
 		goto out;
+	if (handler->protocol_interface != protocol_interface)
+		return EFI_NOT_FOUND;
 	/* Disconnect controllers */
 	r = efi_disconnect_all_drivers(handle, NULL, NULL);
 	if (r != EFI_SUCCESS) {
-- 
2.39.2

[PATCH 5/5] efi_selftests: add extra testcases on controller handling

[Ilias Apalodimas]

We recently fixed a few issues wrt to controller handling.  Add a few
test cases to cover the new code.
- add a second driver in the same controller handle which will refuse to
  unbind on the first protocol removal
- add tests to verify controllers are reconnected when uninstalling a
  protocol fails
- add tests to make sure EFI_NOT_FOUND is returned if a non existent
  interface is being removed

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_selftest/efi_selftest_controllers.c | 221 ++++++++++++++++++--
 1 file changed, 198 insertions(+), 23 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_controllers.c b/lib/efi_selftest/efi_selftest_controllers.c
index 79bc86fb0c3a..d2a974079329 100644
--- a/lib/efi_selftest/efi_selftest_controllers.c
+++ b/lib/efi_selftest/efi_selftest_controllers.c
@@ -13,6 +13,8 @@
 #include <efi_selftest.h>
 
 #define NUMBER_OF_CHILD_CONTROLLERS 4
+#define CONTROLLER1_DRIVERS (1 + NUMBER_OF_CHILD_CONTROLLERS)
+#define CONTROLLER2_DRIVERS 1
 
 static int interface1 = 1;
 static int interface2 = 2;
@@ -22,24 +24,32 @@ const efi_guid_t guid_driver_binding_protocol =
 static efi_guid_t guid_controller =
 	EFI_GUID(0xe6ab1d96, 0x6bff, 0xdb42,
 		 0xaa, 0x05, 0xc8, 0x1f, 0x7f, 0x45, 0x26, 0x34);
+
+static efi_guid_t guid_controller2 =
+	EFI_GUID(0xe6ab1d96, 0x6bff, 0xdb42,
+		 0xaa, 0x50, 0x8c, 0xf1, 0xf7, 0x54, 0x62, 0x43);
+
 static efi_guid_t guid_child_controller =
 	EFI_GUID(0x1d41f6f5, 0x2c41, 0xddfb,
 		 0xe2, 0x9b, 0xb8, 0x0e, 0x2e, 0xe8, 0x3a, 0x85);
 static efi_handle_t handle_controller;
 static efi_handle_t handle_child_controller[NUMBER_OF_CHILD_CONTROLLERS];
 static efi_handle_t handle_driver;
+static efi_handle_t handle_driver2;
+
+static bool allow_remove;
 
 /*
- * Count child controllers
+ * Count controllers
  *
- * @handle	handle on which child controllers are installed
+ * @handle	handle on which controllers and children are installed
  * @protocol	protocol for which the child controllers were installed
  * @count	number of child controllers
+ * @children:   count children only
  * Return:	status code
  */
-static efi_status_t count_child_controllers(efi_handle_t handle,
-					    efi_guid_t *protocol,
-					    efi_uintn_t *count)
+static efi_status_t count_controllers(efi_handle_t handle, efi_guid_t *protocol,
+				      efi_uintn_t *count, bool children)
 {
 	efi_status_t ret;
 	efi_uintn_t entry_count;
@@ -52,10 +62,14 @@ static efi_status_t count_child_controllers(efi_handle_t handle,
 		return ret;
 	if (!entry_count)
 		return EFI_SUCCESS;
-	while (entry_count) {
-		if (entry_buffer[--entry_count].attributes &
-		    EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER)
-			++*count;
+	if (!children) {
+		*count = entry_count;
+	} else {
+		while (entry_count) {
+			if (entry_buffer[--entry_count].attributes &
+			EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER)
+				++*count;
+		}
 	}
 	ret = boottime->free_pool(entry_buffer);
 	if (ret != EFI_SUCCESS)
@@ -153,6 +167,22 @@ static efi_status_t EFIAPI start(
 			return EFI_ST_FAILURE;
 		}
 	}
+
+	/* Attach driver to controller */
+	ret = boottime->open_protocol(controller_handle, &guid_controller2,
+				      &interface, handle_driver2,
+				      controller_handle,
+				      EFI_OPEN_PROTOCOL_BY_DRIVER);
+	switch (ret) {
+	case EFI_SUCCESS:
+		return EFI_SUCCESS;
+	case EFI_ALREADY_STARTED:
+	case EFI_ACCESS_DENIED:
+		return ret;
+	default:
+		return EFI_UNSUPPORTED;
+	}
+
 	return ret;
 }
 
@@ -249,6 +279,50 @@ static efi_status_t EFIAPI stop(
 	return EFI_SUCCESS;
 }
 
+/*
+ * Check if the driver supports the controller.
+ *
+ * @this			driver binding protocol
+ * @controller_handle		handle of the controller
+ * @remaining_device_path	path specifying the child controller
+ * Return:			status code
+ */
+static efi_status_t EFIAPI supported2(struct efi_driver_binding_protocol *this,
+				      efi_handle_t controller_handle,
+				      struct efi_device_path *remaining_dp)
+{
+	return EFI_SUCCESS;
+}
+
+/*
+ * Refuse to disconnect the controller.
+ *
+ * @this			driver binding protocol
+ * @controller_handle		handle of the controller
+ * @number_of_children		number of child controllers to remove
+ * @child_handle_buffer		handles of the child controllers to remove
+ * Return:			status code
+ */
+static efi_status_t EFIAPI stop2(struct efi_driver_binding_protocol *this,
+				 efi_handle_t controller_handle,
+				 size_t number_of_children,
+				 efi_handle_t *child_handle_buffer)
+{
+	efi_status_t ret;
+
+	if (!allow_remove)
+		return EFI_DEVICE_ERROR;
+
+	/* Detach driver from controller */
+	ret = boottime->close_protocol(controller_handle, &guid_controller2,
+				       handle_driver2, controller_handle);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Cannot close protocol\n");
+		return ret;
+	}
+	return EFI_SUCCESS;
+}
+
 /* Driver binding protocol interface */
 static struct efi_driver_binding_protocol binding_interface = {
 	supported,
@@ -259,6 +333,15 @@ static struct efi_driver_binding_protocol binding_interface = {
 	NULL,
 	};
 
+static struct efi_driver_binding_protocol binding_interface2 = {
+	supported2,
+	start,
+	stop2,
+	0xffffffff,
+	NULL,
+	NULL,
+	};
+
 /*
  * Setup unit test.
  *
@@ -273,6 +356,18 @@ static int setup(const efi_handle_t img_handle,
 	boottime = systable->boottime;
 	handle_controller =  NULL;
 	handle_driver = NULL;
+	handle_driver2 = NULL;
+	allow_remove = false;
+
+	/* Create controller handles */
+	ret = boottime->install_protocol_interface(&handle_controller,
+						   &guid_controller2,
+						   EFI_NATIVE_INTERFACE,
+						   &interface1);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("InstallProtocolInterface failed\n");
+		return EFI_ST_FAILURE;
+	}
 
 	/* Create controller handle */
 	ret = boottime->install_protocol_interface(
@@ -291,6 +386,16 @@ static int setup(const efi_handle_t img_handle,
 		return EFI_ST_FAILURE;
 	}
 
+	/* Create driver handle which will fail on stop() */
+	ret = boottime->install_protocol_interface(&handle_driver2,
+						   &guid_driver_binding_protocol,
+						   EFI_NATIVE_INTERFACE,
+						   &binding_interface2);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("InstallProtocolInterface failed\n");
+		return EFI_ST_FAILURE;
+	}
+
 	return EFI_ST_SUCCESS;
 }
 
@@ -310,7 +415,7 @@ static int setup(const efi_handle_t img_handle,
  */
 static int execute(void)
 {
-	efi_status_t ret;
+	efi_status_t ret = EFI_SUCCESS;
 	efi_uintn_t count;
 
 	/* Connect controller to driver */
@@ -319,9 +424,79 @@ static int execute(void)
 		efi_st_error("Failed to connect controller\n");
 		return EFI_ST_FAILURE;
 	}
+	/* Check number of drivers  */
+	ret = count_controllers(handle_controller, &guid_controller2,
+				&count, false);
+	if (ret != EFI_SUCCESS || count != CONTROLLER2_DRIVERS) {
+		efi_st_error("Failed to connect controller\n");
+		return EFI_ST_FAILURE;
+	}
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, false);
+	if (ret != EFI_SUCCESS || count != CONTROLLER1_DRIVERS) {
+		efi_st_error("Failed to connect controller\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* Try to uninstall controller protocol which doesn't exist */
+	ret = boottime->uninstall_protocol_interface(handle_controller,
+						     &guid_controller2,
+						     &interface2);
+	if (ret != EFI_NOT_FOUND) {
+		efi_st_error("Interface not checked when uninstalling protocol\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* Try to uninstall controller protocol which can't be stopped */
+	ret = boottime->uninstall_protocol_interface(handle_controller,
+						     &guid_controller,
+						     &interface1);
+	if (ret != EFI_DEVICE_ERROR) {
+		efi_st_error("EFI_DRIVER_BINDING_PROTOCOL.Stop() not checked\n");
+		return EFI_ST_FAILURE;
+	}
+	/* Check number of drivers again to make sure controolers reconnected */
+	ret = count_controllers(handle_controller, &guid_controller2,
+				&count, false);
+	if (ret != EFI_SUCCESS || count != CONTROLLER2_DRIVERS) {
+		efi_st_error("Failed to reconnect controller\n");
+		return EFI_ST_FAILURE;
+	}
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, false);
+	if (ret != EFI_SUCCESS || count != CONTROLLER1_DRIVERS) {
+		efi_st_error("Failed to reconnect controller\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* Try to uninstall controller protocol which can't be stopped */
+	ret = boottime->uninstall_protocol_interface(handle_controller,
+						     &guid_controller2,
+						     &interface1);
+	if (ret != EFI_DEVICE_ERROR) {
+		efi_st_error("EFI_DRIVER_BINDING_PROTOCOL.Stop() not checked\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* Check number of drivers again to make sure controllers reconnected */
+	ret = count_controllers(handle_controller, &guid_controller2,
+				&count, false);
+	if (ret != EFI_SUCCESS || count != CONTROLLER2_DRIVERS) {
+		efi_st_error("Failed to reconnect controller\n");
+		return EFI_ST_FAILURE;
+	}
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, false);
+	if (ret != EFI_SUCCESS || count != CONTROLLER1_DRIVERS) {
+		efi_st_error("Failed to reconnect controller\n");
+		return EFI_ST_FAILURE;
+	}
+
+	allow_remove = true;
+
 	/* Check number of child controllers */
-	ret = count_child_controllers(handle_controller, &guid_controller,
-				      &count);
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, true);
 	if (ret != EFI_SUCCESS || count != NUMBER_OF_CHILD_CONTROLLERS) {
 		efi_st_error("Number of children %u != %u\n",
 			     (unsigned int)count, NUMBER_OF_CHILD_CONTROLLERS);
@@ -335,8 +510,8 @@ static int execute(void)
 		return EFI_ST_FAILURE;
 	}
 	/* Check number of child controllers */
-	ret = count_child_controllers(handle_controller, &guid_controller,
-				      &count);
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, true);
 	if (ret != EFI_SUCCESS || count != NUMBER_OF_CHILD_CONTROLLERS - 1) {
 		efi_st_error("Destroying single child controller failed\n");
 		return EFI_ST_FAILURE;
@@ -348,8 +523,8 @@ static int execute(void)
 		return EFI_ST_FAILURE;
 	}
 	/* Check number of child controllers */
-	ret = count_child_controllers(handle_controller, &guid_controller,
-				      &count);
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, true);
 	if (ret != EFI_SUCCESS || count) {
 		efi_st_error("Destroying child controllers failed\n");
 		return EFI_ST_FAILURE;
@@ -362,8 +537,8 @@ static int execute(void)
 		return EFI_ST_FAILURE;
 	}
 	/* Check number of child controllers */
-	ret = count_child_controllers(handle_controller, &guid_controller,
-				      &count);
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, true);
 	if (ret != EFI_SUCCESS || count != NUMBER_OF_CHILD_CONTROLLERS) {
 		efi_st_error("Number of children %u != %u\n",
 			     (unsigned int)count, NUMBER_OF_CHILD_CONTROLLERS);
@@ -387,8 +562,8 @@ static int execute(void)
 		return EFI_ST_FAILURE;
 	}
 	/* Check number of child controllers */
-	ret = count_child_controllers(handle_controller, &guid_controller,
-				      &count);
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, true);
 	if (ret != EFI_SUCCESS || count != NUMBER_OF_CHILD_CONTROLLERS) {
 		efi_st_error("Number of children %u != %u\n",
 			     (unsigned int)count, NUMBER_OF_CHILD_CONTROLLERS);
@@ -402,14 +577,13 @@ static int execute(void)
 		return EFI_ST_FAILURE;
 	}
 	/* Check number of child controllers */
-	ret = count_child_controllers(handle_controller, &guid_controller,
-				      &count);
+	ret = count_controllers(handle_controller, &guid_controller,
+				&count, true);
 	if (ret == EFI_SUCCESS || count != 0) {
 		efi_st_error("Uninstall failed\n");
 		return EFI_ST_FAILURE;
 	}
 
-
 	return EFI_ST_SUCCESS;
 }
 
@@ -420,6 +594,7 @@ static int execute(void)
 static int teardown(void)
 {
 	efi_status_t ret;
+
 	/* Uninstall binding protocol */
 	ret = boottime->uninstall_protocol_interface(handle_driver,
 						     &guid_driver_binding_protocol,
-- 
2.39.2

Re: [PATCH 1/5] efi_loader: check the status of disconnected drivers

[Heinrich Schuchardt]

On 6/15/23 16:39, Ilias Apalodimas wrote:
> efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never
> checks the return value.  Honor that and return an appropriate error
> if the associated controllers failed to disconnect
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/efi_boottime.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 5006c0e1e4af..68198e6b5ff6 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1353,7 +1353,11 @@ static efi_status_t efi_uninstall_protocol
>   	if (r != EFI_SUCCESS)
>   		goto out;
>   	/* Disconnect controllers */
> -	efi_disconnect_all_drivers(efiobj, protocol, NULL);
> +	r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
> +	if (r != EFI_SUCCESS) {
> +		r = EFI_DEVICE_ERROR;

This return code is not foreseen in this case by the UEFI specification.
I only found:

EFI_ACCESS_DENIED:
The interface was not removed because the interface is still being used
by a driver.

EDK II sets this code in CoreDisconnectControllersUsingProtocolInterface().

Best regards

Heinrich

> +		goto out;
> +	}
>   	/* Close protocol */
>   	list_for_each_entry_safe(item, pos, &handler->open_infos, link) {
>   		if (item->info.attributes ==

Re: [PATCH 4/5] efi_loader: fix the return codes of UninstallProtocol

[Heinrich Schuchardt]

On 6/15/23 16:39, Ilias Apalodimas wrote:
> Up to now we did not check the return value of DisconnectController.
> A previous patch is fixing that taking into account what happened during
> the controller disconnect.  But that check takes place before our code
> is trying to figure out if the interface exists to begin with.  In case a
> driver is not allowed to unbind -- e.g returning EFI_DEVICE_ERROR, we
> will end up returning that error instead of EFI_NOT_FOUND.
>
> Add an extra check on the top of the function to make sure the protocol
> interface exists before trying to disconnect any drivers
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   lib/efi_loader/efi_boottime.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index b148824c7ec5..d6d52d4bbac8 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1373,6 +1373,8 @@ static efi_status_t efi_uninstall_protocol
>   	r = efi_search_protocol(handle, protocol, &handler);
>   	if (r != EFI_SUCCESS)
>   		goto out;
> +	if (handler->protocol_interface != protocol_interface)
> +		return EFI_NOT_FOUND;
>   	/* Disconnect controllers */
>   	r = efi_disconnect_all_drivers(handle, NULL, NULL);
>   	if (r != EFI_SUCCESS) {

Re: [PATCH 1/5] efi_loader: check the status of disconnected drivers

[Ilias Apalodimas]

On Sun, Jun 18, 2023 at 08:39:27AM +0200, Heinrich Schuchardt wrote:
> On 6/15/23 16:39, Ilias Apalodimas wrote:
> > efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never
> > checks the return value.  Honor that and return an appropriate error
> > if the associated controllers failed to disconnect
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_loader/efi_boottime.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 5006c0e1e4af..68198e6b5ff6 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1353,7 +1353,11 @@ static efi_status_t efi_uninstall_protocol
> >   	if (r != EFI_SUCCESS)
> >   		goto out;
> >   	/* Disconnect controllers */
> > -	efi_disconnect_all_drivers(efiobj, protocol, NULL);
> > +	r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
> > +	if (r != EFI_SUCCESS) {
> > +		r = EFI_DEVICE_ERROR;
> 
> This return code is not foreseen in this case by the UEFI specification.
> I only found:
> 
> EFI_ACCESS_DENIED:
> The interface was not removed because the interface is still being used
> by a driver.
> 
> EDK II sets this code in CoreDisconnectControllersUsingProtocolInterface().

Right, I only looked at DisconnectController() returns values...
You are right, I'll switch this and the check in the selftests.

Thanks
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> > +		goto out;
> > +	}
> >   	/* Close protocol */
> >   	list_for_each_entry_safe(item, pos, &handler->open_infos, link) {
> >   		if (item->info.attributes ==
> 

Re: [PATCH 3/5] efi_loader: disconnect all controllers when uninstalling a protocol

[Heinrich Schuchardt]

On 6/15/23 16:39, Ilias Apalodimas wrote:
> When we are trying to uninstall a protocol interface from a controller
> handle we are trying to disconnect drivers related to that protocol.
> However, when we call efi_disconnect_all_drivers() we pass the protocol
> GUID.  If 2 different drivers are using the same protocol interface and
> one of them can't be stopped (e.g by returning EFI_DEVICE_ERROR) we
> should stop uninstalling it.
>
> Instead of explicitly passing the protocol GUID, pass NULL as an argument.
> That will force efi_get_drivers() to return all drivers consuming the
> interface regardless of the protocol GUID.
> While at it call efi_disconnect_all_drivers() with a handle instead of
> the efiobj
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/efi_boottime.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index df675d0ad488..b148824c7ec5 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1374,7 +1374,7 @@ static efi_status_t efi_uninstall_protocol
>   	if (r != EFI_SUCCESS)
>   		goto out;
>   	/* Disconnect controllers */
> -	r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
> +	r = efi_disconnect_all_drivers(handle, NULL, NULL);

When uninstalling a *single* protocol we don't want to disconnect
drivers for *all* protocols on this handle but only for the specified
one. This will not work if you don't pass the protocol GUID to
DisconnectController().

Best regards

Heinrich

>   	if (r != EFI_SUCCESS) {
>   		r = EFI_DEVICE_ERROR;
>   		goto out;
> --
> 2.39.2
>

Re: [PATCH 3/5] efi_loader: disconnect all controllers when uninstalling a protocol

[Ilias Apalodimas]

On Mon, 19 Jun 2023 at 10:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/15/23 16:39, Ilias Apalodimas wrote:
> > When we are trying to uninstall a protocol interface from a controller
> > handle we are trying to disconnect drivers related to that protocol.
> > However, when we call efi_disconnect_all_drivers() we pass the protocol
> > GUID.  If 2 different drivers are using the same protocol interface and
> > one of them can't be stopped (e.g by returning EFI_DEVICE_ERROR) we
> > should stop uninstalling it.
> >
> > Instead of explicitly passing the protocol GUID, pass NULL as an argument.
> > That will force efi_get_drivers() to return all drivers consuming the
> > interface regardless of the protocol GUID.
> > While at it call efi_disconnect_all_drivers() with a handle instead of
> > the efiobj
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_loader/efi_boottime.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index df675d0ad488..b148824c7ec5 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1374,7 +1374,7 @@ static efi_status_t efi_uninstall_protocol
> >       if (r != EFI_SUCCESS)
> >               goto out;
> >       /* Disconnect controllers */
> > -     r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
> > +     r = efi_disconnect_all_drivers(handle, NULL, NULL);
>
> When uninstalling a *single* protocol we don't want to disconnect
> drivers for *all* protocols on this handle but only for the specified
> one. This will not work if you don't pass the protocol GUID to
> DisconnectController().

Right, I missread the spec and the selftests are probably wrong as well...
I'll fix that on v2

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >       if (r != EFI_SUCCESS) {
> >               r = EFI_DEVICE_ERROR;
> >               goto out;
> > --
> > 2.39.2
> >
>