[PATCH 1/1] dm: fix blk_get_devnum_by_uclass_idname()

[Heinrich Schuchardt]

On 10/11/22 07:46, Heinrich Schuchardt wrote:
> 
> 
> On 10/11/22 01:49, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On Thu, 6 Oct 2022 at 14:05, Heinrich Schuchardt
>> <heinrich.schuchardt at canonical.com> wrote:
>>>
>>> On 10/3/22 18:44, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Mon, 3 Oct 2022 at 10:33, Heinrich Schuchardt
>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 10/3/22 16:57, Simon Glass wrote:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>> On Mon, 3 Oct 2022 at 03:36, Heinrich Schuchardt
>>>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>>>
>>>>>>> On the sandbox I run:
>>>>>>>
>>>>>>>        => setenv efi_selftest block device
>>>>>>>        => bootefi selftest
>>>>>>>
>>>>>>> and see the following output:
>>>>>>>
>>>>>>>        ** Bad device specification host 0 **
>>>>>>>        Couldn't find partition host 0:0
>>>>>>>        Cannot read EFI system partition
>>>>>>>
>>>>>>> Running
>>>>>>>
>>>>>>>        => lsblk
>>>>>>>
>>>>>>> yields
>>>>>>>
>>>>>>>        Block Driver          Devices
>>>>>>>        -----------------------------
>>>>>>>        efi_blk             : efiloader 0
>>>>>>>        ide_blk             : <none>
>>>>>>>        mmc_blk             : mmc 2, mmc 1, mmc 0
>>>>>>>        nvme-blk            : <none>
>>>>>>>        sandbox_host_blk    : <none>
>>>>>>>        scsi_blk            : <none>
>>>>>>>        usb_storage_blk     : <none>
>>>>>>>        virtio-blk          : <none>
>>>>>>>
>>>>>>> So a efi_blk device was mistaken for a host device.
>>>>>>>
>>>>>>> I continue with
>>>>>>>
>>>>>>>        => host bind 0 ../sandbox.img
>>>>>>>        => ls host 0:1
>>>>>>>
>>>>>>> and get the following output:
>>>>>>>
>>>>>>>               13   hello.txt
>>>>>>>                7   u-boot.txt
>>>>>>>
>>>>>>>        2 file(s), 0 dir(s)
>>>>>>>
>>>>>>> This is the content of efiblock 0:1 and not of host 0:1 (sic!).
>>>>>>>
>>>>>>> The uclass of the parent device is irrelevant for the 
>>>>>>> determination of the
>>>>>>> uclass of the block device. We must use the uclass stored in the 
>>>>>>> block
>>>>>>> device descriptor.
>>>>>>>
>>>>>>> This issue has been raised repeatedly:
>>>>>>>
>>>>>>> [PATCH 1/1] block: fix blk_get_devnum_by_typename()
>>>>>>> https://lore.kernel.org/u-boot/20220802094933.69170-1-heinrich.schuchardt@canonical.com/
>>>>>>> [PATCH 1/1] blk: simplify blk_get_devnum_by_typename()
>>>>>>> https://lore.kernel.org/u-boot/20211023140647.7661-1-heinrich.schuchardt@canonical.com/
>>>>>>
>>>>>> Yes and you were not able/willing to take on the required work, so
>>>>>> this carried on longer than it should have. I finally did this myself
>>>>>> and it is now in -next.
>>>>>
>>>>> The refactoring was orthogonal to the problem that I reported and 
>>>>> which
>>>>> you unfortunately did not consider in the process.
>>>>
>>>> Well it involved using if_type to work around a problem, just making
>>>> it harder to get rid of. Overall I am in favour of a faster pace of
>>>> migration that we have been following and it would help if people took
>>>> on some of this, instead of fixing their little issue.
>>>>
>>>>>
>>>>>>
>>>>>> So we might finally be able to fix this problem properly, since
>>>>>> if_type is mostly just a work-around concept in -next, with just the
>>>>>> fake uclass_id being used at present.
>>>>>>
>>>>>> Can you use if_type_to_uclass_id() here, which is the work-around
>>>>>> function for now?
>>>>>
>>>>> This function does not exist in origin/next. We won't apply this patch
>>>>> in the 2022-10 cycle.
>>>>
>>>> I think I mean conv_uclass_id() which is the new name.
>>>>
>>>>>
>>>>> Let's fix the bug first before thinking about future refactoring.
>>>>>
>>>>> You may determine the uclass ID for field bdev in struct blk_desc 
>>>>> using
>>>>> function device_get_uclass_id() when refactoring.
>>>>
>>>> So if you call conv_uclass_id() (without any other refactoring) does
>>>> that fix the problem?
>>>
>>> Except for UCLASS_USB that function is a NOP. How could it help to
>>> differentiate between devices with the same parent device?
>>
>> It can't. But the root node should not have UCLASS_BLK children. I
>> think I mentioned that a few months back?
>>
>>>
>>> Would you agree that blk_get_devnum_by_uclass_idname() should not look
>>> at the parent but on the actual device?
>>
>> No, looking at the parent is exactly what it should do. A block device
>> is generic, to the extent possible. Its methods are implemented in the
>> parent uclass and are tightly bound to it. See for example
>> U_BOOT_DRIVER(mmc_blk) in the MMC uclass.
> 
> Let's look at an MMC device
> 
> root_driver/soc/mmc at 1c0f000/mmc at 1c0f000.blk is a block device.
> 
> What do we need to find out that it can be addressed as mmc 0? The 
> driver is mmc_blk  and its index is 0. We don't need any information 
> about the parent device at all.
> 
>>
>> Unfortunately this confusion is my fault since I used the root device
>> for the sandbox block devices. That was a convenience and a way to
>> reduce somewhat the crushing load of driver model migration. But the
>> time for that convenience is gone and we should create a sandbox host
>> parent node for the sandbox block devices and tidy up EFI too.
> 
> The only confusion is in the current blk_get_devnum_by_uclass_idname() 
> code looking into the parent device.
> 
> The parent device is totally irrelevant here. Stop using it.

You already noted when writing conv_uclass_id() that using the uclass 
name does not work properly to find out the CLI name of a devie.

Can we put the CLI name for device types ("mmc", "scsi" ...) into struct 
blk_ops? Then we have a clear separation of the block device from the 
parent device.

Best regards

Heinrich