Hi Heinrich, On Thu, 6 Oct 2022 at 14:05, Heinrich Schuchardt <heinrich.schucha...@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.schucha...@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.schucha...@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.schucha...@canonical.com/ > >>>> [PATCH 1/1] blk: simplify blk_get_devnum_by_typename() > >>>> https://lore.kernel.org/u-boot/20211023140647.7661-1-heinrich.schucha...@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 >
