diff mbox series

[1/2] efi_loader: use efi_install_multiple_protocol_interfaces()

Message ID 20230615065737.323329-1-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series [1/2] efi_loader: use efi_install_multiple_protocol_interfaces() | expand

Commit Message

Ilias Apalodimas June 15, 2023, 6:57 a.m. UTC
The tcg protocol currently adds and removes protocols with
efi_(add/remove)_protocol().  Although this works fine protocol
interfaces should be installed using the EFI API functions instead
of the internal API ones

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

Comments

Heinrich Schuchardt June 18, 2023, 6:03 a.m. UTC | #1
On 6/15/23 08:57, Ilias Apalodimas wrote:
> The tcg protocol currently adds and removes protocols with
> efi_(add/remove)_protocol().  Although this works fine protocol
> interfaces should be installed using the EFI API functions instead
> of the internal API ones

I would prefer the commit message to explicitly state the reason:

Currently DisconnectController() is not called when uninstalling the
TCG2 protocol which does not comply to the UEFI specification.

>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/efi_tcg2.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index a83ae7a46cf3..49f8a5e77cbf 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -1680,8 +1680,8 @@ void tcg2_uninit(void)
>   	if (!is_tcg2_protocol_installed())
>   		return;
>
> -	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
> -				  (void *)&efi_tcg2_protocol);
> +	ret = efi_uninstall_multiple_protocol_interfaces(efi_root, &efi_guid_tcg2_protocol,
> +							 &efi_tcg2_protocol, NULL);

For a single protocol interface efi_uninstall_protocol() is good enough.

>   	if (ret != EFI_SUCCESS)
>   		log_err("Failed to remove EFI TCG2 protocol\n");
>   }
> @@ -2507,8 +2507,8 @@ efi_status_t efi_tcg2_register(void)
>   		goto fail;
>   	}
>
> -	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
> -			       (void *)&efi_tcg2_protocol);
> +	ret = efi_install_multiple_protocol_interfaces(&efi_root, &efi_guid_tcg2_protocol,
> +						       &efi_tcg2_protocol, NULL);

What is the benefit of this change?

Best regards

Heinrich

>   	if (ret != EFI_SUCCESS) {
>   		tcg2_uninit();
>   		goto fail;
Ilias Apalodimas June 18, 2023, 2:14 p.m. UTC | #2
On Sun, Jun 18, 2023 at 08:03:16AM +0200, Heinrich Schuchardt wrote:
> On 6/15/23 08:57, Ilias Apalodimas wrote:
> > The tcg protocol currently adds and removes protocols with
> > efi_(add/remove)_protocol().  Although this works fine protocol
> > interfaces should be installed using the EFI API functions instead
> > of the internal API ones
> 
> I would prefer the commit message to explicitly state the reason:
> 
> Currently DisconnectController() is not called when uninstalling the
> TCG2 protocol which does not comply to the UEFI specification.
> 
Sure, I can send a v2 updating the description.  We could also add that
using UninstallMultiple  instead of protocl_remove() also removes the
handle if not used

> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_loader/efi_tcg2.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index a83ae7a46cf3..49f8a5e77cbf 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -1680,8 +1680,8 @@ void tcg2_uninit(void)
> >   	if (!is_tcg2_protocol_installed())
> >   		return;
> > 
> > -	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
> > -				  (void *)&efi_tcg2_protocol);
> > +	ret = efi_uninstall_multiple_protocol_interfaces(efi_root, &efi_guid_tcg2_protocol,
> > +							 &efi_tcg2_protocol, NULL);
> 
> For a single protocol interface efi_uninstall_protocol() is good enough.
> 

I'd rather keep this as is.  Since I am using InstallMultiple() using this
one to remove is easier to read. 

> >   	if (ret != EFI_SUCCESS)
> >   		log_err("Failed to remove EFI TCG2 protocol\n");
> >   }
> > @@ -2507,8 +2507,8 @@ efi_status_t efi_tcg2_register(void)
> >   		goto fail;
> >   	}
> > 
> > -	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
> > -			       (void *)&efi_tcg2_protocol);
> > +	ret = efi_install_multiple_protocol_interfaces(&efi_root, &efi_guid_tcg2_protocol,
> > +						       &efi_tcg2_protocol, NULL);
> 
> What is the benefit of this change?
> 

efi_add_protocol() doesn't check for a class in installed device paths and
neither does efi_install_protocol_interface().  But apart from all that I'd
like us to move away from using functions interchangeably when installing a
protocol. 

IMHO (and that's what the second path does) we should slowly replace 
efi_add_protocol -> efi_install_multiple_protocol_interfaces() and make 
efi_add_protocol() static as well.  Similarly we should remove protocols
with efi_uninstall_multiple_protocol_interfaces() and remove the handle
automatically as well. 
I also have more patches which I'll send next week [0] which clean the
whole interface usage further.


[0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/02ac924bb6db020dc4e4284936069ecd46806542

Thanks
/Ilias

> Best regards
> 
> Heinrich
> 
> >   	if (ret != EFI_SUCCESS) {
> >   		tcg2_uninit();
> >   		goto fail;
>
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index a83ae7a46cf3..49f8a5e77cbf 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -1680,8 +1680,8 @@  void tcg2_uninit(void)
 	if (!is_tcg2_protocol_installed())
 		return;
 
-	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
-				  (void *)&efi_tcg2_protocol);
+	ret = efi_uninstall_multiple_protocol_interfaces(efi_root, &efi_guid_tcg2_protocol,
+							 &efi_tcg2_protocol, NULL);
 	if (ret != EFI_SUCCESS)
 		log_err("Failed to remove EFI TCG2 protocol\n");
 }
@@ -2507,8 +2507,8 @@  efi_status_t efi_tcg2_register(void)
 		goto fail;
 	}
 
-	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
-			       (void *)&efi_tcg2_protocol);
+	ret = efi_install_multiple_protocol_interfaces(&efi_root, &efi_guid_tcg2_protocol,
+						       &efi_tcg2_protocol, NULL);
 	if (ret != EFI_SUCCESS) {
 		tcg2_uninit();
 		goto fail;