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

[Heinrich Schuchardt]

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?

Would you agree that blk_get_devnum_by_uclass_idname() should not look 
at the parent but on the actual device?

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)

Best regards

Heinrich

> 
>>
>>>
>>> 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