* [PATCH v3 1/1] efi_driver: don't bind internal block devices
@ 2022-09-09 14:11 Heinrich Schuchardt
2022-09-12 1:55 ` AKASHI Takahiro
0 siblings, 1 reply; 3+ messages in thread
From: Heinrich Schuchardt @ 2022-09-09 14:11 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Etienne Carriere, AKASHI Takahiro, u-boot,
Heinrich Schuchardt
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.
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) {
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/1] efi_driver: don't bind internal block devices
2022-09-09 14:11 [PATCH v3 1/1] efi_driver: don't bind internal block devices Heinrich Schuchardt
@ 2022-09-12 1:55 ` AKASHI Takahiro
2022-09-12 7:55 ` Heinrich Schuchardt
0 siblings, 1 reply; 3+ messages in thread
From: AKASHI Takahiro @ 2022-09-12 1:55 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Ilias Apalodimas, Simon Glass, Etienne Carriere, u-boot
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/1] efi_driver: don't bind internal block devices
2022-09-12 1:55 ` AKASHI Takahiro
@ 2022-09-12 7:55 ` Heinrich Schuchardt
0 siblings, 0 replies; 3+ messages in thread
From: Heinrich Schuchardt @ 2022-09-12 7:55 UTC (permalink / raw)
To: AKASHI Takahiro; +Cc: Ilias Apalodimas, Simon Glass, Etienne Carriere, u-boot
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@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
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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-12 7:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-09 14:11 [PATCH v3 1/1] efi_driver: don't bind internal block devices Heinrich Schuchardt
2022-09-12 1:55 ` AKASHI Takahiro
2022-09-12 7:55 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.