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

[Simon Glass]

Hi Andreas,

On 23 April 2017 at 04:55, Andreas Färber <afaerber at suse.de> wrote:
> Am 19.04.2017 um 23:34 schrieb Simon Glass:
>> On 19 April 2017 at 15:06, Andreas Färber <afaerber at suse.de> wrote:
>>> Am 19.04.2017 um 13:26 schrieb Heinrich Schuchardt:
>>>> 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
>>>
>>> "bootefi"
>>>
>>>> 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.
>>>>
>>>> Debug output is provided for the two functions.
>>>>
>>>> Reported-by: Andreas Färber <afaerber at suse.de>
>>>> Cc: Simon Glass <sjg at chromium.org>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>> ---
>>>> v2:
>>>>   As suggested by Simon Glass correct uclass_first_device() and
>>>>   uclass_next_device() instead of uclass_get_device_tail() to
>>>>   avoid side effects.
>>>> v1:
>>>>   The original patch was posted as
>>>>   core/uclass: uclass_get_device_tail: always set devp
>>>>   https://lists.denx.de/pipermail/u-boot/2017-April/288068.html
>>>> ---
>>>>  drivers/core/uclass.c | 44 +++++++++++++++++++++++++++++++++-----------
>>>>  1 file changed, 33 insertions(+), 11 deletions(-)
>>>
>>> Reviewed-by: Andreas Färber <afaerber at suse.de>
>>> Tested-by: Andreas Färber <afaerber at suse.de>
>>>
>>> Confirming that on my Vega S95 Telos this results in a full GRUB menu,
>>> and GRUB sees two disks.
>>>
>>> Many thanks,
>>>
>>> Andreas
>>
>> Just to be clear, I am NAKing this patch. I do not want to change the
>> existing semantics as it requires existing code to check the function
>> return value.
>
> And to be clear I still think you are mistaken in holding on to an
> implementation that assumes that all devices will probe okay.

The current API works pretty well and is used ~90 times in U-Boot so
I'm reluctant to change it. In particular, having to check the return
value to see if you can use dev or not would be a pretty significant
change. I think there are some uses for it, but the majority of the
time you want devices that work.

> be able to fix the issue at hand in different ways - be it at the
> callsite or by adding a new API - but this is papering over other
> potential problems in the codebase. Hardware might malfunction, drivers
> might not be implemented in U-Boot or be disabled by user's config, etc.

This is up to the implementation, but for many boards if an expected
device does not work then it is a problem. I suspect the main
exception to this is removeable devices and situations where any of a
number of boot paths might be chosen, some of which rely on removeable
devices.

Anyway I'm not trying to mandate any one way of dealing with devices,
just pointing out that the existing API works fairly well.

>
> The caller might notice that some error has occurred by checking the
> return code, but what do you seriously expect it to do then? If it's
> supposed to call an alternative iteration function to continue, it could
> just call that in the first place.

In most cases, it would fail to boot and go to some sort of recovery
mode. It often indicates a serious hardware error, and continuing is
bad.

Having thought a bit more I think that ignoring errors is not a great
way to go. So I'm going to update the series to try a different
approach. Please can you reply to that with the tags you would like on
it, or suggest another approach if you don't like it.

Regards,
Simon