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.akashi@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@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@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