[U-Boot] [PATCH v2 2/3] efi_loader: enumerate disk devices every time
Alexander Graf
agraf at suse.de
Wed Jan 23 09:30:59 UTC 2019
On 01/23/2019 09:12 AM, AKASHI Takahiro wrote:
> Alex,
>
> On Tue, Jan 22, 2019 at 10:08:53AM +0100, Alexander Graf wrote:
>>
>> 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.
> So while the issue can be easily reproduced and my fix is quite
> simple and straightforward (but with a bit inefficiency :),
> you won't accept it in the current form.
>
> Right? It's totally up to you.
I'm afraid it drives us further into a corner that we don't want to be
in, yes, sorry :(.
However, I would be thrilled to see the dm object integration move
forward :).
Alex
More information about the U-Boot
mailing list