Hi Michal,
On Mon, 10 Oct 2022 at 15:33, Michal Suchánek <msucha...@suse.de> wrote:
>
> 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 <msucha...@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 <msucha...@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 <msucha...@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?
Well you can add a new driver and a device associated with it, to test that.
Regards,
Simon
