[PATCH 1/1 net-next] net: usb: cdc-ether: unify error handling in probe

[PATCH 1/1 net-next] net: usb: cdc-ether: unify error handling in probe

[Oliver Neukum]

usbnet_generic_cdc_bind() is duplicating the error handling
multiple times. That is bad. Unify it with jumps.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/cdc_ether.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index a032c1ded406..327cfdfe36e0 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -115,7 +115,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	int				len = intf->cur_altsetting->extralen;
 	struct usb_interface_descriptor	*d;
 	struct cdc_state		*info = (void *) &dev->data;
-	int				status;
+	int				status = -ENODEV;
 	int				rndis;
 	bool				android_rndis_quirk = false;
 	struct usb_driver		*driver = driver_of(intf);
@@ -288,16 +288,11 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	if (info->data != info->control) {
 		status = usb_driver_claim_interface(driver, info->data, dev);
 		if (status < 0)
-			return status;
+			goto bad_desc;
 	}
 	status = usbnet_get_endpoints(dev, info->data);
-	if (status < 0) {
-		/* ensure immediate exit from usbnet_disconnect */
-		usb_set_intfdata(info->data, NULL);
-		if (info->data != info->control)
-			usb_driver_release_interface(driver, info->data);
-		return status;
-	}
+	if (status < 0)
+		goto bail_out_and_release;
 
 	/* status endpoint: optional for CDC Ethernet, not RNDIS (or ACM) */
 	if (info->data != info->control)
@@ -317,9 +312,8 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	}
 	if (rndis && !dev->status) {
 		dev_dbg(&intf->dev, "missing RNDIS status endpoint\n");
-		usb_set_intfdata(info->data, NULL);
-		usb_driver_release_interface(driver, info->data);
-		return -ENODEV;
+		status = -ENODEV;
+		goto bail_out_and_release;
 	}
 
 	/* override ethtool_ops */
@@ -327,9 +321,13 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	return 0;
 
+bail_out_and_release:
+	usb_set_intfdata(info->data, NULL);
+	if (info->data != info->control)
+		usb_driver_release_interface(driver, info->data);
 bad_desc:
 	dev_info(&dev->udev->dev, "bad CDC descriptors\n");
-	return -ENODEV;
+	return status;
 }
 EXPORT_SYMBOL_GPL(usbnet_generic_cdc_bind);
 
-- 
2.53.0

Re: [PATCH 1/1 net-next] net: usb: cdc-ether: unify error handling in probe

[Andrew Lunn]

> @@ -288,16 +288,11 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
>  	if (info->data != info->control) {
>  		status = usb_driver_claim_interface(driver, info->data, dev);
>  		if (status < 0)
> -			return status;
> +			goto bad_desc;

>  bad_desc:
>  	dev_info(&dev->udev->dev, "bad CDC descriptors\n");
> -	return -ENODEV;
> +	return status;

Always reporting "bad CDC descriptors" does not seem correct. Not
being able to claim the interface is probably some other sort of
problem.

It might be better to change the dev_dbg()s to dev_err()s, and drop
the dev_info() here. You can then add additional dev_err() before the
goto's you are adding.

    Andrew

---
pw-bot: cr

Re: [PATCH 1/1 net-next] net: usb: cdc-ether: unify error handling in probe

[Oliver Neukum]

On 11.03.26 20:15, Andrew Lunn wrote:

> It might be better to change the dev_dbg()s to dev_err()s, and drop
> the dev_info() here. You can then add additional dev_err() before the
> goto's you are adding.

Excellent suggestion.

	Regards
		Oliver