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

[Heinrich Schuchardt]

On 9/12/22 03:55, AKASHI Takahiro wrote:
> 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 at linaro.org>
>> Fixes: commit 05ef48a2484b ("efi_driver: EFI block driver")
>> 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>
>> ---
>> 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

Yes, the same you could say about your suggestion to always return 
EFI_UNSUPPORTED in the supported() method if field dev is populated.

Overall some rework will have to be done in the DM-UEFI integration in 
the next cycle:

* Implement a stop() method for our UEFI block driver.
* Let efi_disk_probe() use ConnectController() to install all protocol 
interfaces but device-tree and block IO protocol.
* Let efi_disk_remove() use UninstallMultipleProtocolInterfaces() to 
remove all protocols.
* Let efi_disk_remove() return an error code if the handle is not 
removed by Uninstall MultipleProtocolInterfaces().
* Move the generation of device-path nodes to the respective uclasses to 
let the device-tree in DM and UEFI match.

Best regards

Heinrich

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