Skip to contentSkip to conversation

introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

RFC PATCH Superseded Ondrej Zary 4 participants 22 messages Sep 3, 2010 – Sep 9, 2010

Subsystems (6)
Networking Network drivers USB USB networking drivers +2 more subsystems
lore
Share & tools
lore

Ondrej Zary

author
#0 lore raw

RFC PATCH 1/1 introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

Hello,
this patch introduces cx82310_eth driver - driver for USB ethernet port of ADSL routers based on Conexant CX82310 chips. Such routers usually have ethernet port(s) too which are bridged together with the USB ethernet port, allowing the USB-connected machine to communicate to the network (and also internet through the ADSL, of course).

This is my first driver, so please check thoroughly. As there's no protocol documentation, it was done with usbsnoop dumps from Windows driver, some parts (the commands) inspired by cxacru driver and also other usbnet drivers. The driver passed my testing - some real work and also pings sized from 0 to 65507 B.

The only problem I found is the ifconfig error counter. When I return 0 (or 1 but empty skb) from rx_fixup(), usbnet increases the error counter although it's not an error condition (because packets can cross URB boundaries). Maybe the usbnet should be fixed to allow rx_fixup() to return empty skbs (or some other value, e.g. 2)?

The USB ID of my device is 0x0572:0xcb01 which conflicts with some ADSL modems using cxacru driver (they probably use the same chipset but simpler firmware). The modems seem to use bDeviceClass 0 and iProduct "ADSL USB MODEM", my router uses bDeviceClass 255 and iProduct "USB NET CARD". The driver matches only devices with class 255 and checks for the iProduct string during init. The cxacru driver should be modified to ignore these devices.

Signed-off-by Ondrej Zary <[email protected]>
drivers/net/usb/cx82310_eth.c new file +350 -0
@@ -0,0 +1,350 @@
+/*
+ * Driver for USB ethernet port of Conexant CX82310-based ADSL routers
+ * Copyright (C) 2010 by Ondrej Zary
+ * some parts inspired by the cxacru driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/workqueue.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/usbnet.h>
+
+enum cx82310_cmd {
+	CMD_START		= 0x84,	/* no effect? */
+	CMD_STOP		= 0x85,	/* no effect? */
+	CMD_GET_STATUS		= 0x90,	/* returns nothing? */
+	CMD_GET_MAC_ADDR	= 0x91,	/* read MAC address */
+	CMD_GET_LINK_STATUS	= 0x92,	/* not useful, link is always up */
+	CMD_ETHERNET_MODE	= 0x99,	/* unknown, needed during init */
+};
+
+enum cx82310_status {
+	STATUS_UNDEFINED,
+	STATUS_SUCCESS,
+	STATUS_ERROR,
+	STATUS_UNSUPPORTED,
+	STATUS_UNIMPLEMENTED,
+	STATUS_PARAMETER_ERROR,
+	STATUS_DBG_LOOPBACK,
+};
+
+#define CMD_PACKET_SIZE	64
+/* first command after power on can take around 8 seconds */
+#define CMD_TIMEOUT	15000
+#define CMD_REPLY_RETRY 5
+
+#define CX82310_MTU	1514
+#define CMD_EP		0x01
+
+/*
+ * execute control command
+ *  - optionally send some data (command parameters)
+ *  - optionally wait for the reply
+ *  - optionally read some data from the reply
+ */
+static int cx82310_cmd(struct usbnet *dev, enum cx82310_cmd cmd, bool reply,
+		       u8 *wdata, int wlen, u8 *rdata, int rlen)
+{
+	int actual_len, retries, ret;
+	struct usb_device *udev = dev->udev;
+	u8 *buf = kzalloc(CMD_PACKET_SIZE, GFP_KERNEL);
+
+	if (!buf)
+		return -ENOMEM;
+
+	/* create command packet */
+	buf[0] = cmd;
+	if (wdata)
+		memcpy(buf + 4, wdata, min_t(int, wlen, CMD_PACKET_SIZE - 4));
+
+	/* send command packet */
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, CMD_EP), buf,
+			   CMD_PACKET_SIZE, &actual_len, CMD_TIMEOUT);
+	if (ret < 0) {
+		dev_err(&dev->udev->dev, "send command %#x: error %d\n",
+			cmd, ret);
+		goto end;
+	}
+
+	if (reply) {
+		/* wait for reply, retry if it's empty */
+		for (retries = 0; retries < CMD_REPLY_RETRY; retries++) {
+			ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, CMD_EP),
+					   buf, CMD_PACKET_SIZE, &actual_len,
+					   CMD_TIMEOUT);
+			if (ret < 0) {
+				dev_err(&dev->udev->dev,
+					"reply receive error %d\n", ret);
+				goto end;
+			}
+			if (actual_len > 0)
+				break;
+		}
+		if (actual_len == 0) {
+			dev_err(&dev->udev->dev, "no reply to command %#x\n",
+				cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[0] != cmd) {
+			dev_err(&dev->udev->dev,
+				"got reply to command %#x, expected: %#x\n",
+				buf[0], cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[1] != STATUS_SUCCESS) {
+			dev_err(&dev->udev->dev, "command %#x failed: %#x\n",
+				cmd, buf[1]);
+			ret = -EIO;
+			goto end;
+		}
+		if (rdata)
+			memcpy(rdata, buf + 4,
+			       min_t(int, rlen, CMD_PACKET_SIZE - 4));
+	}
+end:
+	kfree(buf);
+	return ret;
+}
+
+#define rx_incomplete	(dev->data[0])
+#define rx_remainder	(dev->data[1])
+#define incomplete_data	(dev->data[2])
+
+static int cx82310_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int ret;
+	char buf[15];
+	struct usb_device *udev = dev->udev;
+
+	/* avoid ADSL modems - continue only if iProduct is "USB NET CARD" */
+	if (udev->descriptor.iProduct &&
+	    usb_string(udev, udev->descriptor.iProduct, buf, sizeof(buf)) &&
+	    strcmp(buf, "USB NET CARD")) {
+		dev_err(&udev->dev,
+			"probably an ADSL modem, use cxacru driver instead\n");
+		return -ENODEV;
+	}
+
+	ret = usbnet_get_endpoints(dev, intf);
+	if (ret)
+		return ret;
+
+	/*
+	 * this must not include ethernet header as the device can send partial
+	 * packets with no header (URB is at least 2 bytes long, so 2 is OK)
+	 */
+	dev->net->hard_header_len = 2;
+	/* we can send at most 1514 bytes of data (+ 2-byte header) per URB */
+	dev->hard_mtu = CX82310_MTU + dev->net->hard_header_len;
+	/* we can receive URBs up to 4KB from the device */
+	dev->rx_urb_size = 4096;
+
+	incomplete_data = (unsigned long) kmalloc(dev->hard_mtu, GFP_KERNEL);
+	if (!incomplete_data)
+		return -ENOMEM;
+
+	/* enable ethernet mode (?) */
+	ret = cx82310_cmd(dev, CMD_ETHERNET_MODE, true, "\x01", 1, NULL, 0);
+	if (ret) {
+		dev_err(&udev->dev, "unable to enable ethernet mode\n");
+		goto err;
+	}
+
+	/* get the MAC address */
+	ret = cx82310_cmd(dev, CMD_GET_MAC_ADDR, true, NULL, 0,
+			  dev->net->dev_addr, ETH_ALEN);
+	if (ret) {
+		dev_err(&udev->dev, "unable to read MAC address\n");
+		goto err;
+	}
+
+	/* start (does not seem to have any effect?) */
+	ret = cx82310_cmd(dev, CMD_START, false, NULL, 0, NULL, 0);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	kfree((void *)incomplete_data);
+	return ret;
+}
+
+static void cx82310_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+	kfree((void *)incomplete_data);
+}
+
+/*
+ * RX is NOT easy - we can receive multiple packets per skb, each having 2-byte
+ * packet length at the beginning.
+ * The last packet might be incomplete (when it crosses the 4KB URB size),
+ * continuing in the next skb (without any headers).
+ * If a packet has odd length, there is one extra byte at the end (before next
+ * packet or at the end of the URB).
+ */
+static int cx82310_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+	int len;
+	struct sk_buff *skb2;
+
+	/*
+	 * If the last skb ended with an incomplete packet, this skb contains
+	 * end of that packet at the beginning.
+	 */
+	if (rx_remainder) {
+		skb2 = alloc_skb(rx_incomplete + rx_remainder, GFP_ATOMIC);
+		if (!skb2)
+			return 0;
+		skb_put(skb2, rx_incomplete + rx_remainder);
+		memcpy(skb2->data, (void *)incomplete_data, rx_incomplete);
+		memcpy(skb2->data + rx_incomplete, skb->data, rx_remainder);
+		usbnet_skb_return(dev, skb2);
+		skb_pull(skb, (rx_remainder + 1) & ~1);
+		rx_remainder = 0;
+		if (skb->len < 2)
+			return 1;
+	}
+
+	if (skb->len < 2) {
+		dev_err(&dev->udev->dev, "RX frame too short: %d B\n",
+			skb->len);
+		return 0;
+	}
+
+	/* a skb can contain multiple packets */
+	while (skb->len > 1) {
+		/* first two bytes are packet length */
+		len = skb->data[0] | (skb->data[1] << 8);
+		skb_pull(skb, 2);
+
+		/* if last packet in the skb, let usbnet to process it */
+		if (len == skb->len || len + 1 == skb->len) {
+			skb_trim(skb, len);
+			break;
+		}
+
+		if (len > CX82310_MTU) {
+			dev_err(&dev->udev->dev, "RX packet too long: %d B\n",
+				len);
+			return 0;
+		}
+
+		/* incomplete packet, save it for the next skb */
+		if (len > skb->len) {
+			rx_incomplete = skb->len;
+			rx_remainder = len - skb->len;
+			memcpy((void *)incomplete_data, skb->data,
+			       rx_incomplete);
+			skb_pull(skb, skb->len);
+			break;
+		}
+
+		skb2 = alloc_skb(len, GFP_ATOMIC);
+		if (!skb2)
+			return 0;
+		skb_put(skb2, len);
+		memcpy(skb2->data, skb->data, len);
+		/* process the packet */
+		usbnet_skb_return(dev, skb2);
+
+		skb_pull(skb, (len + 1) & ~1);
+	}
+
+	/* let usbnet process the last packet */
+	return 1;
+}
+
+/* TX is easy, just add 2 bytes of length at the beginning */
+static struct sk_buff *cx82310_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
+				       gfp_t flags)
+{
+	int len = skb->len;
+
+	if (skb_headroom(skb) < 2) {
+		struct sk_buff *skb2 = skb_copy_expand(skb, 2, 0, flags);
+		dev_kfree_skb_any(skb);
+		skb = skb2;
+		if (!skb)
+			return NULL;
+	}
+	skb_push(skb, 2);
+
+	skb->data[0] = len;
+	skb->data[1] = len >> 8;
+
+	return skb;
+}
+
+
+static const struct driver_info	cx82310_info = {
+	.description	= "Conexant CX82310 USB ethernet",
+	.flags		= FLAG_ETHER,
+	.bind		= cx82310_bind,
+	.unbind		= cx82310_unbind,
+	.rx_fixup	= cx82310_rx_fixup,
+	.tx_fixup	= cx82310_tx_fixup,
+};
+
+#define USB_DEVICE_CLASS(vend, prod, cl, sc, pr) \
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
+		       USB_DEVICE_ID_MATCH_DEV_INFO, \
+	.idVendor = (vend), \
+	.idProduct = (prod), \
+	.bDeviceClass = (cl), \
+	.bDeviceSubClass = (sc), \
+	.bDeviceProtocol = (pr)
+
+static const struct usb_device_id products[] = {
+	{
+		USB_DEVICE_CLASS(0x0572, 0xcb01, 0xff, 0, 0),
+		.driver_info = (unsigned long) &cx82310_info
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(usb, products);
+
+static struct usb_driver cx82310_driver = {
+	.name		= "cx82310_eth",
+	.id_table	= products,
+	.probe		= usbnet_probe,
+	.disconnect	= usbnet_disconnect,
+	.suspend	= usbnet_suspend,
+	.resume		= usbnet_resume,
+};
+
+static int __init cx82310_init(void)
+{
+	return usb_register(&cx82310_driver);
+}
+module_init(cx82310_init);
+
+static void __exit cx82310_exit(void)
+{
+	usb_deregister(&cx82310_driver);
+}
+module_exit(cx82310_exit);
+
+MODULE_AUTHOR("Ondrej Zary");
+MODULE_DESCRIPTION("Conexant CX82310-based ADSL router USB ethernet driver");
+MODULE_LICENSE("GPL");
drivers/net/usb/Kconfig +8 -0
@@ -358,6 +358,14 @@ config USB_NET_ZAURUS
 	  really need this non-conformant variant of CDC Ethernet (or in
 	  some cases CDC MDLM) protocol, not "g_ether".
 
+config USB_NET_CX82310_ETH
+	tristate "Conexant CX82310 USB ethernet port"
+	depends on USB_USBNET
+	help
+	  Choose this option if you're using a Conexant CX82310-based ADSL
+	  router with USB ethernet port. This driver is for routers only,
+	  it will not work with ADSL modems (use cxacru driver instead).
+
 config USB_HSO
 	tristate "Option USB High Speed Mobile Devices"
 	depends on USB && RFKILL
drivers/net/usb/Makefile +1 -0
@@ -25,4 +25,5 @@ obj-$(CONFIG_USB_NET_INT51X1)	+= int51x1
 obj-$(CONFIG_USB_CDC_PHONET)	+= cdc-phonet.o
 obj-$(CONFIG_USB_IPHETH)	+= ipheth.o
 obj-$(CONFIG_USB_SIERRA_NET)	+= sierra_net.o
+obj-$(CONFIG_USB_NET_CX82310_ETH)	+= cx82310_eth.o
Signature · 1 line
Ondrej Zary
#1 lore raw
Quoted 7 lines · Ondrej Zary

Hello,
this patch introduces cx82310_eth driver - driver for USB ethernet port of ADSL routers based on Conexant CX82310 chips. Such routers usually have ethernet port(s) too which are bridged together with the USB ethernet port, allowing the USB-connected machine to communicate to the network (and also internet through the ADSL, of course).

Quoted 5 lines

This is my first driver, so please check thoroughly. As there's no protocol documentation, it was done with usbsnoop dumps from Windows driver, some parts (the commands) inspired by cxacru driver and also other usbnet drivers. The driver passed my testing - some real work and also pings sized from 0 to 65507 B.

Try http://isic.sourceforge.net/, you can send a flood of junk traffic through the device and check what happens. Be careful what else you have on the network as they may not handle it well.

Quoted 6 lines

The only problem I found is the ifconfig error counter. When I return 0 (or 1 but empty skb) from rx_fixup(), usbnet increases the error counter although it's not an error condition (because packets can cross URB boundaries). Maybe the usbnet should be fixed to allow rx_fixup() to return empty skbs (or some other value, e.g. 2)?

Quoted 6 lines

The USB ID of my device is 0x0572:0xcb01 which conflicts with some ADSL modems using cxacru driver (they probably use the same chipset but simpler firmware). The modems seem to use bDeviceClass 0 and iProduct "ADSL USB MODEM", my router uses bDeviceClass 255 and iProduct "USB NET CARD". The driver matches only devices with class 255 and checks for the iProduct string during init. The cxacru driver should be modified to ignore these devices.

You can include the cxacru change as part of your patch; I'll test it.

Quoted 87 lines
Signed-off-by Ondrej Zary <[email protected]>
drivers/net/usb/cx82310_eth.c new file +81 -0
@@ -0,0 +1,350 @@
+/*
+ * Driver for USB ethernet port of Conexant CX82310-based ADSL routers
+ * Copyright (C) 2010 by Ondrej Zary
+ * some parts inspired by the cxacru driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/workqueue.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/usbnet.h>
+
+enum cx82310_cmd {
+	CMD_START		= 0x84,	/* no effect? */
+	CMD_STOP		= 0x85,	/* no effect? */
+	CMD_GET_STATUS		= 0x90,	/* returns nothing? */
+	CMD_GET_MAC_ADDR	= 0x91,	/* read MAC address */
+	CMD_GET_LINK_STATUS	= 0x92,	/* not useful, link is always up */
+	CMD_ETHERNET_MODE	= 0x99,	/* unknown, needed during init */
+};
+
+enum cx82310_status {
+	STATUS_UNDEFINED,
+	STATUS_SUCCESS,
+	STATUS_ERROR,
+	STATUS_UNSUPPORTED,
+	STATUS_UNIMPLEMENTED,
+	STATUS_PARAMETER_ERROR,
+	STATUS_DBG_LOOPBACK,
+};
+
+#define CMD_PACKET_SIZE	64
+/* first command after power on can take around 8 seconds */
+#define CMD_TIMEOUT	15000
+#define CMD_REPLY_RETRY 5
+
+#define CX82310_MTU	1514
+#define CMD_EP		0x01
+
+/*
+ * execute control command
+ *  - optionally send some data (command parameters)
+ *  - optionally wait for the reply
+ *  - optionally read some data from the reply
+ */
+static int cx82310_cmd(struct usbnet *dev, enum cx82310_cmd cmd, bool reply,
+		       u8 *wdata, int wlen, u8 *rdata, int rlen)
+{
+	int actual_len, retries, ret;
+	struct usb_device *udev = dev->udev;
+	u8 *buf = kzalloc(CMD_PACKET_SIZE, GFP_KERNEL);
+
+	if (!buf)
+		return -ENOMEM;
+
+	/* create command packet */
+	buf[0] = cmd;
+	if (wdata)
+		memcpy(buf + 4, wdata, min_t(int, wlen, CMD_PACKET_SIZE - 4));
+
+	/* send command packet */
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, CMD_EP), buf,
+			   CMD_PACKET_SIZE, &actual_len, CMD_TIMEOUT);
Quoted 1 line

From your previous lsusb output this is an interrupt endpoint, although

usb_bulk_msg will auto-detect the type.

Quoted 41 lines
drivers/net/usb/cx82310_eth.c new file +41 -0
+	if (ret < 0) {
+		dev_err(&dev->udev->dev, "send command %#x: error %d\n",
+			cmd, ret);
+		goto end;
+	}
+
+	if (reply) {
+		/* wait for reply, retry if it's empty */
+		for (retries = 0; retries < CMD_REPLY_RETRY; retries++) {
+			ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, CMD_EP),
+					   buf, CMD_PACKET_SIZE, &actual_len,
+					   CMD_TIMEOUT);
+			if (ret < 0) {
+				dev_err(&dev->udev->dev,
+					"reply receive error %d\n", ret);
+				goto end;
+			}
+			if (actual_len > 0)
+				break;
+		}
+		if (actual_len == 0) {
+			dev_err(&dev->udev->dev, "no reply to command %#x\n",
+				cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[0] != cmd) {
+			dev_err(&dev->udev->dev,
+				"got reply to command %#x, expected: %#x\n",
+				buf[0], cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[1] != STATUS_SUCCESS) {
+			dev_err(&dev->udev->dev, "command %#x failed: %#x\n",
+				cmd, buf[1]);
+			ret = -EIO;
+			goto end;
+		}
+		if (rdata)
+			memcpy(rdata, buf + 4,

drivers/net/usb/cx82310_eth.c line 132

Quoted 10 lines
drivers/net/usb/cx82310_eth.c new file +9 -0
+			       min_t(int, rlen, CMD_PACKET_SIZE - 4));
+	}
+end:
+	kfree(buf);
+	return ret;
+}
 
+#define rx_incomplete	(dev->data[0])
+#define rx_remainder	(dev->data[1])
+#define incomplete_data	(dev->data[2])

This doesn't make the rest of the code particularly readable.

drivers/net/usb/cx82310_eth.c line 142

Quoted 9 lines
drivers/net/usb/cx82310_eth.c new file +9 -0
+static int cx82310_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int ret;
+	char buf[15];
+	struct usb_device *udev = dev->udev;
+
+	/* avoid ADSL modems - continue only if iProduct is "USB NET CARD" */
+	if (udev->descriptor.iProduct &&
+	    usb_string(udev, udev->descriptor.iProduct, buf, sizeof(buf)) &&

I'm not sure I like the assignment within the if condition here, although other drivers do it; ret is available to check the result.

Quoted 7 lines
drivers/net/usb/cx82310_eth.c new file +7 -0
+	    strcmp(buf, "USB NET CARD")) {
+		dev_err(&udev->dev,
+			"probably an ADSL modem, use cxacru driver instead\n");
+		return -ENODEV;
+	}
+
+	ret = usbnet_get_endpoints(dev, intf);

drivers/net/usb/cx82310_eth.c line 159

Quoted 10 lines
drivers/net/usb/cx82310_eth.c new file +10 -0
+	if (ret)
+		return ret;
+
+	/*
+	 * this must not include ethernet header as the device can send partial
+	 * packets with no header (URB is at least 2 bytes long, so 2 is OK)
+	 */
+	dev->net->hard_header_len = 2;
+	/* we can send at most 1514 bytes of data (+ 2-byte header) per URB */
+	dev->hard_mtu = CX82310_MTU + dev->net->hard_header_len;

Have you tried sending larger packets?
With another 8 bytes it would support 802.1Q (VLAN).

drivers/net/usb/cx82310_eth.c line 170

Quoted 11 lines
drivers/net/usb/cx82310_eth.c new file +11 -0
+	/* we can receive URBs up to 4KB from the device */
+	dev->rx_urb_size = 4096;
+
+	incomplete_data = (unsigned long) kmalloc(dev->hard_mtu, GFP_KERNEL);
+	if (!incomplete_data)
+		return -ENOMEM;
+
+	/* enable ethernet mode (?) */
+	ret = cx82310_cmd(dev, CMD_ETHERNET_MODE, true, "\x01", 1, NULL, 0);
+	if (ret) {
+		dev_err(&udev->dev, "unable to enable ethernet mode\n");

I'd include the return value in the message, it can be useful to know if it was a timeout response or not.

Quoted 27 lines
drivers/net/usb/cx82310_eth.c new file +27 -0
+		goto err;
+	}
+
+	/* get the MAC address */
+	ret = cx82310_cmd(dev, CMD_GET_MAC_ADDR, true, NULL, 0,
+			  dev->net->dev_addr, ETH_ALEN);
+	if (ret) {
+		dev_err(&udev->dev, "unable to read MAC address\n");
+		goto err;
+	}
+
+	/* start (does not seem to have any effect?) */
+	ret = cx82310_cmd(dev, CMD_START, false, NULL, 0, NULL, 0);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	kfree((void *)incomplete_data);
+	return ret;
+}
+
+static void cx82310_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+	kfree((void *)incomplete_data);
+}
+
Signature · 1 line
Simon Arlott
author
#2 lore raw
Quoted 14 lines · On Saturday 04 September 2010 00:14:54 Simon Arlott wrote:
Quoted 7 lines · Ondrej Zary

Hello,
this patch introduces cx82310_eth driver - driver for USB ethernet port of ADSL routers based on Conexant CX82310 chips. Such routers usually have ethernet port(s) too which are bridged together with the USB ethernet port, allowing the USB-connected machine to communicate to the network (and also internet through the ADSL, of course).

Quoted 5 lines

This is my first driver, so please check thoroughly. As there's no protocol documentation, it was done with usbsnoop dumps from Windows driver, some parts (the commands) inspired by cxacru driver and also other usbnet drivers. The driver passed my testing - some real work and also pings sized from 0 to 65507 B.

Quoted 3 lines

Try http://isic.sourceforge.net/, you can send a flood of junk traffic through the device and check what happens. Be careful what else you have on the network as they may not handle it well.

Thanks, looks like a good testing tool. Too bad it does not work when compiled by hand (maybe that's why it was removed from Debian).
The ping test revealed some RX bugs before so I hope that there are no more.

Quoted 14 lines
Quoted 6 lines

The only problem I found is the ifconfig error counter. When I return 0 (or 1 but empty skb) from rx_fixup(), usbnet increases the error counter although it's not an error condition (because packets can cross URB boundaries). Maybe the usbnet should be fixed to allow rx_fixup() to return empty skbs (or some other value, e.g. 2)?

Quoted 7 lines

The USB ID of my device is 0x0572:0xcb01 which conflicts with some ADSL modems using cxacru driver (they probably use the same chipset but simpler firmware). The modems seem to use bDeviceClass 0 and iProduct "ADSL USB MODEM", my router uses bDeviceClass 255 and iProduct "USB NET CARD". The driver matches only devices with class 255 and checks for the iProduct string during init. The cxacru driver should be modified to ignore these devices.

Quoted 1 line

You can include the cxacru change as part of your patch; I'll test it.

A patch will follow in next e-mail.

Quoted 81 lines
Quoted 81 lines
Signed-off-by Ondrej Zary <[email protected]>
drivers/net/usb/cx82310_eth.c new file +0 -0

23:09:16.000000000 +0200 @@ -0,0 +1,350 @@
+/*
+ * Driver for USB ethernet port of Conexant CX82310-based ADSL routers
+ * Copyright (C) 2010 by Ondrej Zary
+ * some parts inspired by the cxacru driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */

drivers/net/usb/cx82310_eth.c new file +45 -0
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/workqueue.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/usbnet.h>
+
+enum cx82310_cmd {
+	CMD_START		= 0x84,	/* no effect? */
+	CMD_STOP		= 0x85,	/* no effect? */
+	CMD_GET_STATUS		= 0x90,	/* returns nothing? */
+	CMD_GET_MAC_ADDR	= 0x91,	/* read MAC address */
+	CMD_GET_LINK_STATUS	= 0x92,	/* not useful, link is always up */
+	CMD_ETHERNET_MODE	= 0x99,	/* unknown, needed during init */
+};
+
+enum cx82310_status {
+	STATUS_UNDEFINED,
+	STATUS_SUCCESS,
+	STATUS_ERROR,
+	STATUS_UNSUPPORTED,
+	STATUS_UNIMPLEMENTED,
+	STATUS_PARAMETER_ERROR,
+	STATUS_DBG_LOOPBACK,
+};
+
+#define CMD_PACKET_SIZE	64
+/* first command after power on can take around 8 seconds */
+#define CMD_TIMEOUT	15000
+#define CMD_REPLY_RETRY 5
+
+#define CX82310_MTU	1514
+#define CMD_EP		0x01
+
+/*
+ * execute control command
+ *  - optionally send some data (command parameters)
+ *  - optionally wait for the reply
+ *  - optionally read some data from the reply
+ */
+static int cx82310_cmd(struct usbnet *dev, enum cx82310_cmd cmd, bool

reply, + u8 *wdata, int wlen, u8 *rdata, int rlen)

drivers/net/usb/cx82310_eth.c new file +11 -0
+{
+	int actual_len, retries, ret;
+	struct usb_device *udev = dev->udev;
+	u8 *buf = kzalloc(CMD_PACKET_SIZE, GFP_KERNEL);
+
+	if (!buf)
+		return -ENOMEM;
+
+	/* create command packet */
+	buf[0] = cmd;
+	if (wdata)
Quoted 8 lines

drivers/net/usb/cx82310_eth.c line 81

Quoted 5 lines
drivers/net/usb/cx82310_eth.c new file +5 -0
+		memcpy(buf + 4, wdata, min_t(int, wlen, CMD_PACKET_SIZE - 4));
+
+	/* send command packet */
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, CMD_EP), buf,
+			   CMD_PACKET_SIZE, &actual_len, CMD_TIMEOUT);

From your previous lsusb output this is an interrupt endpoint, although usb_bulk_msg will auto-detect the type.

There's also usb_interrupt_msg() function but it calls usb_bulk_msg() directly so it probably does not matter...
Otherwise, I already found out that sending interrupt URBs to bulk endpoints is a very bad idea (when programming Nexio support for usbtouchscreen). Alan Stern then created a patch that checks for this.

Quoted 48 lines
Quoted 48 lines
drivers/net/usb/cx82310_eth.c new file +47 -0
+	if (ret < 0) {
+		dev_err(&dev->udev->dev, "send command %#x: error %d\n",
+			cmd, ret);
+		goto end;
+	}
+
+	if (reply) {
+		/* wait for reply, retry if it's empty */
+		for (retries = 0; retries < CMD_REPLY_RETRY; retries++) {
+			ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, CMD_EP),
+					   buf, CMD_PACKET_SIZE, &actual_len,
+					   CMD_TIMEOUT);
+			if (ret < 0) {
+				dev_err(&dev->udev->dev,
+					"reply receive error %d\n", ret);
+				goto end;
+			}
+			if (actual_len > 0)
+				break;
+		}
+		if (actual_len == 0) {
+			dev_err(&dev->udev->dev, "no reply to command %#x\n",
+				cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[0] != cmd) {
+			dev_err(&dev->udev->dev,
+				"got reply to command %#x, expected: %#x\n",
+				buf[0], cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[1] != STATUS_SUCCESS) {
+			dev_err(&dev->udev->dev, "command %#x failed: %#x\n",
+				cmd, buf[1]);
+			ret = -EIO;
+			goto end;
+		}
+		if (rdata)
+			memcpy(rdata, buf + 4,
+			       min_t(int, rlen, CMD_PACKET_SIZE - 4));
+	}
+end:
+	kfree(buf);
+	return ret;
+}
Quoted 5 lines

drivers/net/usb/cx82310_eth.c line 132

Quoted 3 lines
drivers/net/usb/cx82310_eth.c new file +3 -0
+#define rx_incomplete	(dev->data[0])
+#define rx_remainder	(dev->data[1])
+#define incomplete_data	(dev->data[2])

This doesn't make the rest of the code particularly readable.

I agree. Maybe I should abuse data[0] as a pointer to private data... This data[] array does not seem like a good thing. Various usbnet drivers abuse it in various ways. It should probably be removed and replaced by one priv pointer.

Quoted 4 lines
Quoted 4 lines
drivers/net/usb/cx82310_eth.c new file +4 -0
+static int cx82310_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int ret;
+	char buf[15];
Quoted 8 lines

drivers/net/usb/cx82310_eth.c line 142

Quoted 5 lines
drivers/net/usb/cx82310_eth.c new file +5 -0
+	struct usb_device *udev = dev->udev;
+
+	/* avoid ADSL modems - continue only if iProduct is "USB NET CARD" */
+	if (udev->descriptor.iProduct &&
+	    usb_string(udev, udev->descriptor.iProduct, buf, sizeof(buf)) &&

I'm not sure I like the assignment within the if condition here, although other drivers do it; ret is available to check the result.

Originally, it started as nested if blocks but it was looking too complex for such a simple thing (comparing two strings). So I changed it to this single if condition.

Quoted 12 lines
Quoted 12 lines
drivers/net/usb/cx82310_eth.c new file +12 -0
+	    strcmp(buf, "USB NET CARD")) {
+		dev_err(&udev->dev,
+			"probably an ADSL modem, use cxacru driver instead\n");
+		return -ENODEV;
+	}
+
+	ret = usbnet_get_endpoints(dev, intf);
+	if (ret)
+		return ret;
+
+	/*
+	 * this must not include ethernet header as the device can send partial
Quoted 8 lines

drivers/net/usb/cx82310_eth.c line 159

Quoted 5 lines
drivers/net/usb/cx82310_eth.c new file +5 -0
+	 * packets with no header (URB is at least 2 bytes long, so 2 is OK)
+	 */
+	dev->net->hard_header_len = 2;
+	/* we can send at most 1514 bytes of data (+ 2-byte header) per URB */
+	dev->hard_mtu = CX82310_MTU + dev->net->hard_header_len;

Have you tried sending larger packets?
With another 8 bytes it would support 802.1Q (VLAN).

Larger packets seem to be dropped. No surprise as the router firmware does not support VLANs either.

Quoted 6 lines
Quoted 6 lines
drivers/net/usb/cx82310_eth.c new file +6 -0
+	/* we can receive URBs up to 4KB from the device */
+	dev->rx_urb_size = 4096;
+
+	incomplete_data = (unsigned long) kmalloc(dev->hard_mtu, GFP_KERNEL);
+	if (!incomplete_data)
+		return -ENOMEM;
Quoted 8 lines

drivers/net/usb/cx82310_eth.c line 170

Quoted 5 lines
drivers/net/usb/cx82310_eth.c new file +5 -0
+
+	/* enable ethernet mode (?) */
+	ret = cx82310_cmd(dev, CMD_ETHERNET_MODE, true, "\x01", 1, NULL, 0);
+	if (ret) {
+		dev_err(&udev->dev, "unable to enable ethernet mode\n");

I'd include the return value in the message, it can be useful to know if it was a timeout response or not.

Thanks, looks like a good idea.

Signature · 1 line
Ondrej Zary
author
#4 lore raw

RFC PATCH 1/1 cxacru: ignore ADSL routers

In reply to Simon Arlott

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be driven by cx82310_eth driver.

Signed-off-by Ondrej Zary <[email protected]>
drivers/usb/atm/cxacru.c +11 -0
@@ -1129,6 +1129,17 @@ static int cxacru_bind(struct usbatm_dat
 	struct cxacru_data *instance;
 	struct usb_device *usb_dev = interface_to_usbdev(intf);
 	int ret;
+	char buf[15];
+
+	/* avoid ADSL routers - abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == 0xff &&
+	    usb_dev->descriptor.iProduct &&
+	    usb_string(usb_dev, usb_dev->descriptor.iProduct, buf, sizeof(buf)) &&
+	    !strcmp(buf, "USB NET CARD")) {
+		usb_err(usbatm_instance,
+			"probably an ADSL router, use cx82310_eth driver instead\n");
+		return -ENODEV;
+	}
 
 	/* instance init */
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
Signature · 1 line
Ondrej Zary
author
#20 lore raw

RFC PATCH v2 1/1 introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

In reply to Simon Arlott

This patch introduces cx82310_eth driver - driver for USB ethernet port of ADSL routers based on Conexant CX82310 chips. Such routers usually have ethernet port(s) too which are bridged together with the USB ethernet port, allowing the USB-connected machine to communicate to the network (and also internet through the ADSL, of course).

This is my first driver, so please check thoroughly. As there's no protocol documentation, it was done with usbsnoop dumps from Windows driver, some parts (the commands) inspired by cxacru driver and also other usbnet drivers. The driver passed my testing - some real work and also pings sized from 0 to 65507 B.

The only problem I found is the ifconfig error counter. When I return 0 (or 1 but empty skb) from rx_fixup(), usbnet increases the error counter although it's not an error condition (because packets can cross URB boundaries). Maybe the usbnet should be fixed to allow rx_fixup() to return empty skbs (or some other value, e.g. 2)?

The USB ID of my device is 0x0572:0xcb01 which conflicts with some ADSL modems using cxacru driver (they probably use the same chipset but simpler firmware). The modems seem to use bDeviceClass 0 and iProduct "ADSL USB MODEM", my router uses bDeviceClass 255 and iProduct "USB NET CARD". The driver matches only devices with class 255 and checks for the iProduct string during init. I already posted a patch for the cxacru driver to ignore these devices.

Signed-off-by Ondrej Zary <[email protected]>

---
This is v2 with changed partial packet #defines (hopefully more readable) and error code printing in cx82310_bind().

drivers/net/usb/cx82310_eth.c new file +354 -0
@@ -0,0 +1,354 @@
+/*
+ * Driver for USB ethernet port of Conexant CX82310-based ADSL routers
+ * Copyright (C) 2010 by Ondrej Zary
+ * some parts inspired by the cxacru driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/workqueue.h>
+#include <linux/mii.h>
+#include <linux/usb.h>
+#include <linux/usb/usbnet.h>
+
+enum cx82310_cmd {
+	CMD_START		= 0x84,	/* no effect? */
+	CMD_STOP		= 0x85,	/* no effect? */
+	CMD_GET_STATUS		= 0x90,	/* returns nothing? */
+	CMD_GET_MAC_ADDR	= 0x91,	/* read MAC address */
+	CMD_GET_LINK_STATUS	= 0x92,	/* not useful, link is always up */
+	CMD_ETHERNET_MODE	= 0x99,	/* unknown, needed during init */
+};
+
+enum cx82310_status {
+	STATUS_UNDEFINED,
+	STATUS_SUCCESS,
+	STATUS_ERROR,
+	STATUS_UNSUPPORTED,
+	STATUS_UNIMPLEMENTED,
+	STATUS_PARAMETER_ERROR,
+	STATUS_DBG_LOOPBACK,
+};
+
+#define CMD_PACKET_SIZE	64
+/* first command after power on can take around 8 seconds */
+#define CMD_TIMEOUT	15000
+#define CMD_REPLY_RETRY 5
+
+#define CX82310_MTU	1514
+#define CMD_EP		0x01
+
+/*
+ * execute control command
+ *  - optionally send some data (command parameters)
+ *  - optionally wait for the reply
+ *  - optionally read some data from the reply
+ */
+static int cx82310_cmd(struct usbnet *dev, enum cx82310_cmd cmd, bool reply,
+		       u8 *wdata, int wlen, u8 *rdata, int rlen)
+{
+	int actual_len, retries, ret;
+	struct usb_device *udev = dev->udev;
+	u8 *buf = kzalloc(CMD_PACKET_SIZE, GFP_KERNEL);
+
+	if (!buf)
+		return -ENOMEM;
+
+	/* create command packet */
+	buf[0] = cmd;
+	if (wdata)
+		memcpy(buf + 4, wdata, min_t(int, wlen, CMD_PACKET_SIZE - 4));
+
+	/* send command packet */
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, CMD_EP), buf,
+			   CMD_PACKET_SIZE, &actual_len, CMD_TIMEOUT);
+	if (ret < 0) {
+		dev_err(&dev->udev->dev, "send command %#x: error %d\n",
+			cmd, ret);
+		goto end;
+	}
+
+	if (reply) {
+		/* wait for reply, retry if it's empty */
+		for (retries = 0; retries < CMD_REPLY_RETRY; retries++) {
+			ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, CMD_EP),
+					   buf, CMD_PACKET_SIZE, &actual_len,
+					   CMD_TIMEOUT);
+			if (ret < 0) {
+				dev_err(&dev->udev->dev,
+					"reply receive error %d\n", ret);
+				goto end;
+			}
+			if (actual_len > 0)
+				break;
+		}
+		if (actual_len == 0) {
+			dev_err(&dev->udev->dev, "no reply to command %#x\n",
+				cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[0] != cmd) {
+			dev_err(&dev->udev->dev,
+				"got reply to command %#x, expected: %#x\n",
+				buf[0], cmd);
+			ret = -EIO;
+			goto end;
+		}
+		if (buf[1] != STATUS_SUCCESS) {
+			dev_err(&dev->udev->dev, "command %#x failed: %#x\n",
+				cmd, buf[1]);
+			ret = -EIO;
+			goto end;
+		}
+		if (rdata)
+			memcpy(rdata, buf + 4,
+			       min_t(int, rlen, CMD_PACKET_SIZE - 4));
+	}
+end:
+	kfree(buf);
+	return ret;
+}
+
+#define partial_len	data[0]		/* length of partial packet data */
+#define partial_rem	data[1]		/* remaining (missing) data length */
+#define partial_data	data[2]		/* partial packet data */
+
+static int cx82310_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int ret;
+	char buf[15];
+	struct usb_device *udev = dev->udev;
+
+	/* avoid ADSL modems - continue only if iProduct is "USB NET CARD" */
+	if (udev->descriptor.iProduct &&
+	    usb_string(udev, udev->descriptor.iProduct, buf, sizeof(buf)) &&
+	    strcmp(buf, "USB NET CARD")) {
+		dev_err(&udev->dev,
+			"probably an ADSL modem, use cxacru driver instead\n");
+		return -ENODEV;
+	}
+
+	ret = usbnet_get_endpoints(dev, intf);
+	if (ret)
+		return ret;
+
+	/*
+	 * this must not include ethernet header as the device can send partial
+	 * packets with no header (URB is at least 2 bytes long, so 2 is OK)
+	 */
+	dev->net->hard_header_len = 2;
+	/* we can send at most 1514 bytes of data (+ 2-byte header) per URB */
+	dev->hard_mtu = CX82310_MTU + dev->net->hard_header_len;
+	/* we can receive URBs up to 4KB from the device */
+	dev->rx_urb_size = 4096;
+
+	dev->partial_data = (unsigned long) kmalloc(dev->hard_mtu, GFP_KERNEL);
+	if (!dev->partial_data)
+		return -ENOMEM;
+
+	/* enable ethernet mode (?) */
+	ret = cx82310_cmd(dev, CMD_ETHERNET_MODE, true, "\x01", 1, NULL, 0);
+	if (ret) {
+		dev_err(&udev->dev, "unable to enable ethernet mode: %d\n",
+			ret);
+		goto err;
+	}
+
+	/* get the MAC address */
+	ret = cx82310_cmd(dev, CMD_GET_MAC_ADDR, true, NULL, 0,
+			  dev->net->dev_addr, ETH_ALEN);
+	if (ret) {
+		dev_err(&udev->dev, "unable to read MAC address: %d\n", ret);
+		goto err;
+	}
+
+	/* start (does not seem to have any effect?) */
+	ret = cx82310_cmd(dev, CMD_START, false, NULL, 0, NULL, 0);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	kfree((void *)dev->partial_data);
+	return ret;
+}
+
+static void cx82310_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+	kfree((void *)dev->partial_data);
+}
+
+/*
+ * RX is NOT easy - we can receive multiple packets per skb, each having 2-byte
+ * packet length at the beginning.
+ * The last packet might be incomplete (when it crosses the 4KB URB size),
+ * continuing in the next skb (without any headers).
+ * If a packet has odd length, there is one extra byte at the end (before next
+ * packet or at the end of the URB).
+ */
+static int cx82310_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+	int len;
+	struct sk_buff *skb2;
+
+	/*
+	 * If the last skb ended with an incomplete packet, this skb contains
+	 * end of that packet at the beginning.
+	 */
+	if (dev->partial_rem) {
+		len = dev->partial_len + dev->partial_rem;
+		skb2 = alloc_skb(len, GFP_ATOMIC);
+		if (!skb2)
+			return 0;
+		skb_put(skb2, len);
+		memcpy(skb2->data, (void *)dev->partial_data,
+		       dev->partial_len);
+		memcpy(skb2->data + dev->partial_len, skb->data,
+		       dev->partial_rem);
+		usbnet_skb_return(dev, skb2);
+		skb_pull(skb, (dev->partial_rem + 1) & ~1);
+		dev->partial_rem = 0;
+		if (skb->len < 2)
+			return 1;
+	}
+
+	if (skb->len < 2) {
+		dev_err(&dev->udev->dev, "RX frame too short: %d B\n",
+			skb->len);
+		return 0;
+	}
+
+	/* a skb can contain multiple packets */
+	while (skb->len > 1) {
+		/* first two bytes are packet length */
+		len = skb->data[0] | (skb->data[1] << 8);
+		skb_pull(skb, 2);
+
+		/* if last packet in the skb, let usbnet to process it */
+		if (len == skb->len || len + 1 == skb->len) {
+			skb_trim(skb, len);
+			break;
+		}
+
+		if (len > CX82310_MTU) {
+			dev_err(&dev->udev->dev, "RX packet too long: %d B\n",
+				len);
+			return 0;
+		}
+
+		/* incomplete packet, save it for the next skb */
+		if (len > skb->len) {
+			dev->partial_len = skb->len;
+			dev->partial_rem = len - skb->len;
+			memcpy((void *)dev->partial_data, skb->data,
+			       dev->partial_len);
+			skb_pull(skb, skb->len);
+			break;
+		}
+
+		skb2 = alloc_skb(len, GFP_ATOMIC);
+		if (!skb2)
+			return 0;
+		skb_put(skb2, len);
+		memcpy(skb2->data, skb->data, len);
+		/* process the packet */
+		usbnet_skb_return(dev, skb2);
+
+		skb_pull(skb, (len + 1) & ~1);
+	}
+
+	/* let usbnet process the last packet */
+	return 1;
+}
+
+/* TX is easy, just add 2 bytes of length at the beginning */
+static struct sk_buff *cx82310_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
+				       gfp_t flags)
+{
+	int len = skb->len;
+
+	if (skb_headroom(skb) < 2) {
+		struct sk_buff *skb2 = skb_copy_expand(skb, 2, 0, flags);
+		dev_kfree_skb_any(skb);
+		skb = skb2;
+		if (!skb)
+			return NULL;
+	}
+	skb_push(skb, 2);
+
+	skb->data[0] = len;
+	skb->data[1] = len >> 8;
+
+	return skb;
+}
+
+
+static const struct driver_info	cx82310_info = {
+	.description	= "Conexant CX82310 USB ethernet",
+	.flags		= FLAG_ETHER,
+	.bind		= cx82310_bind,
+	.unbind		= cx82310_unbind,
+	.rx_fixup	= cx82310_rx_fixup,
+	.tx_fixup	= cx82310_tx_fixup,
+};
+
+#define USB_DEVICE_CLASS(vend, prod, cl, sc, pr) \
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
+		       USB_DEVICE_ID_MATCH_DEV_INFO, \
+	.idVendor = (vend), \
+	.idProduct = (prod), \
+	.bDeviceClass = (cl), \
+	.bDeviceSubClass = (sc), \
+	.bDeviceProtocol = (pr)
+
+static const struct usb_device_id products[] = {
+	{
+		USB_DEVICE_CLASS(0x0572, 0xcb01, 0xff, 0, 0),
+		.driver_info = (unsigned long) &cx82310_info
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(usb, products);
+
+static struct usb_driver cx82310_driver = {
+	.name		= "cx82310_eth",
+	.id_table	= products,
+	.probe		= usbnet_probe,
+	.disconnect	= usbnet_disconnect,
+	.suspend	= usbnet_suspend,
+	.resume		= usbnet_resume,
+};
+
+static int __init cx82310_init(void)
+{
+	return usb_register(&cx82310_driver);
+}
+module_init(cx82310_init);
+
+static void __exit cx82310_exit(void)
+{
+	usb_deregister(&cx82310_driver);
+}
+module_exit(cx82310_exit);
+
+MODULE_AUTHOR("Ondrej Zary");
+MODULE_DESCRIPTION("Conexant CX82310-based ADSL router USB ethernet driver");
+MODULE_LICENSE("GPL");
drivers/net/usb/Kconfig +8 -0
@@ -358,6 +358,14 @@ config USB_NET_ZAURUS
 	  really need this non-conformant variant of CDC Ethernet (or in
 	  some cases CDC MDLM) protocol, not "g_ether".
 
+config USB_NET_CX82310_ETH
+	tristate "Conexant CX82310 USB ethernet port"
+	depends on USB_USBNET
+	help
+	  Choose this option if you're using a Conexant CX82310-based ADSL
+	  router with USB ethernet port. This driver is for routers only,
+	  it will not work with ADSL modems (use cxacru driver instead).
+
 config USB_HSO
 	tristate "Option USB High Speed Mobile Devices"
 	depends on USB && RFKILL
drivers/net/usb/Makefile +1 -0
@@ -25,4 +25,5 @@ obj-$(CONFIG_USB_NET_INT51X1)	+= int51x1
 obj-$(CONFIG_USB_CDC_PHONET)	+= cdc-phonet.o
 obj-$(CONFIG_USB_IPHETH)	+= ipheth.o
 obj-$(CONFIG_USB_SIERRA_NET)	+= sierra_net.o
+obj-$(CONFIG_USB_NET_CX82310_ETH)	+= cx82310_eth.o
Signature · 1 line
Ondrej Zary
#3 lore raw
In reply to Ondrej Zary
Quoted 9 lines · Ondrej Zary
Quoted 7 lines · On Saturday 04 September 2010 00:14:54 Simon Arlott wrote:

drivers/net/usb/cx82310_eth.c line 81

Quoted 3 lines · Ondrej Zary
drivers/net/usb/cx82310_eth.c new file +3 -0
+	/* send command packet */
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, CMD_EP), buf,
+			   CMD_PACKET_SIZE, &actual_len, CMD_TIMEOUT);

From your previous lsusb output this is an interrupt endpoint, although usb_bulk_msg will auto-detect the type.

Quoted 5 lines

There's also usb_interrupt_msg() function but it calls usb_bulk_msg() directly so it probably does not matter...
Otherwise, I already found out that sending interrupt URBs to bulk endpoints is a very bad idea (when programming Nexio support for usbtouchscreen). Alan Stern then created a patch that checks for this.

I know... it broke cxacru when the device has a bulk command endpoint.

Quoted 11 lines
Quoted 6 lines

drivers/net/usb/cx82310_eth.c line 132

Quoted 4 lines
drivers/net/usb/cx82310_eth.c new file +3 -0
+#define rx_incomplete	(dev->data[0])
+#define rx_remainder	(dev->data[1])
+#define incomplete_data	(dev->data[2])

This doesn't make the rest of the code particularly readable.

I agree. Maybe I should abuse data[0] as a pointer to private data... This data[] array does not seem like a good thing. Various usbnet drivers abuse it in various ways. It should probably be removed and replaced by one priv pointer.

There's driver_priv, added in 2008 and used only by rndis_wlan.

Quoted 8 lines
Quoted 5 lines

drivers/net/usb/cx82310_eth.c line 159

Quoted 2 lines
drivers/net/usb/cx82310_eth.c new file +2 -0
+	/* we can send at most 1514 bytes of data (+ 2-byte header) per URB */
+	dev->hard_mtu = CX82310_MTU + dev->net->hard_header_len;

Have you tried sending larger packets?
With another 8 bytes it would support 802.1Q (VLAN).

Larger packets seem to be dropped. No surprise as the router firmware does not support VLANs either.

I hadn't considered that it'd need to be able to receive/transmit the larger packets over the built-in ethernet ports too.

Signature · 1 line
Simon Arlott
#5 lore raw

PATCH 1/1 cxacru: ignore cx82310_eth devices

In reply to Ondrej Zary

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>
[SA: Moved to cxacru_usb_probe, message level lowered to info]
Signed-off-by Simon Arlott <[email protected]>
---
 drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
drivers/usb/atm/cxacru.c +16 -2
index 593fc5e..96fa736 100644
@@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_driver = {
 	.tx_padding	= 11,
 };
 
-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
-{
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == 0xff
+			&& usb_dev->descriptor.iProduct
+			&& usb_string(usb_dev,
+				usb_dev->descriptor.iProduct, buf, sizeof(buf))
+			&& !strcmp(buf, "USB NET CARD")) {
+		dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+		return -ENODEV;
+	}
+
 	return usbatm_usb_probe(intf, id, &cxacru_driver);
 }
Signature · 4 lines
1.7.0.4

-- 
Simon Arlott
#6 lore raw

Re: [PATCH] cxacru: ignore cx82310_eth devices

Quoted 22 lines · Simon Arlott

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>
[SA: Moved to cxacru_usb_probe, message level lowered to info]
Signed-off-by Simon Arlott <[email protected]>
---
 drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
drivers/usb/atm/cxacru.c +0 -0
index 593fc5e..96fa736 100644
@@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_driver = {
 	.tx_padding	= 11,
 };

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1334

Quoted 10 lines
drivers/usb/atm/cxacru.c +8 -2
-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
-{
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == 0xff

vendor class? We have a macro for that?

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1335

Quoted 1 line
drivers/usb/atm/cxacru.c +1 -0
+			&& usb_dev->descriptor.iProduct

Almost everyone has a iProduct, right? Why even test for that?

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1337

Quoted 2 lines
drivers/usb/atm/cxacru.c +2 -0
+			&& usb_string(usb_dev,
+				usb_dev->descriptor.iProduct, buf, sizeof(buf))

usb_string() should handle a 0 string, right?

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1338

Quoted 1 line
drivers/usb/atm/cxacru.c +1 -0
+			&& !strcmp(buf, "USB NET CARD")) {

Just to make it a bit easier to follow, how about you do the usb_string() call in the if statment, and then compare the string and then, if that matches you do:

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1340

Quoted 2 lines
drivers/usb/atm/cxacru.c +2 -0
+		dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+		return -ENODEV;

Well, actually, dev_info? I guess so, but do you really want to see this message every time?

thanks,

greg k-h

author
#7 lore raw

Re: [PATCH] cxacru: ignore cx82310_eth devices

Quoted 45 lines · On Sunday 05 September 2010 06:03:52 Greg KH wrote:
Quoted 22 lines · Simon Arlott

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>
[SA: Moved to cxacru_usb_probe, message level lowered to info]
Signed-off-by Simon Arlott <[email protected]>
---
 drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
drivers/usb/atm/cxacru.c +0 -0
index 593fc5e..96fa736 100644
@@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_driver = {
 	.tx_padding	= 11,
 };

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1335

Quoted 10 lines
drivers/usb/atm/cxacru.c +8 -1
-static int cxacru_usb_probe(struct usb_interface *intf, const struct
 usb_device_id *id) -{
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == 0xff

vendor class? We have a macro for that?

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1335

Quoted 1 line
drivers/usb/atm/cxacru.c +1 -0
+			&& usb_dev->descriptor.iProduct

Almost everyone has a iProduct, right? Why even test for that?

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1337

Quoted 2 lines
drivers/usb/atm/cxacru.c +2 -0
+			&& usb_string(usb_dev,
+				usb_dev->descriptor.iProduct, buf, sizeof(buf))

usb_string() should handle a 0 string, right?

Quoted 8 lines

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1338

Quoted 1 line
drivers/usb/atm/cxacru.c +1 -0
+			&& !strcmp(buf, "USB NET CARD")) {

Just to make it a bit easier to follow, how about you do the usb_string() call in the if statment, and then compare the string and

then, if that matches you do:

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1340

Quoted 2 lines
drivers/usb/atm/cxacru.c +2 -0
+		dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+		return -ENODEV;

Does it look that much better?:

	if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC
			&& usb_string(usb_dev, usb_dev->descriptor.iProduct,
				buf, sizeof(buf)) > 0)
		if (!strcmp(buf, "USB NET CARD")) {
			dev_info(&intf->dev, "ignoring cx82310_eth device\n");
			return -ENODEV;
		}

I also found a bug in my first patch - usb_string() can return negative value which can cause problems when not checked for.

Quoted 6 lines

Well, actually, dev_info? I guess so, but do you really want to see this message every time?

thanks,

greg k-h

Signature · 1 line
Ondrej Zary
#8 lore raw

Re: [PATCH] cxacru: ignore cx82310_eth devices

Quoted 57 lines · Ondrej Zary
Quoted 45 lines · On Sunday 05 September 2010 06:03:52 Greg KH wrote:
Quoted 22 lines · Simon Arlott

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>
[SA: Moved to cxacru_usb_probe, message level lowered to info]
Signed-off-by Simon Arlott <[email protected]>
---
 drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
drivers/usb/atm/cxacru.c +0 -0
index 593fc5e..96fa736 100644
@@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_driver = {
 	.tx_padding	= 11,
 };

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1335

Quoted 10 lines
drivers/usb/atm/cxacru.c +8 -1
-static int cxacru_usb_probe(struct usb_interface *intf, const struct
 usb_device_id *id) -{
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == 0xff

vendor class? We have a macro for that?

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1335

Quoted 1 line
drivers/usb/atm/cxacru.c +1 -0
+			&& usb_dev->descriptor.iProduct

Almost everyone has a iProduct, right? Why even test for that?

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1337

Quoted 2 lines
drivers/usb/atm/cxacru.c +2 -0
+			&& usb_string(usb_dev,
+				usb_dev->descriptor.iProduct, buf, sizeof(buf))

usb_string() should handle a 0 string, right?

Quoted 8 lines

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1338

Quoted 1 line
drivers/usb/atm/cxacru.c +1 -0
+			&& !strcmp(buf, "USB NET CARD")) {

Just to make it a bit easier to follow, how about you do the usb_string() call in the if statment, and then compare the string and

then, if that matches you do:

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_driver = { · line 1340

Quoted 2 lines
drivers/usb/atm/cxacru.c +2 -0
+		dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+		return -ENODEV;

Does it look that much better?:

Quoted 7 lines
	if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC
			&& usb_string(usb_dev, usb_dev->descriptor.iProduct,
				buf, sizeof(buf)) > 0)
		if (!strcmp(buf, "USB NET CARD")) {
			dev_info(&intf->dev, "ignoring cx82310_eth device\n");
			return -ENODEV;
		}

Yes, don't you think so?

Quoted 2 lines

I also found a bug in my first patch - usb_string() can return negative value which can cause problems when not checked for.

Ah, nice catch. Care to resend it now?

thanks,

greg k-h

author
#9 lore raw

PATCH 1/1 cxacru: ignore cx82310_eth devices

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>
drivers/usb/atm/cxacru.c +16 -2
@@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive
 	.tx_padding	= 11,
 };
 
-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
-{
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC
+			&& usb_string(usb_dev, usb_dev->descriptor.iProduct,
+				buf, sizeof(buf)) > 0) {
+		if (!strcmp(buf, "USB NET CARD")) {
+			dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+			return -ENODEV;
+		}
+	}
+
 	return usbatm_usb_probe(intf, id, &cxacru_driver);
 }
Signature · 1 line
Ondrej Zary
#10 lore raw

Re: [PATCH] cxacru: ignore cx82310_eth devices

Quoted 12 lines · Ondrej Zary

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_drive · line 1328

Quoted 7 lines
drivers/usb/atm/cxacru.c +2 -2
@@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive
 	.tx_padding	= 11,
 };
 
-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
-{
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {

Ick, what?

Did you run this patch through scripts/checkpatch.pl?

As you didn't change the function arguments, just leave the long line alone, but even if you did change it, the '{' has to be on the next line by itself. See Documentation/CodingStyle for details.

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_drive · line 1341

Quoted 13 lines
drivers/usb/atm/cxacru.c +13 -0
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC
+			&& usb_string(usb_dev, usb_dev->descriptor.iProduct,
+				buf, sizeof(buf)) > 0) {
+		if (!strcmp(buf, "USB NET CARD")) {
+			dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+			return -ENODEV;
+		}
+	}

In thinking about this a bit more, don't you also want to check the vendor and product id? You can't always be sure about the string of any old device, right?

thanks,

greg k-h

#11 lore raw

Re: [PATCH] cxacru: ignore cx82310_eth devices

Quoted 9 lines · Greg KH
Quoted 8 lines · Ondrej Zary

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>

Missing Signed-off-by: you're modifying my changes.

Quoted 6 lines
Quoted 6 lines
drivers/usb/atm/cxacru.c +0 -0
@@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive
 	.tx_padding	= 11,
 };
Quoted 6 lines

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_drive · line 1328

Quoted 4 lines
drivers/usb/atm/cxacru.c +2 -2
-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
-{
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {

Ick, what?

Sorry, this was my fault. I wasn't thinking when I changed it.

Quoted 9 lines
Quoted 9 lines
drivers/usb/atm/cxacru.c +9 -0
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC
+			&& usb_string(usb_dev, usb_dev->descriptor.iProduct,
+				buf, sizeof(buf)) > 0) {
+		if (!strcmp(buf, "USB NET CARD")) {
Quoted 8 lines

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_drive · line 1341

Quoted 4 lines
drivers/usb/atm/cxacru.c +4 -0
+			dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+			return -ENODEV;
+		}
+	}

In thinking about this a bit more, don't you also want to check the vendor and product id? You can't always be sure about the string of any old device, right?

This is already checked using the driver's id_table.

I don't know if the vendor and product IDs for the cx82310_eth device should be claimed by cxacru or not. Either Conexant are sharing IDs between devices, or someone added it without confirming it works. There's no comment in the code from 2003 explaining what device it's supposed to be. For this reason I'd prefer to ignore all cxacru-claimed devices that appear to be an "USB NET CARD", and log that it did so.

The 3 variants of the hardware I have all use an iProduct of "ADSL USB MODEM", but I don't want to restrict cxacru to just that in case some devices have different values.

Signature · 1 line
Simon Arlott
author
#12 lore raw

Re: [PATCH] cxacru: ignore cx82310_eth devices

Quoted 5 lines · Simon Arlott
Quoted 4 lines · Greg KH
Quoted 3 lines · Ondrej Zary

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>

Missing Signed-off-by: you're modifying my changes.

Oops, sorry for that. I wanted to remove only the line between the signed-offs...

Quoted 34 lines
Quoted 7 lines
Quoted 7 lines

--- linux-2.6.36-rc3-orig/drivers/usb/atm/cxacru.c 2010-08-29
17:36:04.000000000 +0200 +++
linux-2.6.36-rc3/drivers/usb/atm/cxacru.c 2010-09-05 18:54:56.000000000
+0200 @@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive
.tx_padding = 11,
};

Quoted 6 lines

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_drive · line 1328

Quoted 4 lines

-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) -{

drivers/usb/atm/cxacru.c +2 -0
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {

Ick, what?

Sorry, this was my fault. I wasn't thinking when I changed it.

Quoted 9 lines
Quoted 9 lines
drivers/usb/atm/cxacru.c +9 -0
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC
+			&& usb_string(usb_dev, usb_dev->descriptor.iProduct,
+				buf, sizeof(buf)) > 0) {
+		if (!strcmp(buf, "USB NET CARD")) {
Quoted 8 lines

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_drive · line 1341

Quoted 4 lines
drivers/usb/atm/cxacru.c +4 -0
+			dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+			return -ENODEV;
+		}
+	}

In thinking about this a bit more, don't you also want to check the vendor and product id? You can't always be sure about the string of any old device, right?

Quoted 8 lines

This is already checked using the driver's id_table.

I don't know if the vendor and product IDs for the cx82310_eth device should be claimed by cxacru or not. Either Conexant are sharing IDs between devices, or someone added it without confirming it works. There's no comment in the code from 2003 explaining what device it's supposed to be. For this reason I'd prefer to ignore all cxacru-claimed devices that appear to be an "USB NET CARD", and log that it did so.

Yes, these "USB NET CARD" devices will not work with cxacru either. We don't lose anything even if they are ignored by cxacru but not claimed by cx82310_eth. If someone has such device, (s)he can report the IDs to be added to cx82310_eth.

Quoted 3 lines

The 3 variants of the hardware I have all use an iProduct of "ADSL USB MODEM", but I don't want to restrict cxacru to just that in case some devices have different values.

Signature · 1 line
Ondrej Zary
maintainer Applied
#21 lore raw
In reply to Ondrej Zary

From: Ondrej Zary <[email protected]>
Date: Sat, 4 Sep 2010 14:39:34 +0200

Quoted 26 lines

This patch introduces cx82310_eth driver - driver for USB ethernet port of ADSL routers based on Conexant CX82310 chips. Such routers usually have ethernet port(s) too which are bridged together with the USB ethernet port, allowing the USB-connected machine to communicate to the network (and also internet through the ADSL, of course).

This is my first driver, so please check thoroughly. As there's no protocol documentation, it was done with usbsnoop dumps from Windows driver, some parts (the commands) inspired by cxacru driver and also other usbnet drivers. The driver passed my testing - some real work and also pings sized from 0 to 65507 B.

The only problem I found is the ifconfig error counter. When I return 0 (or 1 but empty skb) from rx_fixup(), usbnet increases the error counter although it's not an error condition (because packets can cross URB boundaries). Maybe the usbnet should be fixed to allow rx_fixup() to return empty skbs (or some other value, e.g. 2)?

The USB ID of my device is 0x0572:0xcb01 which conflicts with some ADSL modems using cxacru driver (they probably use the same chipset but simpler firmware). The modems seem to use bDeviceClass 0 and iProduct "ADSL USB MODEM", my router uses bDeviceClass 255 and iProduct "USB NET CARD". The driver matches only devices with class 255 and checks for the iProduct string during init. I already posted a patch for the cxacru driver to ignore these devices.

Quoted 1 line
Signed-off-by Ondrej Zary <[email protected]>

Applied, thanks.

author
#15 lore raw

PATCH v3 1/1 cxacru: ignore cx82310_eth devices

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>
Signed-off-by Simon Arlott <[email protected]>
drivers/usb/atm/cxacru.c +16 -1
@@ -1324,8 +1324,23 @@ static struct usbatm_driver cxacru_drive
 	.tx_padding	= 11,
 };
 
-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id)
 {
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	char buf[15];
+
+	/* avoid ADSL routers (cx82310_eth)
+	 * abort if bDeviceClass is 0xff and iProduct is "USB NET CARD" */
+	if (usb_dev->descriptor.bDeviceClass == USB_CLASS_VENDOR_SPEC
+			&& usb_string(usb_dev, usb_dev->descriptor.iProduct,
+				buf, sizeof(buf)) > 0) {
+		if (!strcmp(buf, "USB NET CARD")) {
+			dev_info(&intf->dev, "ignoring cx82310_eth device\n");
+			return -ENODEV;
+		}
+	}
+
 	return usbatm_usb_probe(intf, id, &cxacru_driver);
 }
Signature · 1 line
Ondrej Zary
author
#13 lore raw

Re: [PATCH] cxacru: ignore cx82310_eth devices

In reply to Greg KH
Quoted 17 lines · On Sunday 05 September 2010 23:04:46 Greg KH wrote:
Quoted 16 lines · Ondrej Zary

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>

--- linux-2.6.36-rc3-orig/drivers/usb/atm/cxacru.c 2010-08-29
17:36:04.000000000 +0200 +++
linux-2.6.36-rc3/drivers/usb/atm/cxacru.c 2010-09-05 18:54:56.000000000
+0200 @@ -1324,8 +1324,22 @@ static struct usbatm_driver cxacru_drive
.tx_padding = 11,
};

Quoted 8 lines

drivers/usb/atm/cxacru.c static struct usbatm_driver cxacru_drive · line 1328

Quoted 4 lines

-static int cxacru_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) -{

drivers/usb/atm/cxacru.c +2 -0
+static int cxacru_usb_probe(struct usb_interface *intf,
+		const struct usb_device_id *id) {

Ick, what?

Did you run this patch through scripts/checkpatch.pl?

Yes - and it passed. Maybe checkpatch.pl can't handle multiple lines there.

Signature · 1 line
Ondrej Zary
maintainer
#16 lore raw

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

In reply to Ondrej Zary

From: Ondrej Zary <[email protected]>
Date: Wed, 8 Sep 2010 22:52:12 +0200

Quoted 9 lines

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Signed-off-by Ondrej Zary <[email protected]>
Signed-off-by Simon Arlott <[email protected]>

Applied with some minor comment formatting fixes, thanks!

#17 lore raw

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

In reply to Ondrej Zary
Quoted 6 lines · Ondrej Zary

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Looks good, thanks, I'll go queue it up.

greg k-h

maintainer
#18 lore raw

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

From: Greg KH <[email protected]>
Date: Wed, 8 Sep 2010 20:35:57 -0700

Quoted 9 lines · Ondrej Zary
Quoted 6 lines

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Looks good, thanks, I'll go queue it up.

Greg, see my reply, I already tossed this into net-next-2.6

#19 lore raw

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

Quoted 7 lines · David Miller

From: Greg KH <[email protected]>
Date: Wed, 8 Sep 2010 20:35:57 -0700

Quoted 4 lines · Ondrej Zary
Quoted 3 lines

Ignore ADSL routers, which can have the same vendor and product IDs as ADSL modems but should be handled by the cx82310_eth driver.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

This intentionally ignores device IDs that aren't currently handled by cx82310_eth. There may be other device IDs that perhaps shouldn't be claimed by cxacru.

Looks good, thanks, I'll go queue it up.

Greg, see my reply, I already tossed this into net-next-2.6

Ah, that's fine, thanks for doing this, much appreciated.

greg k-h