Re: [PATCH 1/2] efi_loader: use efi_install_multiple_protocol_interfaces()

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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;
>