[PATCH 1/1] dm: fix blk_get_devnum_by_uclass_idname()
Heinrich Schuchardt
heinrich.schuchardt at canonical.com
Tue Oct 11 22:17:48 CEST 2022
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.
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.
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.
Why can't we have the CLI string for the device type in the driver's
struct blk_ops?
Best regards
Heinrich
More information about the U-Boot
mailing list