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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Tue, Oct 11, 2022 at 03:54:32PM -0600, Simon Glass wrote:
> Hi Heinrich,
> 
> On Tue, 11 Oct 2022 at 14:17, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > On 10/11/22 16:16, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Tue, 11 Oct 2022 at 04:38, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > >>
> > >>
> > >>
> > >> 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@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@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@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@1c0f000/mmc@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.
> > >
> > > If blk is the MMC block device, the fact that is mmc 0 is determined
> > > by dev_seq(dev_get_parent(blk)). We are not parsing strings to find
> > > that out. It is part of the design.
> > >
> > >>>
> > >>>>
> > >>>> 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.
> > >
> > > See below.
> > >
> > >>
> > >> 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.
> > >
> > > There really isn't any separation in driver model...the parent device
> > > does determine the type of the block device. It creates the block
> > > device, using its own uclass. See for example mmc-uclass.c in
> > > mmc_bind():
> > >
> > > ret = blk_create_devicef(dev, "mmc_blk", "blk", UCLASS_MMC,
> > > dev_seq(dev), 512, 0, &bdev);
> > >
> > > The following fields in blk_desc will be dropped at some point:
> > >
> > > - uclass_id since it is the same as the parent*
> > > - bdev (point to block device) since we will stop passing around
> > > blk_desc and will use the block device instead
> > > - devnum since it is the save as dev_seq(blk)
> > >
> > > * Except for the USB weirdness in conv_uclass_id() which we need to fix
> > >
> > > Why do you want this 'separation'? Is this another strange EFI thing
> > > due to it not using driver model properly?
> > >
> > > Also you have not yet replied to my point about needing to create a
> > > parent 'media' device for every block device. That is also part of the
> > > design. Have you done that for EFI, or is your reluctance to do that
> > > behind continued discussions and misalignments on UCLASS_BLK ?
> >
> > If I look at physical devices for MMC I might find:
> >
> > SoC -> PCI root -> MMC controller -> SD card
> >
> > What you call MMC parent device is the MMC controller.
> >
> > This is also what can easily modeled as a device path in EFI.
> 
> OK good. That covers all devices in U-Boot present, I believe.
> 
> >
> > In the case of an iSCSI drive provided by iPXE U-boot would provide a
> > network device which currently has a device path VenHW(root)/MAC().
> >
> > iPXE creates a virtual network card VenHW(root)/MAC()/MAC() consuming
> > the services of the physical one.
> >
> > Next it creates a virtual device VenHW(root)/MAC()/MAC()/IPv6() which
> > exposes the block IO protocol for reading the iSCSI drive.
> >
> > The parent for the block device in the EFI world is a network interface.
> > But the block operations are provided by the block IO protocol which is
> > provided by the virtual device that iPXE has created and not by a
> > network interface. So the parent is irrelevant here.
> 
> Then the virtual device should be the parent? Are we trying to skip
> one level of hierarchy?

+1
IMO, the virtual device is a handle (in UEFI term) of UEFI application,
i.e. iPXE since it actually implements and provides block IO protocol.

In this sense, I don't think that the current implementation of efi_driver
is appropriate.
(I mentioned this in the past.)

What I'm not sure, however, is whether the network device card should
be a *parent* of the application because application may potentially
implement functionality other than block IO using another device.

Furthermore, what I don't understand yet is what the hierarchy of DM tree
means for parent-children relationship other than block device case.

-Takahiro Akashi

> >
> > Sure you could create a single root2 device as parent for all efi_loader
> > devices like you have root for the host devices. But such a device would
> > have no functionality at all except carrying a dummy Uclass to store the
> > CLI string "efiblk" for all of its children.
> 
> I don't think it should be a root2 device. It should really be a child
> of the network device, so far as I understand what you have written
> above.
> 
> >
> > Why can't we have the CLI string for the device type in the driver's
> > struct blk_ops?
> 
> It isn't just about the CLI string. It's also about having a sensible
> device hierarchy with 'dm tree', being able to put things in the
> device tree in a sensible way, etc. This feels like a symptom of the
> lack of alignment between EFI and driver model.
> 
> +Ilias Apalodimas please do see if you can help here.
> 
> Regards,
> Simon