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 selftestand see the following output:
** Bad device specification host 0 ** Couldn't find partition host 0:0 Cannot read EFI system partitionRunning
=> lsblkyields
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:1and 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@ca... [PATCH 1/1] blk: simplify blk_get_devnum_by_typename() https://lore.kernel.org/u-boot/20211023140647.7661-1-heinrich.schuchardt@can...
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.
Yes, it should be
if (device_get_uclass_id(dev) != type)
as dev == desc->dev here.
Best regards
Heinrich
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