[U-Boot] [PATCH v2 1/1] core/uclass: iterate over all devices of a uclass

Wed Apr 19 18:08:48 UTC 2017

On 04/19/2017 07:10 PM, Simon Glass wrote:
> Hi Andreas,
> 
> On 19 April 2017 at 11:00, Andreas Färber <afaerber at 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 <afaerber at suse.de> wrote:
>>>> Am 19.04.2017 um 17:52 schrieb Simon Glass:
>>>>> Hi Andreas,
>>>>>
>>>>> On 19 April 2017 at 09:14, Simon Glass <sjg at chromium.org> wrote:
>>>>>> Hi Andreas,
>>>>>>
>>>>>> On 19 April 2017 at 08:43, Andreas Färber <afaerber at 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.glpk at 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 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.

Best regards

Heinrich Schuchardt

> 
> Regards,
> Simon
> 