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

Alexander Graf
2019-01-10T09:22:04Z

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

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