Netdev List
 help / color / mirror / Atom feed
* [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
@ 2026-08-22 20:21 Sergey Shtylyov
  2026-08-22 20:26 ` Sergey Shtylyov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Sergey Shtylyov @ 2026-08-22 20:21 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev
  Cc: Sergey Shtylyov

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);
 
@@ -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;
 		}
 
+		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);
-- 
2.55.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-22 20:21 [PATCH] r8152: simplify loops in generic_ocp_{read,write}() Sergey Shtylyov
@ 2026-08-22 20:26 ` Sergey Shtylyov
  2026-08-22 20:58   ` Jakub Kicinski
  2026-08-22 21:03 ` Michal Pecio
  2026-08-23  7:52 ` Sergey Shtylyov
  2 siblings, 1 reply; 12+ messages in thread
From: Sergey Shtylyov @ 2026-08-22 20:26 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev

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 <s.shtylyov@auroraos.dev>
[...]

   And of course I forgot [net-next] in the subject... :-)

MBR, Sergey


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-22 20:26 ` Sergey Shtylyov
@ 2026-08-22 20:58   ` Jakub Kicinski
  2026-08-23  8:37     ` Sergey Shtylyov
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2026-08-22 20:58 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
	linux-usb, netdev

On Sat, 22 Aug 2026 23:26:30 +0300 Sergey Shtylyov wrote:
>    And of course I forgot [net-next] in the subject... :-)

And to check if it's open ;)
https://netdev.bots.linux.dev/net-next.html
-- 
pw-bot: defer

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-22 20:21 [PATCH] r8152: simplify loops in generic_ocp_{read,write}() Sergey Shtylyov
  2026-08-22 20:26 ` Sergey Shtylyov
@ 2026-08-22 21:03 ` Michal Pecio
  2026-08-23  8:16   ` David Laight
  2026-08-24 16:00   ` Sergey Shtylyov
  2026-08-23  7:52 ` Sergey Shtylyov
  2 siblings, 2 replies; 12+ messages in thread
From: Michal Pecio @ 2026-08-22 21:03 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev

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

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

Regards,
Michal

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-22 20:21 [PATCH] r8152: simplify loops in generic_ocp_{read,write}() Sergey Shtylyov
  2026-08-22 20:26 ` Sergey Shtylyov
  2026-08-22 21:03 ` Michal Pecio
@ 2026-08-23  7:52 ` Sergey Shtylyov
  2026-08-23  8:11   ` David Laight
  2 siblings, 1 reply; 12+ messages in thread
From: Sergey Shtylyov @ 2026-08-23  7:52 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev

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 <s.shtylyov@auroraos.dev>

   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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-23  7:52 ` Sergey Shtylyov
@ 2026-08-23  8:11   ` David Laight
       [not found]     ` <4a7604ab-c6ef-4552-b63b-3e7146ff42d0@auroraos.dev>
  0 siblings, 1 reply; 12+ messages in thread
From: David Laight @ 2026-08-23  8:11 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev

On Sun, 23 Aug 2026 10:52:12 +0300
Sergey Shtylyov <s.shtylyov@auroraos.dev> 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 <s.shtylyov@auroraos.dev>  
> 
>    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
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-22 21:03 ` Michal Pecio
@ 2026-08-23  8:16   ` David Laight
  2026-08-24 16:00   ` Sergey Shtylyov
  1 sibling, 0 replies; 12+ messages in thread
From: David Laight @ 2026-08-23  8:16 UTC (permalink / raw)
  To: Michal Pecio
  Cc: Sergey Shtylyov, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, linux-usb, netdev

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
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-22 20:58   ` Jakub Kicinski
@ 2026-08-23  8:37     ` Sergey Shtylyov
  2026-08-24 17:59       ` Jakub Kicinski
  0 siblings, 1 reply; 12+ messages in thread
From: Sergey Shtylyov @ 2026-08-23  8:37 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
	linux-usb, netdev

On 8/22/26 11:58 PM, Jakub Kicinski wrote:
[...]

>>    And of course I forgot [net-next] in the subject... :-)
> 
> And to check if it's open ;)
> https://netdev.bots.linux.dev/net-next.html

   I don't do much netdev these days, so I just forgot the usual
routine (and I couldn't wait so much with posting the patch anyway).
Is it still possible to send net-next stuff with [RFC] added to
the subject while net-next is closed?
   As I'll have to do v2 anyway, I'll try to wait for net-next to
open before posting it...

[...]

MBR, Sergey


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
       [not found]     ` <4a7604ab-c6ef-4552-b63b-3e7146ff42d0@auroraos.dev>
@ 2026-08-23  9:51       ` David Laight
  2026-08-23 18:00         ` Sergey Shtylyov
  0 siblings, 1 reply; 12+ messages in thread
From: David Laight @ 2026-08-23  9:51 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev

On Sun, 23 Aug 2026 11:20:30 +0300
Sergey Shtylyov <s.shtylyov@auroraos.dev> 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 <s.shtylyov@auroraos.dev>  
> >>
> >>    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.

David

> 
> > David  
> [...]
> 
> MBR, Sergey
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-23  9:51       ` David Laight
@ 2026-08-23 18:00         ` Sergey Shtylyov
  0 siblings, 0 replies; 12+ messages in thread
From: Sergey Shtylyov @ 2026-08-23 18:00 UTC (permalink / raw)
  To: David Laight
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev

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 <s.shtylyov@auroraos.dev>
>>>>
>>>>    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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-22 21:03 ` Michal Pecio
  2026-08-23  8:16   ` David Laight
@ 2026-08-24 16:00   ` Sergey Shtylyov
  1 sibling, 0 replies; 12+ messages in thread
From: Sergey Shtylyov @ 2026-08-24 16:00 UTC (permalink / raw)
  To: Michal Pecio
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, linux-usb, netdev

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] r8152: simplify loops in generic_ocp_{read,write}()
  2026-08-23  8:37     ` Sergey Shtylyov
@ 2026-08-24 17:59       ` Jakub Kicinski
  0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2026-08-24 17:59 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
	linux-usb, netdev

On Sun, 23 Aug 2026 11:37:11 +0300 Sergey Shtylyov wrote:
> > And to check if it's open ;)
> > https://netdev.bots.linux.dev/net-next.html  
> 
>    I don't do much netdev these days, so I just forgot the usual
> routine (and I couldn't wait so much with posting the patch anyway).
> Is it still possible to send net-next stuff with [RFC] added to
> the subject while net-next is closed?

Yup, RFC is fine (tho FWIW CI coverage of RFCs is limited for cost
reasons)

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-08-24 17:59 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-22 20:21 [PATCH] r8152: simplify loops in generic_ocp_{read,write}() Sergey Shtylyov
2026-08-22 20:26 ` Sergey Shtylyov
2026-08-22 20:58   ` Jakub Kicinski
2026-08-23  8:37     ` Sergey Shtylyov
2026-08-24 17:59       ` Jakub Kicinski
2026-08-22 21:03 ` Michal Pecio
2026-08-23  8:16   ` David Laight
2026-08-24 16:00   ` Sergey Shtylyov
2026-08-23  7:52 ` Sergey Shtylyov
2026-08-23  8:11   ` David Laight
     [not found]     ` <4a7604ab-c6ef-4552-b63b-3e7146ff42d0@auroraos.dev>
2026-08-23  9:51       ` David Laight
2026-08-23 18:00         ` Sergey Shtylyov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox