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

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

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.

> But we have this annoying interim state where we would lose a few boards
> because they haven't been converted to DM. That's what keeps us from it.
> 
> I think what this discussion boils down to is that someone needs to
> start prototyping the DM/EFI integration. Start off with a simple
> subsystem, like BLK.

Even in the current implementation,
* UEFI disk is implemented using UCLASS_BLK devices
  (We can ignore !CONFIG_BLK case now as we have agreed.)
* UEFI-specific block device can be seen as UCLASS_BLK/IF_TYPE_EFI

So how essentially different is the *ultimate* goal from the current form
regarding block devices?

In order to identify UEFI disks with u-boot devices transparently, we will
have to have some sort of *hook* (or hotplug in Alex's language?), either
in create_block_devices or bind/probe?  I don't know, but Simon seems
to be in denial about this idea.

> Then provide a DM path and have a non-DM fallback
> still in its own source file that also provides EFI BLK devices.
> Eventually we just remove the latter.
> 
> That way we can then work on getting hotplug working in the DM path,
> which is the one we want anyway. For non-DM, you simply miss out on that
> amazing new feature, but we don't regress users.
> 
> > So, first, let me reply to each of your comments.
> > Through this process, I hope we will have better understandings
> > of long-term solution as well as a tentative fix.
> > 
> > On Thu, Jan 10, 2019 at 10:22:04AM +0100, Alexander Graf wrote:
> >>
> >>
> >>> Am 10.01.2019 um 10:16 schrieb AKASHI Takahiro <takahiro.akashi@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@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@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.
> >>>
> >>> This event will be expected to be 'signal'ed at every creation/deletion
> >>> of UCLASS_BLK device. Right?
> >>
> >> Correct.
> >>
> >>>
> >>>> 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.
> > 
> > ? Where is the code?
> 
> Ah, I must have misremembered, sorry. I think that was one proposed
> patch a while ago, but we didn't put it in.
> 
> But worst case we can just put additional efi_timer_check() calls at
> strategic places if needed.

Do you expect this kind of mechanism be implemented in my short-term fix?

Thanks,
-Takahiro Akashi
> 
> > 
> >>>
> >>>> 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.
> > 
> > This will be true.
> > 
> >>>
> >>> 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?
> > 
> > "all the UCLASS_BLK deivces" -> all efi_disk_obj's
> > 
> > Let me rephrase it;
> > all efi_disk_obj's will be re-visisted in efi_init_obj_list() to determine
> > its backing u-boot block device is still valid.
> > If not valid, a said efi_disk_obj should be marked so to prevent further
> > accessing in efi world.
> 
> Correct.
> 
> 
> Alex