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

Wed Apr 19 04:48:00 UTC 2017

On 04/19/2017 05:37 AM, Simon Glass wrote:
> Hi,
> 
> On 18 April 2017 at 21:03, Andreas Färber <afaerber at suse.de> wrote:
>> 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?
> 
> I think this is a genuine bug, but exactly where is less obvious.
> 
> The uclass function don't return a device if they get an error. So at
> present you need to iterate through the uclass if the intention is to
> continue after error. That would be uclass_foreach_dev(). But before
> using the device, device_probe() needs to be called since the
> iteration does not probe it automatically. That is why people normally
> use uclass_first/next_device(), since it probes as it goes.
> 
> However in this case I think it is reasonable to change
> uclass_first/next_device() to always return the device, even on error.
> That can best be done by changing those functions rather than
> uclass_get_device_tail(), which is shared by many functions. We should
> not change uclass_get_device_tail() since what is does is correct for
> all other uses (I think).
> 
> In that case, the function comments for the first/next functions
> should be updated to indicate the new behaviour, and a test created,
> perhaps in test/dm/core.c.
> 
> You were asking about uclass_get_device_tail(). This is common code
> and is put into a function to ensure consistent behaviour across all
> functions that return an error code and a device. Consistency is very
> important with driver model since things can get version confusing
> when something odd happens in the bowels of the system, as this bug
> has shown.
> 
> Regards,
> Simon
> 

Thank you Simon for reviewing. I will try to rework the patch.

For reference I append the debug output with and without the current
patch. This should make the call sequence clear.

In uclass_first_device: id 12
12 refers to UCLASS_BLK.

Best regards

Heinrich Schuchardt

Without [PATCH 1/1] core/uclass: uclass_get_device_tail: always set devp

=> bootefi hello
## Starting EFI application at 01000000 ...
WARNING: Invalid device tree, expect boot to fail
efi_add_memory_map: 0x7cf50000 0x1 2 yes
efi_disk_register
uclass_first_device: id 12
uclass_find_first_device: id 12
uclass_get_device_tail: ret 0
uclass_resolve_seq:
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq -1, find_req_seq 0
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq 0, find_req_seq 0
mmc at 70000.blk - -1 -1
mmc at 72000.blk - -1 -1
mmc at 74000.blk - -1 -1
   - not found
uclass_get_device: id 41, index 0
uclass_get_device_tail: ret 0
set_state_simple op missing
find_mmc_device: dev_num 0
blk_get_device: if_type=6, devnum=0: mmc at 70000.blk, 6, 0
mmc_blk_probe
mmc_init: -95, time 1807
Found 0 disks
uclass_first_device: id 18
uclass_find_first_device: id 18
uclass_get_device_tail: ret 0
efi_add_memory_map: 0x7cf4f000 0x1 6 yes
do_bootefi_exec:234 Jumping to 0x7cf50148
EFI: Entry efi_cout_output_string(000000007ff93ac0, 000000007cf50274)
Hello, world!
EFI: Entry efi_exit(000000007ffa47d0, 0, 0, 0000000000000000)
## Application terminated, r = 0
=>

With [PATCH 1/1] core/uclass: uclass_get_device_tail: always set devp

=> bootefi hello
## Starting EFI application at 01000000 ...
WARNING: Invalid device tree, expect boot to fail
efi_add_memory_map: 0x7cf50000 0x1 2 yes
efi_disk_register
uclass_first_device: id 12
uclass_find_first_device: id 12
uclass_get_device_tail: ret 0
uclass_resolve_seq:
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq -1, find_req_seq 0
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq 0, find_req_seq 0
mmc at 70000.blk - -1 -1
mmc at 72000.blk - -1 0
   - found
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq 1, find_req_seq 0
mmc at 70000.blk - -1 -1
mmc at 72000.blk - -1 0
mmc at 74000.blk - -1 -1
   - not found
uclass_get_device: id 41, index 0
uclass_get_device_tail: ret 0
set_state_simple op missing
find_mmc_device: dev_num 0
blk_get_device: if_type=6, devnum=0: mmc at 70000.blk, 6, 0
mmc_blk_probe
mmc_init: -95, time 1806
Scanning disk mmc at 70000.blk...
uclass_next_device:
uclass_find_next_device:
uclass_get_device_tail: ret 0
Scanning disk mmc at 72000.blk...
Adding disk device 'mmc at 72000.blk'
uclass_next_device:
uclass_find_next_device:
uclass_get_device_tail: ret 0
uclass_resolve_seq:
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq -1, find_req_seq 0
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq 0, find_req_seq 0
mmc at 70000.blk - -1 -1
mmc at 72000.blk - -1 0
   - found
uclass_find_device_by_seq: uclass_id 0, seq_or_req_seq 1, find_req_seq 0
mmc at 70000.blk - -1 -1
mmc at 72000.blk - -1 0
mmc at 74000.blk - -1 -1
   - not found
uclass_get_device: id 41, index 0
uclass_get_device_tail: ret 0
set_state_simple op missing
find_mmc_device: dev_num 2
blk_get_device: if_type=6, devnum=2: mmc at 70000.blk, 6, 0
blk_get_device: if_type=6, devnum=2: mmc at 72000.blk, 6, 1
blk_get_device: if_type=6, devnum=2: mmc at 74000.blk, 6, 2
mmc_blk_probe
Card did not respond to voltage select!
mmc_init: -95, time 10
Scanning disk mmc at 74000.blk...
uclass_next_device:
uclass_find_next_device:
Found 3 disks
efi_add_memory_map: 0x7cf4f000 0x1 6 yes
do_bootefi_exec:234 Jumping to 0x7cf50148
EFI: Entry efi_cout_output_string(000000007ff93ad0, 000000007cf50274)
Hello, world!
EFI: Entry efi_exit(000000007ffa47e0, 0, 0, 0000000000000000)
## Application terminated, r = 0
=>