[PATCH 1/1] block: fix blk_get_devnum_by_typename()

[Heinrich Schuchardt]

On 8/2/22 14:41, Simon Glass wrote:
> Hi Heinrich,
> 
> On Tue, 2 Aug 2022 at 03:50, Heinrich Schuchardt
> <heinrich.schuchardt at 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 at 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