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

[Alexander Graf]

On 11.01.19 05:51, AKASHI Takahiro wrote:
> On Thu, Jan 10, 2019 at 05:57:11AM -0700, Simon Glass wrote:
>> Hi,
>>
>> On Wed, 9 Jan 2019 at 02:06, Alexander Graf <agraf at suse.de> 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?
>>>
>>
>> Please no. We don't want EFI hooks around the place. EFI should use
>> DM, not the other way around.
> 
> Right, but every efi disk is associated with a backing "u-boot"
> block devices (even more, this is not 1-to-1 mapping but many-to-1 due to
> partitions).

In this case we may just want to create DM devices for partitions as
well to make things more natural.

> Without any sort of event/hook mechanism, we can know of all block
> devices only by enumerating them at some point as in my current
> approach. Do you want to accept this?
> 
> (Even in a hacky way. See efi_disk_register():
>         for (if_type = 0; if_type < IF_TYPE_COUNT; if_type++)
> 		...
>                 printf("Scanning disks on %s...\n", if_typename);
>                 for (i = 0; i < 4; i++) {    <= !!!
>                         desc = blk_get_devnum_by_type(if_type, i);
> 	...
> )
> 
>>> That hook would obviously only do something (or get registered?) when
> i 
>>> the efi object stack is initialized.
>>>
>>> The long term goal IMHO should still be though to just merge DM and EFI
>>> objects. But we're still waiting on the deprecation of non-DM devices
>>> for that.
>>
>> I think think 'merge' is the right word. Perhaps 'create EFI devices
>> in DM' is a better term.
> 
> How different is your idea from UCLASS_BLK device(efi_block_device.c)?

The UCLASS_BLK is the reverse path. It's exposing a DM device from an
EFI one.


Alex

> 
> The current implementation of UCLASS_BLK, in my opinion, is somehow
> in a half way; an instance of UCLASS_BLK is created only when a
> efi driver is bound to a controller.
> In the meantime, efi disks (efi objects which support UEFI_BLOCK_IO_PROTOCOL)
> is implicitly created in efi_disk_add_dev() without any *binding*.
> 
>> Anyway, let's do that now. As I may have mentioned we should never
>> have enabled EFI on pre-DM boards :-) It has just lead to duplication.
>>
>> In any case, the migration deadline for DM_MMC (for example) is the
>> upcoming merge window, so the time is now.
>>
>> As things stand this patch looks reasonable to me. It is a natural
>> consequence of duplicating the DM tables.
> 
> I think so, yes.
> 
> -Takahiro Akashi
> 
>> Regards,
>> Simon