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

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

V2: Update error logging with every cause a unique message

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

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index a032c1ded406..a0a5740590b9 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);
@@ -169,10 +169,13 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	info->header = header.usb_cdc_header_desc;
 	info->ether = header.usb_cdc_ether_desc;
 	if (!info->u) {
-		if (rndis)
+		if (rndis) {
 			goto skip;
-		else /* in that case a quirk is mandatory */
+		} else {
+			/* in that case a quirk is mandatory */
+			dev_err(&dev->udev->dev, "No union descriptors\n");
 			goto bad_desc;
+		}
 	}
 	/* we need a master/control interface (what we're
 	 * probed with) and a slave/data interface; union
@@ -192,18 +195,20 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 			android_rndis_quirk = true;
 			goto skip;
 		}
+		dev_err(&intf->dev, "bad CDC descriptors\n");
 		goto bad_desc;
 	}
 	if (info->control != intf) {
-		dev_dbg(&intf->dev, "bogus CDC Union\n");
 		/* Ambit USB Cable Modem (and maybe others)
 		 * interchanges master and slave interface.
 		 */
 		if (info->data == intf) {
 			info->data = info->control;
 			info->control = intf;
-		} else
+		} else {
+			dev_err(&intf->dev, "bogus CDC Union\n");
 			goto bad_desc;
+		}
 	}
 
 	/* some devices merge these - skip class check */
@@ -213,7 +218,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	/* a data interface altsetting does the real i/o */
 	d = &info->data->cur_altsetting->desc;
 	if (d->bInterfaceClass != USB_CLASS_CDC_DATA) {
-		dev_dbg(&intf->dev, "slave class %u\n", d->bInterfaceClass);
+		dev_err(&intf->dev, "slave class %u\n", d->bInterfaceClass);
 		goto bad_desc;
 	}
 skip:
@@ -227,7 +232,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 	if (rndis && is_rndis(&intf->cur_altsetting->desc) &&
 	    header.usb_cdc_acm_descriptor &&
 	    header.usb_cdc_acm_descriptor->bmCapabilities) {
-		dev_dbg(&intf->dev,
+		dev_err(&intf->dev,
 			"ACM capabilities %02x, not really RNDIS?\n",
 			header.usb_cdc_acm_descriptor->bmCapabilities);
 		goto bad_desc;
@@ -242,14 +247,14 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	if (header.usb_cdc_mdlm_desc &&
 	    memcmp(header.usb_cdc_mdlm_desc->bGUID, mbm_guid, 16)) {
-		dev_dbg(&intf->dev, "GUID doesn't match\n");
+		dev_err(&intf->dev, "GUID doesn't match\n");
 		goto bad_desc;
 	}
 
 	if (header.usb_cdc_mdlm_detail_desc &&
 		header.usb_cdc_mdlm_detail_desc->bLength <
 			(sizeof(struct usb_cdc_mdlm_detail_desc) + 1)) {
-		dev_dbg(&intf->dev, "Descriptor too short\n");
+		dev_err(&intf->dev, "Descriptor too short\n");
 		goto bad_desc;
 	}
 
@@ -267,7 +272,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 		info->control = usb_ifnum_to_if(dev->udev, 0);
 		info->data = usb_ifnum_to_if(dev->udev, 1);
 		if (!info->control || !info->data || info->control != intf) {
-			dev_dbg(&intf->dev,
+			dev_err(&intf->dev,
 				"rndis: master #0/%p slave #1/%p\n",
 				info->control,
 				info->data);
@@ -275,7 +280,7 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 		}
 
 	} else if (!info->header || (!rndis && !info->ether)) {
-		dev_dbg(&intf->dev, "missing cdc %s%s%sdescriptor\n",
+		dev_err(&intf->dev, "missing cdc %s%s%sdescriptor\n",
 			info->header ? "" : "header ",
 			info->u ? "" : "union ",
 			info->ether ? "" : "ether ");
@@ -287,16 +292,15 @@ 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;
+		if (status < 0) {
+			dev_err(&intf->dev, "Second interface unclaimable\n");
+			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;
+		dev_dbg(&intf->dev, "Mandatory endpoints missing\n");
+		goto bail_out_and_release;
 	}
 
 	/* status endpoint: optional for CDC Ethernet, not RNDIS (or ACM) */
@@ -316,10 +320,9 @@ 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;
+		dev_err(&intf->dev, "missing RNDIS status endpoint\n");
+		status = -ENODEV;
+		goto bail_out_and_release;
 	}
 
 	/* override ethtool_ops */
@@ -327,9 +330,12 @@ 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 v2 net-next] net: usb: cdc-ether: unify error handling in probe

[Markus Elfring]

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

I find such a change description improvable another bit.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.0-rc3#n659


> V2: Update error logging with every cause a unique message

Please move patch version descriptions behind the marker line.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.0-rc3#n785

Regards,
Markus

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 12 Mar 2026 09:45:32 +0100 you wrote:
> usbnet_generic_cdc_bind() is duplicating the error handling
> multiple times. That is bad. Unify it with jumps.
> 
> V2: Update error logging with every cause a unique message
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> 
> [...]

Here is the summary with links:
  - [PATCHv2,1/1,net-next] net: usb: cdc-ether: unify error handling in probe
    https://git.kernel.org/netdev/net-next/c/a99f06e579a3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html