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

Thu Jan 10 08:15:36 UTC 2019

> 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. That event handler creates a new efi event (like a timer w/ timeout=0). This new event's handler can then create the actual efi block device.

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

Alex

> 
> -Takahiro Akashi
> 
> 
>> 
>> Alex