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

[Simon Glass]

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.

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
>