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

[Heinrich Schuchardt]

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.
> 
> 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.
> 
>>
>> If it makes your future refactoring easier, I could use
>>
>>      if (device_get_uclass_id(desc->dev) != type)
>>
>> instead of
>>
>>      if (desc->uclass_id != type)
> 
> One problem with that is the use of desc->dev, since we want to drop
> that field soon.

Yes, it should be

     if (device_get_uclass_id(dev) != type)

as dev == desc->dev here.

Best regards

Heinrich

> 
> So can you fix the use of the root node as a parent of a block device?
> This affects sandbox and EFI, from what I understand.
> 
> Regards,
> Simon
> 
>>
>>>
>>>>
>>>>>
>>>>> Also, I wonder if we can require SPL_BLK and thus get rid of the
>>>>> legacy block interface? Then we can drop drop uclass_id and a few
>>>>> other fields from struct blk_desc.
>>>>
>>>> This is beyond the scope of this patch. Neither host nor efi_loader
>>>> devices exist in SPL.
>>>
>>> Yes I understand that. It was just a question...
>>>
>>> Regards,
>>> Simon
>>