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

Tue Jan 22 09:08:53 UTC 2019

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.

> 
>> 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?

The ultimate goal is that efi_disk_register() and efi_obj_list disappear.

Functionality wise we should be 100% identical to today, so all test
cases would still apply the same way as they do now. This is purely
internal rework, nothing visible to UEFI payloads.

> 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.

Well, if a udevice *is* an efi device, we no longer need hooks. The
object list would simply change.

We may still need to have event notifications at that stage, but that's
a different story.

As transitioning period, we could probably implement 2 efi object roots:
efi_obj_list as well as the udevice based one. Every piece of code that
iterates through devices then just iterates over both. That way we
should be able to slowly move devices from the old object model to the
new one.

>> 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 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.
>>>>>
>>>>> 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?

To be completely honest, I don't think your fix is very critical right
now, since it touches a case that's known broken today already.

I would prefer if we could concentrate on the real path forward, where
everything becomes implicit and we no longer need to sync the two object
trees.

Alex