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

[David Laight <david.laight.linux@gmail.com>]

On Sat, 22 Aug 2026 23:03:26 +0200
Michal Pecio <michal.pecio@gmail.com> wrote:

> On Sat, 22 Aug 2026 23:21:54 +0300, Sergey Shtylyov 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;

I think you meant:
	chunk = min(size, limit);

> 	ret = get_registers(tp, index, type, chunk, data);
> 	if (ret < 0)
> 		break;
> 	index += chunk;
> 	data += chunk;
> 	size -= chunk;
> }

That is the usual pattern...
Although may you need to be careful to stop min() bleating if size
is a signed type.
Changing the loop to 'while (size > 0)' can be enough.

David

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