[U-Boot] [PATCH 1/1] core/uclass: uclass_get_device_tail: always set devp

[Andreas Färber]

Hi Simon,

Am 19.04.2017 um 02:12 schrieb Simon Glass:
> On 18 April 2017 at 12:44, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> Set devp even if probing fails.
>>
>> Without the patch the following problem occurs:
>> If the first block device is not probed successfully no block
>> device is passed by bootefi to the EFI executable.
>>
>> 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.
> 
> Thanks for finding this. Which function is calling this? Can you
> please explain the call sequence to hit this problem? I am worried
> that something else is wrong.

It is lib/efi_loader/efi_disk.c:efi_disk_register() that is calling
uclass_first_device() and uclass_next_device(). Based on the output we
had concluded that uclass_first_device() fails already - therefore Alex
CC'ed you.

Unfortunately I find the function interactions inside uclass.c rather
complex and unintuitive, so I am truely amazed at Heinrich's findings.
(Passing the ret code from one function to the next just to return?!)

Based on his patch we can assume that uclass_find_first_device() set dev
to a non-NULL value, otherwise we wouldn't end up in
uclass_get_device_tail().

So if you're saying this patch is wrong, then that would leave
device_probe(). The actual meson_mmc_probe() function you had given a
late Reviewed-by, and it always returns 0.

Jaehoon had asked about cfg->voltages in meson_mmc_probe(), but I don't
see how that would lead to probe failure here? Whether the values are
correct I have no idea.

The error message "mmc_init: -95, time 1807", which appears to be a
result of this iteration/probe process, comes from
drivers/mmc/mmc.c:mmc_init(), which is called from
mmc-uclass.c:mmc_blk_probe(). mmc_init() calls mmc_start_init(), which
returns this -EOPNOTSUPP if sd_send_op_cond return -ETIMEDOUT and
mmc_send_op_cond() failed. However, our output does not show "Card did
not respond to voltage select!", which it should in that case. So the
error must be coming from elsewhere. sd_send_op_cond() may return
-EOPNOTSUPP through mmc_start_init(), and so does mmc_complete_op_cond()
through mmc_complete_init(). I would expect either to fail, as in my
case it's an SDIO, and in Heinrich's case it's not connected to
anything. So I don't think MMC is at fault here.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi

See sd_mmc_a, which uses an mmc-pwrseq that depends on a pwm-clock,
neither of which have any support in U-Boot, nor is it actually needed
for booting.

In any case, failure to probe one device should not break the iteration
of other devices. I.e., if a device fails to probe then instead of
returning from uclass_{first,next}_device() we should look at the next
device until we find one that's okay or have reached the end of the
list. How to best implement that in uclass.c I'm less sure of.

If I am right you could test this on any board with multiple, e.g., blk
devices where a first or non-last device returns an error code from its
probe, i.e. try changing return 0 in some probe function based on a
static variable not yet being initialized or something. Probably you
could even write some tiny sandbox based test, without actual hardware.

IIUC this patch changes behavior from iterating over no devices to
iterating over all devices including ones not probed successfully? And
what you intended was to instead iterate over only the probed devices?

Regards,
Andreas

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