Hi Tom, Andrew,
On Mon, Jul 14, 2025 at 1:12 PM Andrew Goodbody
<andrew.goodb...@linaro.org> wrote:
>
> On 14/07/2025 17:29, Tom Rini wrote:
> > On Mon, Jul 14, 2025 at 04:38:53PM +0100, Andrew Goodbody wrote:
> >> The two functions blk_find_first and blk_find_next use a for loop with
> >> the content being a 'return 0' which means that the 'increment' code is
> >> unreachable so remove it and also remove the variable ret which is
> >> assigned to but its value is never used.
> >>
> >> This issue found by Smatch.
> >>
> >> Signed-off-by: Andrew Goodbody <andrew.goodb...@linaro.org>
> >> ---
> >> drivers/block/blk-uclass.c | 14 ++++----------
> >> 1 file changed, 4 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> >> index f3ac8db9464..b38c21ffbe2 100644
> >> --- a/drivers/block/blk-uclass.c
> >> +++ b/drivers/block/blk-uclass.c
> >> @@ -613,11 +613,8 @@ static int blk_flags_check(struct udevice *dev, enum
> >> blk_flag_t req_flags)
> >>
> >> int blk_find_first(enum blk_flag_t flags, struct udevice **devp)
> >> {
> >> - int ret;
> >> -
> >> - for (ret = uclass_find_first_device(UCLASS_BLK, devp);
> >> - *devp && !blk_flags_check(*devp, flags);
> >> - ret = uclass_find_next_device(devp))
> >> + for (uclass_find_first_device(UCLASS_BLK, devp);
> >> + *devp && !blk_flags_check(*devp, flags);)
> >> return 0;
> >>
> >> return -ENODEV;
> >> @@ -625,11 +622,8 @@ int blk_find_first(enum blk_flag_t flags, struct
> >> udevice **devp)
> >>
> >> int blk_find_next(enum blk_flag_t flags, struct udevice **devp)
> >> {
> >> - int ret;
> >> -
> >> - for (ret = uclass_find_next_device(devp);
> >> - *devp && !blk_flags_check(*devp, flags);
> >> - ret = uclass_find_next_device(devp))
> >> + for (uclass_find_next_device(devp);
> >> + *devp && !blk_flags_check(*devp, flags);)
> >> return 0;
> >>
> >> return -ENODEV;
> >
> > Should we clean up uclass_find_next_device(...) to not return int? The
> > function comments don't quite make sense (include/dm/uclass-internal.h)
> > and it always returns 0.
>
> OK, that should not be too bad.
>
> Andrew
To me it looks like this function doesn't do what is advertised in
include/blk.h in either case (find the first block device with
matching flags by checking each one until one matches or we run out),
instead it seems to test the first block device for the flags
requested and either returns -ENODEV or 0. Out of curiosity, i looked
for uses of blk_find_first and blk_find_next, and (except for
test/dm/blk.c where all the functions are used) they're only used by
the blk_foreach macro (include/blk.h again), which is only used in
blk_foreach_probe, which is only used in blk_count_devices, which is
used nowhere. Instead users use blk_first_device_err which is a
completely separate path (drivers/block/blk-uclass.c:638).
I was hoping the test code could clarify a bit and it might. In test/dm/blk.c:
dm_test_blk_flags first looks for devices with either flag (BLKF_BOTH)
so the flag check never fails, then it looks for BLKF_FIXED ,which it
indicates must fail because before probing all devices are removable,
then it looks for BLKF_REMOVABLE, which is set for the first device
before probing (even though it will become BLKF_FIXED later). So none
of these are (I think) testing for the situation where the first
device is missing a flag, but the second device has it and
blk_find_first should return the second device. Then in
dm_test_blk_iter, blk_count_devices is used with varying flag
combinations and is expected to give the correct answer, which
suggests that the code does actually work, assuming the test case runs
and succeeds.
I've been looking for something to poke around in and if you're ok
with it I'd be interested in exploring this further to figure out
whether the function does what it is intended to or if there's an
opportunity to improve the test coverage.
Thanks,
Greg
