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

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

Alex,

On Thu, Jan 10, 2019 at 07:21:12AM +0100, Alexander Graf wrote:
> 
> 
> On 10.01.19 03:13, AKASHI Takahiro wrote:
> > Alex,
> > 
> > On Wed, Jan 09, 2019 at 10:06:16AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 13.12.18 08:58, AKASHI Takahiro wrote:
> >>> Heinrich,
> >>>
> >>> On Tue, Dec 11, 2018 at 08:55:41PM +0100, Heinrich Schuchardt wrote:
> >>>> On 11/15/18 5:58 AM, AKASHI Takahiro wrote:
> >>>>> Currently, efi_init_obj_list() scan disk devices only once, and never
> >>>>> change a list of efi disk devices. This will possibly result in failing
> >>>>> to find a removable storage which may be added later on. See [1].
> >>>>>
> >>>>> In this patch, called is efi_disk_update() which is responsible for
> >>>>> re-scanning UCLASS_BLK devices and removing/adding efi disks if necessary.
> >>>>>
> >>>>> For example,
> >>>>>
> >>>>> => efishell devices
> >>>>> Scanning disk pci_mmc.blk...
> >>>>> Found 3 disks
> >>>>> Device Name
> >>>>> ============================================
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> >>>>> => usb start
> >>>>> starting USB...
> >>>>> USB0:   USB EHCI 1.00
> >>>>> scanning bus 0 for devices... 3 USB Device(s) found
> >>>>>        scanning usb for storage devices... 1 Storage Device(s) found
> >>>>> => efishell devices
> >>>>> Scanning disk usb_mass_storage.lun0...
> >>>>> Device Name
> >>>>> ============================================
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)
> >>>>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USBClass(0,0,9,0,1)/USBClass(46f4,1,0,0,0)/HD(1,0x01,0,0x40,0x14fe4c)
> >>>>>
> >>>>> Without this patch, the last device, USB mass storage, won't show up.
> >>>>>
> >>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
> >>>>>
> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>
> >>>> Why should we try to fix something in the EFI subsystems that goes wrong
> >>>> in the handling of device enumeration.
> >>>
> >>> No.
> >>> This is a natural result from how efi disks are currently implemented on u-boot.
> >>> Do you want to totally re-write/re-implement efi disks?
> >>
> >> Could we just make this event based for now? Call a hook from the
> >> storage dm subsystem when a new u-boot block device gets created to
> >> issue a sync of that in the efi subsystem?
> > 
> > If I correctly understand you, your suggestion here corresponds
> > with my proposal#3 in [1] while my current approach is #2.
> > 
> > [1] https://lists.denx.de/pipermail/u-boot/2018-October/345307.html
> 
> Yes, I think so.
> 
> > So we will call, say, efi_disk_create(struct udevice *) in
> > blk_create_device() and efi_dsik_delete() in blk_unbind_all().
> 
> I would prefer if we didn't call them directly, but through an event
> mechanism. So the efi_disk subsystem registers an event with the dm
> block subsystem and that will just call all events when block devices
> get created which will automatically also include the efi disk creation
> callback. Same for reverse.

Do you mean efi event by "event?"
(I don't think there is any generic event interface on DM side.)

Whatever an "event" is or whether we call efi_disk_create() directly
or indirectly via an event, there is one (big?) issue in this approach
(while I've almost finished prototyping):

We cannot call efi_disk_create() within blk_create_device() because
some data fields of struct blk_desc, which are to be used by efi disk,
are initialized *after* blk_create_device() in driver side.

So we need to add a hook at/after every occurrence of blk_create_device()
on driver side. For example,

=== drivers/scsi/scsi.c ===
int do_scsi_scan_one(struct udevice *dev, int id, int lun, bool verbose)
{
	...
	ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1,
				   bd.blksz, bd.lba, &bdev);
	...
	bdesc = dev_get_uclass_platdata(bdev);
	bdesc->target = id;
	bdesc->lun = lun;
	...

	/*
	 * We need have efi_disk_create() called here because bdesc->target
	 * and lun will be used by dp helpers in efi_disk_add_dev().
	 */
	efi_disk_create(bdev);
}

int scsi_scan_dev(struct udevice *dev, bool verbose)
{
        for (i = 0; i < uc_plat->max_id; i++)
                for (lun = 0; lun < uc_plat->max_lun; lun++)
                        do_scsi_scan_one(dev, i, lun, verbose);
	...
}

int scsi_scan(bool verbose)
{
	ret = uclass_get(UCLASS_SCSI, &uc);
	...
        uclass_foreach_dev(dev, uc)
                ret = scsi_scan_dev(dev, verbose);
	...
}
=== ===

Since scsn_scan() can be directly called by "scsi rescan" command,
There seems to be no generic hook, or event, available in order to
call efi_disk_create().

Do I miss anything?

> > UEFI handles for disks, however, will *not* be deleted actually
> > because, as Heinrich suggested in the past, no *reference count*
> > for a handle is maintained in the current implementation. Right?
> 
> Sure, but we can have an internal state that just starts failing all
> callbacks (read, write, etc) when the backing device was destroyed.

Yes, I've already added "flags" to efi_disk_obj.

Thanks,
-Takahiro Akashi

> 
> >> That hook would obviously only do something (or get registered?) when
> >> the efi object stack is initialized.
> >>
> >> The long term goal IMHO should still be though to just merge DM and EFI
> >> objects. But we're still waiting on the deprecation of non-DM devices
> >> for that.
> > 
> > Maybe my #4?
> 
> Not quite.
> 
> We would basically get rid of the efi object list altogether. Instead,
> any DM object would also be an EFI object and expose EFI interfaces if
> awareness exists.
> 
> That way we can merge into the existing object maintenance logic and
> wouldn't need any callbacks.
> 
> Once we're there, we can also start to think about reference counting :).
> 
> 
> Alex