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

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...

> +	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