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

AKASHI Takahiro takahiro.akashi at linaro.org
Fri Jan 11 05:08:31 UTC 2019 

Heinrich,

On Thu, Jan 10, 2019 at 08:22:25PM +0100, Heinrich Schuchardt wrote:
> On 1/10/19 10:22 AM, Alexander Graf wrote:
> > 
> > 
> >> Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
> >>
> >>> On Thu, Jan 10, 2019 at 09:15:36AM +0100, Alexander Graf wrote:
> >>>
> >>>
> >>>>> Am 10.01.2019 um 09:02 schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
> >>>>>
> >>>>> On Thu, Jan 10, 2019 at 08:30:13AM +0100, Alexander Graf wrote:
> >>>>>
> >>>>>
> >>>>>> On 10.01.19 08:26, AKASHI Takahiro wrote:
> >>>>>> 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 at 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?
> >>>>>
> >>>>> Could the event handler that gets called from somewhere around
> >>>>> blk_create_device() just put it into an efi internal "todo list" which
> >>>>> we then process using an efi event?
> >>>>>
> >>>>> EFI events will only get triggered on the next entry to efi land, so by
> >>>>> then we should be safe.
> >>>>
> >>>> I think I now understand your suggestion; we are going to invent
> >>>> a specialized event-queuing mechanism so that we can take any actions
> >>>> later at appropriate time (probably in efi_init_obj_list()?).
> >>>
> >>> Uh, not sure I follow. There would be 2 events. One from the u-boot block layer to the efi_loader disk layer.
> >>
> >> This is a to-be-invented "specialized event-queuing mechanism"
> >> in my language :) as we cannot use efi_create/signal_event() before
> >> initializing EFI environment.
> 
> Why shouldn't we partially initialize the EFI environment when the first
> block device is created?
> I think only the memory part of EFI is needed at this stage.

Maybe, but how long we can guarantee this in the future?

> 
> >>
> >> This event will be expected to be 'signal'ed at every creation/deletion
> >> of UCLASS_BLK device. Right?
> > 
> > Correct.
> 
> Events are not the EFI way of handling drivers. Drivers are connected by
> calling ConnectController.

I didn't get you point very well here, but anyway

> Please, add two new functions in lib/efi_loader/efi_disk.c
> 
> * one called after a new block device is created
> * another called before a device is destroyed
>
> both passing as argument (struct udevice *dev) and the caller being in
> drivers/block/blk-uclass.c

Those are exactly what I proposed in my reply to Alex;
efi_disk_create() and efi_disk_delete().

> 
> A separate patch has to add a struct udevice *dev field to struct
> efi_obj and let efi_block_driver use it to decide if a new device shall
> be created when binding.
> 
> In efi_block_driver.c we have to implement the unbind function.

efi_block_device.c?

The issue I noticed regarding the current implementation of
UCLASS_BLK is, as I said in my reply to Simon, that efi disk objects
for u-boot block devices are created without going through this (bind)
procedure. Yet those objects won't show up as UCLASS_BLK's.

> In a later patch series we will use said functions to create or destroy
> a handle with the EFI_BLOCK_IO_PROTOCOL and wire up ConnectController
> and DisconnectController.

How do you suggest that we can resolve the issue above?

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > 
> >>
> >>> That event handler creates a new efi event (like a timer w/ timeout=0).
> >>
> >> But when is this event handler fired?
> >> I think the only possible timing is at efi_init_obj_list().
> > 
> > We already walk through the event list on any u-boot/efi world switch.
> > 
> >>
> >>> This new event's handler can then create the actual efi block device.
> >>
> >> I assume that this event handler is fired immediately after
> >> efi_signal_event() with timeout=0.
> > 
> > Right, and that signal_event() will happen the next time we go back into efi land. By that time, the dm blk struct will be complete.
> > 
> >>
> >> If so, why do we need to create an efi event? To isolate the disk code
> >> from the other init code?
> > 
> > I don't think we should call init code during runtime, yes. These are 2 paths.
> > 
> >>
> >> (If so, for the same reason, we should re-write efi_init_obj_list()
> >> with events for other efi resources as well.)
> >>
> >>>>
> >>>> But if so, it's not much different from my current approach where
> >>>> a list of efi disks are updated in efi_init_obj_list() :)
> >>>
> >>> The main difference is that disk logic stays in the disc code scope :).
> >>
> >> My efi_disk_update() (and efi_disk_register()) is the only function
> >> visible outside the disk code, isn't it?
> >>
> >> Using some kind of events here is smart, but looks to me a bit overdoing
> >> because we anyhow have to go through all the UCLASS_BLK devices to mark
> >> whether they are still valid or not :)
> > 
> > What do you mean?
> > 
> > Alex
> > 
> > 
> > 
>

More information about the U-Boot mailing list