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. > 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? 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. > 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? Thanks, -Takahiro Akashi > > > > >>> > >>>> 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. > > > > This will be true. > > > >>> > >>> 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? > > > > "all the UCLASS_BLK deivces" -> all efi_disk_obj's > > > > Let me rephrase it; > > all efi_disk_obj's will be re-visisted in efi_init_obj_list() to determine > > its backing u-boot block device is still valid. > > If not valid, a said efi_disk_obj should be marked so to prevent further > > accessing in efi world. > > Correct. > > > Alex _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
