| Message ID | 20230719002158.27004-1-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] efi_driver: fix a parent issue in efi-created block devices | expand |
Hi AKASHI, On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > EFI world, which in turn generates a corresponding U-Boot block device based on > U-Boot's Driver Model. > The latter device, however, doesn't work as U-Boot proper block device > due to an issue in efi_driver's implementation. We saw discussions in the past, > most recently in [1]. > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > This RFC patch tries to address (part of) the issue. > If it is considered acceptable, I will create a formal patch. > > Withtout this patch, > ===8<=== > => env set efi_selftest 'block device' > => bootefi selftest > ... > > Summary: 0 failures > > => dm tree > Class Index Probed Driver Name > ----------------------------------------------------------- > root 0 [ + ] root_driver root_driver > ... > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > blk 0 [ + ] efi_blk `-- efiblk#0 > partition 0 [ + ] blk_partition `-- efiblk#0:1 > => ls efiloader 0:1 > ** Bad device specification efiloader 0 ** > Couldn't find partition efiloader 0:1 > ===>8=== > > With this patch applied, efiblk#0(:1) now gets accessible. > > ===8<=== > => env set efi_selftest 'block device' > => bootefi selftest > ... > > Summary: 0 failures > > => dm tree > Class Index Probed Driver Name > ----------------------------------------------------------- > root 0 [ + ] root_driver root_driver > ... > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > blk 0 [ + ] efi_blk `-- efiblk#0 > partition 0 [ + ] blk_partition `-- efiblk#0:1 > => ls efiloader 0:1 > 13 hello.txt > 7 u-boot.txt > > 2 file(s), 0 dir(s) > ===>8=== > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > include/efi_driver.h | 2 +- > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > lib/efi_driver/efi_uclass.c | 8 +++++++- > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > 4 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/include/efi_driver.h b/include/efi_driver.h > index 63a95e4cf800..ed66796f3519 100644 > --- a/include/efi_driver.h > +++ b/include/efi_driver.h > @@ -42,7 +42,7 @@ struct efi_driver_ops { > const efi_guid_t *child_protocol; > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > - efi_handle_t handle, void *interface); > + efi_handle_t handle, void *interface, char *name); > }; > > #endif /* _EFI_DRIVER_H */ > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > index add00eeebbea..43b7ed7c973c 100644 > --- a/lib/efi_driver/efi_block_device.c > +++ b/lib/efi_driver/efi_block_device.c > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > * Return: status code > */ > static efi_status_t > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > { > - struct udevice *bdev = NULL, *parent = dm_root(); > + struct udevice *bdev = NULL; > efi_status_t ret; > int devnum; > char *name; > @@ -181,7 +181,7 @@ err: > */ > static efi_status_t efi_bl_bind( > struct efi_driver_binding_extended_protocol *this, > - efi_handle_t handle, void *interface) > + efi_handle_t handle, void *interface, char *name) > { > efi_status_t ret = EFI_SUCCESS; > struct efi_object *obj = efi_search_obj(handle); > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > if (!obj || !interface) > return EFI_INVALID_PARAMETER; > > - if (!handle->dev) > - ret = efi_bl_create_block_device(handle, interface); > + if (!handle->dev) { > + struct udevice *parent; > + > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); Can you use a non-root device as the parent? > + if (!ret) > + ret = efi_bl_create_block_device(handle, interface, parent); > + else > + ret = EFI_DEVICE_ERROR; > + } > > return ret; > } > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > index 45f935198874..bf669742783e 100644 > --- a/lib/efi_driver/efi_uclass.c > +++ b/lib/efi_driver/efi_uclass.c > @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( > ret = check_node_type(controller_handle); > if (ret != EFI_SUCCESS) > goto err; > - ret = bp->ops->bind(bp, controller_handle, interface); > + > + struct efi_handler *handler; > + char tmpname[256] = "AppName"; > + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, > + &handler); > + snprintf(tmpname, 256, "%pD", handler->protocol_interface); > + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); > if (ret == EFI_SUCCESS) > goto out; > > diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c > index a367e8b89d17..0ab8e4590dfe 100644 > --- a/lib/efi_selftest/efi_selftest_block_device.c > +++ b/lib/efi_selftest/efi_selftest_block_device.c > @@ -248,6 +248,7 @@ static int teardown(void) > { > efi_status_t r = EFI_ST_SUCCESS; > > +#if 0 /* Temporarily out for confirmation */ > if (disk_handle) { > r = boottime->uninstall_protocol_interface(disk_handle, > &guid_device_path, > @@ -273,6 +274,7 @@ static int teardown(void) > return EFI_ST_FAILURE; > } > } > +#endif > return r; > } > > -- > 2.41.0 > Otherwise this looks good to me. We should have DM devices for all EFI ones (in fact EFI ones should just be some extra data on top of DM ones). Regards, Simon
Hi Simon, On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > Hi AKASHI, > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > EFI world, which in turn generates a corresponding U-Boot block device based on > > U-Boot's Driver Model. > > The latter device, however, doesn't work as U-Boot proper block device > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > most recently in [1]. > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > This RFC patch tries to address (part of) the issue. > > If it is considered acceptable, I will create a formal patch. > > > > Withtout this patch, > > ===8<=== > > => env set efi_selftest 'block device' > > => bootefi selftest > > ... > > > > Summary: 0 failures > > > > => dm tree > > Class Index Probed Driver Name > > ----------------------------------------------------------- > > root 0 [ + ] root_driver root_driver > > ... > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > blk 0 [ + ] efi_blk `-- efiblk#0 > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > => ls efiloader 0:1 > > ** Bad device specification efiloader 0 ** > > Couldn't find partition efiloader 0:1 > > ===>8=== > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > ===8<=== > > => env set efi_selftest 'block device' > > => bootefi selftest > > ... > > > > Summary: 0 failures > > > > => dm tree > > Class Index Probed Driver Name > > ----------------------------------------------------------- > > root 0 [ + ] root_driver root_driver > > ... > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > blk 0 [ + ] efi_blk `-- efiblk#0 > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > => ls efiloader 0:1 > > 13 hello.txt > > 7 u-boot.txt > > > > 2 file(s), 0 dir(s) > > ===>8=== > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > include/efi_driver.h | 2 +- > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > index 63a95e4cf800..ed66796f3519 100644 > > --- a/include/efi_driver.h > > +++ b/include/efi_driver.h > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > const efi_guid_t *child_protocol; > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > - efi_handle_t handle, void *interface); > > + efi_handle_t handle, void *interface, char *name); > > }; > > > > #endif /* _EFI_DRIVER_H */ > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > index add00eeebbea..43b7ed7c973c 100644 > > --- a/lib/efi_driver/efi_block_device.c > > +++ b/lib/efi_driver/efi_block_device.c > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > * Return: status code > > */ > > static efi_status_t > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > { > > - struct udevice *bdev = NULL, *parent = dm_root(); > > + struct udevice *bdev = NULL; > > efi_status_t ret; > > int devnum; > > char *name; > > @@ -181,7 +181,7 @@ err: > > */ > > static efi_status_t efi_bl_bind( > > struct efi_driver_binding_extended_protocol *this, > > - efi_handle_t handle, void *interface) > > + efi_handle_t handle, void *interface, char *name) > > { > > efi_status_t ret = EFI_SUCCESS; > > struct efi_object *obj = efi_search_obj(handle); > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > if (!obj || !interface) > > return EFI_INVALID_PARAMETER; > > > > - if (!handle->dev) > > - ret = efi_bl_create_block_device(handle, interface); > > + if (!handle->dev) { > > + struct udevice *parent; > > + > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > Can you use a non-root device as the parent? I have no idea what else can be the parent in this case. Please note: > > => dm tree > > Class Index Probed Driver Name > > ----------------------------------------------------------- > > root 0 [ + ] root_driver root_driver > > ... > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) and don't have any practical parent. > > blk 0 [ + ] efi_blk `-- efiblk#0 > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > + if (!ret) > > + ret = efi_bl_create_block_device(handle, interface, parent); > > + else > > + ret = EFI_DEVICE_ERROR; > > + } > > > > return ret; > > } > > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > > index 45f935198874..bf669742783e 100644 > > --- a/lib/efi_driver/efi_uclass.c > > +++ b/lib/efi_driver/efi_uclass.c > > @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( > > ret = check_node_type(controller_handle); > > if (ret != EFI_SUCCESS) > > goto err; > > - ret = bp->ops->bind(bp, controller_handle, interface); > > + > > + struct efi_handler *handler; > > + char tmpname[256] = "AppName"; > > + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, > > + &handler); > > + snprintf(tmpname, 256, "%pD", handler->protocol_interface); > > + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); > > if (ret == EFI_SUCCESS) > > goto out; > > > > diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c > > index a367e8b89d17..0ab8e4590dfe 100644 > > --- a/lib/efi_selftest/efi_selftest_block_device.c > > +++ b/lib/efi_selftest/efi_selftest_block_device.c > > @@ -248,6 +248,7 @@ static int teardown(void) > > { > > efi_status_t r = EFI_ST_SUCCESS; > > > > +#if 0 /* Temporarily out for confirmation */ > > if (disk_handle) { > > r = boottime->uninstall_protocol_interface(disk_handle, > > &guid_device_path, > > @@ -273,6 +274,7 @@ static int teardown(void) > > return EFI_ST_FAILURE; > > } > > } > > +#endif > > return r; > > } > > > > -- > > 2.41.0 > > > > Otherwise this looks good to me. We should have DM devices for all EFI > ones (in fact EFI ones should just be some extra data on top of DM > ones). Unfortunately, in this specific case (efi_block_device.c), UEFI object (handle) is set to be created first, then U-Boot device (efiblk#xxx). So "some extra data on top of DM ones" is not accurate (doesn't reflect the current implementation). Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c are totally different things. -Takahiro Akashi > > Regards, > Simon
On 7/19/23 03:08, Simon Glass wrote: > Hi AKASHI, > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: >> >> An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in >> EFI world, which in turn generates a corresponding U-Boot block device based on >> U-Boot's Driver Model. >> The latter device, however, doesn't work as U-Boot proper block device >> due to an issue in efi_driver's implementation. We saw discussions in the past, >> most recently in [1]. >> >> [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html >> >> This RFC patch tries to address (part of) the issue. >> If it is considered acceptable, I will create a formal patch. >> >> Withtout this patch, >> ===8<=== >> => env set efi_selftest 'block device' >> => bootefi selftest >> ... >> >> Summary: 0 failures >> >> => dm tree >> Class Index Probed Driver Name >> ----------------------------------------------------------- >> root 0 [ + ] root_driver root_driver >> ... >> bootmeth 7 [ ] vbe_simple | `-- vbe_simple >> blk 0 [ + ] efi_blk `-- efiblk#0 >> partition 0 [ + ] blk_partition `-- efiblk#0:1 >> => ls efiloader 0:1 >> ** Bad device specification efiloader 0 ** >> Couldn't find partition efiloader 0:1 >> ===>8=== >> >> With this patch applied, efiblk#0(:1) now gets accessible. >> >> ===8<=== >> => env set efi_selftest 'block device' >> => bootefi selftest >> ... >> >> Summary: 0 failures >> >> => dm tree >> Class Index Probed Driver Name >> ----------------------------------------------------------- >> root 0 [ + ] root_driver root_driver >> ... >> bootmeth 7 [ ] vbe_simple | `-- vbe_simple >> efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) >> blk 0 [ + ] efi_blk `-- efiblk#0 >> partition 0 [ + ] blk_partition `-- efiblk#0:1 >> => ls efiloader 0:1 >> 13 hello.txt >> 7 u-boot.txt >> >> 2 file(s), 0 dir(s) >> ===>8=== >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> include/efi_driver.h | 2 +- >> lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- >> lib/efi_driver/efi_uclass.c | 8 +++++++- >> lib/efi_selftest/efi_selftest_block_device.c | 2 ++ >> 4 files changed, 22 insertions(+), 7 deletions(-) >> >> diff --git a/include/efi_driver.h b/include/efi_driver.h >> index 63a95e4cf800..ed66796f3519 100644 >> --- a/include/efi_driver.h >> +++ b/include/efi_driver.h >> @@ -42,7 +42,7 @@ struct efi_driver_ops { >> const efi_guid_t *child_protocol; >> efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); >> efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, >> - efi_handle_t handle, void *interface); >> + efi_handle_t handle, void *interface, char *name); >> }; >> >> #endif /* _EFI_DRIVER_H */ >> diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c >> index add00eeebbea..43b7ed7c973c 100644 >> --- a/lib/efi_driver/efi_block_device.c >> +++ b/lib/efi_driver/efi_block_device.c >> @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, >> * Return: status code >> */ >> static efi_status_t >> -efi_bl_create_block_device(efi_handle_t handle, void *interface) >> +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) >> { >> - struct udevice *bdev = NULL, *parent = dm_root(); >> + struct udevice *bdev = NULL; >> efi_status_t ret; >> int devnum; >> char *name; >> @@ -181,7 +181,7 @@ err: >> */ >> static efi_status_t efi_bl_bind( >> struct efi_driver_binding_extended_protocol *this, >> - efi_handle_t handle, void *interface) >> + efi_handle_t handle, void *interface, char *name) >> { >> efi_status_t ret = EFI_SUCCESS; >> struct efi_object *obj = efi_search_obj(handle); >> @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( >> if (!obj || !interface) >> return EFI_INVALID_PARAMETER; >> >> - if (!handle->dev) >> - ret = efi_bl_create_block_device(handle, interface); >> + if (!handle->dev) { >> + struct udevice *parent; >> + >> + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > Can you use a non-root device as the parent? The parent of an iSCSI drive handled by iPXE will be a network interface. For a RAM drive there will be no physical parent at all. The design bug is in blk_get_devnum_by_uclass_idname() which instead of looking at blk_desc->uclass_id looks at the parent. When will you fix this Simon? Best regards Heinrich > >> + if (!ret) >> + ret = efi_bl_create_block_device(handle, interface, parent); >> + else >> + ret = EFI_DEVICE_ERROR; >> + } >> >> return ret; >> } >> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c >> index 45f935198874..bf669742783e 100644 >> --- a/lib/efi_driver/efi_uclass.c >> +++ b/lib/efi_driver/efi_uclass.c >> @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( >> ret = check_node_type(controller_handle); >> if (ret != EFI_SUCCESS) >> goto err; >> - ret = bp->ops->bind(bp, controller_handle, interface); >> + >> + struct efi_handler *handler; >> + char tmpname[256] = "AppName"; >> + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, >> + &handler); >> + snprintf(tmpname, 256, "%pD", handler->protocol_interface); >> + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); >> if (ret == EFI_SUCCESS) >> goto out; >> >> diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c >> index a367e8b89d17..0ab8e4590dfe 100644 >> --- a/lib/efi_selftest/efi_selftest_block_device.c >> +++ b/lib/efi_selftest/efi_selftest_block_device.c >> @@ -248,6 +248,7 @@ static int teardown(void) >> { >> efi_status_t r = EFI_ST_SUCCESS; >> >> +#if 0 /* Temporarily out for confirmation */ >> if (disk_handle) { >> r = boottime->uninstall_protocol_interface(disk_handle, >> &guid_device_path, >> @@ -273,6 +274,7 @@ static int teardown(void) >> return EFI_ST_FAILURE; >> } >> } >> +#endif >> return r; >> } >> >> -- >> 2.41.0 >> > > Otherwise this looks good to me. We should have DM devices for all EFI > ones (in fact EFI ones should just be some extra data on top of DM > ones). > > Regards, > Simon
Hi, On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Simon, > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > > Hi AKASHI, > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > > EFI world, which in turn generates a corresponding U-Boot block device based on > > > U-Boot's Driver Model. > > > The latter device, however, doesn't work as U-Boot proper block device > > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > > most recently in [1]. > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > > > This RFC patch tries to address (part of) the issue. > > > If it is considered acceptable, I will create a formal patch. > > > > > > Withtout this patch, > > > ===8<=== > > > => env set efi_selftest 'block device' > > > => bootefi selftest > > > ... > > > > > > Summary: 0 failures > > > > > > => dm tree > > > Class Index Probed Driver Name > > > ----------------------------------------------------------- > > > root 0 [ + ] root_driver root_driver > > > ... > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > => ls efiloader 0:1 > > > ** Bad device specification efiloader 0 ** > > > Couldn't find partition efiloader 0:1 > > > ===>8=== > > > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > > > ===8<=== > > > => env set efi_selftest 'block device' > > > => bootefi selftest > > > ... > > > > > > Summary: 0 failures > > > > > > => dm tree > > > Class Index Probed Driver Name > > > ----------------------------------------------------------- > > > root 0 [ + ] root_driver root_driver > > > ... > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > => ls efiloader 0:1 > > > 13 hello.txt > > > 7 u-boot.txt > > > > > > 2 file(s), 0 dir(s) > > > ===>8=== > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > include/efi_driver.h | 2 +- > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > > index 63a95e4cf800..ed66796f3519 100644 > > > --- a/include/efi_driver.h > > > +++ b/include/efi_driver.h > > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > > const efi_guid_t *child_protocol; > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > > - efi_handle_t handle, void *interface); > > > + efi_handle_t handle, void *interface, char *name); > > > }; > > > > > > #endif /* _EFI_DRIVER_H */ > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > > index add00eeebbea..43b7ed7c973c 100644 > > > --- a/lib/efi_driver/efi_block_device.c > > > +++ b/lib/efi_driver/efi_block_device.c > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > > * Return: status code > > > */ > > > static efi_status_t > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > > { > > > - struct udevice *bdev = NULL, *parent = dm_root(); > > > + struct udevice *bdev = NULL; > > > efi_status_t ret; > > > int devnum; > > > char *name; > > > @@ -181,7 +181,7 @@ err: > > > */ > > > static efi_status_t efi_bl_bind( > > > struct efi_driver_binding_extended_protocol *this, > > > - efi_handle_t handle, void *interface) > > > + efi_handle_t handle, void *interface, char *name) > > > { > > > efi_status_t ret = EFI_SUCCESS; > > > struct efi_object *obj = efi_search_obj(handle); > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > > if (!obj || !interface) > > > return EFI_INVALID_PARAMETER; > > > > > > - if (!handle->dev) > > > - ret = efi_bl_create_block_device(handle, interface); > > > + if (!handle->dev) { > > > + struct udevice *parent; > > > + > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > > > Can you use a non-root device as the parent? > > I have no idea what else can be the parent in this case. I suggest an EFI_MEDIA device. > > Please note: > > > => dm tree > > > Class Index Probed Driver Name > > > ----------------------------------------------------------- > > > root 0 [ + ] root_driver root_driver > > > ... > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) > and don't have any practical parent. Block devices must have a media device as their parent. This seems to be a persistent area of confusion...probably when the uclass ID goes away from blk_desc it will be more obvious. > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > + if (!ret) > > > + ret = efi_bl_create_block_device(handle, interface, parent); > > > + else > > > + ret = EFI_DEVICE_ERROR; > > > + } > > > > > > return ret; > > > } > > > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > > > index 45f935198874..bf669742783e 100644 > > > --- a/lib/efi_driver/efi_uclass.c > > > +++ b/lib/efi_driver/efi_uclass.c > > > @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( > > > ret = check_node_type(controller_handle); > > > if (ret != EFI_SUCCESS) > > > goto err; > > > - ret = bp->ops->bind(bp, controller_handle, interface); > > > + > > > + struct efi_handler *handler; > > > + char tmpname[256] = "AppName"; > > > + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, > > > + &handler); > > > + snprintf(tmpname, 256, "%pD", handler->protocol_interface); > > > + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); > > > if (ret == EFI_SUCCESS) > > > goto out; > > > > > > diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c > > > index a367e8b89d17..0ab8e4590dfe 100644 > > > --- a/lib/efi_selftest/efi_selftest_block_device.c > > > +++ b/lib/efi_selftest/efi_selftest_block_device.c > > > @@ -248,6 +248,7 @@ static int teardown(void) > > > { > > > efi_status_t r = EFI_ST_SUCCESS; > > > > > > +#if 0 /* Temporarily out for confirmation */ > > > if (disk_handle) { > > > r = boottime->uninstall_protocol_interface(disk_handle, > > > &guid_device_path, > > > @@ -273,6 +274,7 @@ static int teardown(void) > > > return EFI_ST_FAILURE; > > > } > > > } > > > +#endif > > > return r; > > > } > > > > > > -- > > > 2.41.0 > > > > > > > Otherwise this looks good to me. We should have DM devices for all EFI > > ones (in fact EFI ones should just be some extra data on top of DM > > ones). > > Unfortunately, in this specific case (efi_block_device.c), UEFI object > (handle) is set to be created first, then U-Boot device (efiblk#xxx). > So "some extra data on top of DM ones" is not accurate (doesn't reflect > the current implementation). OK, so we should really sort that out :-) > > Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c > are totally different things. OK Regards, Simon
On 19.07.23 15:04, Simon Glass wrote: > Hi, > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: >> >> Hi Simon, >> >> On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: >>> Hi AKASHI, >>> >>> On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro >>> <takahiro.akashi@linaro.org> wrote: >>>> >>>> An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in >>>> EFI world, which in turn generates a corresponding U-Boot block device based on >>>> U-Boot's Driver Model. >>>> The latter device, however, doesn't work as U-Boot proper block device >>>> due to an issue in efi_driver's implementation. We saw discussions in the past, >>>> most recently in [1]. >>>> >>>> [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html >>>> >>>> This RFC patch tries to address (part of) the issue. >>>> If it is considered acceptable, I will create a formal patch. >>>> >>>> Withtout this patch, >>>> ===8<=== >>>> => env set efi_selftest 'block device' >>>> => bootefi selftest >>>> ... >>>> >>>> Summary: 0 failures >>>> >>>> => dm tree >>>> Class Index Probed Driver Name >>>> ----------------------------------------------------------- >>>> root 0 [ + ] root_driver root_driver >>>> ... >>>> bootmeth 7 [ ] vbe_simple | `-- vbe_simple >>>> blk 0 [ + ] efi_blk `-- efiblk#0 >>>> partition 0 [ + ] blk_partition `-- efiblk#0:1 >>>> => ls efiloader 0:1 >>>> ** Bad device specification efiloader 0 ** >>>> Couldn't find partition efiloader 0:1 >>>> ===>8=== >>>> >>>> With this patch applied, efiblk#0(:1) now gets accessible. >>>> >>>> ===8<=== >>>> => env set efi_selftest 'block device' >>>> => bootefi selftest >>>> ... >>>> >>>> Summary: 0 failures >>>> >>>> => dm tree >>>> Class Index Probed Driver Name >>>> ----------------------------------------------------------- >>>> root 0 [ + ] root_driver root_driver >>>> ... >>>> bootmeth 7 [ ] vbe_simple | `-- vbe_simple >>>> efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) >>>> blk 0 [ + ] efi_blk `-- efiblk#0 >>>> partition 0 [ + ] blk_partition `-- efiblk#0:1 >>>> => ls efiloader 0:1 >>>> 13 hello.txt >>>> 7 u-boot.txt >>>> >>>> 2 file(s), 0 dir(s) >>>> ===>8=== >>>> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>> --- >>>> include/efi_driver.h | 2 +- >>>> lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- >>>> lib/efi_driver/efi_uclass.c | 8 +++++++- >>>> lib/efi_selftest/efi_selftest_block_device.c | 2 ++ >>>> 4 files changed, 22 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/include/efi_driver.h b/include/efi_driver.h >>>> index 63a95e4cf800..ed66796f3519 100644 >>>> --- a/include/efi_driver.h >>>> +++ b/include/efi_driver.h >>>> @@ -42,7 +42,7 @@ struct efi_driver_ops { >>>> const efi_guid_t *child_protocol; >>>> efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); >>>> efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, >>>> - efi_handle_t handle, void *interface); >>>> + efi_handle_t handle, void *interface, char *name); >>>> }; >>>> >>>> #endif /* _EFI_DRIVER_H */ >>>> diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c >>>> index add00eeebbea..43b7ed7c973c 100644 >>>> --- a/lib/efi_driver/efi_block_device.c >>>> +++ b/lib/efi_driver/efi_block_device.c >>>> @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, >>>> * Return: status code >>>> */ >>>> static efi_status_t >>>> -efi_bl_create_block_device(efi_handle_t handle, void *interface) >>>> +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) >>>> { >>>> - struct udevice *bdev = NULL, *parent = dm_root(); >>>> + struct udevice *bdev = NULL; >>>> efi_status_t ret; >>>> int devnum; >>>> char *name; >>>> @@ -181,7 +181,7 @@ err: >>>> */ >>>> static efi_status_t efi_bl_bind( >>>> struct efi_driver_binding_extended_protocol *this, >>>> - efi_handle_t handle, void *interface) >>>> + efi_handle_t handle, void *interface, char *name) >>>> { >>>> efi_status_t ret = EFI_SUCCESS; >>>> struct efi_object *obj = efi_search_obj(handle); >>>> @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( >>>> if (!obj || !interface) >>>> return EFI_INVALID_PARAMETER; >>>> >>>> - if (!handle->dev) >>>> - ret = efi_bl_create_block_device(handle, interface); >>>> + if (!handle->dev) { >>>> + struct udevice *parent; >>>> + >>>> + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); >>> >>> Can you use a non-root device as the parent? >> >> I have no idea what else can be the parent in this case. > > I suggest an EFI_MEDIA device. > >> >> Please note: >>>> => dm tree >>>> Class Index Probed Driver Name >>>> ----------------------------------------------------------- >>>> root 0 [ + ] root_driver root_driver >>>> ... >>>> bootmeth 7 [ ] vbe_simple | `-- vbe_simple >>>> efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) >> >> This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) >> and don't have any practical parent. > > Block devices must have a media device as their parent. This seems to > be a persistent area of confusion...probably when the uclass ID goes > away from blk_desc it will be more obvious. Dear Simon, The only reason why you request to add an otherwise parent device is that you use it to determine the device class name used in the CLI (mmc, usb, nvme, ...). That concept worked fine when all devices had physical parents from which such an information could be derived. This is not the case UCLASS_EFI block devices. We should not introduce any DM devices which have no meaning in the EFI world. If there is no other benefit, we should do the reasonable and keep a field in blk_desc and use it to derive the CLI name of the block device. Best regards Heinrich > >> >>>> blk 0 [ + ] efi_blk `-- efiblk#0 >>>> partition 0 [ + ] blk_partition `-- efiblk#0:1 >> >> >>>> + if (!ret) >>>> + ret = efi_bl_create_block_device(handle, interface, parent); >>>> + else >>>> + ret = EFI_DEVICE_ERROR; >>>> + } >>>> >>>> return ret; >>>> } >>>> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c >>>> index 45f935198874..bf669742783e 100644 >>>> --- a/lib/efi_driver/efi_uclass.c >>>> +++ b/lib/efi_driver/efi_uclass.c >>>> @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( >>>> ret = check_node_type(controller_handle); >>>> if (ret != EFI_SUCCESS) >>>> goto err; >>>> - ret = bp->ops->bind(bp, controller_handle, interface); >>>> + >>>> + struct efi_handler *handler; >>>> + char tmpname[256] = "AppName"; >>>> + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, >>>> + &handler); >>>> + snprintf(tmpname, 256, "%pD", handler->protocol_interface); >>>> + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); >>>> if (ret == EFI_SUCCESS) >>>> goto out; >>>> >>>> diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c >>>> index a367e8b89d17..0ab8e4590dfe 100644 >>>> --- a/lib/efi_selftest/efi_selftest_block_device.c >>>> +++ b/lib/efi_selftest/efi_selftest_block_device.c >>>> @@ -248,6 +248,7 @@ static int teardown(void) >>>> { >>>> efi_status_t r = EFI_ST_SUCCESS; >>>> >>>> +#if 0 /* Temporarily out for confirmation */ >>>> if (disk_handle) { >>>> r = boottime->uninstall_protocol_interface(disk_handle, >>>> &guid_device_path, >>>> @@ -273,6 +274,7 @@ static int teardown(void) >>>> return EFI_ST_FAILURE; >>>> } >>>> } >>>> +#endif >>>> return r; >>>> } >>>> >>>> -- >>>> 2.41.0 >>>> >>> >>> Otherwise this looks good to me. We should have DM devices for all EFI >>> ones (in fact EFI ones should just be some extra data on top of DM >>> ones). >> >> Unfortunately, in this specific case (efi_block_device.c), UEFI object >> (handle) is set to be created first, then U-Boot device (efiblk#xxx). >> So "some extra data on top of DM ones" is not accurate (doesn't reflect >> the current implementation). > > OK, so we should really sort that out :-) > >> >> Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c >> are totally different things. > > OK > > Regards, > Simon
On Wed, Jul 19, 2023 at 03:15:10PM +0200, Heinrich Schuchardt wrote: > On 19.07.23 15:04, Simon Glass wrote: > > Hi, > > > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > > > > Hi AKASHI, > > > > > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > > > > EFI world, which in turn generates a corresponding U-Boot block device based on > > > > > U-Boot's Driver Model. > > > > > The latter device, however, doesn't work as U-Boot proper block device > > > > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > > > > most recently in [1]. > > > > > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > > > > > > > This RFC patch tries to address (part of) the issue. > > > > > If it is considered acceptable, I will create a formal patch. > > > > > > > > > > Withtout this patch, > > > > > ===8<=== > > > > > => env set efi_selftest 'block device' > > > > > => bootefi selftest > > > > > ... > > > > > > > > > > Summary: 0 failures > > > > > > > > > > => dm tree > > > > > Class Index Probed Driver Name > > > > > ----------------------------------------------------------- > > > > > root 0 [ + ] root_driver root_driver > > > > > ... > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > => ls efiloader 0:1 > > > > > ** Bad device specification efiloader 0 ** > > > > > Couldn't find partition efiloader 0:1 > > > > > ===>8=== > > > > > > > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > > > > > > > ===8<=== > > > > > => env set efi_selftest 'block device' > > > > > => bootefi selftest > > > > > ... > > > > > > > > > > Summary: 0 failures > > > > > > > > > > => dm tree > > > > > Class Index Probed Driver Name > > > > > ----------------------------------------------------------- > > > > > root 0 [ + ] root_driver root_driver > > > > > ... > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > => ls efiloader 0:1 > > > > > 13 hello.txt > > > > > 7 u-boot.txt > > > > > > > > > > 2 file(s), 0 dir(s) > > > > > ===>8=== > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > --- > > > > > include/efi_driver.h | 2 +- > > > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > > > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > > > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > > > > index 63a95e4cf800..ed66796f3519 100644 > > > > > --- a/include/efi_driver.h > > > > > +++ b/include/efi_driver.h > > > > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > > > > const efi_guid_t *child_protocol; > > > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > > > > - efi_handle_t handle, void *interface); > > > > > + efi_handle_t handle, void *interface, char *name); > > > > > }; > > > > > > > > > > #endif /* _EFI_DRIVER_H */ > > > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > > > > index add00eeebbea..43b7ed7c973c 100644 > > > > > --- a/lib/efi_driver/efi_block_device.c > > > > > +++ b/lib/efi_driver/efi_block_device.c > > > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > > > > * Return: status code > > > > > */ > > > > > static efi_status_t > > > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > > > > { > > > > > - struct udevice *bdev = NULL, *parent = dm_root(); > > > > > + struct udevice *bdev = NULL; > > > > > efi_status_t ret; > > > > > int devnum; > > > > > char *name; > > > > > @@ -181,7 +181,7 @@ err: > > > > > */ > > > > > static efi_status_t efi_bl_bind( > > > > > struct efi_driver_binding_extended_protocol *this, > > > > > - efi_handle_t handle, void *interface) > > > > > + efi_handle_t handle, void *interface, char *name) > > > > > { > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > struct efi_object *obj = efi_search_obj(handle); > > > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > > > > if (!obj || !interface) > > > > > return EFI_INVALID_PARAMETER; > > > > > > > > > > - if (!handle->dev) > > > > > - ret = efi_bl_create_block_device(handle, interface); > > > > > + if (!handle->dev) { > > > > > + struct udevice *parent; > > > > > + > > > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > > > > > > > Can you use a non-root device as the parent? > > > > > > I have no idea what else can be the parent in this case. > > > > I suggest an EFI_MEDIA device. > > > > > > > > Please note: > > > > > => dm tree > > > > > Class Index Probed Driver Name > > > > > ----------------------------------------------------------- > > > > > root 0 [ + ] root_driver root_driver > > > > > ... > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) > > > and don't have any practical parent. > > > > Block devices must have a media device as their parent. This seems to > > be a persistent area of confusion...probably when the uclass ID goes > > away from blk_desc it will be more obvious. > > Dear Simon, > > The only reason why you request to add an otherwise parent device is > that you use it to determine the device class name used in the CLI (mmc, > usb, nvme, ...). > > That concept worked fine when all devices had physical parents from > which such an information could be derived. > > This is not the case UCLASS_EFI block devices. We should not introduce > any DM devices which have no meaning in the EFI world. Regarding my RFC patch, I have not invented any new DM device, instead I reuse the existing one, UCLASS_EFI_LOADER, which strangely never appears in DM tree under the current implementation. With my patch, a new instance (device) is created and associated with a "controller handle" (in UEFI jargon) which is passed on to EFI_DRIVER_BINDING_PROTOCOL.start() by a UEFI app. So the hierarchy looks like: ROOT UCLASS_EFI_LOADER - controller UCLASS_BLK - raw device UCLASS_PARTITION - partition It seems to me that it perfectly matches to DM concept. It has nothing different from other ordinary block devices. > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) The guid here is exactly what you gave to the controller handle (disk_handle) in your lib/efi_selftest/efi_selftest_block_device.c. -Takahiro Akashi > If there is no other benefit, we should do the reasonable and keep a > field in blk_desc and use it to derive the CLI name of the block device. > > Best regards > > Heinrich > > > > > > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > > > > > > + if (!ret) > > > > > + ret = efi_bl_create_block_device(handle, interface, parent); > > > > > + else > > > > > + ret = EFI_DEVICE_ERROR; > > > > > + } > > > > > > > > > > return ret; > > > > > } > > > > > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > > > > > index 45f935198874..bf669742783e 100644 > > > > > --- a/lib/efi_driver/efi_uclass.c > > > > > +++ b/lib/efi_driver/efi_uclass.c > > > > > @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( > > > > > ret = check_node_type(controller_handle); > > > > > if (ret != EFI_SUCCESS) > > > > > goto err; > > > > > - ret = bp->ops->bind(bp, controller_handle, interface); > > > > > + > > > > > + struct efi_handler *handler; > > > > > + char tmpname[256] = "AppName"; > > > > > + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, > > > > > + &handler); > > > > > + snprintf(tmpname, 256, "%pD", handler->protocol_interface); > > > > > + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); > > > > > if (ret == EFI_SUCCESS) > > > > > goto out; > > > > > > > > > > diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c > > > > > index a367e8b89d17..0ab8e4590dfe 100644 > > > > > --- a/lib/efi_selftest/efi_selftest_block_device.c > > > > > +++ b/lib/efi_selftest/efi_selftest_block_device.c > > > > > @@ -248,6 +248,7 @@ static int teardown(void) > > > > > { > > > > > efi_status_t r = EFI_ST_SUCCESS; > > > > > > > > > > +#if 0 /* Temporarily out for confirmation */ > > > > > if (disk_handle) { > > > > > r = boottime->uninstall_protocol_interface(disk_handle, > > > > > &guid_device_path, > > > > > @@ -273,6 +274,7 @@ static int teardown(void) > > > > > return EFI_ST_FAILURE; > > > > > } > > > > > } > > > > > +#endif > > > > > return r; > > > > > } > > > > > > > > > > -- > > > > > 2.41.0 > > > > > > > > > > > > > Otherwise this looks good to me. We should have DM devices for all EFI > > > > ones (in fact EFI ones should just be some extra data on top of DM > > > > ones). > > > > > > Unfortunately, in this specific case (efi_block_device.c), UEFI object > > > (handle) is set to be created first, then U-Boot device (efiblk#xxx). > > > So "some extra data on top of DM ones" is not accurate (doesn't reflect > > > the current implementation). > > > > OK, so we should really sort that out :-) > > > > > > > > Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c > > > are totally different things. > > > > OK > > > > Regards, > > Simon >
Hi Simon, On Wed, Jul 19, 2023 at 07:04:06AM -0600, Simon Glass wrote: > Hi, > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Hi Simon, > > > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > > > Hi AKASHI, > > > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > > > EFI world, which in turn generates a corresponding U-Boot block device based on > > > > U-Boot's Driver Model. > > > > The latter device, however, doesn't work as U-Boot proper block device > > > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > > > most recently in [1]. > > > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > > > > > This RFC patch tries to address (part of) the issue. > > > > If it is considered acceptable, I will create a formal patch. > > > > > > > > Withtout this patch, > > > > ===8<=== > > > > => env set efi_selftest 'block device' > > > > => bootefi selftest > > > > ... > > > > > > > > Summary: 0 failures > > > > > > > > => dm tree > > > > Class Index Probed Driver Name > > > > ----------------------------------------------------------- > > > > root 0 [ + ] root_driver root_driver > > > > ... > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > => ls efiloader 0:1 > > > > ** Bad device specification efiloader 0 ** > > > > Couldn't find partition efiloader 0:1 > > > > ===>8=== > > > > > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > > > > > ===8<=== > > > > => env set efi_selftest 'block device' > > > > => bootefi selftest > > > > ... > > > > > > > > Summary: 0 failures > > > > > > > > => dm tree > > > > Class Index Probed Driver Name > > > > ----------------------------------------------------------- > > > > root 0 [ + ] root_driver root_driver > > > > ... > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > => ls efiloader 0:1 > > > > 13 hello.txt > > > > 7 u-boot.txt > > > > > > > > 2 file(s), 0 dir(s) > > > > ===>8=== > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > include/efi_driver.h | 2 +- > > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > > > index 63a95e4cf800..ed66796f3519 100644 > > > > --- a/include/efi_driver.h > > > > +++ b/include/efi_driver.h > > > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > > > const efi_guid_t *child_protocol; > > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > > > - efi_handle_t handle, void *interface); > > > > + efi_handle_t handle, void *interface, char *name); > > > > }; > > > > > > > > #endif /* _EFI_DRIVER_H */ > > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > > > index add00eeebbea..43b7ed7c973c 100644 > > > > --- a/lib/efi_driver/efi_block_device.c > > > > +++ b/lib/efi_driver/efi_block_device.c > > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > > > * Return: status code > > > > */ > > > > static efi_status_t > > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > > > { > > > > - struct udevice *bdev = NULL, *parent = dm_root(); > > > > + struct udevice *bdev = NULL; > > > > efi_status_t ret; > > > > int devnum; > > > > char *name; > > > > @@ -181,7 +181,7 @@ err: > > > > */ > > > > static efi_status_t efi_bl_bind( > > > > struct efi_driver_binding_extended_protocol *this, > > > > - efi_handle_t handle, void *interface) > > > > + efi_handle_t handle, void *interface, char *name) > > > > { > > > > efi_status_t ret = EFI_SUCCESS; > > > > struct efi_object *obj = efi_search_obj(handle); > > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > > > if (!obj || !interface) > > > > return EFI_INVALID_PARAMETER; > > > > > > > > - if (!handle->dev) > > > > - ret = efi_bl_create_block_device(handle, interface); > > > > + if (!handle->dev) { > > > > + struct udevice *parent; > > > > + > > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > > > > > Can you use a non-root device as the parent? > > > > I have no idea what else can be the parent in this case. > > I suggest an EFI_MEDIA device. > > > > > Please note: > > > > => dm tree > > > > Class Index Probed Driver Name > > > > ----------------------------------------------------------- > > > > root 0 [ + ] root_driver root_driver > > > > ... > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) > > and don't have any practical parent. > > Block devices must have a media device as their parent. This seems to > be a persistent area of confusion...probably when the uclass ID goes > away from blk_desc it will be more obvious. Please refer to my other comment to Heinrich in this thread. UCLASS_EFI_LOADER works exactly as UCLASS_EFI_MEDIA does as a "media" device which is set to invoke "bind" for associated devices (blocks in this case). -Takahiro Akashi > > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > > > + if (!ret) > > > > + ret = efi_bl_create_block_device(handle, interface, parent); > > > > + else > > > > + ret = EFI_DEVICE_ERROR; > > > > + } > > > > > > > > return ret; > > > > } > > > > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > > > > index 45f935198874..bf669742783e 100644 > > > > --- a/lib/efi_driver/efi_uclass.c > > > > +++ b/lib/efi_driver/efi_uclass.c > > > > @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( > > > > ret = check_node_type(controller_handle); > > > > if (ret != EFI_SUCCESS) > > > > goto err; > > > > - ret = bp->ops->bind(bp, controller_handle, interface); > > > > + > > > > + struct efi_handler *handler; > > > > + char tmpname[256] = "AppName"; > > > > + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, > > > > + &handler); > > > > + snprintf(tmpname, 256, "%pD", handler->protocol_interface); > > > > + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); > > > > if (ret == EFI_SUCCESS) > > > > goto out; > > > > > > > > diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c > > > > index a367e8b89d17..0ab8e4590dfe 100644 > > > > --- a/lib/efi_selftest/efi_selftest_block_device.c > > > > +++ b/lib/efi_selftest/efi_selftest_block_device.c > > > > @@ -248,6 +248,7 @@ static int teardown(void) > > > > { > > > > efi_status_t r = EFI_ST_SUCCESS; > > > > > > > > +#if 0 /* Temporarily out for confirmation */ > > > > if (disk_handle) { > > > > r = boottime->uninstall_protocol_interface(disk_handle, > > > > &guid_device_path, > > > > @@ -273,6 +274,7 @@ static int teardown(void) > > > > return EFI_ST_FAILURE; > > > > } > > > > } > > > > +#endif > > > > return r; > > > > } > > > > > > > > -- > > > > 2.41.0 > > > > > > > > > > Otherwise this looks good to me. We should have DM devices for all EFI > > > ones (in fact EFI ones should just be some extra data on top of DM > > > ones). > > > > Unfortunately, in this specific case (efi_block_device.c), UEFI object > > (handle) is set to be created first, then U-Boot device (efiblk#xxx). > > So "some extra data on top of DM ones" is not accurate (doesn't reflect > > the current implementation). > > OK, so we should really sort that out :-) > > > > > Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c > > are totally different things. > > OK > > Regards, > Simon
Hi, On Wed, 19 Jul 2023 at 18:14, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Wed, Jul 19, 2023 at 03:15:10PM +0200, Heinrich Schuchardt wrote: > > On 19.07.23 15:04, Simon Glass wrote: > > > Hi, > > > > > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > > > > > Hi AKASHI, > > > > > > > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > > > > > EFI world, which in turn generates a corresponding U-Boot block device based on > > > > > > U-Boot's Driver Model. > > > > > > The latter device, however, doesn't work as U-Boot proper block device > > > > > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > > > > > most recently in [1]. > > > > > > > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > > > > > > > > > This RFC patch tries to address (part of) the issue. > > > > > > If it is considered acceptable, I will create a formal patch. > > > > > > > > > > > > Withtout this patch, > > > > > > ===8<=== > > > > > > => env set efi_selftest 'block device' > > > > > > => bootefi selftest > > > > > > ... > > > > > > > > > > > > Summary: 0 failures > > > > > > > > > > > > => dm tree > > > > > > Class Index Probed Driver Name > > > > > > ----------------------------------------------------------- > > > > > > root 0 [ + ] root_driver root_driver > > > > > > ... > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > => ls efiloader 0:1 > > > > > > ** Bad device specification efiloader 0 ** > > > > > > Couldn't find partition efiloader 0:1 > > > > > > ===>8=== > > > > > > > > > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > > > > > > > > > ===8<=== > > > > > > => env set efi_selftest 'block device' > > > > > > => bootefi selftest > > > > > > ... > > > > > > > > > > > > Summary: 0 failures > > > > > > > > > > > > => dm tree > > > > > > Class Index Probed Driver Name > > > > > > ----------------------------------------------------------- > > > > > > root 0 [ + ] root_driver root_driver > > > > > > ... > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > => ls efiloader 0:1 > > > > > > 13 hello.txt > > > > > > 7 u-boot.txt > > > > > > > > > > > > 2 file(s), 0 dir(s) > > > > > > ===>8=== > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > --- > > > > > > include/efi_driver.h | 2 +- > > > > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > > > > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > > > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > > > > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > > > > > index 63a95e4cf800..ed66796f3519 100644 > > > > > > --- a/include/efi_driver.h > > > > > > +++ b/include/efi_driver.h > > > > > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > > > > > const efi_guid_t *child_protocol; > > > > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > > > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > > > > > - efi_handle_t handle, void *interface); > > > > > > + efi_handle_t handle, void *interface, char *name); > > > > > > }; > > > > > > > > > > > > #endif /* _EFI_DRIVER_H */ > > > > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > > > > > index add00eeebbea..43b7ed7c973c 100644 > > > > > > --- a/lib/efi_driver/efi_block_device.c > > > > > > +++ b/lib/efi_driver/efi_block_device.c > > > > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > > > > > * Return: status code > > > > > > */ > > > > > > static efi_status_t > > > > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > > > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > > > > > { > > > > > > - struct udevice *bdev = NULL, *parent = dm_root(); > > > > > > + struct udevice *bdev = NULL; > > > > > > efi_status_t ret; > > > > > > int devnum; > > > > > > char *name; > > > > > > @@ -181,7 +181,7 @@ err: > > > > > > */ > > > > > > static efi_status_t efi_bl_bind( > > > > > > struct efi_driver_binding_extended_protocol *this, > > > > > > - efi_handle_t handle, void *interface) > > > > > > + efi_handle_t handle, void *interface, char *name) > > > > > > { > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > struct efi_object *obj = efi_search_obj(handle); > > > > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > > > > > if (!obj || !interface) > > > > > > return EFI_INVALID_PARAMETER; > > > > > > > > > > > > - if (!handle->dev) > > > > > > - ret = efi_bl_create_block_device(handle, interface); > > > > > > + if (!handle->dev) { > > > > > > + struct udevice *parent; > > > > > > + > > > > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > > > > > > > > > Can you use a non-root device as the parent? > > > > > > > > I have no idea what else can be the parent in this case. > > > > > > I suggest an EFI_MEDIA device. > > > > > > > > > > > Please note: > > > > > > => dm tree > > > > > > Class Index Probed Driver Name > > > > > > ----------------------------------------------------------- > > > > > > root 0 [ + ] root_driver root_driver > > > > > > ... > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) > > > > and don't have any practical parent. > > > > > > Block devices must have a media device as their parent. This seems to > > > be a persistent area of confusion...probably when the uclass ID goes > > > away from blk_desc it will be more obvious. > > > > Dear Simon, > > > > The only reason why you request to add an otherwise parent device is > > that you use it to determine the device class name used in the CLI (mmc, > > usb, nvme, ...). Yes and also (at present) we number the devices within their uclass, so that we can have a block device 0 for both mmc and nvme, for example. > > > > That concept worked fine when all devices had physical parents from > > which such an information could be derived. > > > > This is not the case UCLASS_EFI block devices. We should not introduce > > any DM devices which have no meaning in the EFI world. Actually I feel the opposite. EFI should just be using DM devices. If they don't exist, create them. EFI cannot be a parallel universe. > > Regarding my RFC patch, I have not invented any new DM device, instead > I reuse the existing one, UCLASS_EFI_LOADER, which strangely never appears > in DM tree under the current implementation. > > With my patch, a new instance (device) is created and associated with > a "controller handle" (in UEFI jargon) which is passed on to > EFI_DRIVER_BINDING_PROTOCOL.start() by a UEFI app. > So the hierarchy looks like: > ROOT > UCLASS_EFI_LOADER - controller > UCLASS_BLK - raw device > UCLASS_PARTITION - partition > > It seems to me that it perfectly matches to DM concept. > It has nothing different from other ordinary block devices. Yes that seems fine to me. I'm sorry that I misunderstood that. Should we use EFI_MEDIA instead of EFI_LOADER? > > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > The guid here is exactly what you gave to the controller handle > (disk_handle) in your lib/efi_selftest/efi_selftest_block_device.c. What does it mean? We need to use names instead of GUIDs. I never want a guid to be shown to the poor, hard-pressed, confused user. It would be like using a sha256 hash instead of a filename. > > -Takahiro Akashi > > > If there is no other benefit, we should do the reasonable and keep a > > field in blk_desc and use it to derive the CLI name of the block device. I don't see any down-side to having a parent device like EFI_MEDIA. Is there one? > > > > Best regards > > > > Heinrich > > Regards, Simon
On Wed, Jul 19, 2023 at 07:29:57PM -0600, Simon Glass wrote: > Hi, > > On Wed, 19 Jul 2023 at 18:14, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > On Wed, Jul 19, 2023 at 03:15:10PM +0200, Heinrich Schuchardt wrote: > > > On 19.07.23 15:04, Simon Glass wrote: > > > > Hi, > > > > > > > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > > > > > > Hi AKASHI, > > > > > > > > > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > > > > > > EFI world, which in turn generates a corresponding U-Boot block device based on > > > > > > > U-Boot's Driver Model. > > > > > > > The latter device, however, doesn't work as U-Boot proper block device > > > > > > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > > > > > > most recently in [1]. > > > > > > > > > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > > > > > > > > > > > This RFC patch tries to address (part of) the issue. > > > > > > > If it is considered acceptable, I will create a formal patch. > > > > > > > > > > > > > > Withtout this patch, > > > > > > > ===8<=== > > > > > > > => env set efi_selftest 'block device' > > > > > > > => bootefi selftest > > > > > > > ... > > > > > > > > > > > > > > Summary: 0 failures > > > > > > > > > > > > > > => dm tree > > > > > > > Class Index Probed Driver Name > > > > > > > ----------------------------------------------------------- > > > > > > > root 0 [ + ] root_driver root_driver > > > > > > > ... > > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > > => ls efiloader 0:1 > > > > > > > ** Bad device specification efiloader 0 ** > > > > > > > Couldn't find partition efiloader 0:1 > > > > > > > ===>8=== > > > > > > > > > > > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > > > > > > > > > > > ===8<=== > > > > > > > => env set efi_selftest 'block device' > > > > > > > => bootefi selftest > > > > > > > ... > > > > > > > > > > > > > > Summary: 0 failures > > > > > > > > > > > > > > => dm tree > > > > > > > Class Index Probed Driver Name > > > > > > > ----------------------------------------------------------- > > > > > > > root 0 [ + ] root_driver root_driver > > > > > > > ... > > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > > => ls efiloader 0:1 > > > > > > > 13 hello.txt > > > > > > > 7 u-boot.txt > > > > > > > > > > > > > > 2 file(s), 0 dir(s) > > > > > > > ===>8=== > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > --- > > > > > > > include/efi_driver.h | 2 +- > > > > > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > > > > > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > > > > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > > > > > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > > > > > > index 63a95e4cf800..ed66796f3519 100644 > > > > > > > --- a/include/efi_driver.h > > > > > > > +++ b/include/efi_driver.h > > > > > > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > > > > > > const efi_guid_t *child_protocol; > > > > > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > > > > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > > > > > > - efi_handle_t handle, void *interface); > > > > > > > + efi_handle_t handle, void *interface, char *name); > > > > > > > }; > > > > > > > > > > > > > > #endif /* _EFI_DRIVER_H */ > > > > > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > > > > > > index add00eeebbea..43b7ed7c973c 100644 > > > > > > > --- a/lib/efi_driver/efi_block_device.c > > > > > > > +++ b/lib/efi_driver/efi_block_device.c > > > > > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > > > > > > * Return: status code > > > > > > > */ > > > > > > > static efi_status_t > > > > > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > > > > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > > > > > > { > > > > > > > - struct udevice *bdev = NULL, *parent = dm_root(); > > > > > > > + struct udevice *bdev = NULL; > > > > > > > efi_status_t ret; > > > > > > > int devnum; > > > > > > > char *name; > > > > > > > @@ -181,7 +181,7 @@ err: > > > > > > > */ > > > > > > > static efi_status_t efi_bl_bind( > > > > > > > struct efi_driver_binding_extended_protocol *this, > > > > > > > - efi_handle_t handle, void *interface) > > > > > > > + efi_handle_t handle, void *interface, char *name) > > > > > > > { > > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > > struct efi_object *obj = efi_search_obj(handle); > > > > > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > > > > > > if (!obj || !interface) > > > > > > > return EFI_INVALID_PARAMETER; > > > > > > > > > > > > > > - if (!handle->dev) > > > > > > > - ret = efi_bl_create_block_device(handle, interface); > > > > > > > + if (!handle->dev) { > > > > > > > + struct udevice *parent; > > > > > > > + > > > > > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > > > > > > > > > > > Can you use a non-root device as the parent? > > > > > > > > > > I have no idea what else can be the parent in this case. > > > > > > > > I suggest an EFI_MEDIA device. > > > > > > > > > > > > > > Please note: > > > > > > > => dm tree > > > > > > > Class Index Probed Driver Name > > > > > > > ----------------------------------------------------------- > > > > > > > root 0 [ + ] root_driver root_driver > > > > > > > ... > > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > > > > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) > > > > > and don't have any practical parent. > > > > > > > > Block devices must have a media device as their parent. This seems to > > > > be a persistent area of confusion...probably when the uclass ID goes > > > > away from blk_desc it will be more obvious. > > > > > > Dear Simon, > > > > > > The only reason why you request to add an otherwise parent device is > > > that you use it to determine the device class name used in the CLI (mmc, > > > usb, nvme, ...). > > Yes and also (at present) we number the devices within their uclass, > so that we can have a block device 0 for both mmc and nvme, for > example. > > > > > > > That concept worked fine when all devices had physical parents from > > > which such an information could be derived. > > > > > > This is not the case UCLASS_EFI block devices. We should not introduce > > > any DM devices which have no meaning in the EFI world. > > Actually I feel the opposite. EFI should just be using DM devices. If > they don't exist, create them. EFI cannot be a parallel universe. > > > > > Regarding my RFC patch, I have not invented any new DM device, instead > > I reuse the existing one, UCLASS_EFI_LOADER, which strangely never appears > > in DM tree under the current implementation. > > > > With my patch, a new instance (device) is created and associated with > > a "controller handle" (in UEFI jargon) which is passed on to > > EFI_DRIVER_BINDING_PROTOCOL.start() by a UEFI app. > > So the hierarchy looks like: > > ROOT > > UCLASS_EFI_LOADER - controller > > UCLASS_BLK - raw device > > UCLASS_PARTITION - partition > > > > It seems to me that it perfectly matches to DM concept. > > It has nothing different from other ordinary block devices. > > Yes that seems fine to me. I'm sorry that I misunderstood that. Should > we use EFI_MEDIA instead of EFI_LOADER? My understanding is as follows: UCLASS_EFI_MEDIA is usable when U-Boot is invoked as an UEFI application against the underlying UEFI firmware. On the other hand, UCLASS_EFI_LOADER serves an UEFI application loaded via U-Boot UEFI when U-Boot works as UEFI firmware. So I don't think that the two uclasses may not co-exist (or unified). > > > > > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > The guid here is exactly what you gave to the controller handle > > (disk_handle) in your lib/efi_selftest/efi_selftest_block_device.c. > > What does it mean? We need to use names instead of GUIDs. I never want > a guid to be shown to the poor, hard-pressed, confused user. It would > be like using a sha256 hash instead of a filename. Well, what matters here is not a guid, but a UEFI device path which represents an UEFI object uniquely in UEFI world. "/VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)" is a human-readable presentation for the device path given to the controller handle by an UEFI app (again, this is efi_selftest_block_device.c). Since it may be in another form (possibly without guid?) in another application, we have no control over the naming here. I even believe that using a device path as a udevice's name is sane because it is the only way to interwine the two worlds. -Takahiro Akashi > > > > > -Takahiro Akashi > > > > > If there is no other benefit, we should do the reasonable and keep a > > > field in blk_desc and use it to derive the CLI name of the block device. > > I don't see any down-side to having a parent device like EFI_MEDIA. Is > there one? > > > > > > > Best regards > > > > > > Heinrich > > > > > Regards, > Simon
Hi, On Wed, 19 Jul 2023 at 20:56, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Wed, Jul 19, 2023 at 07:29:57PM -0600, Simon Glass wrote: > > Hi, > > > > On Wed, 19 Jul 2023 at 18:14, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > On Wed, Jul 19, 2023 at 03:15:10PM +0200, Heinrich Schuchardt wrote: > > > > On 19.07.23 15:04, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > On Tue, Jul 18, 2023 at 07:08:45PM -0600, Simon Glass wrote: > > > > > > > Hi AKASHI, > > > > > > > > > > > > > > On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro > > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in > > > > > > > > EFI world, which in turn generates a corresponding U-Boot block device based on > > > > > > > > U-Boot's Driver Model. > > > > > > > > The latter device, however, doesn't work as U-Boot proper block device > > > > > > > > due to an issue in efi_driver's implementation. We saw discussions in the past, > > > > > > > > most recently in [1]. > > > > > > > > > > > > > > > > [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html > > > > > > > > > > > > > > > > This RFC patch tries to address (part of) the issue. > > > > > > > > If it is considered acceptable, I will create a formal patch. > > > > > > > > > > > > > > > > Withtout this patch, > > > > > > > > ===8<=== > > > > > > > > => env set efi_selftest 'block device' > > > > > > > > => bootefi selftest > > > > > > > > ... > > > > > > > > > > > > > > > > Summary: 0 failures > > > > > > > > > > > > > > > > => dm tree > > > > > > > > Class Index Probed Driver Name > > > > > > > > ----------------------------------------------------------- > > > > > > > > root 0 [ + ] root_driver root_driver > > > > > > > > ... > > > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > > > => ls efiloader 0:1 > > > > > > > > ** Bad device specification efiloader 0 ** > > > > > > > > Couldn't find partition efiloader 0:1 > > > > > > > > ===>8=== > > > > > > > > > > > > > > > > With this patch applied, efiblk#0(:1) now gets accessible. > > > > > > > > > > > > > > > > ===8<=== > > > > > > > > => env set efi_selftest 'block device' > > > > > > > > => bootefi selftest > > > > > > > > ... > > > > > > > > > > > > > > > > Summary: 0 failures > > > > > > > > > > > > > > > > => dm tree > > > > > > > > Class Index Probed Driver Name > > > > > > > > ----------------------------------------------------------- > > > > > > > > root 0 [ + ] root_driver root_driver > > > > > > > > ... > > > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > > > blk 0 [ + ] efi_blk `-- efiblk#0 > > > > > > > > partition 0 [ + ] blk_partition `-- efiblk#0:1 > > > > > > > > => ls efiloader 0:1 > > > > > > > > 13 hello.txt > > > > > > > > 7 u-boot.txt > > > > > > > > > > > > > > > > 2 file(s), 0 dir(s) > > > > > > > > ===>8=== > > > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > > --- > > > > > > > > include/efi_driver.h | 2 +- > > > > > > > > lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- > > > > > > > > lib/efi_driver/efi_uclass.c | 8 +++++++- > > > > > > > > lib/efi_selftest/efi_selftest_block_device.c | 2 ++ > > > > > > > > 4 files changed, 22 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > > > > > > > index 63a95e4cf800..ed66796f3519 100644 > > > > > > > > --- a/include/efi_driver.h > > > > > > > > +++ b/include/efi_driver.h > > > > > > > > @@ -42,7 +42,7 @@ struct efi_driver_ops { > > > > > > > > const efi_guid_t *child_protocol; > > > > > > > > efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); > > > > > > > > efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, > > > > > > > > - efi_handle_t handle, void *interface); > > > > > > > > + efi_handle_t handle, void *interface, char *name); > > > > > > > > }; > > > > > > > > > > > > > > > > #endif /* _EFI_DRIVER_H */ > > > > > > > > diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c > > > > > > > > index add00eeebbea..43b7ed7c973c 100644 > > > > > > > > --- a/lib/efi_driver/efi_block_device.c > > > > > > > > +++ b/lib/efi_driver/efi_block_device.c > > > > > > > > @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, > > > > > > > > * Return: status code > > > > > > > > */ > > > > > > > > static efi_status_t > > > > > > > > -efi_bl_create_block_device(efi_handle_t handle, void *interface) > > > > > > > > +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) > > > > > > > > { > > > > > > > > - struct udevice *bdev = NULL, *parent = dm_root(); > > > > > > > > + struct udevice *bdev = NULL; > > > > > > > > efi_status_t ret; > > > > > > > > int devnum; > > > > > > > > char *name; > > > > > > > > @@ -181,7 +181,7 @@ err: > > > > > > > > */ > > > > > > > > static efi_status_t efi_bl_bind( > > > > > > > > struct efi_driver_binding_extended_protocol *this, > > > > > > > > - efi_handle_t handle, void *interface) > > > > > > > > + efi_handle_t handle, void *interface, char *name) > > > > > > > > { > > > > > > > > efi_status_t ret = EFI_SUCCESS; > > > > > > > > struct efi_object *obj = efi_search_obj(handle); > > > > > > > > @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( > > > > > > > > if (!obj || !interface) > > > > > > > > return EFI_INVALID_PARAMETER; > > > > > > > > > > > > > > > > - if (!handle->dev) > > > > > > > > - ret = efi_bl_create_block_device(handle, interface); > > > > > > > > + if (!handle->dev) { > > > > > > > > + struct udevice *parent; > > > > > > > > + > > > > > > > > + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); > > > > > > > > > > > > > > Can you use a non-root device as the parent? > > > > > > > > > > > > I have no idea what else can be the parent in this case. > > > > > > > > > > I suggest an EFI_MEDIA device. > > > > > > > > > > > > > > > > > Please note: > > > > > > > > => dm tree > > > > > > > > Class Index Probed Driver Name > > > > > > > > ----------------------------------------------------------- > > > > > > > > root 0 [ + ] root_driver root_driver > > > > > > > > ... > > > > > > > > bootmeth 7 [ ] vbe_simple | `-- vbe_simple > > > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > > > > > > > This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c) > > > > > > and don't have any practical parent. > > > > > > > > > > Block devices must have a media device as their parent. This seems to > > > > > be a persistent area of confusion...probably when the uclass ID goes > > > > > away from blk_desc it will be more obvious. > > > > > > > > Dear Simon, > > > > > > > > The only reason why you request to add an otherwise parent device is > > > > that you use it to determine the device class name used in the CLI (mmc, > > > > usb, nvme, ...). > > > > Yes and also (at present) we number the devices within their uclass, > > so that we can have a block device 0 for both mmc and nvme, for > > example. > > > > > > > > > > That concept worked fine when all devices had physical parents from > > > > which such an information could be derived. > > > > > > > > This is not the case UCLASS_EFI block devices. We should not introduce > > > > any DM devices which have no meaning in the EFI world. > > > > Actually I feel the opposite. EFI should just be using DM devices. If > > they don't exist, create them. EFI cannot be a parallel universe. > > > > > > > > Regarding my RFC patch, I have not invented any new DM device, instead > > > I reuse the existing one, UCLASS_EFI_LOADER, which strangely never appears > > > in DM tree under the current implementation. > > > > > > With my patch, a new instance (device) is created and associated with > > > a "controller handle" (in UEFI jargon) which is passed on to > > > EFI_DRIVER_BINDING_PROTOCOL.start() by a UEFI app. > > > So the hierarchy looks like: > > > ROOT > > > UCLASS_EFI_LOADER - controller > > > UCLASS_BLK - raw device > > > UCLASS_PARTITION - partition > > > > > > It seems to me that it perfectly matches to DM concept. > > > It has nothing different from other ordinary block devices. > > > > Yes that seems fine to me. I'm sorry that I misunderstood that. Should > > we use EFI_MEDIA instead of EFI_LOADER? > > My understanding is as follows: > UCLASS_EFI_MEDIA is usable when U-Boot is invoked as an UEFI application against > the underlying UEFI firmware. On the other hand, UCLASS_EFI_LOADER serves an UEFI > application loaded via U-Boot UEFI when U-Boot works as UEFI firmware. > So I don't think that the two uclasses may not co-exist (or unified). It looks like EFI_LOADER is for strange EFI things - it messes around in the bowels of driver model in ways that it probably shouldn't. But it seems to be used as a media device: devnum = blk_next_free_devnum(UCLASS_EFI_LOADER) For EFI_MEDIA, yes it is used in the app, although I don't see a problem with it being used elsewhere. But we should settle on one, I think. > > > > > > > > > > > > > > efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) > > > > > > The guid here is exactly what you gave to the controller handle > > > (disk_handle) in your lib/efi_selftest/efi_selftest_block_device.c. > > > > What does it mean? We need to use names instead of GUIDs. I never want > > a guid to be shown to the poor, hard-pressed, confused user. It would > > be like using a sha256 hash instead of a filename. > > Well, what matters here is not a guid, but a UEFI device path which > represents an UEFI object uniquely in UEFI world. > > "/VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)" is a human-readable > presentation for the device path given to the controller handle > by an UEFI app (again, this is efi_selftest_block_device.c). > Since it may be in another form (possibly without guid?) in another > application, we have no control over the naming here. Really? We need a way to show meaningful names and not GUIDs. > > I even believe that using a device path as a udevice's name > is sane because it is the only way to interwine the two worlds. I very much want to avoid ever needing to read a GUID with driver model. Since there must be a corresponding driver model device for each EFI device, this should be possible. For example, we name MMC devices based on their DT node name, we can name other devices based on their uclass name and sequence number, block devices based on their parent, etc. Regards, Simon > > -Takahiro Akashi > > > > > > > > > -Takahiro Akashi > > > > > > > If there is no other benefit, we should do the reasonable and keep a > > > > field in blk_desc and use it to derive the CLI name of the block device. > > > > I don't see any down-side to having a parent device like EFI_MEDIA. Is > > there one? > > Regards, Simon
diff --git a/include/efi_driver.h b/include/efi_driver.h index 63a95e4cf800..ed66796f3519 100644 --- a/include/efi_driver.h +++ b/include/efi_driver.h @@ -42,7 +42,7 @@ struct efi_driver_ops { const efi_guid_t *child_protocol; efi_status_t (*init)(struct efi_driver_binding_extended_protocol *this); efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this, - efi_handle_t handle, void *interface); + efi_handle_t handle, void *interface, char *name); }; #endif /* _EFI_DRIVER_H */ diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c index add00eeebbea..43b7ed7c973c 100644 --- a/lib/efi_driver/efi_block_device.c +++ b/lib/efi_driver/efi_block_device.c @@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt, * Return: status code */ static efi_status_t -efi_bl_create_block_device(efi_handle_t handle, void *interface) +efi_bl_create_block_device(efi_handle_t handle, void *interface, struct udevice *parent) { - struct udevice *bdev = NULL, *parent = dm_root(); + struct udevice *bdev = NULL; efi_status_t ret; int devnum; char *name; @@ -181,7 +181,7 @@ err: */ static efi_status_t efi_bl_bind( struct efi_driver_binding_extended_protocol *this, - efi_handle_t handle, void *interface) + efi_handle_t handle, void *interface, char *name) { efi_status_t ret = EFI_SUCCESS; struct efi_object *obj = efi_search_obj(handle); @@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind( if (!obj || !interface) return EFI_INVALID_PARAMETER; - if (!handle->dev) - ret = efi_bl_create_block_device(handle, interface); + if (!handle->dev) { + struct udevice *parent; + + ret = device_bind_driver(dm_root(), "EFI block driver", name, &parent); + if (!ret) + ret = efi_bl_create_block_device(handle, interface, parent); + else + ret = EFI_DEVICE_ERROR; + } return ret; } diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c index 45f935198874..bf669742783e 100644 --- a/lib/efi_driver/efi_uclass.c +++ b/lib/efi_driver/efi_uclass.c @@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start( ret = check_node_type(controller_handle); if (ret != EFI_SUCCESS) goto err; - ret = bp->ops->bind(bp, controller_handle, interface); + + struct efi_handler *handler; + char tmpname[256] = "AppName"; + ret = efi_search_protocol(controller_handle, &efi_guid_device_path, + &handler); + snprintf(tmpname, 256, "%pD", handler->protocol_interface); + ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname)); if (ret == EFI_SUCCESS) goto out; diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c index a367e8b89d17..0ab8e4590dfe 100644 --- a/lib/efi_selftest/efi_selftest_block_device.c +++ b/lib/efi_selftest/efi_selftest_block_device.c @@ -248,6 +248,7 @@ static int teardown(void) { efi_status_t r = EFI_ST_SUCCESS; +#if 0 /* Temporarily out for confirmation */ if (disk_handle) { r = boottime->uninstall_protocol_interface(disk_handle, &guid_device_path, @@ -273,6 +274,7 @@ static int teardown(void) return EFI_ST_FAILURE; } } +#endif return r; }
An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in EFI world, which in turn generates a corresponding U-Boot block device based on U-Boot's Driver Model. The latter device, however, doesn't work as U-Boot proper block device due to an issue in efi_driver's implementation. We saw discussions in the past, most recently in [1]. [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html This RFC patch tries to address (part of) the issue. If it is considered acceptable, I will create a formal patch. Withtout this patch, ===8<=== => env set efi_selftest 'block device' => bootefi selftest ... Summary: 0 failures => dm tree Class Index Probed Driver Name ----------------------------------------------------------- root 0 [ + ] root_driver root_driver ... bootmeth 7 [ ] vbe_simple | `-- vbe_simple blk 0 [ + ] efi_blk `-- efiblk#0 partition 0 [ + ] blk_partition `-- efiblk#0:1 => ls efiloader 0:1 ** Bad device specification efiloader 0 ** Couldn't find partition efiloader 0:1 ===>8=== With this patch applied, efiblk#0(:1) now gets accessible. ===8<=== => env set efi_selftest 'block device' => bootefi selftest ... Summary: 0 failures => dm tree Class Index Probed Driver Name ----------------------------------------------------------- root 0 [ + ] root_driver root_driver ... bootmeth 7 [ ] vbe_simple | `-- vbe_simple efi 0 [ + ] EFI block driver `-- /VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8) blk 0 [ + ] efi_blk `-- efiblk#0 partition 0 [ + ] blk_partition `-- efiblk#0:1 => ls efiloader 0:1 13 hello.txt 7 u-boot.txt 2 file(s), 0 dir(s) ===>8=== Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/efi_driver.h | 2 +- lib/efi_driver/efi_block_device.c | 17 ++++++++++++----- lib/efi_driver/efi_uclass.c | 8 +++++++- lib/efi_selftest/efi_selftest_block_device.c | 2 ++ 4 files changed, 22 insertions(+), 7 deletions(-)