| Message ID | 20220802094933.69170-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] block: fix blk_get_devnum_by_typename() | expand |
Hi Heinrich, On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > Both the 'host' and the 'efiloader' block devices use the same parent > uclass root. Thus the parent uclass is not an indicator the interface type. > > Currently the following fails: > > setenv efi_selftest block device > bootefi selftest > part list efiloader 0 > > Struct blk_desc contains the interface type. So we can check it directly > without caring about the parent uclass. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > drivers/block/blk-uclass.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) We've had this discussion before, but this patch will make it difficult to migrate away from IF_TYPE. Instead we should fix EFI. Having the root as a parent of a block device seems wrong to me. What is the actual device that provides the block device? > > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c > index 21c5209bb6..779cda7834 100644 > --- a/drivers/block/blk-uclass.c > +++ b/drivers/block/blk-uclass.c > @@ -122,15 +122,11 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum) > > debug("%s: if_type=%d, devnum=%d: %s, %d, %d\n", __func__, > if_type, devnum, dev->name, desc->if_type, desc->devnum); > - 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->if_type != if_type) > + continue; > + if (desc->devnum != devnum) > continue; > - } > > if (device_probe(dev)) > return NULL; > -- > 2.36.1 > Regards, Simon
On 8/2/22 14:41, Simon Glass wrote: > Hi Heinrich, > > On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> Both the 'host' and the 'efiloader' block devices use the same parent >> uclass root. Thus the parent uclass is not an indicator the interface type. >> >> Currently the following fails: >> >> setenv efi_selftest block device >> bootefi selftest >> part list efiloader 0 >> >> Struct blk_desc contains the interface type. So we can check it directly >> without caring about the parent uclass. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> drivers/block/blk-uclass.c | 10 +++------- >> 1 file changed, 3 insertions(+), 7 deletions(-) > > We've had this discussion before, but this patch will make it Yes, you blocked the obvious solution. > difficult to migrate away from IF_TYPE. My patch does not have any impact on the migration as function blk_get_devnum_by_typename() will simply vanish together with IF_TYPE. Migrating away from IF_TYPE could follow the following path if you wanted to keep struct blk_desc: Just replace devnum by the udevice in struct blk_desc and add the GUI representation of the device type (e.g. "mmc") as field to struct blk_ops. The field devnum only made sense in the world of legacy drivers. By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers? A better solution would be to completely do away with struct blk_desc and instead always use the udevice. > > Instead we should fix EFI. Having the root as a parent of a block > device seems wrong to me. What is the actual device that provides the > block device? There is no actual parent device. In lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM disk. This is the same situation as with the sandbox host device where you have chosen root as the dummy parent for good reason. In "[1/1] drivers: add memory disk support" https://patchwork.ozlabs.org/project/uboot/patch/20220419211641.316935-1-heinrich.schuchardt@canonical.com/ I have proposed a further block device type that has no actual parent. The idea of using a parent device to match a block device was always a dead end. Let's bury it now. Best regards Heinrich > >> >> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c >> index 21c5209bb6..779cda7834 100644 >> --- a/drivers/block/blk-uclass.c >> +++ b/drivers/block/blk-uclass.c >> @@ -122,15 +122,11 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum) >> >> debug("%s: if_type=%d, devnum=%d: %s, %d, %d\n", __func__, >> if_type, devnum, dev->name, desc->if_type, desc->devnum); >> - 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->if_type != if_type) >> + continue; >> + if (desc->devnum != devnum) >> continue; >> - } >> >> if (device_probe(dev)) >> return NULL; >> -- >> 2.36.1 >> > > Regards, > Simon
Hi Heinrich, On Tue, 2 Aug 2022 at 10:22, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 8/2/22 14:41, Simon Glass wrote: > > Hi Heinrich, > > > > On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> Both the 'host' and the 'efiloader' block devices use the same parent > >> uclass root. Thus the parent uclass is not an indicator the interface type. > >> > >> Currently the following fails: > >> > >> setenv efi_selftest block device > >> bootefi selftest > >> part list efiloader 0 > >> > >> Struct blk_desc contains the interface type. So we can check it directly > >> without caring about the parent uclass. > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> drivers/block/blk-uclass.c | 10 +++------- > >> 1 file changed, 3 insertions(+), 7 deletions(-) > > > > We've had this discussion before, but this patch will make it > > Yes, you blocked the obvious solution. Yes, I explained the problem with that at the time. > > > difficult to migrate away from IF_TYPE. > > My patch does not have any impact on the migration as function > blk_get_devnum_by_typename() will simply vanish together with IF_TYPE. > > Migrating away from IF_TYPE could follow the following path if you > wanted to keep struct blk_desc: > > Just replace devnum by the udevice in struct blk_desc and add the GUI > representation of the device type (e.g. "mmc") as field to struct blk_ops. > > The field devnum only made sense in the world of legacy drivers. > By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers? > > A better solution would be to completely do away with struct blk_desc > and instead always use the udevice. > > > > > Instead we should fix EFI. Having the root as a parent of a block > > device seems wrong to me. What is the actual device that provides the > > block device? > > There is no actual parent device. In > lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM > disk. This is the same situation as with the sandbox host device where > you have chosen root as the dummy parent for good reason. Is it a RAM disk on disk, or an in-memory one? > > In > "[1/1] drivers: add memory disk support" > https://patchwork.ozlabs.org/project/uboot/patch/20220419211641.316935-1-heinrich.schuchardt@canonical.com/ > I have proposed a further block device type that has no actual parent. > > The idea of using a parent device to match a block device was always a > dead end. Let's bury it now. Possibly, but it is how we can drop the IF_TYPE stuff. Let me spend a bit of time looking at this and see what can be done. [..] Regards, Simon
On 8/3/22 20:14, Simon Glass wrote: > Hi Heinrich, > > On Tue, 2 Aug 2022 at 10:22, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> >> >> On 8/2/22 14:41, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt >>> <heinrich.schuchardt@canonical.com> wrote: >>>> >>>> Both the 'host' and the 'efiloader' block devices use the same parent >>>> uclass root. Thus the parent uclass is not an indicator the interface type. >>>> >>>> Currently the following fails: >>>> >>>> setenv efi_selftest block device >>>> bootefi selftest >>>> part list efiloader 0 >>>> >>>> Struct blk_desc contains the interface type. So we can check it directly >>>> without caring about the parent uclass. >>>> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>> --- >>>> drivers/block/blk-uclass.c | 10 +++------- >>>> 1 file changed, 3 insertions(+), 7 deletions(-) >>> >>> We've had this discussion before, but this patch will make it >> >> Yes, you blocked the obvious solution. > > Yes, I explained the problem with that at the time. > >> >>> difficult to migrate away from IF_TYPE. >> >> My patch does not have any impact on the migration as function >> blk_get_devnum_by_typename() will simply vanish together with IF_TYPE. >> >> Migrating away from IF_TYPE could follow the following path if you >> wanted to keep struct blk_desc: >> >> Just replace devnum by the udevice in struct blk_desc and add the GUI >> representation of the device type (e.g. "mmc") as field to struct blk_ops. >> >> The field devnum only made sense in the world of legacy drivers. >> By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers? >> >> A better solution would be to completely do away with struct blk_desc >> and instead always use the udevice. >> >>> >>> Instead we should fix EFI. Having the root as a parent of a block >>> device seems wrong to me. What is the actual device that provides the >>> block device? >> >> There is no actual parent device. In >> lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM >> disk. This is the same situation as with the sandbox host device where >> you have chosen root as the dummy parent for good reason. > > Is it a RAM disk on disk, or an in-memory one? With the patch it is just an memory area embedded the U-Boot binary. But in future you might also use it to declare a memory area in the rest of RAM as backing a RAM disk. > >> >> In >> "[1/1] drivers: add memory disk support" >> https://patchwork.ozlabs.org/project/uboot/patch/20220419211641.316935-1-heinrich.schuchardt@canonical.com/ >> I have proposed a further block device type that has no actual parent. >> >> The idea of using a parent device to match a block device was always a >> dead end. Let's bury it now. > > Possibly, but it is how we can drop the IF_TYPE stuff. Let me spend a > bit of time looking at this and see what can be done. To me the device driver of the actual device would be the most natural indicator of the device type. Looking forward to your suggestion. Best regards Heinrich > > [..] > > Regards, > Simon
Hi Heinrich, On Thu, 4 Aug 2022 at 00:06, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 8/3/22 20:14, Simon Glass wrote: > > Hi Heinrich, > > > > On Tue, 2 Aug 2022 at 10:22, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> > >> > >> On 8/2/22 14:41, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt > >>> <heinrich.schuchardt@canonical.com> wrote: > >>>> > >>>> Both the 'host' and the 'efiloader' block devices use the same parent > >>>> uclass root. Thus the parent uclass is not an indicator the interface type. > >>>> > >>>> Currently the following fails: > >>>> > >>>> setenv efi_selftest block device > >>>> bootefi selftest > >>>> part list efiloader 0 > >>>> > >>>> Struct blk_desc contains the interface type. So we can check it directly > >>>> without caring about the parent uclass. > >>>> > >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >>>> --- > >>>> drivers/block/blk-uclass.c | 10 +++------- > >>>> 1 file changed, 3 insertions(+), 7 deletions(-) > >>> > >>> We've had this discussion before, but this patch will make it > >> > >> Yes, you blocked the obvious solution. > > > > Yes, I explained the problem with that at the time. > > > >> > >>> difficult to migrate away from IF_TYPE. > >> > >> My patch does not have any impact on the migration as function > >> blk_get_devnum_by_typename() will simply vanish together with IF_TYPE. > >> > >> Migrating away from IF_TYPE could follow the following path if you > >> wanted to keep struct blk_desc: > >> > >> Just replace devnum by the udevice in struct blk_desc and add the GUI > >> representation of the device type (e.g. "mmc") as field to struct blk_ops. > >> > >> The field devnum only made sense in the world of legacy drivers. > >> By the way why do I still find CONFIG_IS_ENABLED(BLK) in block drivers? > >> > >> A better solution would be to completely do away with struct blk_desc > >> and instead always use the udevice. > >> > >>> > >>> Instead we should fix EFI. Having the root as a parent of a block > >>> device seems wrong to me. What is the actual device that provides the > >>> block device? > >> > >> There is no actual parent device. In > >> lib/efi_selftest/efi_selftest_block_device.c the block device is a RAM > >> disk. This is the same situation as with the sandbox host device where > >> you have chosen root as the dummy parent for good reason. > > > > Is it a RAM disk on disk, or an in-memory one? > > With the patch it is just an memory area embedded the U-Boot binary. But > in future you might also use it to declare a memory area in the rest of > RAM as backing a RAM disk. I sent a series to clean this up. Regards, Simon
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index 21c5209bb6..779cda7834 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -122,15 +122,11 @@ struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum) debug("%s: if_type=%d, devnum=%d: %s, %d, %d\n", __func__, if_type, devnum, dev->name, desc->if_type, desc->devnum); - 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->if_type != if_type) + continue; + if (desc->devnum != devnum) continue; - } if (device_probe(dev)) return NULL;
Both the 'host' and the 'efiloader' block devices use the same parent uclass root. Thus the parent uclass is not an indicator the interface type. Currently the following fails: setenv efi_selftest block device bootefi selftest part list efiloader 0 Struct blk_desc contains the interface type. So we can check it directly without caring about the parent uclass. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- drivers/block/blk-uclass.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)