Re: [PATCH v3 1/1] efi_driver: don't bind internal block devices

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Fri, Sep 09, 2022 at 04:11:18PM +0200, Heinrich Schuchardt wrote:
> UEFI block devices can either mirror U-Boot's internal devices or be
> provided by an EFI application like iPXE.
> 
> When ConnectController() is invoked for the EFI_BLOCK_IO_PROTOCOL
> interface for such an application provided device we create a virtual
> U-Boot block device of type "efi_blk".
> 
> Currently we do not call ConnectController() when handles for U-Boot's
> internal block devices are created. If an EFI application calls
> ConnectController() for a handle relating to an internal block device,
> we erroneously create an extra "efi_blk" block device.
> 
> E.g. the UEFI shell has a command 'connect -r' which calls
> ConnectController() for all handles with device path protocol.
> 
> In the Supported() method of our EFI_DRIVER_BINDING_PROTOCOL return
> EFI_UNSUPPORTED when dealing with an U-Boot internal device.

Yes, but see the below.

> Reported-by: Etienne Carriere <etienne.carriere@linaro.org>
> Fixes: commit 05ef48a2484b ("efi_driver: EFI block driver")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
> Tested-by: Etienne Carriere <etienne.carriere@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> v3:
> 	return EFI_UNSUPPORTED
> 	changes Fixes: line
> 
> v2:
> 	add code comment
> ---
>  lib/efi_driver/efi_uclass.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
> index b01ce89c84..1d7a0f57e3 100644
> --- a/lib/efi_driver/efi_uclass.c
> +++ b/lib/efi_driver/efi_uclass.c
> @@ -71,6 +71,15 @@ static efi_status_t EFIAPI efi_uc_supported(
>  	EFI_ENTRY("%p, %p, %ls", this, controller_handle,
>  		  efi_dp_str(remaining_device_path));
>  
> +	/*
> +	 * U-Boot internal devices install protocols interfaces without calling
> +	 * ConnectController(). Hence we should not bind an extra driver.
> +	 */
> +	if (controller_handle->dev) {

This check is not good enough to distinguish the two cases,
ordinary block devices and EFI-app-based devices.
Remember that "dev" field is also set by start() for the latter.
(We expect EFI_ALREADY_STARTED in this case.)

I think that you should look at dev's uclass (and/or blk_desc's if_type)
for now.
Logically, I believe, controller_handle's device path could be used
to determine if the handle is to be managed by this driver.

UEFI spec says,
"Drivers will typically use the device path attached to ControllerHandle and/or
the services from the bus I/O abstraction attached to ControllerHandle to
determine if the driver supports ControllerHandle."

-Takahiro Akashi

> +		ret = EFI_UNSUPPORTED;
> +		goto out;
> +	}
> +
>  	ret = EFI_CALL(systab.boottime->open_protocol(
>  			controller_handle, bp->ops->protocol,
>  			&interface, this->driver_binding_handle,
> -- 
> 2.37.2
>