[PATCH 1/1] blk: simplify blk_get_devnum_by_typename()

[Heinrich Schuchardt]

On 10/25/21 21:46, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 25 Oct 2021 at 13:37, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>>
>>
>> On 10/25/21 21:03, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 25 Oct 2021 at 12:41, Heinrich Schuchardt
>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/25/21 19:29, Simon Glass wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt
>>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>>
>>>>>> On 10/25/21 17:18, Simon Glass wrote:
>>>>>>> Hi Heinrich,
>>>>>>>
>>>>>>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt
>>>>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>>>>
>>>>>>>> On 10/25/21 09:54, Heinrich Schuchardt wrote:
>>>>>>>>> On 10/24/21 21:54, Simon Glass wrote:
>>>>>>>>>> Hi Heinrich,
>>>>>>>>>>
>>>>>>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt
>>>>>>>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> The block descriptor contains the if_type. There is no need to first
>>>>>>>>>>> look
>>>>>>>>>>> up the uclass for the if_type and then to check the parent device's
>>>>>>>>>>> uclass
>>>>>>>>>>> to know if the device has the correct if_type.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       drivers/block/blk-uclass.c | 35 +----------------------------------
>>>>>>>>>>>       1 file changed, 1 insertion(+), 34 deletions(-)
>>>>>>>>>>
>>>>>>>>>> This seems to be heading in the wrong direction though.
>>>>>>>>>>
>>>>>>>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID,
>>>>>>>>>> I think.
>>>>>>>>>>
>>>>>>>>>> At present we have lots of calls to blk_create_device_f() which
>>>>>>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that
>>>>>>>>>> function and have it figured out from the uclass, in the interim.
>>>>>>>>>>
>>>>>>>>>> But why do we need IF_TYPE now?
>>>>>>>>>
>>>>>>>>> I admit that this patch is just an intermediate step.
>>>>>>>>>
>>>>>>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always
>>>>>>>>> using the driver model. Have we reached this point yet? I have not seen
>>>>>>>>> drivers/block/blk_legacy.c being deleted.
>>>>>>>>
>>>>>>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of
>>>>>>>> defconfigs requiring drivers/block/blk_legacy.c
>>>>>>>
>>>>>>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for
>>>>>>> example) uses IDE.
>>>>>>>
>>>>>>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used.
>>>>>>>
>>>>>>> +Tom Rini
>>>>>>>
>>>>>>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN?
>>>>>>>
>>>>>>>>
>>>>>>>> Best regards
>>>>>>>>
>>>>>>>> Heinrich
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Removing if_type_uclass_id[] is anyway on the right path.
>>>>>>>
>>>>>>> Are you sure? Instead, could we use the uclass ID in more places?
>>>>>>
>>>>>> Yes, you want to get rid of if_type. This means the table will become
>>>>>> obsolete.
>>>>>
>>>>> Yes.
>>>>>
>>>>>> Once the legacy drivers are removed we can replace all occurrences of
>>>>>> if_type by uclass_id. That uclass_id we should not take from blk_desc
>>>>>> but from udevice.
>>>>>
>>>>> How do we get from a blk_desc to a udevice, though?
>>>>>
>>>>> Could you instead look at moving from using blk_desc to using udevice?
>>>>> If we had that, I think I can see your point with this patch. At
>>>>> present, it looks like a backwards step because you are reducing the
>>>>> usage of the uclass and in fact removing it altogether.
>>>>>
>>>>> Can you see what I am getting at? Let's move towards migration
>>>>> incrementally, not destroy the bridges we have built towards the goal.
>>>>
>>>> Once you move from if_type to uclass_id you will simply replace
>>>>
>>>>      if (desc->if_type != if_type)
>>>>
>>>> by
>>>>
>>>>      if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id)
>>>>
>>>> Except for this line this patch brings you to the final code.
>>>
>>> device_get_uclass_id() BTW
>>>
>>> But where does uclass_id come from?! You are removing the function
>>> that produces it!
>>
>> In future you will pass uclass_id as parameter instead of if_type
>> because if_type will be replaced by uclass_id!
> 
> Perhaps we can discuss this on the contributor call as we are not
> getting anywhere.
> 
> You are removing the ability to translate between the old value and
> the new value, but still using the old value in the function call. I
> have already suggested how you could contribute to the migration
> effort if you have time. Rather than increasing reliance on if_type,
> we should be removing use of it, e.g. dropping it from blk_desc, etc.
> 
> 
> - Simon
> 
>>
>>>
>>> How about, instead, you update blk_create_devicef() to drop the
>>> if_type parameter and just use the device's uclass? That would
>>> actually head forwards towards migration, rather than away from it.
>>>
>>> Regards,
>>> Simon
>>>

Without this patch the following fails:

make sandconfig
make menuconfig
# set CONFIG_EFI_SELFTEST=y
make -j$(nproc)
./u-boot -T
setenv efi_selftest block device
bootefi selftest
ls efi 0:1
load efi 0:1 $fdt_addr_r hello.txt

Best regards

Heinrich