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.aka...@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.aka...@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.aka...@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. Thanks, -Takahiro Akashi > 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
