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

On 8/23/26 12:51 PM, David Laight wrote:
[...]

>> On 8/23/26 11:11 AM, David Laight 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 <[email protected]>
>>>>
>>>>    Actually, scratch this patch -- it's not entirely correct... :-/
>>>>
>>>> [...]
>>>>
>>>>> 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;
>>>>>     }
>>>>>
>>>>
>>>>    I forgot to check size for 0 here...
>>>
>>> I don't think it can be zero - assuming it isn't zero on entry.
>>
>>    Even if so, anyways it can -- if size % limit == 0 on entry...
> 
> Not with the 'size > limit' check at the top of the loop.

   Ah, indeed, I didn't realize this check is also "strange": the
code of the last iteration gets always executed, even if !(size %
limit)... Anyway, the current code is safe against !size on entry
and mine is not. I think I should play safe and keep that behavior...

[...]

MBR, Sergey