| Message ID | 20221003093550.106304-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [1/1] dm: fix blk_get_devnum_by_uclass_idname() | expand |
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. 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? 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. Regards, Simon
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. > > 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. 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. > > 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. Best regards Heinrich
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? > > > > > 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
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? Would you agree that blk_get_devnum_by_uclass_idname() should not look at the parent but on the actual device? 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) Best regards Heinrich > >> >>> >>> 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
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. 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 >
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. > > 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. Best regards Heinrich
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. > > 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 >>
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. > >> >> 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. 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. Best regards Heinrich
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 ? Regards, Simon
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. 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
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? > > 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
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
On 10/12/22 02:30, AKASHI Takahiro wrote: > 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. The hierarchy in UEFI is defined by the device paths installed on the handles. Nothing stops a UEFI application to create a first handle with device path /a and a second with /a/b/c without creating /a/b. It could even create /a after /a/b/c. An EFI application can legally remove or replace a device path. So a device /a could suddenly become /a/b/c/d or /e/f. This makes tracking the EFI's hierarchy in DM fruitless. On the other hand we would be able to create a device path and handle for each device originating in U-Boot. Best regards Heinrich >> >> +Ilias Apalodimas please do see if you can help here. >> >> Regards, >> Simon
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index 7d12d5413f..8cb1398624 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -133,12 +133,8 @@ struct blk_desc *blk_get_devnum_by_uclass_idname(const char *uclass_idname, int if (desc->devnum != devnum) continue; - /* Find out the parent device uclass */ - if (device_get_uclass_id(dev->parent) != uclass_id) { - debug("%s: parent uclass %d, this dev %d\n", __func__, - device_get_uclass_id(dev->parent), uclass_id); + if (desc->uclass_id != uclass_id) continue; - } if (device_probe(dev)) return NULL;
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/ Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- drivers/block/blk-uclass.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)