Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()

[Sergey Shtylyov <s.shtylyov@auroraos.dev>]

On 8/23/26 12:03 AM, Michal Pecio wrote:

[...]

>> In generic_ocp_{read,write}(), the *while* loops look very strange:
>> the last iteration is executed differently to the prior ones, doing
>> some useless assignments before *break*. Move the code for the last
>> iteration out of the loop bodies, dropping the pointless statements
>> as well...
>>
>> Found by Linux Verification Center (linuxtesting.org) with the Svace
>> static analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>
>>
>> ---
>>  drivers/net/usb/r8152.c | 67 ++++++++++++++++-------------------------
>>  1 file changed, 26 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>> index f61686433031..de9738bdce85 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
>> @@ -1431,27 +1431,19 @@ static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
>>       if ((u32)index + (u32)size > 0xffff)
>>               return -EPERM;
>>
>> -     while (size) {
>> -             if (size > limit) {
>> -                     ret = get_registers(tp, index, type, limit, data);
>> -                     if (ret < 0)
>> -                             break;
>> -
>> -                     index += limit;
>> -                     data += limit;
>> -                     size -= limit;
>> -             } else {
>> -                     ret = get_registers(tp, index, type, size, data);
>> -                     if (ret < 0)
>> -                             break;
>> +     while (size > limit) {
>> +             ret = get_registers(tp, index, type, limit, data);
>> +             if (ret < 0)
>> +                     goto error1;
>>
>> -                     index += size;
>> -                     data += size;
>> -                     size = 0;
>> -                     break;
>> -             }
>> +             index += limit;
>> +             data += limit;
>> +             size -= limit;
>>       }
>>
>> +     ret = get_registers(tp, index, type, size, data);
>> +
>> +error1:
>>       if (ret == -ENODEV)
>>               rtl_set_unplug(tp);
> 
> Looks like it could be shorter still.
> 
> s/limit/chunk/
> 
> while (size) {
>         if (size < chunk)
>                 chunk = size;
>         ret = get_registers(tp, index, type, chunk, data);
>         if (ret < 0)
>                 break;
>         index += chunk;
>         data += chunk;
>         size -= chunk;
> }

   That's definitely better, thank you! :-)
   I'll rewrite the patch along these lines, mentioning you in the
Suggested-by tag...

> Then it could be do-while, because we know size > 0, though
> I suppose compilers may figure it out themselves anyway.

   I'd prefer to leave handling of !size as it is now, so I'll
keep using the *while* loop...

> Regards,
> Michal

MBR, Sergey