Message ID | 76875ef0301a144797b7aee456ecb0a699a0418f.1664093812.git.msuchanek@suse.de |
---|---|
State | Superseded |
Delegated to: | Simon Glass |
Headers | show |
Series | Do not stop uclass iteration on error | expand |
Hi Michal, On Sun, 25 Sept 2022 at 02:28, Michal Suchanek <msuchanek@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@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. > > return -ENODEV; > } > > +int blk_first_device(int if_type, struct udevice **devp) > +{ > + uclass_find_first_device(UCLASS_BLK, devp); > + > + return _blk_next_device(if_type, devp); > +} > + > int blk_next_device(struct udevice **devp) > { > struct blk_desc *desc; > - int ret, if_type; > + int if_type; > > desc = dev_get_uclass_plat(*devp); > if_type = desc->if_type; > - do { > - ret = uclass_find_next_device(devp); > - if (ret) > - return ret; > - if (!*devp) > - return -ENODEV; > - desc = dev_get_uclass_plat(*devp); > - if (desc->if_type == if_type) > - return 0; > - } while (1); > + uclass_find_next_device(devp); > + > + return _blk_next_device(if_type, devp); > } > > int blk_find_device(int if_type, int devnum, struct udevice **devp) > -- > 2.37.3 > Regards, Simon
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@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@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
Hi Michal, On Sun, 2 Oct 2022 at 13:34, Michal Suchánek <msuchanek@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@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@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
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@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@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@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. Thanks Michal
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@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@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@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
Hi Michal, On Mon, 10 Oct 2022 at 15:33, Michal Suchánek <msuchanek@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 <msuchanek@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@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@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
On Mon, 10 Oct 2022 at 16:33, Simon Glass <sjg@chromium.org> wrote: > > Hi Michal, > > On Mon, 10 Oct 2022 at 15:33, Michal Suchánek <msuchanek@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 <msuchanek@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@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@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. > Applied to u-boot-dm (please check)
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); return -ENODEV; } +int blk_first_device(int if_type, struct udevice **devp) +{ + uclass_find_first_device(UCLASS_BLK, devp); + + return _blk_next_device(if_type, devp); +} + int blk_next_device(struct udevice **devp) { struct blk_desc *desc; - int ret, if_type; + int if_type; desc = dev_get_uclass_plat(*devp); if_type = desc->if_type; - do { - ret = uclass_find_next_device(devp); - if (ret) - return ret; - if (!*devp) - return -ENODEV; - desc = dev_get_uclass_plat(*devp); - if (desc->if_type == if_type) - return 0; - } while (1); + uclass_find_next_device(devp); + + return _blk_next_device(if_type, devp); } int blk_find_device(int if_type, int devnum, struct udevice **devp)
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@suse.de> --- drivers/block/blk-uclass.c | 46 ++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 24 deletions(-)