[PATCH] block: Remove unreachable code

[Andrew Goodbody]

On 14/07/2025 20:13, 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 also going to follow up on how much this code was actually used. 
So yes thanks for confirming that all this code has no user. In which 
case I would suggest that it can just be removed.

In the meantime I will post my V2 to include clean up of 
uclass_find_next_device as it is at least a step in the right direction.

Andrew

> 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