[PATCH] block: Remove unreachable code

[Tom Rini]

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
> <andrew.goodbody at 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.goodbody at 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.

If you would like to dig in here further that would be much appreciated,
thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250714/8072db86/attachment.sig>