On 19 April 2017 at 12:08, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > On 04/19/2017 07:10 PM, Simon Glass wrote: >> Hi Andreas, >> >> On 19 April 2017 at 11:00, Andreas Färber <afaer...@suse.de> wrote: >>> Am 19.04.2017 um 18:56 schrieb Simon Glass: >>>> Hi Andreas, >>>> >>>> On 19 April 2017 at 10:37, Andreas Färber <afaer...@suse.de> wrote: >>>>> Am 19.04.2017 um 17:52 schrieb Simon Glass: >>>>>> Hi Andreas, >>>>>> >>>>>> On 19 April 2017 at 09:14, Simon Glass <s...@chromium.org> wrote: >>>>>>> Hi Andreas, >>>>>>> >>>>>>> On 19 April 2017 at 08:43, Andreas Färber <afaer...@suse.de> wrote: >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> Am 19.04.2017 um 16:28 schrieb Simon Glass: >>>>>>>>> On 19 April 2017 at 05:26, Heinrich Schuchardt <xypron.g...@gmx.de> >>>>>>>>> wrote: >>>>>>>>>> When iterating over the devices of an uclass the iteration stops >>>>>>>>>> at the first device that cannot be probed. >>>>>>>>>> When calling booefi this will result in no block device being >>>>>>>>>> passed to the EFI executable if the first device cannot be probed. >>>>>>>>>> >>>>>>>>>> The problem was reported by Andreas Färber in >>>>>>>>>> https://lists.denx.de/pipermail/u-boot/2017-April/287432.html >>>>>>>>>> >>>>>>>>>> For testing I used an odroid-c2 with a dts including >>>>>>>>>> &sd_emmc_a { >>>>>>>>>> status = "okay"; >>>>>>>>>> } >>>>>>>>>> This device does not exist on the board and cannot be initialized. >>>>>>>>>> >>>>>>>>>> With the patch uclass_first_device and uclass_next_device >>>>>>>>>> iterate internally until they find the first device that can be >>>>>>>>>> probed or the end of the device list is reached. >>>>>>>>> >>>>>>>>> I would like to avoid changing the API that much. Can you please >>>>>>>>> change it to stop calling the tail function and always set the device, >>>>>>>>> like you did in v1? >>>>>>>> >>>>>>>> I fear you're missing the key point I made in my lengthy explanation: >>>>>>> >>>>>>> That's not entirely impossible. >>>>>>> >>>>>>>> >>>>>>>> Our caller (EFI) wants to iterate over the available devices. SDIO is >>>>>>>> not available. If we return a non-NULL device it will try to scan the >>>>>>>> disk. Therefore I think v2 is more correct (not yet tested). >>>>>>>> >>>>>>>> So really the question is what your desired semantics of this function >>>>>>>> are and how callers here and elsewhere are handling it. If we want to >>>>>>>> return non-probed devices to the caller, as you now suggest, then we >>>>>>>> would need to audit and amend all callers of the API with some "if >>>>>>>> !is_probed() then continue" check. If we simply skip them internally, >>>>>>>> as >>>>>>>> done here IIUC, we require no changes on the caller side, thus much >>>>>>>> less >>>>>>>> invasive. >>>>>>> >>>>>>> Well the value of 'ret' gives you that information if you want it. But >>>>>>> yes it is a change and on second thoughts I'm not entirely comfortable >>>>>>> with it. >>>>>>> >>>>>>>> >>>>>>>> Maybe we need a new API uclass_{first,next}_available_device() to make >>>>>>>> this clear? The change would then only affect callers of the new API, >>>>>>>> and EFI and possibly others would again need to be audited and updated. >>>>>> >>>>>> If you think this is generally useful then you could add this. I think >>>>>> it would be something like: >>>>>> >>>>>> for (ret = uclass_first_avail_device(UCLASS_..., &dev; dev; ret = >>>>>> uclass_next_avail_device(&dev)) { >>>>>> if (!ret) { >>>>>> // do something >>>>>> } >>>>>> } >>>>>> >>>>>> Does that sounds right? >>>>> >>>>> No. I think that should be the semantics of uclass_first_device(), i.e. >>>>> Heinrich's v1, possibly moved out of _tail() as you requested. >>>>> >>>>> The idea behind a separate ..._available_device() implementation was to >>>>> not have to do that ret check and to simply replace the function name to >>>>> change the semantics, i.e. Heinrich's v2 implementation, but not inside >>>>> uclass_{first,next}_device() but in copies with different name. >>>> >>>> How do you know what the error was? If you return a 'dev' then how do >>>> you know whether it is OK to process it? I'd prefer that this is >>>> provided via the return value rather than checking device_active(), >>>> etc. >>> >>> You misunderstand: That ret handling should be hidden inside >>> _available_device, not exposed to its caller! :) >>> >>> Note that v2 adds a for loop inside both functions! >>> >>> So the caller only sees the devices that probed and would never see a >>> non-zero ret, i.e. they could become void (or return the dev, which >>> would change the calling convention). >> >> I see, so you want a 'higher level' function that only returns devices >> which probe without error. Then if you want to report the errors you >> would have to iterate in a different way. Is that right? So the return >> value would always be 0, right? Or would you make the function return >> the device pointer directly? > > uclass_first_device and uclass_next_device are these "higher level" > functions: they return only devices probing without error. Unfortunately > in some circumstances they do not return all devices. As Álvaro pointed > out this bug is a pain in other places too. > > This is what my patch fixes. There is no need for any new function.
I do not what uclass_first/next_device to skip devices that don't probe. We need to have new functions. This discussion seems to be going into the weeds. I'll see if I can work up a few patches and some tests to verify things. > >> >> I am not very keen on this sort of function, since I suspect it would >> be useful only rarely. But I might be wrong and if you want to put it >> in, please go ahead. >> >> BTW another point I should have made, is that really this sort of >> thing should be handled dynamically as needed. The EFI loader should >> not build its own tables parallel to DM. For example, if I run grub >> and then insert an MMC card or USB stick, grub won't know about it, >> right? > > According to the UEFI standard EFI_BOOT_SERVICES.LocateHandle() can be > called until boot services are terminated. > > You could create the drivers tables when this call occurs. > > The current implementation first builds lists of all drivers and then > calls the executable. > > Rewriting this is probably a big job. (this is off-topic from this patch but let me continue :-) Actually this is how it should have been done in the first place IMO. Since everything is moving to driver model, it doesn't make a lot of sense to design EFI on the pre-driver-model approach. I think implementing this for the boot-time interface is pretty easy, and I did in fact point this out in the original code review. However for the run-time interface, it is potentially quite painful, which has also already been discussed. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
