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

[AKASHI Takahiro]

On Fri, Sep 09, 2022 at 03:00:57PM +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.

Not exactly.
ConnectController() will be called against handles which hold
DevicePathProtocol.

> In the Supported() method of our EFI_DRIVER_BINDING_PROTOCOL return
> EFI_ALREADY_STARTED when dealing with an U-Boot internal device.

No. EFI_ALREADY_STARTED is not the right return value
for "ordinary block devices" since this driver does not manage
those devices under the current implementation.
Return EFI_UNSUPPORTED instead.

ALREADY_STARTED is a right one for EFI-app driver.
You must distinguish the two cases.
That is why I said that efi_uc_supported() should have a stricter check.

> Reported-by: Etienne Carriere <etienne.carriere at linaro.org>
> Fixes: b406eb04c360 ("efi_loader: disk: a helper function to delete efi_disk objects")

NAK.
The bug, "erroneously create an extra "efi_blk" block device", has existed
since you introduced efi_driver/efi_block_device.c.

Use 'Fixes: commit 05ef48a2484b ("efi_driver: EFI block driver")'.

-Takahiro Akashi

> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> Reviewed-by: Etienne Carriere <etienne.carriere at linaro.org>
> Tested-by: Etienne Carriere <etienne.carriere at linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
> v2:
> 	add a 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) {
> +		ret = EFI_ALREADY_STARTED;
> +		goto out;
> +	}
> +
>  	ret = EFI_CALL(systab.boottime->open_protocol(
>  			controller_handle, bp->ops->protocol,
>  			&interface, this->driver_binding_handle,
> -- 
> 2.37.2
>