Skip to contentSkip to conversation

Cleanup usbnet_probe() return value handling

PATCH Applied Peter Korsgaard 3 participants 5 messages Jul 2, 2007 – Jul 10, 2007

Subsystems (4)
Networking Network drivers USB networking drivers USB "usbnet" driver framework
lore
Share & tools
lore

Peter Korsgaard

author
#0 lore raw

PATCH 1/1 Cleanup usbnet_probe() return value handling

usbnet_probe() handles a positive return value from the driver bind() function as success, but will later only setup the status handler if the return value was zero, leading to confusion. Patch adjusts this to only accept 0 as success in both checks.

Signed-off-by Peter Korsgaard <[email protected]>
---
 drivers/net/usb/usbnet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/usb/usbnet.c +1 -1
Index: linux-2.6.22-rc7/drivers/net/usb/usbnet.c
===================================================================
@@ -1182,7 +1182,7 @@
 	// NOTE net->name still not usable ...
 	if (info->bind) {
 		status = info->bind (dev, udev);
-		if (status < 0)
+		if (status != 0)
 			goto out1;
 
 		// heuristic:  "usb%d" for links we know are two-host,
Signature · 1 line
Bye, Peter Korsgaard
#1 lore raw
Quoted 4 lines · Peter Korsgaard

usbnet_probe() handles a positive return value from the driver bind() function as success, but will later only setup the status handler if the return value was zero, leading to confusion. Patch adjusts this to only accept 0 as success in both checks.

I'd rather see the later test updated to match this one. (Good catch!)

The return convention is "negative means error". There's code in USB which has multiple nonnegative success codes.

In particular usb_control_msg(), which would very naturally used as the body of a bind() method, returns negative or the number of bytes transferred.

- Dave

Quoted 22 lines
Signed-off-by Peter Korsgaard <[email protected]>
---
 drivers/net/usb/usbnet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.22-rc7/drivers/net/usb/usbnet.c
===================================================================

drivers/net/usb/usbnet.c +1 -1
@@ -1182,7 +1182,7 @@
 	// NOTE net->name still not usable ...
 	if (info->bind) {
 		status = info->bind (dev, udev);
-		if (status < 0)
+		if (status != 0)
 			goto out1;
 
 		// heuristic:  "usb%d" for links we know are two-host,
Signature · 2 lines
-- 
Bye, Peter Korsgaard
author
#2 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"David" == David Brownell <[email protected]> writes:

Hi,

David> I'd rather see the later test updated to match this one. David> (Good catch!)

David> The return convention is "negative means error". There's David> code in USB which has multiple nonnegative success codes.

Ok, updated patch does that instead.

David> In particular usb_control_msg(), which would very naturally David> used as the body of a bind() method, returns negative or David> the number of bytes transferred.

Yeah, that was the original problem in my dm9601 driver.

usbnet_probe() handles a positive return value from the driver bind() function as success, but will later only setup the status handler if the return value was zero, leading to confusion. Patch adjusts this to accept positive values as success in both checks.

Signed-off-by Peter Korsgaard <[email protected]>
---
 drivers/net/usb/usbnet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/usb/usbnet.c +1 -1
Index: linux-2.6.22-rc7/drivers/net/usb/usbnet.c
===================================================================
@@ -1208,7 +1208,7 @@
 			status = 0;
 
 	}
-	if (status == 0 && dev->status)
+	if (status >= 0 && dev->status)
 		status = init_status (dev, udev);
 	if (status < 0)
 		goto out3;
Signature · 1 line
Bye, Peter Korsgaard
#3 lore raw
Quoted 6 lines · Peter Korsgaard

usbnet_probe() handles a positive return value from the driver bind() function as success, but will later only setup the status handler if the return value was zero, leading to confusion. Patch adjusts this to accept positive values as success in both checks.

Signed-off-by Peter Korsgaard <[email protected]>
Signed-off-by David Brownell <[email protected]>

... though I'd adjust comments to say "non-negative" rather than "positive". Most folks won't say that zero is positive.

Quoted 21 lines
---
 drivers/net/usb/usbnet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.22-rc7/drivers/net/usb/usbnet.c
===================================================================

drivers/net/usb/usbnet.c +1 -1
@@ -1208,7 +1208,7 @@
 			status = 0;
 
 	}
-	if (status == 0 && dev->status)
+	if (status >= 0 && dev->status)
 		status = init_status (dev, udev);
 	if (status < 0)
 		goto out3;
Signature · 2 lines
-- 
Bye, Peter Korsgaard
Applied
#4 lore raw
In reply to Peter Korsgaard
Quoted 23 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"David" == David Brownell <[email protected]> writes:

Hi,

David> I'd rather see the later test updated to match this one. David> (Good catch!)

David> The return convention is "negative means error". There's David> code in USB which has multiple nonnegative success codes.

Ok, updated patch does that instead.

David> In particular usb_control_msg(), which would very naturally David> used as the body of a bind() method, returns negative or David> the number of bytes transferred.

Yeah, that was the original problem in my dm9601 driver.

usbnet_probe() handles a positive return value from the driver bind() function as success, but will later only setup the status handler if the return value was zero, leading to confusion. Patch adjusts this to accept positive values as success in both checks.

Quoted 4 lines
Signed-off-by Peter Korsgaard <[email protected]>
---
 drivers/net/usb/usbnet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

applied