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

On Sun, 23 Aug 2026 10:52:12 +0300
Sergey Shtylyov <[email protected]> wrote:

> On 8/22/26 11:21 PM, 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 <[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.

David

> 
> > +	ret = get_registers(tp, index, type, size, data);
> > +
> > +error1:
> >  	if (ret == -ENODEV)
> >  		rtl_set_unplug(tp);
> >  
> > @@ -1498,31 +1490,24 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
> >  		if (byen != BYTE_EN_DWORD)
> >  			size -= 4;
> >  
> > -		while (size) {
> > -			if (size > limit) {
> > -				ret = set_registers(tp, index,
> > -						    type | BYTE_EN_DWORD,
> > -						    limit, data);
> > -				if (ret < 0)
> > -					goto error1;
> > -
> > -				index += limit;
> > -				data += limit;
> > -				size -= limit;
> > -			} else {
> > -				ret = set_registers(tp, index,
> > -						    type | BYTE_EN_DWORD,
> > -						    size, data);
> > -				if (ret < 0)
> > -					goto error1;
> > -
> > -				index += size;
> > -				data += size;
> > -				size = 0;
> > -				break;
> > -			}
> > +		while (size > limit) {
> > +			ret = set_registers(tp, index, type | BYTE_EN_DWORD,
> > +					    limit, data);
> > +			if (ret < 0)
> > +				goto error1;
> > +
> > +			index += limit;
> > +			data += limit;
> > +			size -= limit;
> >  		}
> >    
> 
>    And here...
> 
> > +		ret = set_registers(tp, index, type | BYTE_EN_DWORD, size, data);
> > +		if (ret < 0)
> > +			goto error1;
> > +
> > +		index += size;
> > +		data += size;
> > +
> >  		/* Set the last DWORD */
> >  		if (byen != BYTE_EN_DWORD)
> >  			ret = set_registers(tp, index, type | byen, 4, data);  
> 
> MBR, Sergey
> 
>