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

[Michal Suchánek]

On Mon, Oct 10, 2022 at 09:49:20PM +0200, Michal Suchánek wrote:
> On Sun, Oct 02, 2022 at 07:10:40PM -0600, Simon Glass wrote:
> > 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 two options:
> 
>  - probe the device, and skip it if it fails, potentially probing
>    multiple devices before returning one
>  - decide what device to return, probe it, and if it fails return
>    non-activated device
> 
> > > 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?
> 
> They are not ready to use so they cannot be returned by the current
> description?
> 
> > 
> > Also it really does need a test.
> 
> Right, tests are good to prevent similar regression in the future.

But we don't have the boilerplate for testing failure in block
devices, only in the special probe test class.

Or do we?

Thanks

Michal