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