Skip to contentSkip to conversation

Signedness issue in drivers/net/3c515.c

PATCH In review Eric Sesterhenn 3 participants 4 messages Aug 19, 2006 – Aug 21, 2006

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric Sesterhenn

author
#0 lore raw

PATCH 1/1 Signedness issue in drivers/net/3c515.c

hi,

while playing with gcc 4.1 -Wextra warnings, I came across this one:

drivers/net/3c515.c:1027: warning: comparison of unsigned expression >= 0 is always true

Since i is unsigned the >= 0 check in the for loop is always true, so we might spin there forever unless the if condition triggers. Since i is only used in this loop, this patch changes it to an integer.

Signed-off-by Eric Sesterhenn <[email protected]>
drivers/net/3c515.c renamed from drivers/net/3c515.c.orig +2 -1
@@ -1003,7 +1003,8 @@ static int corkscrew_start_xmit(struct s
 		/* Calculate the next Tx descriptor entry. */
 		int entry = vp->cur_tx % TX_RING_SIZE;
 		struct boom_tx_desc *prev_entry;
-		unsigned long flags, i;
+		unsigned long flags;
+		int i;
 
 		if (vp->tx_full)	/* No room to transmit with */
 			return 1;
Acked-by on v1 1/1
#1 lore raw

Ar Sad, 2006-08-19 am 19:37 +0200, ysgrifennodd Eric Sesterhenn:

Quoted 4 lines

hi,

while playing with gcc 4.1 -Wextra warnings, I came across this one:

Quoted 9 lines

drivers/net/3c515.c:1027: warning: comparison of unsigned expression >= 0 is always true

Since i is unsigned the >= 0 check in the for loop is always true, so we might spin there forever unless the if condition triggers. Since i is only used in this loop, this patch changes it to an integer.

Signed-off-by Eric Sesterhenn <[email protected]>
Acked-by Alan Cox <[email protected]>

[And put JG on the Cc:]

#2 lore raw
In reply to Eric Sesterhenn
Quoted 14 lines · On Sat, 19 Aug 2006 19:37:57 +0200 Eric Sesterhenn <[email protected]> wrote:

while playing with gcc 4.1 -Wextra warnings, I came across this one:

drivers/net/3c515.c:1027: warning: comparison of unsigned expression >= 0 is always true

Since i is unsigned the >= 0 check in the for loop is always true, so we might spin there forever unless the if condition triggers. Since i is only used in this loop, this patch changes it to an integer.

Signed-off-by Eric Sesterhenn <[email protected]>

drivers/net/3c515.c static int corkscrew_start_xmit(struct s · line 1010

Quoted 9 lines
drivers/net/3c515.c renamed from drivers/net/3c515.c.orig +2 -1
@@ -1003,7 +1003,8 @@ static int corkscrew_start_xmit(struct s
 		/* Calculate the next Tx descriptor entry. */
 		int entry = vp->cur_tx % TX_RING_SIZE;
 		struct boom_tx_desc *prev_entry;
-		unsigned long flags, i;
+		unsigned long flags;
+		int i;
 
 		if (vp->tx_full)	/* No room to transmit with */
 			return 1;

Which affects this loop:

	/* Wait for the stall to complete. */
	for (i = 20; i >= 0; i--)
		if ((inw(ioaddr + EL3_STATUS) & CmdInProgress) == 0) 
			break;

Your fix will convert this indefinit wait into a bounded one. It might cause the driver to malfunction.

Given that our pool of 3c515 testers is less than enormous, a more prudent change might be to remove `i' and simply formalise the existing behaviour into a while(1) loop.

#3 lore raw

Ar Llu, 2006-08-21 am 14:05 -0700, ysgrifennodd Andrew Morton:

Quoted 7 lines
	/* Wait for the stall to complete. */
	for (i = 20; i >= 0; i--)
		if ((inw(ioaddr + EL3_STATUS) & CmdInProgress) == 0) 
			break;

Your fix will convert this indefinit wait into a bounded one. It might cause the driver to malfunction.

The change is correct. The docs guarantee it can't take that long.