[PATCH v4 04/21] dm: blk: Add probe in blk_first_device/blk_next_device

[Michal Suchánek]

On Thu, Sep 29, 2022 at 04:00:26AM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Sun, 25 Sept 2022 at 02:28, Michal Suchanek <msuchanek at suse.de> wrote:
> >
> > The description claims that the device is probed but it isn't.
> >
> > Add the device_probe() call.
> >
> > Also consolidate the iteration into one function.
> >
> > Fixes: 8a5cbc065d ("dm: blk: Use uclass_find_first/next_device() in blk_first/next_device()")
> > Signed-off-by: Michal Suchanek <msuchanek at suse.de>
> > ---
> >  drivers/block/blk-uclass.c | 46 ++++++++++++++++++--------------------
> >  1 file changed, 22 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> > index 21c5209bb6..992f8ad3da 100644
> > --- a/drivers/block/blk-uclass.c
> > +++ b/drivers/block/blk-uclass.c
> > @@ -361,45 +361,43 @@ int blk_dselect_hwpart(struct blk_desc *desc, int hwpart)
> >         return blk_select_hwpart(desc->bdev, hwpart);
> >  }
> >
> > -int blk_first_device(int if_type, struct udevice **devp)
> > +static int _blk_next_device(int if_type, struct udevice **devp)
> >  {
> >         struct blk_desc *desc;
> > -       int ret;
> > +       int ret = 0;
> > +
> > +       for (; *devp; uclass_find_next_device(devp)) {
> > +               desc = dev_get_uclass_plat(*devp);
> > +               if (desc->if_type == if_type) {
> > +                       ret = device_probe(*devp);
> > +                       if (!ret)
> > +                               return 0;
> > +               }
> > +       }
> >
> > -       ret = uclass_find_first_device(UCLASS_BLK, devp);
> >         if (ret)
> >                 return ret;
> > -       if (!*devp)
> > -               return -ENODEV;
> > -       do {
> > -               desc = dev_get_uclass_plat(*devp);
> > -               if (desc->if_type == if_type)
> > -                       return 0;
> > -               ret = uclass_find_next_device(devp);
> > -               if (ret)
> > -                       return ret;
> > -       } while (*devp);
> 
> This looks wrong since a media device may have other devices under it,
> e.g. UCLASS_BOOTDEV so I think you should keep the existing code and
> just call uclass_probe() at the end.
> 
> You could add a test for this by checking that only the BLK device is probed.

The description says that it returns ready to use device, and that's not
possible when the device is only probed at the end when it is to be
returned.

There are some tests of this function but very few users so it may be OK
to change the semantic again to resemble the _check variant uclass
iterator and retorn broken devices but I don't think that was the intent
here with using uclass_first_device/uclass_next_device originally.

Also this change only makes a difference to the amount of devices probed
for callers that only call the blk_first_device and never move on to the
next. Callers that use the functions for iteration will move on to the
next device and probe it anyway.

Thanks

Michal