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

[Simon Glass]

Hi Michal,

On Sun, 2 Oct 2022 at 13:34, Michal Suchánek <msuchanek at suse.de> wrote:
>
> 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.

Why is that?

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

I agree.

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

OK, perhaps I understand this. But don't you need to update the
comment in the header file to say that devices that don't probe are
silently skipped?

Also it really does need a test.

Regards,
Simon