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

Simon Glass sjg at chromium.org
Tue Oct 11 23:54:32 CEST 2022


Hi Heinrich,

On Tue, 11 Oct 2022 at 14:17, Heinrich Schuchardt
<heinrich.schuchardt at 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 at 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 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.
> >
> > 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?

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


More information about the U-Boot mailing list