All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] can: Add driver for esd CAN-USB/2 device
@ 2010-05-26  9:14 Matthias Fuchs
       [not found] ` <201005261114.03214.matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
  2010-05-26 13:47 ` Oliver Neukum
  0 siblings, 2 replies; 9+ messages in thread
From: Matthias Fuchs @ 2010-05-26  9:14 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w, linux-usb-u79uwXL29TY76Z2rM5mHXA

This patch adds a driver for esd's USB high speed
CAN interface. The driver supports devices with
multiple CAN interfaces.

Signed-off-by: Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
---
version 3: 
 - remove bus-error reporting feature because 
   it cannot be controlled by user on demand
   with current device's firmware
 - rebased against current net-next-2.6 tree

version 2: 
 - use bus-error reporting and counters
 - minor cleanup
 - rebased against current net-next-2.6 tree
 - initial post to linux-usb list for review

 drivers/net/can/usb/Kconfig    |    6 +
 drivers/net/can/usb/Makefile   |    1 +
 drivers/net/can/usb/esd_usb2.c | 1125 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1132 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/usb/esd_usb2.c

diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index 97ff6fe..0452549 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -7,4 +7,10 @@ config CAN_EMS_USB
 	  This driver is for the one channel CPC-USB/ARM7 CAN/USB interface
 	  from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de).
 
+config CAN_ESD_USB2
+        tristate "ESD USB/2 CAN/USB interface"
+        ---help---
+          This driver supports the CAN-USB/2 interface
+          from esd electronic system design gmbh (http://www.esd.eu).
+
 endmenu
diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
index 0afd51d..fce3cf1 100644
--- a/drivers/net/can/usb/Makefile
+++ b/drivers/net/can/usb/Makefile
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
+obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
new file mode 100644
index 0000000..ff1a5ee
--- /dev/null
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -0,0 +1,1125 @@
+/*
+ * CAN driver for esd CAN-USB/2
+ *
+ * Copyright (C) 2010 Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>, esd gmbh
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#include <linux/init.h>
+#include <linux/signal.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/usb.h>
+
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+
+MODULE_AUTHOR("Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>");
+MODULE_DESCRIPTION("CAN driver for esd CAN-USB/2 interfaces");
+MODULE_LICENSE("GPL v2");
+
+/* Define these values to match your devices */
+#define USB_ESDGMBH_VENDOR_ID	0x0ab4
+#define USB_CANUSB2_PRODUCT_ID	0x0010
+
+#define ESD_USB2_CAN_CLOCK	60000000
+#define ESD_USB2_MAX_NETS	2
+
+/* USB2 commands */
+#define CMD_VERSION		1 /* also used for VERSION_REPLY */
+#define CMD_CAN_RX		2 /* device to host only */
+#define CMD_CAN_TX		3 /* also used for TX_DONE */
+#define CMD_SETBAUD		4 /* also used for SETBAUD_REPLY */
+#define CMD_TS			5 /* also used for TS_REPLY */
+#define CMD_IDADD		6 /* also used for IDADD_REPLY */
+
+/* esd CAN message flags - dlc field */
+#define ESD_RTR			0x10
+
+/* esd CAN message flags - id field */
+#define ESD_EXTID		0x20000000
+#define ESD_EVENT		0x40000000
+#define ESD_IDMASK		0x1fffffff
+
+/* esd CAN event ids used by this driver */
+#define ESD_EV_CAN_ERROR_EXT	2
+
+/* baudrate message flags */
+#define ESD_USB2_UBR		0x80000000
+#define ESD_USB2_LOM		0x40000000
+#define ESD_USB2_NO_BAUDRATE	0x7fffffff
+#define ESD_USB2_TSEG1_MIN	1
+#define ESD_USB2_TSEG1_MAX	16
+#define ESD_USB2_TSEG1_SHIFT	16
+#define ESD_USB2_TSEG2_MIN	1
+#define ESD_USB2_TSEG2_MAX	8
+#define ESD_USB2_TSEG2_SHIFT	20
+#define ESD_USB2_SJW_MAX	4
+#define ESD_USB2_SJW_SHIFT	14
+#define ESD_USB2_BRP_MIN	1
+#define ESD_USB2_BRP_MAX	1024
+#define ESD_USB2_BRP_INC	1
+#define ESD_USB2_3_SAMPLES	0x00800000
+
+/* esd IDADD message */
+#define ESD_ID_ENABLE		0x80
+#define ESD_MAX_ID_SEGMENT	64
+
+/* SJA1000 ECC register (emulated by usb2 firmware) */
+#define SJA1000_ECC_SEG		0x1F
+#define SJA1000_ECC_DIR		0x20
+#define SJA1000_ECC_ERR		0x06
+#define SJA1000_ECC_BIT		0x00
+#define SJA1000_ECC_FORM	0x40
+#define SJA1000_ECC_STUFF	0x80
+#define SJA1000_ECC_MASK	0xc0
+
+/* esd bus state event codes */
+#define ESD_BUSSTATE_MASK	0xc0
+#define ESD_BUSSTATE_WARN	0x40
+#define ESD_BUSSTATE_ERRPASSIVE	0x80
+#define ESD_BUSSTATE_BUSOFF	0xc0
+
+#define RX_BUFFER_SIZE		1024
+#define MAX_RX_URBS		4
+#define MAX_TX_URBS		16 /* must be power of 2 */
+
+struct header_msg {
+	u8 len; /* len is always the total message length in 32bit words */
+	u8 cmd;
+	u8 rsvd[2];
+};
+
+struct version_msg {
+	u8 len;
+	u8 cmd;
+	u8 rsvd;
+	u8 flags;
+	__le32 drv_version;
+};
+
+struct version_reply_msg {
+	u8 len;
+	u8 cmd;
+	u8 nets;
+	u8 features;
+	__le32 version;
+	u8 name[16];
+	__le32 rsvd;
+	__le32 ts;
+};
+
+struct rx_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 dlc;
+	__le32 ts;
+	__le32 id; /* upper 3 bits contain flags */
+	u8 data[8];
+};
+
+struct tx_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 dlc;
+	__le32 hnd;
+	__le32 id; /* upper 3 bits contain flags */
+	u8 data[8];
+};
+
+struct tx_done_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 status;
+	__le32 hnd;
+	__le32 ts;
+};
+
+struct id_filter_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 option;
+	__le32 mask[ESD_MAX_ID_SEGMENT + 1];
+};
+
+struct set_baudrate_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 rsvd;
+	__le32 baud;
+};
+
+/* Main message type used between library and application */
+struct __attribute__ ((packed)) esd_usb2_msg {
+	union {
+		struct header_msg hdr;
+		struct version_msg version;
+		struct version_reply_msg version_reply;
+		struct rx_msg rx;
+		struct tx_msg tx;
+		struct tx_done_msg txdone;
+		struct set_baudrate_msg setbaud;
+		struct id_filter_msg filter;
+	} msg;
+};
+
+static struct usb_device_id esd_usb2_table[] = {
+	{USB_DEVICE(USB_ESDGMBH_VENDOR_ID, USB_CANUSB2_PRODUCT_ID)},
+	{}
+};
+MODULE_DEVICE_TABLE(usb, esd_usb2_table);
+
+struct esd_usb2_net_priv;
+
+struct esd_tx_urb_context {
+	struct esd_usb2_net_priv *priv;
+	u32 echo_index;
+	int dlc;
+};
+
+struct esd_usb2 {
+	struct usb_device *udev;
+	struct esd_usb2_net_priv *nets[ESD_USB2_MAX_NETS];
+
+	struct usb_anchor rx_submitted;
+
+	int net_count;
+	u32 version;
+	int rxinitdone;
+};
+
+struct esd_usb2_net_priv {
+	struct can_priv can; /* must be the first member */
+
+	atomic_t active_tx_jobs;
+	struct usb_anchor tx_submitted;
+	struct esd_tx_urb_context tx_contexts[MAX_TX_URBS];
+
+	int open_time;
+	struct esd_usb2 *usb2;
+	struct net_device *netdev;
+	int index;
+	u8 old_state;
+	struct can_berr_counter bec;
+};
+
+static void esd_usb2_rx_event(struct esd_usb2_net_priv *priv,
+			      struct esd_usb2_msg *msg)
+{
+	struct net_device_stats *stats = &priv->netdev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
+
+	if (id == ESD_EV_CAN_ERROR_EXT) {
+		u8 state = msg->msg.rx.data[0];
+		u8 ecc = msg->msg.rx.data[1];
+		u8 txerr = msg->msg.rx.data[2];
+		u8 rxerr = msg->msg.rx.data[3];
+
+		skb = alloc_can_err_skb(priv->netdev, &cf);
+		if (skb == NULL) {
+			stats->rx_dropped++;
+			return;
+		}
+
+		if (state != priv->old_state) {
+			priv->old_state = state;
+
+			switch (state & ESD_BUSSTATE_MASK) {
+			case ESD_BUSSTATE_BUSOFF:
+				priv->can.state = CAN_STATE_BUS_OFF;
+				cf->can_id |= CAN_ERR_BUSOFF;
+				can_bus_off(priv->netdev);
+				break;
+			case ESD_BUSSTATE_WARN:
+				priv->can.state = CAN_STATE_ERROR_WARNING;
+				priv->can.can_stats.error_warning++;
+				break;
+			case ESD_BUSSTATE_ERRPASSIVE:
+				priv->can.state = CAN_STATE_ERROR_PASSIVE;
+				priv->can.can_stats.error_passive++;
+				break;
+			default:
+				priv->can.state = CAN_STATE_ERROR_ACTIVE;
+				break;
+			}
+		} else {
+			priv->can.can_stats.bus_error++;
+			stats->rx_errors++;
+
+			cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+			switch (ecc & SJA1000_ECC_MASK) {
+			case SJA1000_ECC_BIT:
+				cf->data[2] |= CAN_ERR_PROT_BIT;
+				break;
+			case SJA1000_ECC_FORM:
+				cf->data[2] |= CAN_ERR_PROT_FORM;
+				break;
+			case SJA1000_ECC_STUFF:
+				cf->data[2] |= CAN_ERR_PROT_STUFF;
+				break;
+			default:
+				cf->data[2] |= CAN_ERR_PROT_UNSPEC;
+				cf->data[3] = ecc & SJA1000_ECC_SEG;
+				break;
+			}
+
+			/* Error occured during transmission? */
+			if (!(ecc & SJA1000_ECC_DIR))
+				cf->data[2] |= CAN_ERR_PROT_TX;
+
+			if (priv->can.state == CAN_STATE_ERROR_WARNING ||
+			    priv->can.state == CAN_STATE_ERROR_PASSIVE) {
+				cf->data[1] = (txerr > rxerr) ?
+					CAN_ERR_CRTL_TX_PASSIVE :
+					CAN_ERR_CRTL_RX_PASSIVE;
+			}
+			cf->data[6] = txerr;
+			cf->data[7] = rxerr;
+		}
+
+		netif_rx(skb);
+
+		priv->bec.txerr = txerr;
+		priv->bec.rxerr = rxerr;
+
+		stats->rx_packets++;
+		stats->rx_bytes += cf->can_dlc;
+	}
+}
+
+static void esd_usb2_rx_can_msg(struct esd_usb2_net_priv *priv,
+				struct esd_usb2_msg *msg)
+{
+	struct net_device_stats *stats = &priv->netdev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	int i;
+	u32 id;
+
+	if (!netif_device_present(priv->netdev))
+		return;
+
+	id = le32_to_cpu(msg->msg.rx.id);
+
+	if (id & ESD_EVENT) {
+		esd_usb2_rx_event(priv, msg);
+	} else {
+		skb = alloc_can_skb(priv->netdev, &cf);
+		if (skb == NULL) {
+			stats->rx_dropped++;
+			return;
+		}
+
+		cf->can_id = id & ESD_IDMASK;
+		cf->can_dlc = get_can_dlc(msg->msg.rx.dlc);
+
+		if (id & ESD_EXTID)
+			cf->can_id |= CAN_EFF_FLAG;
+
+		if (msg->msg.rx.dlc & ESD_RTR) {
+			cf->can_id |= CAN_RTR_FLAG;
+		} else {
+			for (i = 0; i < cf->can_dlc; i++)
+				cf->data[i] = msg->msg.rx.data[i];
+		}
+
+		netif_rx(skb);
+
+		stats->rx_packets++;
+		stats->rx_bytes += cf->can_dlc;
+	}
+
+	return;
+}
+
+static void esd_usb2_tx_done_msg(struct esd_usb2_net_priv *priv,
+				 struct esd_usb2_msg *msg)
+{
+	struct net_device_stats *stats = &priv->netdev->stats;
+	struct net_device *netdev = priv->netdev;
+	struct esd_tx_urb_context *context;
+
+	if (!netif_device_present(netdev))
+		return;
+
+	context = &priv->tx_contexts[msg->msg.txdone.hnd & (MAX_TX_URBS - 1)];
+
+	if (!msg->msg.txdone.status) {
+		stats->tx_packets++;
+		stats->tx_bytes += context->dlc;
+		can_get_echo_skb(netdev, context->echo_index);
+	} else {
+		stats->tx_errors++;
+		can_free_echo_skb(netdev, context->echo_index);
+	}
+
+	/* Release context */
+	context->echo_index = MAX_TX_URBS;
+	atomic_dec(&priv->active_tx_jobs);
+
+	netif_wake_queue(netdev);
+}
+
+static void esd_usb2_read_bulk_callback(struct urb *urb)
+{
+	struct esd_usb2 *dev = urb->context;
+	int retval;
+	int pos = 0;
+	int i;
+
+	switch (urb->status) {
+	case 0: /* success */
+		break;
+
+	case -ENOENT:
+	case -ESHUTDOWN:
+		return;
+
+	default:
+		dev_info(dev->udev->dev.parent,
+			 "Rx URB aborted (%d)\n", urb->status);
+		goto resubmit_urb;
+	}
+
+	while (pos < urb->actual_length) {
+		struct esd_usb2_msg *msg;
+
+		msg = (struct esd_usb2_msg *)(urb->transfer_buffer + pos);
+
+		switch (msg->msg.hdr.cmd) {
+		case CMD_CAN_RX:
+			esd_usb2_rx_can_msg(dev->nets[msg->msg.rx.net], msg);
+			break;
+
+		case CMD_CAN_TX:
+			esd_usb2_tx_done_msg(dev->nets[msg->msg.txdone.net],
+					     msg);
+			break;
+		}
+
+		pos += msg->msg.hdr.len << 2;
+
+		if (pos > urb->actual_length) {
+			dev_err(dev->udev->dev.parent, "format error\n");
+			break;
+		}
+	}
+
+resubmit_urb:
+	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
+			  urb->transfer_buffer, RX_BUFFER_SIZE,
+			  esd_usb2_read_bulk_callback, dev);
+
+	retval = usb_submit_urb(urb, GFP_ATOMIC);
+	if (retval == -ENODEV) {
+		for (i = 0; i < dev->net_count; i++) {
+			if (dev->nets[i])
+				netif_device_detach(dev->nets[i]->netdev);
+		}
+	} else if (retval) {
+		dev_err(dev->udev->dev.parent,
+			"failed resubmitting read bulk urb: %d\n", retval);
+	}
+
+	return;
+}
+
+/*
+ * callback for bulk IN urb
+ */
+static void esd_usb2_write_bulk_callback(struct urb *urb)
+{
+	struct esd_tx_urb_context *context = urb->context;
+	struct esd_usb2_net_priv *priv;
+	struct esd_usb2 *dev;
+	struct net_device *netdev;
+	size_t size = sizeof(struct esd_usb2_msg);
+
+	BUG_ON(!context);
+
+	priv = context->priv;
+	netdev = priv->netdev;
+	dev = priv->usb2;
+
+	/* free up our allocated buffer */
+	usb_buffer_free(urb->dev, size,
+			urb->transfer_buffer, urb->transfer_dma);
+
+	if (!netif_device_present(netdev))
+		return;
+
+	if (urb->status)
+		dev_info(netdev->dev.parent, "Tx URB aborted (%d)\n",
+			 urb->status);
+
+	netdev->trans_start = jiffies;
+}
+
+#ifdef CONFIG_SYSFS
+static ssize_t show_firmware(struct device *d,
+			     struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(d);
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+
+	return sprintf(buf, "%d.%d.%d\n",
+		       (dev->version >> 12) & 0xf,
+		       (dev->version >> 8) & 0xf,
+		       dev->version & 0xff);
+}
+static DEVICE_ATTR(firmware, S_IRUGO, show_firmware, NULL);
+
+static ssize_t show_hardware(struct device *d,
+			     struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(d);
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+
+	return sprintf(buf, "%d.%d.%d\n",
+		       (dev->version >> 28) & 0xf,
+		       (dev->version >> 24) & 0xf,
+		       (dev->version >> 16) & 0xff);
+}
+static DEVICE_ATTR(hardware, S_IRUGO, show_hardware, NULL);
+
+static ssize_t show_nets(struct device *d,
+			 struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(d);
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+
+	return sprintf(buf, "%d", dev->net_count);
+}
+static DEVICE_ATTR(nets, S_IRUGO, show_nets, NULL);
+#endif
+
+static int esd_usb2_send_msg(struct esd_usb2 *dev, struct esd_usb2_msg *msg)
+{
+	int actual_length;
+
+	return usb_bulk_msg(dev->udev,
+			    usb_sndbulkpipe(dev->udev, 2),
+			    msg,
+			    msg->msg.hdr.len << 2,
+			    &actual_length,
+			    1000);
+}
+
+static int esd_usb2_wait_msg(struct esd_usb2 *dev,
+			     struct esd_usb2_msg *msg)
+{
+	int actual_length;
+
+	return usb_bulk_msg(dev->udev,
+			    usb_rcvbulkpipe(dev->udev, 1),
+			    msg,
+			    sizeof(*msg),
+			    &actual_length,
+			    1000);
+}
+
+static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev)
+{
+	int i, err = 0;
+
+	if (dev->rxinitdone)
+		return 0;
+
+	for (i = 0; i < MAX_RX_URBS; i++) {
+		struct urb *urb = NULL;
+		u8 *buf = NULL;
+
+		/* create a URB, and a buffer for it */
+		urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!urb) {
+			dev_warn(dev->udev->dev.parent,
+				 "No memory left for URBs\n");
+			err = -ENOMEM;
+			break;
+		}
+
+		buf = usb_buffer_alloc(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
+				       &urb->transfer_dma);
+		if (!buf) {
+			dev_warn(dev->udev->dev.parent,
+				 "No memory left for USB buffer\n");
+			err = -ENOMEM;
+			goto freeurb;
+		}
+
+		usb_fill_bulk_urb(urb, dev->udev,
+				  usb_rcvbulkpipe(dev->udev, 1),
+				  buf, RX_BUFFER_SIZE,
+				  esd_usb2_read_bulk_callback, dev);
+		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+		usb_anchor_urb(urb, &dev->rx_submitted);
+
+		err = usb_submit_urb(urb, GFP_KERNEL);
+		if (err) {
+			usb_unanchor_urb(urb);
+			usb_buffer_free(dev->udev, RX_BUFFER_SIZE, buf,
+					urb->transfer_dma);
+		}
+
+freeurb:
+		/* Drop reference, USB core will take care of freeing it */
+		usb_free_urb(urb);
+		if (err)
+			break;
+	}
+
+	/* Did we submit any URBs */
+	if (i == 0) {
+		dev_err(dev->udev->dev.parent, "couldn't setup read URBs\n");
+		return err;
+	}
+
+	/* Warn if we've couldn't transmit all the URBs */
+	if (i < MAX_RX_URBS) {
+		dev_warn(dev->udev->dev.parent,
+			 "rx performance may be slow\n");
+	}
+
+	dev->rxinitdone = 1;
+	return 0;
+}
+
+/*
+ * Start interface
+ */
+static int esd_usb2_start(struct esd_usb2_net_priv *priv)
+{
+	struct esd_usb2 *dev = priv->usb2;
+	struct net_device *netdev = priv->netdev;
+	struct esd_usb2_msg msg;
+	int err, i;
+
+	/*
+	 * Enable all IDs
+	 * The IDADD message takes up to 64 32 bit bitmasks (2048 bits).
+	 * Each bit represents one 11 bit CAN identifier. A set bit
+	 * enables reception of the corresponding CAN identifier. A cleared
+	 * bit disabled this identifier. An additional bitmask value
+	 * following the CAN 2.0A bits is used to enable reception of
+	 * extended CAN frames. Only the LSB of this final mask is checked
+	 * for the complete 29 bit ID range. The IDADD message also allows
+	 * filter configuration for an ID subset. In this case you can add
+	 * the number of the starting bitmask (0..64) to the filter.option
+	 * field followed by only some bitmasks.
+	 */
+	msg.msg.hdr.cmd = CMD_IDADD;
+	msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+	msg.msg.filter.net = priv->index;
+	msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
+	for (i = 0; i < ESD_MAX_ID_SEGMENT; i++)
+		msg.msg.filter.mask[i] = cpu_to_le32(0xffffffff);
+	/* enable 29bit extended IDs */
+	msg.msg.filter.mask[ESD_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
+
+	err = esd_usb2_send_msg(dev, &msg);
+	if (err)
+		goto failed;
+
+	err = esd_usb2_setup_rx_urbs(dev);
+	if (err)
+		goto failed;
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	return 0;
+
+failed:
+	if (err == -ENODEV)
+		netif_device_detach(netdev);
+
+	dev_err(netdev->dev.parent, "couldn't start device: %d\n", err);
+
+	return err;
+}
+
+static void unlink_all_urbs(struct esd_usb2 *dev)
+{
+	struct esd_usb2_net_priv *priv;
+	int i;
+
+	usb_kill_anchored_urbs(&dev->rx_submitted);
+	for (i = 0; i < dev->net_count; i++) {
+		priv = dev->nets[i];
+		if (priv) {
+			usb_kill_anchored_urbs(&priv->tx_submitted);
+			atomic_set(&priv->active_tx_jobs, 0);
+
+			for (i = 0; i < MAX_TX_URBS; i++)
+				priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+		}
+	}
+}
+
+static int esd_usb2_open(struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	int err;
+
+	/* common open */
+	err = open_candev(netdev);
+	if (err)
+		return err;
+
+	/* finally start device */
+	err = esd_usb2_start(priv);
+	if (err) {
+		dev_warn(netdev->dev.parent,
+			 "couldn't start device: %d\n", err);
+		close_candev(netdev);
+		return err;
+	}
+
+	priv->open_time = jiffies;
+
+	netif_start_queue(netdev);
+
+	return 0;
+}
+
+static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb,
+				      struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	struct esd_usb2 *dev = priv->usb2;
+	struct esd_tx_urb_context *context = NULL;
+	struct net_device_stats *stats = &netdev->stats;
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct esd_usb2_msg *msg;
+	struct urb *urb;
+	u8 *buf;
+	int i, err;
+	int ret = NETDEV_TX_OK;
+	size_t size = sizeof(struct esd_usb2_msg);
+
+	if (can_dropped_invalid_skb(netdev, skb))
+		return NETDEV_TX_OK;
+
+	/* create a URB, and a buffer for it, and copy the data to the URB */
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		dev_err(netdev->dev.parent, "No memory left for URBs\n");
+		stats->tx_dropped++;
+		dev_kfree_skb(skb);
+		goto nourbmem;
+	}
+
+	buf = usb_buffer_alloc(dev->udev, size, GFP_ATOMIC, &urb->transfer_dma);
+	if (!buf) {
+		dev_err(netdev->dev.parent, "No memory left for USB buffer\n");
+		stats->tx_dropped++;
+		dev_kfree_skb(skb);
+		goto nobufmem;
+	}
+
+	msg = (struct esd_usb2_msg *)buf;
+
+	msg->msg.hdr.len = 3; /* minimal length */
+	msg->msg.hdr.cmd = CMD_CAN_TX;
+	msg->msg.tx.net = priv->index;
+	msg->msg.tx.dlc = cf->can_dlc;
+	msg->msg.tx.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
+
+	if (cf->can_id & CAN_RTR_FLAG)
+		msg->msg.tx.dlc |= ESD_RTR;
+
+	if (cf->can_id & CAN_EFF_FLAG)
+		msg->msg.tx.id |= cpu_to_le32(ESD_EXTID);
+
+	for (i = 0; i < cf->can_dlc; i++)
+		msg->msg.tx.data[i] = cf->data[i];
+
+	msg->msg.hdr.len += (cf->can_dlc + 3) >> 2;
+
+	for (i = 0; i < MAX_TX_URBS; i++) {
+		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
+			context = &priv->tx_contexts[i];
+			break;
+		}
+	}
+
+	/*
+	 * This may never happen.
+	 */
+	if (!context) {
+		dev_warn(netdev->dev.parent, "couldn't find free context\n");
+		ret = NETDEV_TX_BUSY;
+		goto releasebuf;
+	}
+
+	context->priv = priv;
+	context->echo_index = i;
+	context->dlc = cf->can_dlc;
+
+	/* hnd must not be 0 - MSB is stripped in txdone handling */
+	msg->msg.tx.hnd = 0x80000000 | i; /* returned in TX done message */
+
+	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
+			  msg->msg.hdr.len << 2,
+			  esd_usb2_write_bulk_callback, context);
+
+	urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+
+	usb_anchor_urb(urb, &priv->tx_submitted);
+
+	can_put_echo_skb(skb, netdev, context->echo_index);
+
+	atomic_inc(&priv->active_tx_jobs);
+
+	err = usb_submit_urb(urb, GFP_ATOMIC);
+	if (err) {
+		can_free_echo_skb(netdev, context->echo_index);
+
+		atomic_dec(&priv->active_tx_jobs);
+		usb_unanchor_urb(urb);
+
+		stats->tx_dropped++;
+
+		if (err == -ENODEV)
+			netif_device_detach(netdev);
+		else
+			dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
+
+		goto releasebuf;
+	}
+
+	netdev->trans_start = jiffies;
+
+	/* Slow down tx path */
+	if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
+		netif_stop_queue(netdev);
+
+	/*
+	 * Release our reference to this URB, the USB core will eventually free
+	 * it entirely.
+	 */
+	usb_free_urb(urb);
+
+	return NETDEV_TX_OK;
+
+releasebuf:
+	usb_buffer_free(dev->udev, size, buf, urb->transfer_dma);
+
+nobufmem:
+	usb_free_urb(urb);
+
+nourbmem:
+	return ret;
+}
+
+static int esd_usb2_close(struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	struct esd_usb2_msg msg;
+	int i;
+
+	/* Disable all IDs (see esd_usb2_start()) */
+	msg.msg.hdr.cmd = CMD_IDADD;
+	msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+	msg.msg.filter.net = priv->index;
+	msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
+	for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++)
+		msg.msg.filter.mask[i] = 0;
+	esd_usb2_send_msg(priv->usb2, &msg);
+
+	/* set CAN controller to reset mode */
+	msg.msg.hdr.len = 2;
+	msg.msg.hdr.cmd = CMD_SETBAUD;
+	msg.msg.setbaud.net = priv->index;
+	msg.msg.setbaud.rsvd = 0;
+	msg.msg.setbaud.baud = cpu_to_le32(ESD_USB2_NO_BAUDRATE);
+	esd_usb2_send_msg(priv->usb2, &msg);
+
+	priv->can.state = CAN_STATE_STOPPED;
+
+	netif_stop_queue(netdev);
+
+	close_candev(netdev);
+
+	priv->open_time = 0;
+
+	return 0;
+}
+
+static const struct net_device_ops esd_usb2_netdev_ops = {
+	.ndo_open = esd_usb2_open,
+	.ndo_stop = esd_usb2_close,
+	.ndo_start_xmit = esd_usb2_start_xmit,
+};
+
+static struct can_bittiming_const esd_usb2_bittiming_const = {
+	.name = "esd_usb2",
+	.tseg1_min = ESD_USB2_TSEG1_MIN,
+	.tseg1_max = ESD_USB2_TSEG1_MAX,
+	.tseg2_min = ESD_USB2_TSEG2_MIN,
+	.tseg2_max = ESD_USB2_TSEG2_MAX,
+	.sjw_max = ESD_USB2_SJW_MAX,
+	.brp_min = ESD_USB2_BRP_MIN,
+	.brp_max = ESD_USB2_BRP_MAX,
+	.brp_inc = ESD_USB2_BRP_INC,
+};
+
+static int esd_usb2_set_bittiming(struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	struct esd_usb2_msg msg;
+	u32 canbtr;
+
+	canbtr = ESD_USB2_UBR;
+	canbtr |= (bt->brp - 1) & (ESD_USB2_BRP_MAX - 1);
+	canbtr |= ((bt->sjw - 1) & (ESD_USB2_SJW_MAX - 1))
+		<< ESD_USB2_SJW_SHIFT;
+	canbtr |= ((bt->prop_seg + bt->phase_seg1 - 1)
+		   & (ESD_USB2_TSEG1_MAX - 1))
+		<< ESD_USB2_TSEG1_SHIFT;
+	canbtr |= ((bt->phase_seg2 - 1) & (ESD_USB2_TSEG2_MAX - 1))
+		<< ESD_USB2_TSEG2_SHIFT;
+	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		canbtr |= ESD_USB2_3_SAMPLES;
+
+	msg.msg.hdr.len = 2;
+	msg.msg.hdr.cmd = CMD_SETBAUD;
+	msg.msg.setbaud.net = priv->index;
+	msg.msg.setbaud.rsvd = 0;
+	msg.msg.setbaud.baud = cpu_to_le32(canbtr);
+
+	dev_info(netdev->dev.parent, "setting BTR=%#x\n", canbtr);
+
+	return esd_usb2_send_msg(priv->usb2, &msg);
+}
+
+static int esd_usb2_get_berr_counter(const struct net_device *netdev,
+				     struct can_berr_counter *bec)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+
+	bec->txerr = priv->bec.txerr;
+	bec->rxerr = priv->bec.rxerr;
+
+	return 0;
+}
+
+static int esd_usb2_set_mode(struct net_device *netdev, enum can_mode mode)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+
+	if (!priv->open_time)
+		return -EINVAL;
+
+	switch (mode) {
+	case CAN_MODE_START:
+		netif_wake_queue(netdev);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int esd_usb2_probe_one_net(struct usb_interface *intf, int index)
+{
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+	struct net_device *netdev;
+	struct esd_usb2_net_priv *priv;
+	int err;
+	int i;
+
+	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	if (!netdev) {
+		dev_err(&intf->dev, "couldn't alloc candev\n");
+		return -ENOMEM;
+	}
+
+	priv = netdev_priv(netdev);
+
+	init_usb_anchor(&priv->tx_submitted);
+	atomic_set(&priv->active_tx_jobs, 0);
+
+	for (i = 0; i < MAX_TX_URBS; i++)
+		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+
+	priv->usb2 = dev;
+	priv->netdev = netdev;
+	priv->index = index;
+
+	priv->can.state = CAN_STATE_STOPPED;
+	priv->can.clock.freq = ESD_USB2_CAN_CLOCK;
+	priv->can.bittiming_const = &esd_usb2_bittiming_const;
+	priv->can.do_set_bittiming = esd_usb2_set_bittiming;
+	priv->can.do_set_mode = esd_usb2_set_mode;
+	priv->can.do_get_berr_counter = esd_usb2_get_berr_counter;
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
+
+	netdev->flags |= IFF_ECHO; /* we support local echo */
+
+	netdev->netdev_ops = &esd_usb2_netdev_ops;
+
+	SET_NETDEV_DEV(netdev, &intf->dev);
+
+	err = register_candev(netdev);
+	if (err) {
+		dev_err(&intf->dev,
+			"couldn't register CAN device: %d\n", err);
+		free_candev(netdev);
+		return -ENOMEM;
+	}
+
+	dev->nets[index] = priv;
+	dev_info(netdev->dev.parent, "device %s registered\n", netdev->name);
+	return 0;
+}
+
+/*
+ * probe function for new USB2 devices
+ *
+ * check version information and number of available
+ * CAN interfaces
+ */
+static int esd_usb2_probe(struct usb_interface *intf,
+			 const struct usb_device_id *id)
+{
+	struct esd_usb2 *dev;
+	struct esd_usb2_msg msg;
+	int i, err = -ENOMEM;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->udev = interface_to_usbdev(intf);
+
+	init_usb_anchor(&dev->rx_submitted);
+
+	usb_set_intfdata(intf, dev);
+
+	/* query number of CAN interfaces (nets) */
+	msg.msg.hdr.cmd = CMD_VERSION;
+	msg.msg.hdr.len = 2;
+	msg.msg.version.rsvd = 0;
+	msg.msg.version.flags = 0;
+	msg.msg.version.drv_version = 0;
+
+	if (esd_usb2_send_msg(dev, &msg) < 0) {
+		dev_err(&intf->dev, "sending version message failed\n");
+		goto free_dev;
+	}
+
+	if (esd_usb2_wait_msg(dev, &msg) < 0) {
+		dev_err(&intf->dev, "no version message answer\n");
+		goto free_dev;
+	}
+
+	dev->net_count = (int)msg.msg.version_reply.nets;
+	dev->version = le32_to_cpu(msg.msg.version_reply.version);
+
+#ifdef CONFIG_SYSFS
+	if (device_create_file(&intf->dev, &dev_attr_firmware))
+		dev_err(&intf->dev,
+			"Couldn't create device file for firmware\n");
+
+	if (device_create_file(&intf->dev, &dev_attr_hardware))
+		dev_err(&intf->dev,
+			"Couldn't create device file for hardware\n");
+
+	if (device_create_file(&intf->dev, &dev_attr_nets))
+		dev_err(&intf->dev,
+			"Couldn't create device file for nets\n");
+#endif
+
+	/* do per device probing */
+	for (i = 0; i < dev->net_count; i++)
+		esd_usb2_probe_one_net(intf, i);
+
+	return 0;
+
+free_dev:
+	kfree(dev);
+	return err;
+}
+
+/*
+ * called by the usb core when the device is removed from the system
+ */
+static void esd_usb2_disconnect(struct usb_interface *intf)
+{
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+	struct net_device *netdev;
+	int i;
+
+#ifdef CONFIG_SYSFS
+	device_remove_file(&intf->dev, &dev_attr_firmware);
+	device_remove_file(&intf->dev, &dev_attr_hardware);
+	device_remove_file(&intf->dev, &dev_attr_nets);
+#endif
+	usb_set_intfdata(intf, NULL);
+
+	if (dev) {
+		for (i = 0; i < dev->net_count; i++) {
+			if (dev->nets[i]) {
+				netdev = dev->nets[i]->netdev;
+				unregister_netdev(netdev);
+				free_candev(netdev);
+			}
+		}
+		unlink_all_urbs(dev);
+	}
+}
+
+/* usb specific object needed to register this driver with the usb subsystem */
+static struct usb_driver esd_usb2_driver = {
+	.name = "esd_usb2",
+	.probe = esd_usb2_probe,
+	.disconnect = esd_usb2_disconnect,
+	.id_table = esd_usb2_table,
+};
+
+static int __init esd_usb2_init(void)
+{
+	int err;
+
+	/* register this driver with the USB subsystem */
+	err = usb_register(&esd_usb2_driver);
+
+	if (err) {
+		err("usb_register failed. Error number %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+module_init(esd_usb2_init);
+
+static void __exit esd_usb2_exit(void)
+{
+	/* deregister this driver with the USB subsystem */
+	usb_deregister(&esd_usb2_driver);
+}
+module_exit(esd_usb2_exit);
-- 
1.5.6.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
       [not found] ` <201005261114.03214.matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
@ 2010-05-26 10:15   ` Wolfgang Grandegger
       [not found]     ` <4BFCF4AD.6030000-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
  2010-05-26 11:31   ` Viral Mehta
  1 sibling, 1 reply; 9+ messages in thread
From: Wolfgang Grandegger @ 2010-05-26 10:15 UTC (permalink / raw)
  To: Matthias Fuchs
  Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA

Hi Matthias,

On 05/26/2010 11:14 AM, Matthias Fuchs wrote:
> This patch adds a driver for esd's USB high speed
> CAN interface. The driver supports devices with
> multiple CAN interfaces.
> 
> Signed-off-by: Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
> ---
> version 3: 
>  - remove bus-error reporting feature because 
>    it cannot be controlled by user on demand
>    with current device's firmware
>  - rebased against current net-next-2.6 tree
> 
> version 2: 
>  - use bus-error reporting and counters
>  - minor cleanup
>  - rebased against current net-next-2.6 tree
>  - initial post to linux-usb list for review

I get the following compiler warnings when compiling the kernel:

 CC      drivers/net/can/usb/esd_usb2.o
drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_write_bulk_callback':
drivers/net/can/usb/esd_usb2.c:466: error: implicit declaration of function 'usb_buffer_free'
drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_setup_rx_urbs':
drivers/net/can/usb/esd_usb2.c:562: error: implicit declaration of function 'usb_buffer_alloc'
drivers/net/can/usb/esd_usb2.c:563: warning: assignment makes pointer from integer without a cast
drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_start_xmit':
drivers/net/can/usb/esd_usb2.c:732: warning: assignment makes pointer from integer without a cast
make[4]: *** [drivers/net/can/usb/esd_usb2.o] Error 1

This is due to commit e26bcf37234c67624f62d9fc95f922b8dbda1363.
You need a similar fix like for ems_usb.c. Are you using a recent
version of the net-next-2.6 tree?

Wolfgang.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v3] can: Add driver for esd CAN-USB/2 device
       [not found] ` <201005261114.03214.matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
  2010-05-26 10:15   ` Wolfgang Grandegger
@ 2010-05-26 11:31   ` Viral Mehta
       [not found]     ` <70376CA23424B34D86F1C7DE6B9973430254343AD9-fVvhrSDAkVjuuPCCJ6VnObSn4PsL5ZDKvpI+GvaZM4lBDgjK7y7TUQ@public.gmane.org>
  1 sibling, 1 reply; 9+ messages in thread
From: Viral Mehta @ 2010-05-26 11:31 UTC (permalink / raw)
  To: Matthias Fuchs, netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w, linux-usb-u79uwXL29TY76Z2rM5mHXA

Hi,
>________________________________________
>From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Matthias Fuchs [matthias.fuchs-iOnpLzIbIdM@public.gmane.org]
>Sent: Wednesday, May 26, 2010 2:44 PM
>To: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: [PATCH v3] can: Add driver for esd CAN-USB/2 device
>
>This patch adds a driver for esd's USB high speed
>CAN interface. The driver supports devices with
>multiple CAN interfaces.
>
>Signed-off-by: Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
>---

>+static void esd_usb2_write_bulk_callback(struct urb *urb)
>+{
>+       struct esd_tx_urb_context *context = urb->context;
>+       struct esd_usb2_net_priv *priv;
>+       struct esd_usb2 *dev;
>+       struct net_device *netdev;
>+       size_t size = sizeof(struct esd_usb2_msg);
>+
>+       BUG_ON(!context);

It is preferred to used WARN_ON and avoid using BUG_ON and thus dont kill the whole system....
[...]
>+
>+       priv = context->priv;
>+       netdev = priv->netdev;
>+       dev = priv->usb2;
>+       err = usb_submit_urb(urb, GFP_ATOMIC);
>+       if (err) {
>+               can_free_echo_skb(netdev, context->echo_index);
>+
>+               atomic_dec(&priv->active_tx_jobs);
>+               usb_unanchor_urb(urb);
>+
>+               stats->tx_dropped++;
>+
>+               if (err == -ENODEV)
>+                       netif_device_detach(netdev);
>+               else
>+                       dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
>+
>+               goto releasebuf;

You probably want to set "ret" here or do you really want to return NETDEV_TX_OK
>+       }

[...]
>+static int esd_usb2_close(struct net_device *netdev)
>+{
>+       struct esd_usb2_net_priv *priv = netdev_priv(netdev);
>+       struct esd_usb2_msg msg;
>+       int i;
>+
>+       /* Disable all IDs (see esd_usb2_start()) */
>+       msg.msg.hdr.cmd = CMD_IDADD;
>+       msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
>+       msg.msg.filter.net = priv->index;
>+       msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
>+       for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++)
>+               msg.msg.filter.mask[i] = 0;
>+       esd_usb2_send_msg(priv->usb2, &msg);

Probably, you may not care but it can return error value....
>+
>+       /* set CAN controller to reset mode */
>+       msg.msg.hdr.len = 2;
>+       msg.msg.hdr.cmd = CMD_SETBAUD;
>+       msg.msg.setbaud.net = priv->index;
>+       msg.msg.setbaud.rsvd = 0;
>+       msg.msg.setbaud.baud = cpu_to_le32(ESD_USB2_NO_BAUDRATE);
>+       esd_usb2_send_msg(priv->usb2, &msg);

Probably, you may not care but it can return error value....

>+
>+       priv->can.state = CAN_STATE_STOPPED;
>+
>+       netif_stop_queue(netdev);
>+
>+       close_candev(netdev);
>+

[...]
>+static int esd_usb2_probe(struct usb_interface *intf,
>+                        const struct usb_device_id *id)
>+{
>+       struct esd_usb2 *dev;
>+       struct esd_usb2_msg msg;
>+       int i, err = -ENOMEM;
>+
>+       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>+       if (!dev)
>+               return -ENOMEM;

You probably would like to rewrite "return" statements.... for example, above you define err=-ENOMEM
and then here you are saying "return -ENOMEM" later on at the end you are saying "return err";

Also, people prefer and practice to have only one return point. Read further.

>+
>+       dev->udev = interface_to_usbdev(intf);
>+
>+       init_usb_anchor(&dev->rx_submitted);
>+
>+       usb_set_intfdata(intf, dev);
>+
>+       /* query number of CAN interfaces (nets) */
>+       msg.msg.hdr.cmd = CMD_VERSION;
>+       msg.msg.hdr.len = 2;
>+       msg.msg.version.rsvd = 0;
>+       msg.msg.version.flags = 0;
>+       msg.msg.version.drv_version = 0;
>+
>+       if (esd_usb2_send_msg(dev, &msg) < 0) {
>+               dev_err(&intf->dev, "sending version message failed\n");
>+               goto free_dev;

You might like to set "err" here, since you are not propogating error values properly.....
Irrespective of what esd_usb2_send_msg and thus in turn usb_bulk_msg returns,
you are ignoring those values and always returning -ENOMEM at the end......
>+       }
>+
>+       if (esd_usb2_wait_msg(dev, &msg) < 0) {
>+               dev_err(&intf->dev, "no version message answer\n");
>+               goto free_dev;

Same here.

>+free_dev:
>+       kfree(dev);
>+       return err;
>+}

HTH,
Viral Mehta

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
       [not found]     ` <4BFCF4AD.6030000-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
@ 2010-05-26 12:38       ` Matthias Fuchs
  0 siblings, 0 replies; 9+ messages in thread
From: Matthias Fuchs @ 2010-05-26 12:38 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
	Socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	linux-usb-u79uwXL29TY76Z2rM5mHXA

Hi Wolfgang,

oops. I did my tests againt latest net-next-2.6. But
I forgot to commit these missing changes. I attached 
a fixed version of patch v3. v4 will propably come when 
I finally checked Viral's comments.

Matthias

On Wednesday 26 May 2010 12:15, Wolfgang Grandegger wrote:
> Hi Matthias,
> 
> On 05/26/2010 11:14 AM, Matthias Fuchs wrote:
> > This patch adds a driver for esd's USB high speed
> > CAN interface. The driver supports devices with
> > multiple CAN interfaces.
> > 
> > Signed-off-by: Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
> > ---
> > version 3: 
> >  - remove bus-error reporting feature because 
> >    it cannot be controlled by user on demand
> >    with current device's firmware
> >  - rebased against current net-next-2.6 tree
> > 
> > version 2: 
> >  - use bus-error reporting and counters
> >  - minor cleanup
> >  - rebased against current net-next-2.6 tree
> >  - initial post to linux-usb list for review
> 
> I get the following compiler warnings when compiling the kernel:
> 
>  CC      drivers/net/can/usb/esd_usb2.o
> drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_write_bulk_callback':
> drivers/net/can/usb/esd_usb2.c:466: error: implicit declaration of function 'usb_buffer_free'
> drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_setup_rx_urbs':
> drivers/net/can/usb/esd_usb2.c:562: error: implicit declaration of function 'usb_buffer_alloc'
> drivers/net/can/usb/esd_usb2.c:563: warning: assignment makes pointer from integer without a cast
> drivers/net/can/usb/esd_usb2.c: In function 'esd_usb2_start_xmit':
> drivers/net/can/usb/esd_usb2.c:732: warning: assignment makes pointer from integer without a cast
> make[4]: *** [drivers/net/can/usb/esd_usb2.o] Error 1
> 
> This is due to commit e26bcf37234c67624f62d9fc95f922b8dbda1363.
> You need a similar fix like for ems_usb.c. Are you using a recent
> version of the net-next-2.6 tree?
> 
> Wolfgang.
> 
> 

---
 drivers/net/can/usb/Kconfig    |    6 +
 drivers/net/can/usb/Makefile   |    1 +
 drivers/net/can/usb/esd_usb2.c | 1126 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1133 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/usb/esd_usb2.c

diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index 97ff6fe..0452549 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -7,4 +7,10 @@ config CAN_EMS_USB
 	  This driver is for the one channel CPC-USB/ARM7 CAN/USB interface
 	  from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de).
 
+config CAN_ESD_USB2
+        tristate "ESD USB/2 CAN/USB interface"
+        ---help---
+          This driver supports the CAN-USB/2 interface
+          from esd electronic system design gmbh (http://www.esd.eu).
+
 endmenu
diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
index 0afd51d..fce3cf1 100644
--- a/drivers/net/can/usb/Makefile
+++ b/drivers/net/can/usb/Makefile
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
+obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
 
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
new file mode 100644
index 0000000..8f32638
--- /dev/null
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -0,0 +1,1126 @@
+/*
+ * CAN driver for esd CAN-USB/2
+ *
+ * Copyright (C) 2010 Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>, esd gmbh
+ *
+ * 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; version 2 of the License.
+ *
+ * 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.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#include <linux/init.h>
+#include <linux/signal.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/usb.h>
+
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+
+MODULE_AUTHOR("Matthias Fuchs <matthias.fuchs-iOnpLzIbIdM@public.gmane.org>");
+MODULE_DESCRIPTION("CAN driver for esd CAN-USB/2 interfaces");
+MODULE_LICENSE("GPL v2");
+
+/* Define these values to match your devices */
+#define USB_ESDGMBH_VENDOR_ID	0x0ab4
+#define USB_CANUSB2_PRODUCT_ID	0x0010
+
+#define ESD_USB2_CAN_CLOCK	60000000
+#define ESD_USB2_MAX_NETS	2
+
+/* USB2 commands */
+#define CMD_VERSION		1 /* also used for VERSION_REPLY */
+#define CMD_CAN_RX		2 /* device to host only */
+#define CMD_CAN_TX		3 /* also used for TX_DONE */
+#define CMD_SETBAUD		4 /* also used for SETBAUD_REPLY */
+#define CMD_TS			5 /* also used for TS_REPLY */
+#define CMD_IDADD		6 /* also used for IDADD_REPLY */
+
+/* esd CAN message flags - dlc field */
+#define ESD_RTR			0x10
+
+/* esd CAN message flags - id field */
+#define ESD_EXTID		0x20000000
+#define ESD_EVENT		0x40000000
+#define ESD_IDMASK		0x1fffffff
+
+/* esd CAN event ids used by this driver */
+#define ESD_EV_CAN_ERROR_EXT	2
+
+/* baudrate message flags */
+#define ESD_USB2_UBR		0x80000000
+#define ESD_USB2_LOM		0x40000000
+#define ESD_USB2_NO_BAUDRATE	0x7fffffff
+#define ESD_USB2_TSEG1_MIN	1
+#define ESD_USB2_TSEG1_MAX	16
+#define ESD_USB2_TSEG1_SHIFT	16
+#define ESD_USB2_TSEG2_MIN	1
+#define ESD_USB2_TSEG2_MAX	8
+#define ESD_USB2_TSEG2_SHIFT	20
+#define ESD_USB2_SJW_MAX	4
+#define ESD_USB2_SJW_SHIFT	14
+#define ESD_USB2_BRP_MIN	1
+#define ESD_USB2_BRP_MAX	1024
+#define ESD_USB2_BRP_INC	1
+#define ESD_USB2_3_SAMPLES	0x00800000
+
+/* esd IDADD message */
+#define ESD_ID_ENABLE		0x80
+#define ESD_MAX_ID_SEGMENT	64
+
+/* SJA1000 ECC register (emulated by usb2 firmware) */
+#define SJA1000_ECC_SEG		0x1F
+#define SJA1000_ECC_DIR		0x20
+#define SJA1000_ECC_ERR		0x06
+#define SJA1000_ECC_BIT		0x00
+#define SJA1000_ECC_FORM	0x40
+#define SJA1000_ECC_STUFF	0x80
+#define SJA1000_ECC_MASK	0xc0
+
+/* esd bus state event codes */
+#define ESD_BUSSTATE_MASK	0xc0
+#define ESD_BUSSTATE_WARN	0x40
+#define ESD_BUSSTATE_ERRPASSIVE	0x80
+#define ESD_BUSSTATE_BUSOFF	0xc0
+
+#define RX_BUFFER_SIZE		1024
+#define MAX_RX_URBS		4
+#define MAX_TX_URBS		16 /* must be power of 2 */
+
+struct header_msg {
+	u8 len; /* len is always the total message length in 32bit words */
+	u8 cmd;
+	u8 rsvd[2];
+};
+
+struct version_msg {
+	u8 len;
+	u8 cmd;
+	u8 rsvd;
+	u8 flags;
+	__le32 drv_version;
+};
+
+struct version_reply_msg {
+	u8 len;
+	u8 cmd;
+	u8 nets;
+	u8 features;
+	__le32 version;
+	u8 name[16];
+	__le32 rsvd;
+	__le32 ts;
+};
+
+struct rx_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 dlc;
+	__le32 ts;
+	__le32 id; /* upper 3 bits contain flags */
+	u8 data[8];
+};
+
+struct tx_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 dlc;
+	__le32 hnd;
+	__le32 id; /* upper 3 bits contain flags */
+	u8 data[8];
+};
+
+struct tx_done_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 status;
+	__le32 hnd;
+	__le32 ts;
+};
+
+struct id_filter_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 option;
+	__le32 mask[ESD_MAX_ID_SEGMENT + 1];
+};
+
+struct set_baudrate_msg {
+	u8 len;
+	u8 cmd;
+	u8 net;
+	u8 rsvd;
+	__le32 baud;
+};
+
+/* Main message type used between library and application */
+struct __attribute__ ((packed)) esd_usb2_msg {
+	union {
+		struct header_msg hdr;
+		struct version_msg version;
+		struct version_reply_msg version_reply;
+		struct rx_msg rx;
+		struct tx_msg tx;
+		struct tx_done_msg txdone;
+		struct set_baudrate_msg setbaud;
+		struct id_filter_msg filter;
+	} msg;
+};
+
+static struct usb_device_id esd_usb2_table[] = {
+	{USB_DEVICE(USB_ESDGMBH_VENDOR_ID, USB_CANUSB2_PRODUCT_ID)},
+	{}
+};
+MODULE_DEVICE_TABLE(usb, esd_usb2_table);
+
+struct esd_usb2_net_priv;
+
+struct esd_tx_urb_context {
+	struct esd_usb2_net_priv *priv;
+	u32 echo_index;
+	int dlc;
+};
+
+struct esd_usb2 {
+	struct usb_device *udev;
+	struct esd_usb2_net_priv *nets[ESD_USB2_MAX_NETS];
+
+	struct usb_anchor rx_submitted;
+
+	int net_count;
+	u32 version;
+	int rxinitdone;
+};
+
+struct esd_usb2_net_priv {
+	struct can_priv can; /* must be the first member */
+
+	atomic_t active_tx_jobs;
+	struct usb_anchor tx_submitted;
+	struct esd_tx_urb_context tx_contexts[MAX_TX_URBS];
+
+	int open_time;
+	struct esd_usb2 *usb2;
+	struct net_device *netdev;
+	int index;
+	u8 old_state;
+	struct can_berr_counter bec;
+};
+
+static void esd_usb2_rx_event(struct esd_usb2_net_priv *priv,
+			      struct esd_usb2_msg *msg)
+{
+	struct net_device_stats *stats = &priv->netdev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
+
+	if (id == ESD_EV_CAN_ERROR_EXT) {
+		u8 state = msg->msg.rx.data[0];
+		u8 ecc = msg->msg.rx.data[1];
+		u8 txerr = msg->msg.rx.data[2];
+		u8 rxerr = msg->msg.rx.data[3];
+
+		skb = alloc_can_err_skb(priv->netdev, &cf);
+		if (skb == NULL) {
+			stats->rx_dropped++;
+			return;
+		}
+
+		if (state != priv->old_state) {
+			priv->old_state = state;
+
+			switch (state & ESD_BUSSTATE_MASK) {
+			case ESD_BUSSTATE_BUSOFF:
+				priv->can.state = CAN_STATE_BUS_OFF;
+				cf->can_id |= CAN_ERR_BUSOFF;
+				can_bus_off(priv->netdev);
+				break;
+			case ESD_BUSSTATE_WARN:
+				priv->can.state = CAN_STATE_ERROR_WARNING;
+				priv->can.can_stats.error_warning++;
+				break;
+			case ESD_BUSSTATE_ERRPASSIVE:
+				priv->can.state = CAN_STATE_ERROR_PASSIVE;
+				priv->can.can_stats.error_passive++;
+				break;
+			default:
+				priv->can.state = CAN_STATE_ERROR_ACTIVE;
+				break;
+			}
+		} else {
+			priv->can.can_stats.bus_error++;
+			stats->rx_errors++;
+
+			cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+			switch (ecc & SJA1000_ECC_MASK) {
+			case SJA1000_ECC_BIT:
+				cf->data[2] |= CAN_ERR_PROT_BIT;
+				break;
+			case SJA1000_ECC_FORM:
+				cf->data[2] |= CAN_ERR_PROT_FORM;
+				break;
+			case SJA1000_ECC_STUFF:
+				cf->data[2] |= CAN_ERR_PROT_STUFF;
+				break;
+			default:
+				cf->data[2] |= CAN_ERR_PROT_UNSPEC;
+				cf->data[3] = ecc & SJA1000_ECC_SEG;
+				break;
+			}
+
+			/* Error occured during transmission? */
+			if (!(ecc & SJA1000_ECC_DIR))
+				cf->data[2] |= CAN_ERR_PROT_TX;
+
+			if (priv->can.state == CAN_STATE_ERROR_WARNING ||
+			    priv->can.state == CAN_STATE_ERROR_PASSIVE) {
+				cf->data[1] = (txerr > rxerr) ?
+					CAN_ERR_CRTL_TX_PASSIVE :
+					CAN_ERR_CRTL_RX_PASSIVE;
+			}
+			cf->data[6] = txerr;
+			cf->data[7] = rxerr;
+		}
+
+		netif_rx(skb);
+
+		priv->bec.txerr = txerr;
+		priv->bec.rxerr = rxerr;
+
+		stats->rx_packets++;
+		stats->rx_bytes += cf->can_dlc;
+	}
+}
+
+static void esd_usb2_rx_can_msg(struct esd_usb2_net_priv *priv,
+				struct esd_usb2_msg *msg)
+{
+	struct net_device_stats *stats = &priv->netdev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	int i;
+	u32 id;
+
+	if (!netif_device_present(priv->netdev))
+		return;
+
+	id = le32_to_cpu(msg->msg.rx.id);
+
+	if (id & ESD_EVENT) {
+		esd_usb2_rx_event(priv, msg);
+	} else {
+		skb = alloc_can_skb(priv->netdev, &cf);
+		if (skb == NULL) {
+			stats->rx_dropped++;
+			return;
+		}
+
+		cf->can_id = id & ESD_IDMASK;
+		cf->can_dlc = get_can_dlc(msg->msg.rx.dlc);
+
+		if (id & ESD_EXTID)
+			cf->can_id |= CAN_EFF_FLAG;
+
+		if (msg->msg.rx.dlc & ESD_RTR) {
+			cf->can_id |= CAN_RTR_FLAG;
+		} else {
+			for (i = 0; i < cf->can_dlc; i++)
+				cf->data[i] = msg->msg.rx.data[i];
+		}
+
+		netif_rx(skb);
+
+		stats->rx_packets++;
+		stats->rx_bytes += cf->can_dlc;
+	}
+
+	return;
+}
+
+static void esd_usb2_tx_done_msg(struct esd_usb2_net_priv *priv,
+				 struct esd_usb2_msg *msg)
+{
+	struct net_device_stats *stats = &priv->netdev->stats;
+	struct net_device *netdev = priv->netdev;
+	struct esd_tx_urb_context *context;
+
+	if (!netif_device_present(netdev))
+		return;
+
+	context = &priv->tx_contexts[msg->msg.txdone.hnd & (MAX_TX_URBS - 1)];
+
+	if (!msg->msg.txdone.status) {
+		stats->tx_packets++;
+		stats->tx_bytes += context->dlc;
+		can_get_echo_skb(netdev, context->echo_index);
+	} else {
+		stats->tx_errors++;
+		can_free_echo_skb(netdev, context->echo_index);
+	}
+
+	/* Release context */
+	context->echo_index = MAX_TX_URBS;
+	atomic_dec(&priv->active_tx_jobs);
+
+	netif_wake_queue(netdev);
+}
+
+static void esd_usb2_read_bulk_callback(struct urb *urb)
+{
+	struct esd_usb2 *dev = urb->context;
+	int retval;
+	int pos = 0;
+	int i;
+
+	switch (urb->status) {
+	case 0: /* success */
+		break;
+
+	case -ENOENT:
+	case -ESHUTDOWN:
+		return;
+
+	default:
+		dev_info(dev->udev->dev.parent,
+			 "Rx URB aborted (%d)\n", urb->status);
+		goto resubmit_urb;
+	}
+
+	while (pos < urb->actual_length) {
+		struct esd_usb2_msg *msg;
+
+		msg = (struct esd_usb2_msg *)(urb->transfer_buffer + pos);
+
+		switch (msg->msg.hdr.cmd) {
+		case CMD_CAN_RX:
+			esd_usb2_rx_can_msg(dev->nets[msg->msg.rx.net], msg);
+			break;
+
+		case CMD_CAN_TX:
+			esd_usb2_tx_done_msg(dev->nets[msg->msg.txdone.net],
+					     msg);
+			break;
+		}
+
+		pos += msg->msg.hdr.len << 2;
+
+		if (pos > urb->actual_length) {
+			dev_err(dev->udev->dev.parent, "format error\n");
+			break;
+		}
+	}
+
+resubmit_urb:
+	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
+			  urb->transfer_buffer, RX_BUFFER_SIZE,
+			  esd_usb2_read_bulk_callback, dev);
+
+	retval = usb_submit_urb(urb, GFP_ATOMIC);
+	if (retval == -ENODEV) {
+		for (i = 0; i < dev->net_count; i++) {
+			if (dev->nets[i])
+				netif_device_detach(dev->nets[i]->netdev);
+		}
+	} else if (retval) {
+		dev_err(dev->udev->dev.parent,
+			"failed resubmitting read bulk urb: %d\n", retval);
+	}
+
+	return;
+}
+
+/*
+ * callback for bulk IN urb
+ */
+static void esd_usb2_write_bulk_callback(struct urb *urb)
+{
+	struct esd_tx_urb_context *context = urb->context;
+	struct esd_usb2_net_priv *priv;
+	struct esd_usb2 *dev;
+	struct net_device *netdev;
+	size_t size = sizeof(struct esd_usb2_msg);
+
+	BUG_ON(!context);
+
+	priv = context->priv;
+	netdev = priv->netdev;
+	dev = priv->usb2;
+
+	/* free up our allocated buffer */
+	usb_free_coherent(urb->dev, size,
+			  urb->transfer_buffer, urb->transfer_dma);
+
+	if (!netif_device_present(netdev))
+		return;
+
+	if (urb->status)
+		dev_info(netdev->dev.parent, "Tx URB aborted (%d)\n",
+			 urb->status);
+
+	netdev->trans_start = jiffies;
+}
+
+#ifdef CONFIG_SYSFS
+static ssize_t show_firmware(struct device *d,
+			     struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(d);
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+
+	return sprintf(buf, "%d.%d.%d\n",
+		       (dev->version >> 12) & 0xf,
+		       (dev->version >> 8) & 0xf,
+		       dev->version & 0xff);
+}
+static DEVICE_ATTR(firmware, S_IRUGO, show_firmware, NULL);
+
+static ssize_t show_hardware(struct device *d,
+			     struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(d);
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+
+	return sprintf(buf, "%d.%d.%d\n",
+		       (dev->version >> 28) & 0xf,
+		       (dev->version >> 24) & 0xf,
+		       (dev->version >> 16) & 0xff);
+}
+static DEVICE_ATTR(hardware, S_IRUGO, show_hardware, NULL);
+
+static ssize_t show_nets(struct device *d,
+			 struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(d);
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+
+	return sprintf(buf, "%d", dev->net_count);
+}
+static DEVICE_ATTR(nets, S_IRUGO, show_nets, NULL);
+#endif
+
+static int esd_usb2_send_msg(struct esd_usb2 *dev, struct esd_usb2_msg *msg)
+{
+	int actual_length;
+
+	return usb_bulk_msg(dev->udev,
+			    usb_sndbulkpipe(dev->udev, 2),
+			    msg,
+			    msg->msg.hdr.len << 2,
+			    &actual_length,
+			    1000);
+}
+
+static int esd_usb2_wait_msg(struct esd_usb2 *dev,
+			     struct esd_usb2_msg *msg)
+{
+	int actual_length;
+
+	return usb_bulk_msg(dev->udev,
+			    usb_rcvbulkpipe(dev->udev, 1),
+			    msg,
+			    sizeof(*msg),
+			    &actual_length,
+			    1000);
+}
+
+static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev)
+{
+	int i, err = 0;
+
+	if (dev->rxinitdone)
+		return 0;
+
+	for (i = 0; i < MAX_RX_URBS; i++) {
+		struct urb *urb = NULL;
+		u8 *buf = NULL;
+
+		/* create a URB, and a buffer for it */
+		urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!urb) {
+			dev_warn(dev->udev->dev.parent,
+				 "No memory left for URBs\n");
+			err = -ENOMEM;
+			break;
+		}
+
+		buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
+					 &urb->transfer_dma);
+		if (!buf) {
+			dev_warn(dev->udev->dev.parent,
+				 "No memory left for USB buffer\n");
+			err = -ENOMEM;
+			goto freeurb;
+		}
+
+		usb_fill_bulk_urb(urb, dev->udev,
+				  usb_rcvbulkpipe(dev->udev, 1),
+				  buf, RX_BUFFER_SIZE,
+				  esd_usb2_read_bulk_callback, dev);
+		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+		usb_anchor_urb(urb, &dev->rx_submitted);
+
+		err = usb_submit_urb(urb, GFP_KERNEL);
+		if (err) {
+			usb_unanchor_urb(urb);
+			usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
+					  urb->transfer_dma);
+		}
+
+freeurb:
+		/* Drop reference, USB core will take care of freeing it */
+		usb_free_urb(urb);
+		if (err)
+			break;
+	}
+
+	/* Did we submit any URBs */
+	if (i == 0) {
+		dev_err(dev->udev->dev.parent, "couldn't setup read URBs\n");
+		return err;
+	}
+
+	/* Warn if we've couldn't transmit all the URBs */
+	if (i < MAX_RX_URBS) {
+		dev_warn(dev->udev->dev.parent,
+			 "rx performance may be slow\n");
+	}
+
+	dev->rxinitdone = 1;
+	return 0;
+}
+
+/*
+ * Start interface
+ */
+static int esd_usb2_start(struct esd_usb2_net_priv *priv)
+{
+	struct esd_usb2 *dev = priv->usb2;
+	struct net_device *netdev = priv->netdev;
+	struct esd_usb2_msg msg;
+	int err, i;
+
+	/*
+	 * Enable all IDs
+	 * The IDADD message takes up to 64 32 bit bitmasks (2048 bits).
+	 * Each bit represents one 11 bit CAN identifier. A set bit
+	 * enables reception of the corresponding CAN identifier. A cleared
+	 * bit disabled this identifier. An additional bitmask value
+	 * following the CAN 2.0A bits is used to enable reception of
+	 * extended CAN frames. Only the LSB of this final mask is checked
+	 * for the complete 29 bit ID range. The IDADD message also allows
+	 * filter configuration for an ID subset. In this case you can add
+	 * the number of the starting bitmask (0..64) to the filter.option
+	 * field followed by only some bitmasks.
+	 */
+	msg.msg.hdr.cmd = CMD_IDADD;
+	msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+	msg.msg.filter.net = priv->index;
+	msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
+	for (i = 0; i < ESD_MAX_ID_SEGMENT; i++)
+		msg.msg.filter.mask[i] = cpu_to_le32(0xffffffff);
+	/* enable 29bit extended IDs */
+	msg.msg.filter.mask[ESD_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
+
+	err = esd_usb2_send_msg(dev, &msg);
+	if (err)
+		goto failed;
+
+	err = esd_usb2_setup_rx_urbs(dev);
+	if (err)
+		goto failed;
+
+	priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+	return 0;
+
+failed:
+	if (err == -ENODEV)
+		netif_device_detach(netdev);
+
+	dev_err(netdev->dev.parent, "couldn't start device: %d\n", err);
+
+	return err;
+}
+
+static void unlink_all_urbs(struct esd_usb2 *dev)
+{
+	struct esd_usb2_net_priv *priv;
+	int i;
+
+	usb_kill_anchored_urbs(&dev->rx_submitted);
+	for (i = 0; i < dev->net_count; i++) {
+		priv = dev->nets[i];
+		if (priv) {
+			usb_kill_anchored_urbs(&priv->tx_submitted);
+			atomic_set(&priv->active_tx_jobs, 0);
+
+			for (i = 0; i < MAX_TX_URBS; i++)
+				priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+		}
+	}
+}
+
+static int esd_usb2_open(struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	int err;
+
+	/* common open */
+	err = open_candev(netdev);
+	if (err)
+		return err;
+
+	/* finally start device */
+	err = esd_usb2_start(priv);
+	if (err) {
+		dev_warn(netdev->dev.parent,
+			 "couldn't start device: %d\n", err);
+		close_candev(netdev);
+		return err;
+	}
+
+	priv->open_time = jiffies;
+
+	netif_start_queue(netdev);
+
+	return 0;
+}
+
+static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb,
+				      struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	struct esd_usb2 *dev = priv->usb2;
+	struct esd_tx_urb_context *context = NULL;
+	struct net_device_stats *stats = &netdev->stats;
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct esd_usb2_msg *msg;
+	struct urb *urb;
+	u8 *buf;
+	int i, err;
+	int ret = NETDEV_TX_OK;
+	size_t size = sizeof(struct esd_usb2_msg);
+
+	if (can_dropped_invalid_skb(netdev, skb))
+		return NETDEV_TX_OK;
+
+	/* create a URB, and a buffer for it, and copy the data to the URB */
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		dev_err(netdev->dev.parent, "No memory left for URBs\n");
+		stats->tx_dropped++;
+		dev_kfree_skb(skb);
+		goto nourbmem;
+	}
+
+	buf = usb_alloc_coherent(dev->udev, size, GFP_ATOMIC,
+				 &urb->transfer_dma);
+	if (!buf) {
+		dev_err(netdev->dev.parent, "No memory left for USB buffer\n");
+		stats->tx_dropped++;
+		dev_kfree_skb(skb);
+		goto nobufmem;
+	}
+
+	msg = (struct esd_usb2_msg *)buf;
+
+	msg->msg.hdr.len = 3; /* minimal length */
+	msg->msg.hdr.cmd = CMD_CAN_TX;
+	msg->msg.tx.net = priv->index;
+	msg->msg.tx.dlc = cf->can_dlc;
+	msg->msg.tx.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
+
+	if (cf->can_id & CAN_RTR_FLAG)
+		msg->msg.tx.dlc |= ESD_RTR;
+
+	if (cf->can_id & CAN_EFF_FLAG)
+		msg->msg.tx.id |= cpu_to_le32(ESD_EXTID);
+
+	for (i = 0; i < cf->can_dlc; i++)
+		msg->msg.tx.data[i] = cf->data[i];
+
+	msg->msg.hdr.len += (cf->can_dlc + 3) >> 2;
+
+	for (i = 0; i < MAX_TX_URBS; i++) {
+		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
+			context = &priv->tx_contexts[i];
+			break;
+		}
+	}
+
+	/*
+	 * This may never happen.
+	 */
+	if (!context) {
+		dev_warn(netdev->dev.parent, "couldn't find free context\n");
+		ret = NETDEV_TX_BUSY;
+		goto releasebuf;
+	}
+
+	context->priv = priv;
+	context->echo_index = i;
+	context->dlc = cf->can_dlc;
+
+	/* hnd must not be 0 - MSB is stripped in txdone handling */
+	msg->msg.tx.hnd = 0x80000000 | i; /* returned in TX done message */
+
+	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
+			  msg->msg.hdr.len << 2,
+			  esd_usb2_write_bulk_callback, context);
+
+	urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+
+	usb_anchor_urb(urb, &priv->tx_submitted);
+
+	can_put_echo_skb(skb, netdev, context->echo_index);
+
+	atomic_inc(&priv->active_tx_jobs);
+
+	err = usb_submit_urb(urb, GFP_ATOMIC);
+	if (err) {
+		can_free_echo_skb(netdev, context->echo_index);
+
+		atomic_dec(&priv->active_tx_jobs);
+		usb_unanchor_urb(urb);
+
+		stats->tx_dropped++;
+
+		if (err == -ENODEV)
+			netif_device_detach(netdev);
+		else
+			dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
+
+		goto releasebuf;
+	}
+
+	netdev->trans_start = jiffies;
+
+	/* Slow down tx path */
+	if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
+		netif_stop_queue(netdev);
+
+	/*
+	 * Release our reference to this URB, the USB core will eventually free
+	 * it entirely.
+	 */
+	usb_free_urb(urb);
+
+	return NETDEV_TX_OK;
+
+releasebuf:
+	usb_free_coherent(dev->udev, size, buf, urb->transfer_dma);
+
+nobufmem:
+	usb_free_urb(urb);
+
+nourbmem:
+	return ret;
+}
+
+static int esd_usb2_close(struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	struct esd_usb2_msg msg;
+	int i;
+
+	/* Disable all IDs (see esd_usb2_start()) */
+	msg.msg.hdr.cmd = CMD_IDADD;
+	msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+	msg.msg.filter.net = priv->index;
+	msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
+	for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++)
+		msg.msg.filter.mask[i] = 0;
+	esd_usb2_send_msg(priv->usb2, &msg);
+
+	/* set CAN controller to reset mode */
+	msg.msg.hdr.len = 2;
+	msg.msg.hdr.cmd = CMD_SETBAUD;
+	msg.msg.setbaud.net = priv->index;
+	msg.msg.setbaud.rsvd = 0;
+	msg.msg.setbaud.baud = cpu_to_le32(ESD_USB2_NO_BAUDRATE);
+	esd_usb2_send_msg(priv->usb2, &msg);
+
+	priv->can.state = CAN_STATE_STOPPED;
+
+	netif_stop_queue(netdev);
+
+	close_candev(netdev);
+
+	priv->open_time = 0;
+
+	return 0;
+}
+
+static const struct net_device_ops esd_usb2_netdev_ops = {
+	.ndo_open = esd_usb2_open,
+	.ndo_stop = esd_usb2_close,
+	.ndo_start_xmit = esd_usb2_start_xmit,
+};
+
+static struct can_bittiming_const esd_usb2_bittiming_const = {
+	.name = "esd_usb2",
+	.tseg1_min = ESD_USB2_TSEG1_MIN,
+	.tseg1_max = ESD_USB2_TSEG1_MAX,
+	.tseg2_min = ESD_USB2_TSEG2_MIN,
+	.tseg2_max = ESD_USB2_TSEG2_MAX,
+	.sjw_max = ESD_USB2_SJW_MAX,
+	.brp_min = ESD_USB2_BRP_MIN,
+	.brp_max = ESD_USB2_BRP_MAX,
+	.brp_inc = ESD_USB2_BRP_INC,
+};
+
+static int esd_usb2_set_bittiming(struct net_device *netdev)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	struct esd_usb2_msg msg;
+	u32 canbtr;
+
+	canbtr = ESD_USB2_UBR;
+	canbtr |= (bt->brp - 1) & (ESD_USB2_BRP_MAX - 1);
+	canbtr |= ((bt->sjw - 1) & (ESD_USB2_SJW_MAX - 1))
+		<< ESD_USB2_SJW_SHIFT;
+	canbtr |= ((bt->prop_seg + bt->phase_seg1 - 1)
+		   & (ESD_USB2_TSEG1_MAX - 1))
+		<< ESD_USB2_TSEG1_SHIFT;
+	canbtr |= ((bt->phase_seg2 - 1) & (ESD_USB2_TSEG2_MAX - 1))
+		<< ESD_USB2_TSEG2_SHIFT;
+	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+		canbtr |= ESD_USB2_3_SAMPLES;
+
+	msg.msg.hdr.len = 2;
+	msg.msg.hdr.cmd = CMD_SETBAUD;
+	msg.msg.setbaud.net = priv->index;
+	msg.msg.setbaud.rsvd = 0;
+	msg.msg.setbaud.baud = cpu_to_le32(canbtr);
+
+	dev_info(netdev->dev.parent, "setting BTR=%#x\n", canbtr);
+
+	return esd_usb2_send_msg(priv->usb2, &msg);
+}
+
+static int esd_usb2_get_berr_counter(const struct net_device *netdev,
+				     struct can_berr_counter *bec)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+
+	bec->txerr = priv->bec.txerr;
+	bec->rxerr = priv->bec.rxerr;
+
+	return 0;
+}
+
+static int esd_usb2_set_mode(struct net_device *netdev, enum can_mode mode)
+{
+	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
+
+	if (!priv->open_time)
+		return -EINVAL;
+
+	switch (mode) {
+	case CAN_MODE_START:
+		netif_wake_queue(netdev);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int esd_usb2_probe_one_net(struct usb_interface *intf, int index)
+{
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+	struct net_device *netdev;
+	struct esd_usb2_net_priv *priv;
+	int err;
+	int i;
+
+	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	if (!netdev) {
+		dev_err(&intf->dev, "couldn't alloc candev\n");
+		return -ENOMEM;
+	}
+
+	priv = netdev_priv(netdev);
+
+	init_usb_anchor(&priv->tx_submitted);
+	atomic_set(&priv->active_tx_jobs, 0);
+
+	for (i = 0; i < MAX_TX_URBS; i++)
+		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+
+	priv->usb2 = dev;
+	priv->netdev = netdev;
+	priv->index = index;
+
+	priv->can.state = CAN_STATE_STOPPED;
+	priv->can.clock.freq = ESD_USB2_CAN_CLOCK;
+	priv->can.bittiming_const = &esd_usb2_bittiming_const;
+	priv->can.do_set_bittiming = esd_usb2_set_bittiming;
+	priv->can.do_set_mode = esd_usb2_set_mode;
+	priv->can.do_get_berr_counter = esd_usb2_get_berr_counter;
+	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
+
+	netdev->flags |= IFF_ECHO; /* we support local echo */
+
+	netdev->netdev_ops = &esd_usb2_netdev_ops;
+
+	SET_NETDEV_DEV(netdev, &intf->dev);
+
+	err = register_candev(netdev);
+	if (err) {
+		dev_err(&intf->dev,
+			"couldn't register CAN device: %d\n", err);
+		free_candev(netdev);
+		return -ENOMEM;
+	}
+
+	dev->nets[index] = priv;
+	dev_info(netdev->dev.parent, "device %s registered\n", netdev->name);
+	return 0;
+}
+
+/*
+ * probe function for new USB2 devices
+ *
+ * check version information and number of available
+ * CAN interfaces
+ */
+static int esd_usb2_probe(struct usb_interface *intf,
+			 const struct usb_device_id *id)
+{
+	struct esd_usb2 *dev;
+	struct esd_usb2_msg msg;
+	int i, err = -ENOMEM;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->udev = interface_to_usbdev(intf);
+
+	init_usb_anchor(&dev->rx_submitted);
+
+	usb_set_intfdata(intf, dev);
+
+	/* query number of CAN interfaces (nets) */
+	msg.msg.hdr.cmd = CMD_VERSION;
+	msg.msg.hdr.len = 2;
+	msg.msg.version.rsvd = 0;
+	msg.msg.version.flags = 0;
+	msg.msg.version.drv_version = 0;
+
+	if (esd_usb2_send_msg(dev, &msg) < 0) {
+		dev_err(&intf->dev, "sending version message failed\n");
+		goto free_dev;
+	}
+
+	if (esd_usb2_wait_msg(dev, &msg) < 0) {
+		dev_err(&intf->dev, "no version message answer\n");
+		goto free_dev;
+	}
+
+	dev->net_count = (int)msg.msg.version_reply.nets;
+	dev->version = le32_to_cpu(msg.msg.version_reply.version);
+
+#ifdef CONFIG_SYSFS
+	if (device_create_file(&intf->dev, &dev_attr_firmware))
+		dev_err(&intf->dev,
+			"Couldn't create device file for firmware\n");
+
+	if (device_create_file(&intf->dev, &dev_attr_hardware))
+		dev_err(&intf->dev,
+			"Couldn't create device file for hardware\n");
+
+	if (device_create_file(&intf->dev, &dev_attr_nets))
+		dev_err(&intf->dev,
+			"Couldn't create device file for nets\n");
+#endif
+
+	/* do per device probing */
+	for (i = 0; i < dev->net_count; i++)
+		esd_usb2_probe_one_net(intf, i);
+
+	return 0;
+
+free_dev:
+	kfree(dev);
+	return err;
+}
+
+/*
+ * called by the usb core when the device is removed from the system
+ */
+static void esd_usb2_disconnect(struct usb_interface *intf)
+{
+	struct esd_usb2 *dev = usb_get_intfdata(intf);
+	struct net_device *netdev;
+	int i;
+
+#ifdef CONFIG_SYSFS
+	device_remove_file(&intf->dev, &dev_attr_firmware);
+	device_remove_file(&intf->dev, &dev_attr_hardware);
+	device_remove_file(&intf->dev, &dev_attr_nets);
+#endif
+	usb_set_intfdata(intf, NULL);
+
+	if (dev) {
+		for (i = 0; i < dev->net_count; i++) {
+			if (dev->nets[i]) {
+				netdev = dev->nets[i]->netdev;
+				unregister_netdev(netdev);
+				free_candev(netdev);
+			}
+		}
+		unlink_all_urbs(dev);
+	}
+}
+
+/* usb specific object needed to register this driver with the usb subsystem */
+static struct usb_driver esd_usb2_driver = {
+	.name = "esd_usb2",
+	.probe = esd_usb2_probe,
+	.disconnect = esd_usb2_disconnect,
+	.id_table = esd_usb2_table,
+};
+
+static int __init esd_usb2_init(void)
+{
+	int err;
+
+	/* register this driver with the USB subsystem */
+	err = usb_register(&esd_usb2_driver);
+
+	if (err) {
+		err("usb_register failed. Error number %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+module_init(esd_usb2_init);
+
+static void __exit esd_usb2_exit(void)
+{
+	/* deregister this driver with the USB subsystem */
+	usb_deregister(&esd_usb2_driver);
+}
+module_exit(esd_usb2_exit);
-- 
1.5.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
  2010-05-26  9:14 [PATCH v3] can: Add driver for esd CAN-USB/2 device Matthias Fuchs
       [not found] ` <201005261114.03214.matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
@ 2010-05-26 13:47 ` Oliver Neukum
       [not found]   ` <201005261547.34596.oneukum-l3A5Bk7waGM@public.gmane.org>
  1 sibling, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2010-05-26 13:47 UTC (permalink / raw)
  To: Matthias Fuchs; +Cc: netdev, Socketcan-core, linux-usb

Am Mittwoch, 26. Mai 2010 11:14:03 schrieb Matthias Fuchs:
> +       netdev->trans_start = jiffies;
> +
> +       /* Slow down tx path */
> +       if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
> +               netif_stop_queue(netdev);

Where is the queue started again?

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
       [not found]   ` <201005261547.34596.oneukum-l3A5Bk7waGM@public.gmane.org>
@ 2010-05-28 14:19     ` Matthias Fuchs
  2010-05-28 15:26       ` Oliver Neukum
  0 siblings, 1 reply; 9+ messages in thread
From: Matthias Fuchs @ 2010-05-28 14:19 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA

On Wednesday 26 May 2010 15:47, Oliver Neukum wrote:
> Am Mittwoch, 26. Mai 2010 11:14:03 schrieb Matthias Fuchs:
> > +       netdev->trans_start = jiffies;
> > +
> > +       /* Slow down tx path */
> > +       if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
> > +               netif_stop_queue(netdev);
> 
> Where is the queue started again?
> 
> 	Regards
> 		Oliver

This is done in the ack-patch for sent messages:

static void esd_usb2_tx_done_msg(struct esd_usb2_net_priv *priv,
				 struct esd_usb2_msg *msg)
{
...
	atomic_dec(&priv->active_tx_jobs);

	netif_wake_queue(netdev);
}

Matthias

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
       [not found]     ` <70376CA23424B34D86F1C7DE6B9973430254343AD9-fVvhrSDAkVjuuPCCJ6VnObSn4PsL5ZDKvpI+GvaZM4lBDgjK7y7TUQ@public.gmane.org>
@ 2010-05-28 15:25       ` Matthias Fuchs
  2010-05-31  6:35         ` Viral Mehta
  0 siblings, 1 reply; 9+ messages in thread
From: Matthias Fuchs @ 2010-05-28 15:25 UTC (permalink / raw)
  To: Viral Mehta
  Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA

Hi Viral,

thanks for review. Please see some comments below.

On Wednesday 26 May 2010 13:31, Viral Mehta wrote:
> Hi,
> >________________________________________
> >From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Matthias Fuchs [matthias.fuchs-iOnpLzIbIdM@public.gmane.org]
> >Sent: Wednesday, May 26, 2010 2:44 PM
> >To: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >Subject: [PATCH v3] can: Add driver for esd CAN-USB/2 device
> >+
> >+       BUG_ON(!context);
> 
> It is preferred to used WARN_ON and avoid using BUG_ON and thus dont kill the whole system....
Really? Even when next line will reference a NULL pointer in this case? Ok. Will be changed.

> [...]
> >+
> >+       priv = context->priv;
> >+       netdev = priv->netdev;
> >+       dev = priv->usb2;
> >+       err = usb_submit_urb(urb, GFP_ATOMIC);
> >+       if (err) {
> >+               can_free_echo_skb(netdev, context->echo_index);
> >+
> >+               atomic_dec(&priv->active_tx_jobs);
> >+               usb_unanchor_urb(urb);
> >+
> >+               stats->tx_dropped++;
> >+
> >+               if (err == -ENODEV)
> >+                       netif_device_detach(netdev);
> >+               else
> >+                       dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
> >+
> >+               goto releasebuf;
> 
> You probably want to set "ret" here or do you really want to return NETDEV_TX_OK
As far as I can see netword device drivers xmit_start() return NETDEV_TX_OK or _BUSY.
There are no real alternatives. But please correct me when I am wrong.

Your other comments will be fixed by version 4 of my patch.

Matthias

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
  2010-05-28 14:19     ` Matthias Fuchs
@ 2010-05-28 15:26       ` Oliver Neukum
  0 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2010-05-28 15:26 UTC (permalink / raw)
  To: Matthias Fuchs; +Cc: netdev, Socketcan-core, linux-usb

Am Freitag, 28. Mai 2010 16:19:33 schrieb Matthias Fuchs:
> On Wednesday 26 May 2010 15:47, Oliver Neukum wrote:
> > Am Mittwoch, 26. Mai 2010 11:14:03 schrieb Matthias Fuchs:
> > > +       netdev->trans_start = jiffies;
> > > +
> > > +       /* Slow down tx path */
> > > +       if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
> > > +               netif_stop_queue(netdev);
> > 
> > Where is the queue started again?
> > 
> > 	Regards
> > 		Oliver
> 
> This is done in the ack-patch for sent messages:
> 
> static void esd_usb2_tx_done_msg(struct esd_usb2_net_priv *priv,
> 				 struct esd_usb2_msg *msg)
> {
> ...
> 	atomic_dec(&priv->active_tx_jobs);
> 
> 	netif_wake_queue(netdev);
> }

Oh, I see. I am afraid this is a race. You have no guarantee that
esd_usb2_tx_done_msg() runs after you stop the queue. If you do
it that way you must stop the queue before you submit the URB
due to which you want to stop it (and do the error handling)

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v3] can: Add driver for esd CAN-USB/2 device
  2010-05-28 15:25       ` Matthias Fuchs
@ 2010-05-31  6:35         ` Viral Mehta
  0 siblings, 0 replies; 9+ messages in thread
From: Viral Mehta @ 2010-05-31  6:35 UTC (permalink / raw)
  To: Matthias Fuchs; +Cc: netdev, Socketcan-core, linux-usb

Hi,

> -----Original Message-----
> From: Matthias Fuchs [mailto:matthias.fuchs@esd.eu]
> Sent: Friday, May 28, 2010 8:56 PM
> To: Viral Mehta
> Cc: netdev@vger.kernel.org; Socketcan-core@lists.berlios.de; linux-
> usb@vger.kernel.org
> Subject: Re: [PATCH v3] can: Add driver for esd CAN-USB/2 device
>
> Hi Viral,
>
> thanks for review. Please see some comments below.
>
> On Wednesday 26 May 2010 13:31, Viral Mehta wrote:
> > Hi,
> > >________________________________________
> > >From: linux-usb-owner@vger.kernel.org [linux-usb-
> owner@vger.kernel.org] On Behalf Of Matthias Fuchs
> [matthias.fuchs@esd.eu]
> > >Sent: Wednesday, May 26, 2010 2:44 PM
> > >To: netdev@vger.kernel.org
> > >Cc: Socketcan-core@lists.berlios.de; linux-usb@vger.kernel.org
> > >Subject: [PATCH v3] can: Add driver for esd CAN-USB/2 device
> > >+
> > >+       BUG_ON(!context);
> >
> > It is preferred to used WARN_ON and avoid using BUG_ON and thus dont
> kill the whole system....
> Really? Even when next line will reference a NULL pointer in this case?
> Ok. Will be changed.

How about,
if(!context)
        return;

Look at the other drivers for example.
I, personally, dont care since I will not be using your driver but I am sure that other people, too, wont like BUG_ON....

>
> > [...]
> > >+
> > >+       priv = context->priv;
> > >+       netdev = priv->netdev;
> > >+       dev = priv->usb2;
> > >+       err = usb_submit_urb(urb, GFP_ATOMIC);
> > >+       if (err) {
> > >+               can_free_echo_skb(netdev, context->echo_index);
> > >+
> > >+               atomic_dec(&priv->active_tx_jobs);
> > >+               usb_unanchor_urb(urb);
> > >+
> > >+               stats->tx_dropped++;
> > >+
> > >+               if (err == -ENODEV)
> > >+                       netif_device_detach(netdev);
> > >+               else
> > >+                       dev_warn(netdev->dev.parent, "failed tx_urb
> %d\n", err);
> > >+
> > >+               goto releasebuf;
> >
> > You probably want to set "ret" here or do you really want to return
> NETDEV_TX_OK
> As far as I can see netword device drivers xmit_start() return
> NETDEV_TX_OK or _BUSY.
> There are no real alternatives.
Yup. this is okie....

>But please correct me when I am wrong.
>
> Your other comments will be fixed by version 4 of my patch.
>
> Matthias
>
> ______________________________________________________________________

This Email may contain confidential or privileged information for the intended recipient (s) If you are not the intended recipient, please do not use or disseminate the information, notify the sender and delete it from your system.

______________________________________________________________________

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2010-05-31  6:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-26  9:14 [PATCH v3] can: Add driver for esd CAN-USB/2 device Matthias Fuchs
     [not found] ` <201005261114.03214.matthias.fuchs-iOnpLzIbIdM@public.gmane.org>
2010-05-26 10:15   ` Wolfgang Grandegger
     [not found]     ` <4BFCF4AD.6030000-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2010-05-26 12:38       ` Matthias Fuchs
2010-05-26 11:31   ` Viral Mehta
     [not found]     ` <70376CA23424B34D86F1C7DE6B9973430254343AD9-fVvhrSDAkVjuuPCCJ6VnObSn4PsL5ZDKvpI+GvaZM4lBDgjK7y7TUQ@public.gmane.org>
2010-05-28 15:25       ` Matthias Fuchs
2010-05-31  6:35         ` Viral Mehta
2010-05-26 13:47 ` Oliver Neukum
     [not found]   ` <201005261547.34596.oneukum-l3A5Bk7waGM@public.gmane.org>
2010-05-28 14:19     ` Matthias Fuchs
2010-05-28 15:26       ` Oliver Neukum

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.