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

[Simon Glass]

On 19 April 2017 at 12:08, Heinrich Schuchardt <xypron.glpk at 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 <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 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