Re: [RFC] efi_driver: fix a parent issue in efi-created block devices

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

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