[U-Boot] [PATCH v2 2/3] efi_loader: enumerate disk devices every time

[Simon Glass]

Hi,

On Mon, 28 Jan 2019 at 01:55, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> On Fri, Jan 25, 2019 at 10:31:20AM +0100, Alexander Graf wrote:
> >
> >
> > On 25.01.19 10:18, AKASHI Takahiro wrote:
> > > On Fri, Jan 25, 2019 at 09:52:31AM +0100, Alexander Graf wrote:
> > >>
> > >>
> > >> On 25.01.19 09:27, AKASHI Takahiro wrote:
> > >>> Alex,
> > >>>
> > >>> On Wed, Jan 23, 2019 at 10:51:29AM +0100, Alexander Graf wrote:
> > >>>> On 01/22/2019 08:39 PM, Simon Glass wrote:
> > >>>>> Hi Alex,
> > >>>>>
> > >>>>> On Tue, 22 Jan 2019 at 22:08, Alexander Graf <agraf at suse.de> wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>> On 22.01.19 09:29, AKASHI Takahiro wrote:
> > >>>>>>> Alex, Simon,
> > >>>>>>>
> > >>>>>>> Apologies for my slow response on this matter,
> > >>>>>>>
> > >>>>>>> On Fri, Jan 11, 2019 at 08:57:05AM +0100, Alexander Graf wrote:
> > >>>>>>>>
> > >>>>>>>> On 11.01.19 05:29, AKASHI Takahiro wrote:
> > >>>>>>>>> Alex, Heinrich and Simon,
> > >>>>>>>>>
> > >>>>>>>>> Thank you for your comments, they are all valuable but also make me
> > >>>>>>>>> confused as different people have different requirements :)
> > >>>>>>>>> I'm not sure that all of us share the same *ultimate* goal here.
> > >>>>>>>> The shared ultimate goal is to "merge" (as Simon put it) dm and efi objects.
> > >>>>>>> I don't still understand what "merge" means very well.
> > >>>>>> It basically means that "struct efi_object" moves into "struct udevice".
> > >>>>>> Every udevice instance of type UCLASS_BLK would expose the block and
> > >>>>>> device_path protocols.
> > >>>>>>
> > >>>>>> This will be a slightly bigger rework, but eventually allows us to
> > >>>>>> basically get rid of efi_init_obj_list() I think.
> > >>>>> I envisaged something like:
> > >>>>>
> > >>>>> - EFI objects have their own UCLASS_EFI uclass
> > >>>>
> > >>>> ... and then we need to create our own sub object model around the
> > >>>> UCLASS_EFI devices again. I' not convinced that's a great idea yet :). I
> > >>>> really see little reason not to just expose every dm device as EFI handle.
> > >>>> Things would plug in quite naturally I think.
> > >>>
> > >>> You said that the ultimate goal is to remove all efi_object data.
> > >>> Do you think that all the existing efi_object can be mapped to
> > >>> one of existing u-boot uclass devices?
> > >>>
> > >>> If so, what would be an real entity of a UEFI handle?
> > >>> struct udevice *?
> > >>>
> > >>> But Simon seems not to agree to adding any UEFI-specific members
> > >>> in struct udevice.
> > >>
> > >> I think we'll have to experiment with both approaches. I personally
> > >> would like to have struct udevice * be the UEFI handle, yes.
> > >>
> > >>>
> > >>>> But either way, someone would need to sit down and prototype things to be
> > >>>> sure.
> > >>>>
> > >>>
> > >>> The most simplest prototype would include
> > >>> * event mechanism (just registration and execution of hook/handler)
> > >>>     event: udevice creation (and deletion)
> > >>> * efi_disk hook for udevice(UCLASS_BLK) creation
> > >>> * modified block device's enumeration code, say, scsi_scan(),
> > >>>   to add an event hook at udevice creation
> > >>> * removing efi_disk_register() from efi_init_obj_list()
> > >>> * Optionally(?) UCLASS_PARTITION
> > >>>   (Partition udevices would be created in part_init().)
> > >>
> > >> Almost.
> > >>
> > >> The simplest prototype would be to add a struct efi_object into struct
> > >> udevice. Then whenever we're looping over efi_obj_list in the code, we
> > >> additionally loop over all udevices to find the handle.
> > >
> > > Ah, yes. You're going further :)
> > >
> > >> Then, we could slowly give the uclasses explicit knowledge of uefi
> > >> protocols. So most of the logic of efi_disk_register() would move into
> > >> (or get called by) drivers/block/blk-uclass.c:blk_create_device().
> > >
> > > Via event? Otherwise, we cannot decouple u-boot and UEFI world.
> >
> > For a prototype, just make it explicit and see how far that gets us.
> >
> > >> Instead of creating diskobj and adding calling efi_add_handle(), we
> > >> could then just use existing data structure from the udevice (and its
> > >> platdata).
> > >
> > > I don't have good confidence that we can remove struct efi_disk_obj,
> > > at least, for the time being as some of its members are quite UEFI-specific.
> >
> > Maybe we can move them into struct blk_desc? It's a matter of
> > experimenting I guess.
> >
> > >
> > >>
> > >> Does this make sense? Less events, more implicity :).
> > >
> > > I'll go for it.
> >
> > Thanks a lot :). Feel free to pick an easier target for starters too if
> > you prefer.
>
> Prototyping is done :)
> Since it was so easy and simple, now I'm thinking of implementing
> UCLASS_PARTITION. But it is not so straightforward as I expected,
> and it won't bring us lots of advantages.
> (I think that blk_desc should also support a partition in its own.)

blk_desc is in UCLASS_BLK. So we already support partitions within
blk_desc. Can you expand a bit on what you mean?

>
>
> Once it gets working, may I send out a patch?

Yes indeed.

Regards,
Simon

>
> -Takahiro Akashi
>
>
> >
> > Alex