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

[Andreas Färber]

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).

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)