[PATCH] [RFC] introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

[PATCH] [RFC] introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

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

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 <linux@rainbow-software.org>

--- /dev/null	2010-09-03 21:17:56.916000000 +0200
+++ linux-2.6.35-rc3/drivers/net/usb/cx82310_eth.c	2010-09-03 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
+ */
+
+#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");
--- linux-2.6.35-rc2/drivers/net/usb/Kconfig	2010-06-06 05:43:24.000000000 +0200
+++ linux-2.6.35-rc3/drivers/net/usb/Kconfig	2010-09-01 19:20:36.000000000 +0200
@@ -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
--- linux-2.6.35-rc2/drivers/net/usb/Makefile	2010-06-06 05:43:24.000000000 +0200
+++ linux-2.6.35-rc3/drivers/net/usb/Makefile	2010-09-03 23:14:46.000000000 +0200
@@ -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
 


-- 
Ondrej Zary

Re: [PATCH] [RFC] introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

[Simon Arlott]

On 03/09/10 22:17, Ondrej Zary wrote:
> 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.

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.

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

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

> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> 
> --- /dev/null	2010-09-03 21:17:56.916000000 +0200
> +++ linux-2.6.35-rc3/drivers/net/usb/cx82310_eth.c	2010-09-03 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
> + */
> +
> +#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);

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

> +	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])

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

> +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.

> +	    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;

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

> +	/* 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.

> +		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);
> +}
> +

-- 
Simon Arlott

Re: [PATCH] [RFC] introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

[Ondrej Zary]

On Saturday 04 September 2010 00:14:54 Simon Arlott wrote:
> On 03/09/10 22:17, Ondrej Zary wrote:
> > 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.
>
> 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.

> > 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.
>
> You can include the cxacru change as part of your patch; I'll test it.

A patch will follow in next e-mail.

> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > --- /dev/null	2010-09-03 21:17:56.916000000 +0200
> > +++ linux-2.6.35-rc3/drivers/net/usb/cx82310_eth.c	2010-09-03
> > 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 + */
> > +
> > +#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);
>
> 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.

> > +	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])
>
> 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.

> > +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.

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.

> > +	    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;
>
> 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.

> > +	/* 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.

Thanks, looks like a good idea.

-- 
Ondrej Zary

[PATCH] [RFC] cxacru: ignore ADSL routers

[Ondrej Zary]

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 <linux@rainbow-software.org>

--- linux-2.6.35-rc2/drivers/usb/atm/cxacru.c	2010-06-06 05:43:24.000000000 +0200
+++ linux-2.6.35-rc3/drivers/usb/atm/cxacru.c	2010-09-04 11:54:35.000000000 +0200
@@ -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);


-- 
Ondrej Zary

[PATCH v2] [RFC] introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

[Ondrej Zary]

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 <linux@rainbow-software.org>
---
This is v2 with changed partial packet #defines (hopefully more readable)
and error code printing in cx82310_bind().

--- /dev/null	2010-09-04 12:04:48.863999999 +0200
+++ linux-2.6.35-rc3/drivers/net/usb/cx82310_eth.c	2010-09-04 14:30:39.000000000 +0200
@@ -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");
--- linux-2.6.35-rc2/drivers/net/usb/Kconfig	2010-06-06 05:43:24.000000000 +0200
+++ linux-2.6.35-rc3/drivers/net/usb/Kconfig	2010-09-01 19:20:36.000000000 +0200
@@ -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
--- linux-2.6.35-rc2/drivers/net/usb/Makefile	2010-06-06 05:43:24.000000000 +0200
+++ linux-2.6.35-rc3/drivers/net/usb/Makefile	2010-09-03 23:14:46.000000000 +0200
@@ -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
 


-- 
Ondrej Zary

Re: [PATCH] [RFC] introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

[Simon Arlott]

On 04/09/10 12:57, Ondrej Zary wrote:
> On Saturday 04 September 2010 00:14:54 Simon Arlott wrote:
>> On 03/09/10 22:17, Ondrej Zary wrote:
>> > +	/* 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.

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

>> >
>> > +#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.

>> > +	/* 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.

-- 
Simon Arlott

[PATCH] cxacru: ignore cx82310_eth devices

[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 <linux@rainbow-software.org>
[SA: Moved to cxacru_usb_probe, message level lowered to info]
Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 593fc5e..96fa736 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -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);
 }
 
-- 
1.7.0.4

-- 
Simon Arlott

Re: [PATCH] cxacru: ignore cx82310_eth devices

[Greg KH]

On Sat, Sep 04, 2010 at 05:30:20PM +0100, Simon Arlott wrote:
> 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 <linux@rainbow-software.org>
> [SA: Moved to cxacru_usb_probe, message level lowered to info]
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
>  drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
> index 593fc5e..96fa736 100644
> --- a/drivers/usb/atm/cxacru.c
> +++ b/drivers/usb/atm/cxacru.c
> @@ -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

vendor class?  We have a macro for that?

> +			&& usb_dev->descriptor.iProduct

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

> +			&& usb_string(usb_dev,
> +				usb_dev->descriptor.iProduct, buf, sizeof(buf))

usb_string() should handle a 0 string, right?

> +			&& !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:

> +		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

Re: [PATCH] cxacru: ignore cx82310_eth devices

[Ondrej Zary]

On Sunday 05 September 2010 06:03:52 Greg KH wrote:
> On Sat, Sep 04, 2010 at 05:30:20PM +0100, Simon Arlott wrote:
> > 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 <linux@rainbow-software.org>
> > [SA: Moved to cxacru_usb_probe, message level lowered to info]
> > Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> > ---
> >  drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
> >  1 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
> > index 593fc5e..96fa736 100644
> > --- a/drivers/usb/atm/cxacru.c
> > +++ b/drivers/usb/atm/cxacru.c
> > @@ -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
>
> vendor class?  We have a macro for that?
>
> > +			&& usb_dev->descriptor.iProduct
>
> Almost everyone has a iProduct, right?  Why even test for that?
>
> > +			&& usb_string(usb_dev,
> > +				usb_dev->descriptor.iProduct, buf, sizeof(buf))
>
> usb_string() should handle a 0 string, right?
>
> > +			&& !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:
> > +		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.

> Well, actually, dev_info?  I guess so, but do you really want to see
> this message every time?
>
> thanks,
>
> greg k-h



-- 
Ondrej Zary

Re: [PATCH] cxacru: ignore cx82310_eth devices

[Greg KH]

On Sun, Sep 05, 2010 at 07:01:11PM +0200, Ondrej Zary wrote:
> On Sunday 05 September 2010 06:03:52 Greg KH wrote:
> > On Sat, Sep 04, 2010 at 05:30:20PM +0100, Simon Arlott wrote:
> > > 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 <linux@rainbow-software.org>
> > > [SA: Moved to cxacru_usb_probe, message level lowered to info]
> > > Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> > > ---
> > >  drivers/usb/atm/cxacru.c |   18 ++++++++++++++++--
> > >  1 files changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
> > > index 593fc5e..96fa736 100644
> > > --- a/drivers/usb/atm/cxacru.c
> > > +++ b/drivers/usb/atm/cxacru.c
> > > @@ -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
> >
> > vendor class?  We have a macro for that?
> >
> > > +			&& usb_dev->descriptor.iProduct
> >
> > Almost everyone has a iProduct, right?  Why even test for that?
> >
> > > +			&& usb_string(usb_dev,
> > > +				usb_dev->descriptor.iProduct, buf, sizeof(buf))
> >
> > usb_string() should handle a 0 string, right?
> >
> > > +			&& !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:
> > > +		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;
> 		}

Yes, don't you think so?

> 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

[PATCH] cxacru: ignore cx82310_eth devices

[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 <linux@rainbow-software.org>

--- 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,
 };
 
-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);
 }
 


-- 
Ondrej Zary

Re: [PATCH] cxacru: ignore cx82310_eth devices

[Greg KH]

On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote:
> 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 <linux@rainbow-software.org>
> 
> --- 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,
>  };
>  
> -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.

> +	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

Re: [PATCH] cxacru: ignore cx82310_eth devices

[Simon Arlott]

On Sun, September 5, 2010 22:04, Greg KH wrote:
> On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote:
>> 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 <linux@rainbow-software.org>

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

>> --- 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,
>>  };
>>
>> -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.

>> +	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?

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.

-- 
Simon Arlott

Re: [PATCH] cxacru: ignore cx82310_eth devices

[Ondrej Zary]

On Monday 06 September 2010, Simon Arlott wrote:
> On Sun, September 5, 2010 22:04, Greg KH wrote:
> > On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote:
> >> 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 <linux@rainbow-software.org>
>
> Missing Signed-off-by: you're modifying my changes.

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

> >> --- 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,
> >>  };
> >>
> >> -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.
>
> >> +	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?
>
> 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.

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

-- 
Ondrej Zary

Re: [PATCH v2] [RFC] introduce cx82310_eth: Conexant CX82310-based ADSL router USB ethernet driver

[David Miller]

From: Ondrej Zary <linux@rainbow-software.org>
Date: Sat, 4 Sep 2010 14:39:34 +0200

> 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 <linux@rainbow-software.org>

Applied, thanks.

Re: [PATCH] cxacru: ignore cx82310_eth devices

[David Miller]

Please submit a fixed up version of this patch based upon the
feedback given.

Thanks.

[PATCH v3] cxacru: ignore cx82310_eth devices

[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 <linux@rainbow-software.org>
Signed-off-by: Simon Arlott <simon@fire.lp0.eu>

--- 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-08 22:42:47.000000000 +0200
@@ -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);
 }
 

-- 
Ondrej Zary

Re: [PATCH] cxacru: ignore cx82310_eth devices

[Ondrej Zary]

On Sunday 05 September 2010 23:04:46 Greg KH wrote:
> On Sun, Sep 05, 2010 at 10:12:33PM +0200, Ondrej Zary wrote:
> > 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 <linux@rainbow-software.org>
> >
> > --- 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,
> >  };
> >
> > -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?

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

-- 
Ondrej Zary

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

[Greg KH]

On Wed, Sep 08, 2010 at 10:52:12PM +0200, Ondrej Zary wrote:
> 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

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

[David Miller]

From: Ondrej Zary <linux@rainbow-software.org>
Date: Wed, 8 Sep 2010 22:52:12 +0200

> 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 <linux@rainbow-software.org>
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>

Applied with some minor comment formatting fixes, thanks!

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

[David Miller]

From: Greg KH <greg@kroah.com>
Date: Wed, 8 Sep 2010 20:35:57 -0700

> On Wed, Sep 08, 2010 at 10:52:12PM +0200, Ondrej Zary wrote:
>> 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

Re: [PATCH v3] cxacru: ignore cx82310_eth devices

[Greg KH]

On Wed, Sep 08, 2010 at 11:07:47PM -0700, David Miller wrote:
> From: Greg KH <greg@kroah.com>
> Date: Wed, 8 Sep 2010 20:35:57 -0700
> 
> > On Wed, Sep 08, 2010 at 10:52:12PM +0200, Ondrej Zary wrote:
> >> 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

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

greg k-h