Heinrich, On Thu, Jan 10, 2019 at 08:22:25PM +0100, Heinrich Schuchardt wrote: > On 1/10/19 10:22 AM, 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. > > Why shouldn't we partially initialize the EFI environment when the first > block device is created? > I think only the memory part of EFI is needed at this stage.
Maybe, but how long we can guarantee this in the future? > > >> > >> This event will be expected to be 'signal'ed at every creation/deletion > >> of UCLASS_BLK device. Right? > > > > Correct. > > Events are not the EFI way of handling drivers. Drivers are connected by > calling ConnectController. I didn't get you point very well here, but anyway > Please, add two new functions in lib/efi_loader/efi_disk.c > > * one called after a new block device is created > * another called before a device is destroyed > > both passing as argument (struct udevice *dev) and the caller being in > drivers/block/blk-uclass.c Those are exactly what I proposed in my reply to Alex; efi_disk_create() and efi_disk_delete(). > > A separate patch has to add a struct udevice *dev field to struct > efi_obj and let efi_block_driver use it to decide if a new device shall > be created when binding. > > In efi_block_driver.c we have to implement the unbind function. efi_block_device.c? The issue I noticed regarding the current implementation of UCLASS_BLK is, as I said in my reply to Simon, that efi disk objects for u-boot block devices are created without going through this (bind) procedure. Yet those objects won't show up as UCLASS_BLK's. > In a later patch series we will use said functions to create or destroy > a handle with the EFI_BLOCK_IO_PROTOCOL and wire up ConnectController > and DisconnectController. How do you suggest that we can resolve the issue above? Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > > >> > >>> 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. > > > >> > >>> 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. > > > >> > >> 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? > > > > Alex > > > > > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
