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

[Simon Glass]

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