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

[Heinrich Schuchardt]

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!

> 
> 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
>