On Mon, Jul 14, 2025 at 03:13:26PM -0400, Greg Malysa wrote: > Hi Tom, Andrew, > > On Mon, Jul 14, 2025 at 1:12 PM Andrew Goodbody > <[email protected]> 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 <[email protected]> > > >> --- > > >> 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.
If you would like to dig in here further that would be much appreciated, thanks! -- Tom
signature.asc
Description: PGP signature
