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