Subject: Re: [PATCH] can: kvaser_usb: Add support for Kvaser CAN/USB devices

From: Olivier Sobrie <[email-masked]>
Date: Tue, 31 Jul 2012 15:06:50 +0200

On Tue, Jul 31, 2012 at 11:56:22AM +0200, Marc Kleine-Budde wrote:
> On 07/30/2012 03:33 PM, Olivier Sobrie wrote:
> > Hi Marc,
> > 
> > On Mon, Jul 30, 2012 at 01:11:46PM +0200, Marc Kleine-Budde wrote:
> >> On 07/30/2012 07:32 AM, Olivier Sobrie wrote:
> >>> This driver provides support for several Kvaser CAN/USB devices.
> >>> Such kind of devices supports up to three can network interfaces.
> >>>
> >>> It has been tested with a Kvaser USB Leaf Light (one network interface)
> >>> connected to a pch_can interface.
> >>> The firmware version of the Kvaser device was 2.5.205.
> >>
> >> Please add [email-masked] to Cc for review of the USB part.
> > 
> > Ok I'll do it when I send the new version of the patch.
> > But it might be a good idea to add an entry in the MAINTAINERS file so
> > that when someone sends a patch they are aware of this when the
> > get_maintainer script is invoked.
> 
> Interesting Idea. We should discuss this here, however we should not
> bother the USB List when sending USB unrelated patches.
> 
> >> Please combine .h and .c file. Please make use of netdev_LEVEL() for
> >> error printing, not dev_LEVEL().
> > 
> > I'll combine the .c and .h.
> > I used the netdev_LEVEL() everywhere it was possible. It requires to
> > have access to a pointer to netdev which is not always possible;
> > that's the reason why I used dev_LEVEL().
> 
> I see, you used it when channel is invalid. So you have obviously no netdev.

Indeed.

> 
> >> Please review if all members of the struct kvaser_msg are properly
> >> aligned. You never access the struct kvaser_msg_* members directly, as
> >> they are unaligned. Please check for le16 and le32 access. You missed to
> >> convert the bitrate.
> > 
> > Indeed. Thanks. I'll check if I didn't missed another one.
> 
> Tnx
> 
> >> Please check if your driver survives hot-unplugging while sending and
> >> receiving CAN frames at maximum laod.
> > 
> > I tested this with two Kvaser sending frames with "cangen can0 -g 0 -i"
> > never saw a crash.
> 
> Please test send sending and receiving at the same time.

Yes that's what I did; "cangen can0 -g 0 -i" on both sides.

> 
> >> More comments inline,
> >> regards, Marc
> >>
> >>> List of Kvaser devices supported by the driver:
> >>>   - Kvaser Leaf prototype (P010v2 and v3)
> >>>   - Kvaser Leaf Light (P010v3)
> >>>   - Kvaser Leaf Professional HS
> >>>   - Kvaser Leaf SemiPro HS
> >>>   - Kvaser Leaf Professional LS
> >>>   - Kvaser Leaf Professional SWC
> >>>   - Kvaser Leaf Professional LIN
> >>>   - Kvaser Leaf SemiPro LS
> >>>   - Kvaser Leaf SemiPro SWC
> >>>   - Kvaser Memorator II, Prototype
> >>>   - Kvaser Memorator II HS/HS
> >>>   - Kvaser USBcan Professional HS/HS
> >>>   - Kvaser Leaf Light GI
> >>>   - Kvaser Leaf Professional HS (OBD-II connector)
> >>>   - Kvaser Memorator Professional HS/LS
> >>>   - Kvaser Leaf Light "China"
> >>>   - Kvaser BlackBird SemiPro
> >>>   - Kvaser OEM Mercury
> >>>   - Kvaser OEM Leaf
> >>>   - Kvaser USBcan R
> >>>
> >>> Signed-off-by: Olivier Sobrie <[email-masked]>
> >>> ---
> >>>  drivers/net/can/usb/Kconfig      |   33 ++
> >>>  drivers/net/can/usb/Makefile     |    1 +
> >>>  drivers/net/can/usb/kvaser_usb.c | 1062 ++++++++++++++++++++++++++++++++++++++
> >>>  drivers/net/can/usb/kvaser_usb.h |  237 +++++++++
> >>>  4 files changed, 1333 insertions(+)
> >>>  create mode 100644 drivers/net/can/usb/kvaser_usb.c
> >>>  create mode 100644 drivers/net/can/usb/kvaser_usb.h
> >>>
> >>> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> >>> index 0a68768..578955f 100644
> >>> --- a/drivers/net/can/usb/Kconfig
> >>> +++ b/drivers/net/can/usb/Kconfig
> >>> @@ -13,6 +13,39 @@ config CAN_ESD_USB2
> >>>            This driver supports the CAN-USB/2 interface
> >>>            from esd electronic system design gmbh (http://www.esd.eu).
> >>>  
> >>> +config CAN_KVASER_USB
> >>> +	tristate "Kvaser CAN/USB interface"
> >>> +	---help---
> >>> +	  This driver adds support for Kvaser CAN/USB devices like Kvaser
> >>> +	  Leaf Light.
> >>> +
> >>> +	  The driver gives support for the following devices:
> >>> +	    - Kvaser Leaf prototype (P010v2 and v3)
> >>> +	    - Kvaser Leaf Light (P010v3)
> >>> +	    - Kvaser Leaf Professional HS
> >>> +	    - Kvaser Leaf SemiPro HS
> >>> +	    - Kvaser Leaf Professional LS
> >>> +	    - Kvaser Leaf Professional SWC
> >>> +	    - Kvaser Leaf Professional LIN
> >>> +	    - Kvaser Leaf SemiPro LS
> >>> +	    - Kvaser Leaf SemiPro SWC
> >>> +	    - Kvaser Memorator II, Prototype
> >>> +	    - Kvaser Memorator II HS/HS
> >>> +	    - Kvaser USBcan Professional HS/HS
> >>> +	    - Kvaser Leaf Light GI
> >>> +	    - Kvaser Leaf Professional HS (OBD-II connector)
> >>> +	    - Kvaser Memorator Professional HS/LS
> >>> +	    - Kvaser Leaf Light "China"
> >>> +	    - Kvaser BlackBird SemiPro
> >>> +	    - Kvaser OEM Mercury
> >>> +	    - Kvaser OEM Leaf
> >>> +	    - Kvaser USBcan R
> >>> +
> >>> +	  If unsure, say N.
> >>> +
> >>> +	  To compile this driver as a module, choose M here: the
> >>> +	  module will be called kvaser_usb.
> >>> +
> >>>  config CAN_PEAK_USB
> >>>  	tristate "PEAK PCAN-USB/USB Pro interfaces"
> >>>  	---help---
> >>> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> >>> index da6d1d3..80a2ee4 100644
> >>> --- a/drivers/net/can/usb/Makefile
> >>> +++ b/drivers/net/can/usb/Makefile
> >>> @@ -4,6 +4,7 @@
> >>>  
> >>>  obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
> >>>  obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
> >>> +obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
> >>>  obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> >>>  
> >>>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> >>> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> >>> new file mode 100644
> >>> index 0000000..4965480
> >>> --- /dev/null
> >>> +++ b/drivers/net/can/usb/kvaser_usb.c
> >>> @@ -0,0 +1,1062 @@
> >>> +/*
> >>
> >> Please add a license statement and probably your copyright:
> >>
> >>  * 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.
> >>
> >> You also should copy the copyright from the drivers you used:
> >>
> >>> + * Parts of this driver are based on the following:
> >>> + *  - Kvaser linux leaf driver (version 4.78)
> >>
> >> I just downloaded their driver and noticed that it's quite sparse on
> >> stating the license the code is released under.
> >> "doc/HTMLhelp/copyright.htx" is quite restrictive, the word GPL occurs 3
> >> times, all in MODULE_LICENSE("GPL"). Running modinfo on the usbcan.ko
> >> shows "license: GPL"
> > 
> > I'll add the license statement.
> > In fact it's the leaf.ko which is used for this device and it is under
> > GPL as modinfo said.
> 
> I just talked to my boss and we're the same opinion, that
> MODULE_LICENSE("GPL") is a technical term and not relevant if the
> included license doesn't say a word about GPL. If the kvaser tarball
> violates the GPL, however is written on different sheet of paper (as we
> say in Germany).
> 
> So I cannot put my S-o-b under this driver as long as we haven't talked
> to kvaser.

Ok I thought it was sufficient enough to have MODULE_LICENSE("GPL") in
the code to indicate it is a GPL driver. I'll ask Kvaser before sending
any new version of the patch.

> 
> >>> + *  - CAN driver for esd CAN-USB/2
> >>> + */
> >>> +
> >>> +#include <linux/init.h>
> >>> +#include <linux/completion.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>
> >>> +
> >>> +#include "kvaser_usb.h"
> >>> +
> >>> +struct kvaser_usb_tx_urb_context {
> >>> +	struct kvaser_usb_net_priv *priv;
> >>
> >> Huh - how does this work without forward declaration?
> > 
> > It works.
> 
> Yes, obviously :)
> 
> > "In C and C++ it is possible to declare pointers to structs before
> > declaring their struct layout, provided the pointers are not
> > dereferenced--this is known as forward declaration."
> > 
> > See http://www.linuxtopia.org/online_books/an_introduction_to_gcc/gccintro_94.html
> 
> Thanks for the link.
> >>
> >>> +	u32 echo_index;
> >>> +	int dlc;
> >>> +};
> >>> +
> >>> +struct kvaser_usb {
> >>> +	struct usb_device *udev;
> >>> +	struct kvaser_usb_net_priv *nets[MAX_NET_DEVICES];
> >>> +
> >>> +	struct usb_anchor rx_submitted;
> >>> +
> >>> +	u32 fw_version;
> >>> +	unsigned int nchannels;
> >>> +
> >>> +	bool rxinitdone;
> >>> +};
> >>> +
> >>> +struct kvaser_usb_net_priv {
> >>> +	struct can_priv can;
> >>> +
> >>> +	atomic_t active_tx_urbs;
> >>> +	struct usb_anchor tx_submitted;
> >>> +	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
> >>> +
> >>> +	int open_time;
> >>
> >> please remove open_time
> > 
> > Ok.
> > 
> >>
> >>> +	struct completion start_stop_comp;
> >>> +
> >>> +	struct kvaser_usb *dev;
> >>> +	struct net_device *netdev;
> >>> +	int channel;
> >>> +	struct can_berr_counter bec;
> >>> +};
> >>> +
> >>> +static struct usb_device_id kvaser_usb_table[] = {
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LS_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_SWC_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LIN_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_LS_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_SWC_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_DEVEL_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSHS_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_UPRO_HSHS_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_GI_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_OBDII_PRODUCT_ID),
> >>> +		.driver_info = KVASER_HAS_SILENT_MODE },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSLS_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_CH_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_BLACKBIRD_SPRO_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_MERCURY_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_LEAF_PRODUCT_ID) },
> >>> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_CAN_R_PRODUCT_ID) },
> >>> +	{ }
> >>> +};
> >>> +MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
> >>> +
> >>> +static int kvaser_usb_send_msg(const struct kvaser_usb *dev,
> >>> +			       struct kvaser_msg *msg)
> >> inline?
> > 
> > Ok.
> > 
> >>> +{
> >>> +	int actual_len;
> >>> +
> >>> +	return usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, 1),
> >>                                                                   ^
> >> Can you please introduce a #define for this.
> > 
> > Ok. No problem.
> > 
> >>
> >>> +			    msg, msg->len, &actual_len, USB_SEND_TIMEOUT);
> >>> +}
> >>> +
> >>> +static int kvaser_usb_wait_msg(const struct kvaser_usb *dev, u8 id,
> >>> +			       struct kvaser_msg *msg)
> >>> +{
> >>> +	struct kvaser_msg *tmp;
> >>> +	void *buf;
> >>> +	int actual_len;
> >>> +	int err;
> >>> +	int pos = 0;
> >>> +
> >>> +	buf = kzalloc(RX_BUFFER_SIZE, GFP_KERNEL);
> >>> +	if (!buf)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	err = usb_bulk_msg(dev->udev, usb_rcvbulkpipe(dev->udev, 129),
> >>                                                                  ^^^
> >> dito
> > 
> > Ok too.
> > 
> >>
> >>> +			   buf, RX_BUFFER_SIZE, &actual_len,
> >>> +			   USB_RECEIVE_TIMEOUT);
> >>> +	if (err < 0) {
> >>> +		kfree(buf);
> >>> +		return err;
> >>> +	}
> >>> +
> >>> +	while (pos < actual_len) {
> >>
> >> Please check that pos + sizeof(*msg) is < actual_len, as you fill access
> >> it later.
> > 
> > I'll instead perform a check on 'pos + tmp->len < actual_len' and copy
> > only tmp->len instead of sizeof(*msg).
> > Thanks.
> 
> Even better, saves some bytes to be copied. Take care not to deref tmp,
> unless you checked that tmp is in valid memory.

Ok. I changed the loop by 'while (pos <= actual_len - MSG_HEADER_LEN)'
and then perform the check on 'pos + tmp->len < actual_len'.

> 
> >>
> >>> +		tmp = buf + pos;
> >>> +
> >>> +		if (!tmp->len)
> >>> +			break;
> >>> +
> >>> +		if (tmp->id == id) {
> >>> +			memcpy(msg, tmp, sizeof(*msg));
> >>> +			kfree(buf);
> >>> +			return 0;
> >>> +		}
> >>> +
> >>> +		pos += tmp->len;
> >>> +	}
> >>> +
> >>> +	kfree(buf);
> >>> +
> >>> +	return -EINVAL;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_send_simple_msg(const struct kvaser_usb *dev,
> >>> +				      u8 msg_id, int channel)
> >>> +{
> >>> +	struct kvaser_msg msg;
> >>> +	int err;
> >>> +
> >>> +	msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple);
> >>> +	msg.id = msg_id;
> >>> +	msg.u.simple.channel = channel;
> >>> +	msg.u.simple.tid = 0xff;
> >>
> >> Please use C99 struct initializer.
> >>
> >> struct kvaser_msg msg = {
> >> 	.len = ,
> >> 	.id =,
> >> };
> > 
> > Ok.
> > 
> >>
> >>
> >>> +
> >>> +	err = kvaser_usb_send_msg(dev, &msg);
> >>
> >> 	just:
> >> 	return err;
> > 
> > Ok.
> > 
> >>
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
> >>> +{
> >>> +	struct kvaser_msg msg;
> >>> +	int err;
> >>> +
> >>> +	err = kvaser_usb_send_simple_msg(dev, CMD_GET_SOFTWARE_INFO, 0);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	err = kvaser_usb_wait_msg(dev, CMD_GET_SOFTWARE_INFO_REPLY, &msg);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
> >>> +{
> >>> +	struct kvaser_msg msg;
> >>> +	int err;
> >>> +
> >>> +	err = kvaser_usb_send_simple_msg(dev, CMD_GET_CARD_INFO, 0);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	err = kvaser_usb_wait_msg(dev, CMD_GET_CARD_INFO_REPLY, &msg);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	dev->nchannels = msg.u.cardinfo.nchannels;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> >>> +				      const struct kvaser_msg *msg)
> >>> +{
> >>> +	struct net_device_stats *stats;
> >>> +	struct kvaser_usb_tx_urb_context *context;
> >>> +	struct kvaser_usb_net_priv *priv;
> >>> +	u8 channel = msg->u.tx_acknowledge.channel;
> >>> +	u8 tid = msg->u.tx_acknowledge.tid;
> >>> +
> >>> +	if (channel >= dev->nchannels) {
> >>> +		dev_err(dev->udev->dev.parent,
> >>> +			"Invalid channel number (%d)\n", channel);
> >>> +		return;
> >>> +	}
> >>
> >> can you do a check for (channel >= dev->nchannels), in a central place?
> >> e.g. kvaser_usb_handle_message()?
> > 
> > The problem is that channel is not always at the same place in the
> > messages I get from the hardware. 'tid' and 'channel' are inverted for
> > tx and rx frames.
> > e.g.
> 
> Grr...who's written that firmware :D
> 
> > 
> > struct kvaser_msg_tx_can {
> >         u8 channel;
> >         u8 tid;
> >         u8 msg[14];
> >         u8 padding;
> >         u8 flags;
> > } __packed;
> > 
> > struct kvaser_msg_busparams {
> >         u8 tid;
> >         u8 channel;
> >         __le32 bitrate;
> >         u8 tseg1;
> >         u8 tseg2;
> >         u8 sjw;
> >         u8 no_samp;
> > } __packed;
> > 
> >>
> >>> +
> >>> +	priv = dev->nets[channel];
> >>> +
> >>> +	if (!netif_device_present(priv->netdev))
> >>> +		return;
> >>> +
> >>> +	stats = &priv->netdev->stats;
> >>> +
> >>> +	context = &priv->tx_contexts[tid % MAX_TX_URBS];
> >>> +
> >>> +	/*
> >>> +	 * It looks like the firmware never sets the flags field of the
> >>> +	 * tx_acknowledge frame and never reports a transmit failure.
> >>> +	 * If the can message can't be transmited (e.g. incompatible
> >>> +	 * bitrates), a frame CMD_CAN_ERROR_EVENT is sent (with a null
> >>> +	 * tid) and the firmware tries to transmit again the packet until
> >>> +	 * it succeeds. Once the packet is successfully transmitted, then
> >>> +	 * the tx_acknowledge frame is sent.
> >>> +	 */
> >>> +
> >>> +	stats->tx_packets++;
> >>> +	stats->tx_bytes += context->dlc;
> >>> +	can_get_echo_skb(priv->netdev, context->echo_index);
> >>> +
> >>> +	context->echo_index = MAX_TX_URBS;
> >>> +	atomic_dec(&priv->active_tx_urbs);
> >>> +
> >>> +	netif_wake_queue(priv->netdev);
> >>> +}
> >>> +
> >>> +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> >>> +				const struct kvaser_msg *msg)
> >>> +{
> >>> +	struct can_frame *cf;
> >>> +	struct sk_buff *skb;
> >>> +	struct net_device_stats *stats;
> >>> +	struct kvaser_usb_net_priv *priv;
> >>> +	u8 channel, status, txerr, rxerr;
> >>> +
> >>> +	if (msg->id == CMD_CAN_ERROR_EVENT) {
> >>> +		channel = msg->u.error_event.channel;
> >>> +		status =  msg->u.error_event.status;
> >>> +		txerr = msg->u.error_event.tx_errors_count;
> >>> +		rxerr = msg->u.error_event.rx_errors_count;
> >>> +	} else {
> >>> +		channel = msg->u.chip_state_event.channel;
> >>> +		status =  msg->u.chip_state_event.status;
> >>> +		txerr = msg->u.chip_state_event.tx_errors_count;
> >>> +		rxerr = msg->u.chip_state_event.rx_errors_count;
> >>> +	}
> >>> +
> >>> +	if (channel >= dev->nchannels) {
> >>> +		dev_err(dev->udev->dev.parent,
> >>> +			"Invalid channel number (%d)\n", channel);
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	priv = dev->nets[channel];
> >>> +	stats = &priv->netdev->stats;
> >>> +
> >>> +	skb = alloc_can_err_skb(priv->netdev, &cf);
> >>> +	if (!skb) {
> >>> +		stats->rx_dropped++;
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	if ((status & M16C_STATE_BUS_OFF) ||
> >>> +	    (status & M16C_STATE_BUS_RESET)) {
> >>> +		priv->can.state = CAN_STATE_BUS_OFF;
> >>> +		cf->can_id |= CAN_ERR_BUSOFF;
> >>> +		can_bus_off(priv->netdev);
> 
> you should increment priv->can.can_stats.bus_off
> What does the firmware do in this state? Does it automatically try to
> recover and try to send the outstanding frames?

Yes that's what I observed.

> 
> If so, you should turn of the CAN interface, it possible. See:
> http://lxr.free-electrons.com/source/drivers/net/can/at91_can.c#L986

Ok I'll have a look at this.

> 
> Please test Bus-Off behaviour:
> - setup working CAN network
> - short circuit CAN-H and CAN-L wires
> - start "candump any,0:0,#FFFFFFFF" on one shell
> - send one can frame on the other
> 
> then
> 
> - remove the short circuit
> - see if the can frame is transmitted to the other side
> - it should show up as an echo'ed CAN frame on the sender side
> 
> Repeat the same test with disconnecting CAN-H and CAN-L from the other
> CAN station instead of short circuit.
> 
> Please send the output from candump.

1) With the short circuit:

I perform the test you described. It showed that the Kvaser passes from
ERROR-WARNING to ERROR-PASSIVE and then BUS-OFF. But after going to the
state BUS-OFF it comes back to ERROR-WARNING.

  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  ...
  can1  200000C8  [8] 00 00 90 00 00 00 00 00   ERRORFRAME  <-- bus off
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  ...
  can1  20000088  [8] 00 10 90 00 00 00 00 00   ERRORFRAME
  can1  123  [2] 11 22	          <-- short circuit removed

I see the echo and on the other end I see the frame coming in.
By the way I see that the txerr and rxerr fields of the structure
kvaser_msg_error_event stay at 0.

2) With CAN-H and CAN-L disconnected:

I never see the bus going in OFF state. It stays in PASSIVE mode.

  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  ...
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  20000088  [8] 00 10 80 1B 00 00 00 00   ERRORFRAME
  can1  123  [2] 11 22            <-- other end connected

> 
> >>> +	} else if (status & M16C_STATE_BUS_ERROR) {
> >>> +		priv->can.state = CAN_STATE_ERROR_WARNING;
> >>> +		priv->can.can_stats.error_warning++;
> >>> +	} else if (status & M16C_STATE_BUS_PASSIVE) {
> >>> +		priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >>> +		priv->can.can_stats.error_passive++;
> >>> +	} else {
> >>> +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>> +		cf->can_id |= CAN_ERR_PROT;
> >>> +		cf->data[2] = CAN_ERR_PROT_ACTIVE;
> >>> +	}
> >>> +
> >>> +	if (msg->id == CMD_CAN_ERROR_EVENT) {
> >>> +		u8 error_factor = msg->u.error_event.error_factor;
> >>> +
> >>> +		priv->can.can_stats.bus_error++;
> >>> +		stats->rx_errors++;
> >>> +
> >>> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> >>> +
> >>> +		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;
> >>> +		}
> >>> +
> >>> +		if (error_factor & M16C_EF_ACKE)
> >>> +			cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
> >>> +					CAN_ERR_PROT_LOC_ACK_DEL);
> >>> +		if (error_factor & M16C_EF_CRCE)
> >>> +			cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> >>> +					CAN_ERR_PROT_LOC_CRC_DEL);
> >>> +		if (error_factor & M16C_EF_FORME)
> >>> +			cf->data[2] |= CAN_ERR_PROT_FORM;
> >>> +		if (error_factor & M16C_EF_STFE)
> >>> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
> >>> +		if (error_factor & M16C_EF_BITE0)
> >>> +			cf->data[2] |= CAN_ERR_PROT_BIT0;
> >>> +		if (error_factor & M16C_EF_BITE1)
> >>> +			cf->data[2] |= CAN_ERR_PROT_BIT1;
> >>> +		if (error_factor & M16C_EF_TRE)
> >>> +			cf->data[2] |= CAN_ERR_PROT_TX;
> >>> +	}
> >>> +
> >>> +	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 kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> >>> +				  const struct kvaser_msg *msg)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv;
> >>> +	struct can_frame *cf;
> >>> +	struct sk_buff *skb;
> >>> +	struct net_device_stats *stats;
> >>> +	u8 channel = msg->u.rx_can.channel;
> >>> +
> >>> +	if (channel >= dev->nchannels) {
> >>> +		dev_err(dev->udev->dev.parent,
> >>> +			"Invalid channel number (%d)\n", channel);
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	priv = dev->nets[channel];
> >>> +	stats = &priv->netdev->stats;
> >>> +
> >>> +	skb = alloc_can_skb(priv->netdev, &cf);
> >>> +	if (skb == NULL) {
> >>> +		stats->tx_dropped++;
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
> >>> +		     (msg->u.rx_can.msg[1] & 0x3f);
> >>> +	cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
> >>> +
> >>> +	if (msg->id == CMD_RX_EXT_MESSAGE) {
> >>> +		cf->can_id <<= 18;
> >>> +		cf->can_id += ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
> >>                            |=
> >>
> >> is more appropriate here
> > 
> > Ok.
> > 
> >>
> >>> +			      ((msg->u.rx_can.msg[3] & 0xff) << 6) |
> >>> +			      (msg->u.rx_can.msg[4] & 0x3f);
> >>> +		cf->can_id |= CAN_EFF_FLAG;
> >>> +	}
> >>> +
> >>> +	if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) {
> >>> +		cf->can_id |= CAN_RTR_FLAG;
> >>> +	} else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> >>> +						MSG_FLAG_NERR)) {
> >>> +		cf->can_id |= CAN_ERR_FLAG;
> >>> +		cf->can_dlc = CAN_ERR_DLC;
> >>
> >> What kind of error is this? Can you set cf->data? What about the
> >> original cd->can_id? What about the stats->rx_*error* stats?
> > 
> > Good question I've to take a look to this.
> > 
> >>
> >>> +	} else if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
> >>> +		cf->can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
> >>> +		cf->can_dlc = CAN_ERR_DLC;
> >>> +		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> >>> +
> >>> +		stats->rx_over_errors++;
> >>> +		stats->rx_errors++;
> >>> +	} else if (!msg->u.rx_can.flag) {
> >>> +		memcpy(cf->data, &msg->u.rx_can.msg[6], cf->can_dlc);
> >>> +	} else {
> >>> +		kfree_skb(skb);
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	netif_rx(skb);
> >>> +
> >>> +	stats->rx_packets++;
> >>> +	stats->rx_bytes += cf->can_dlc;
> >>> +}
> >>> +
> >>> +static void kvaser_usb_start_stop_chip_reply(const struct kvaser_usb *dev,
> >>> +					     const struct kvaser_msg *msg)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv;
> >>> +	u8 channel = msg->u.simple.channel;
> >>> +
> >>> +	if (channel >= dev->nchannels) {
> >>> +		dev_err(dev->udev->dev.parent,
> >>> +			"Invalid channel number (%d)\n", channel);
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	priv = dev->nets[channel];
> >>> +
> >>> +	complete(&priv->start_stop_comp);
> >>> +}
> >>> +
> >>> +static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
> >>> +				      const struct kvaser_msg *msg)
> >>> +{
> >>> +	switch (msg->id) {
> >>> +	case CMD_START_CHIP_REPLY:
> >>> +	case CMD_STOP_CHIP_REPLY:
> >>> +		kvaser_usb_start_stop_chip_reply(dev, msg);
> >>> +		break;
> >>> +
> >>> +	case CMD_RX_STD_MESSAGE:
> >>> +	case CMD_RX_EXT_MESSAGE:
> >>> +		kvaser_usb_rx_can_msg(dev, msg);
> >>> +		break;
> >>> +
> >>> +	case CMD_CHIP_STATE_EVENT:
> >>> +	case CMD_CAN_ERROR_EVENT:
> >>> +		kvaser_usb_rx_error(dev, msg);
> >>> +		break;
> >>> +
> >>> +	case CMD_TX_ACKNOWLEDGE:
> >>> +		kvaser_usb_tx_acknowledge(dev, msg);
> >>> +		break;
> >>> +
> >>> +	default:
> >>> +		dev_warn(dev->udev->dev.parent,
> >>> +			 "Unhandled message (%d)\n", msg->id);
> >>> +		break;
> >>> +	}
> >>> +}
> >>> +
> >>> +static void kvaser_usb_read_bulk_callback(struct urb *urb)
> >>> +{
> >>> +	struct kvaser_usb *dev = urb->context;
> >>> +	struct kvaser_msg *msg;
> >>> +	int pos = 0;
> >>> +	int err, i;
> >>> +
> >>> +	switch (urb->status) {
> >>> +	case 0:
> >>> +		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) {
> >>
> >> please check here for pos + sizeof(*msg), too
> > 
> > Same as above.
> > 
> >>
> >>> +		msg = urb->transfer_buffer + pos;
> >>> +
> >>> +		if (!msg->len)
> >>> +			break;
> >>> +
> >>> +		kvaser_usb_handle_message(dev, msg);
> >>> +
> >>> +		if (pos > urb->actual_length) {
> >>> +			dev_err(dev->udev->dev.parent, "Format error\n");
> >>> +			break;
> >>> +		}
> >>> +
> >>> +		pos += msg->len;
> >>> +	}
> >>> +
> >>> +resubmit_urb:
> >>> +	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 129),
> >>                                                                      ^^^
> >>
> >> use #define
> > 
> > Ok.
> > 
> >>
> >>> +			  urb->transfer_buffer, RX_BUFFER_SIZE,
> >>> +			  kvaser_usb_read_bulk_callback, dev);
> >>> +
> >>> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> >>> +	if (err == -ENODEV) {
> >>> +		for (i = 0; i < dev->nchannels; i++) {
> >>> +			if (!dev->nets[i])
> >>> +				continue;
> >>> +
> >>> +			netif_device_detach(dev->nets[i]->netdev);
> >>> +		}
> >>> +	} else if (err) {
> >>> +		dev_err(dev->udev->dev.parent,
> >>> +			"Failed resubmitting read bulk urb: %d\n", err);
> >>> +	}
> >>> +
> >>> +	return;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_setup_rx_urbs(struct kvaser_usb *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;
> >>> +
> >>> +		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");
> >>> +			usb_free_urb(urb);
> >>> +			err = -ENOMEM;
> >>> +			break;
> >>> +		}
> >>> +
> >>> +		usb_fill_bulk_urb(urb, dev->udev,
> >>> +				  usb_rcvbulkpipe(dev->udev, 129),
> >>
> >> use #define
> > 
> > Ok.
> > 
> >>
> >>> +				  buf, RX_BUFFER_SIZE,
> >>> +				  kvaser_usb_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);
> >>> +			break;
> >>> +		}
> >>> +
> >>> +		usb_free_urb(urb);
> >>> +	}
> >>> +
> >>> +	if (i == 0) {
> >>> +		dev_warn(dev->udev->dev.parent,
> >>> +			 "Cannot setup read URBs, error %d\n", err);
> >>> +		return err;
> >>> +	} else if (i < MAX_RX_URBS) {
> >>> +		dev_warn(dev->udev->dev.parent,
> >>> +			 "RX performances may be slow\n");
> >>> +	}
> >>> +
> >>> +	dev->rxinitdone = true;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_set_opt_mode(const struct kvaser_usb_net_priv *priv)
> >>> +{
> >>> +	struct kvaser_msg msg;
> >>> +
> >>> +	memset(&msg, 0x00, sizeof(msg));
> >>> +	msg.id = CMD_SET_CTRL_MODE;
> >>> +	msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_ctrl_mode);
> >>> +	msg.u.ctrl_mode.tid = 0xff;
> >>> +	msg.u.ctrl_mode.channel = priv->channel;
> >>
> >> please use C99 struct initializers
> > 
> > Ok.
> > 
> >>
> >>> +
> >>> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> >>> +		msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_SILENT;
> >>> +	else
> >>> +		msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_NORMAL;
> >>> +
> >>> +	return kvaser_usb_send_msg(priv->dev, &msg);
> >>> +}
> >>> +
> >>> +static int kvaser_usb_start_chip(struct kvaser_usb_net_priv *priv)
> >>> +{
> >>> +	int err;
> >>> +
> >>> +	init_completion(&priv->start_stop_comp);
> >>> +
> >>> +	err = kvaser_usb_send_simple_msg(priv->dev, CMD_START_CHIP,
> >>> +					 priv->channel);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	if (!wait_for_completion_timeout(&priv->start_stop_comp,
> >>> +					 msecs_to_jiffies(START_TIMEOUT)))
> >>> +		return -ETIMEDOUT;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_open(struct net_device *netdev)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> >>> +	struct kvaser_usb *dev = priv->dev;
> >>> +	int err;
> >>> +
> >>> +	err = open_candev(netdev);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	err = kvaser_usb_setup_rx_urbs(dev);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	err = kvaser_usb_set_opt_mode(priv);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	err = kvaser_usb_start_chip(priv);
> >>> +	if (err) {
> >>> +		netdev_warn(netdev, "Cannot start device, error %d\n", err);
> >>> +		close_candev(netdev);
> >>> +		return err;
> >>> +	}
> >>> +
> >>> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>> +	priv->open_time = jiffies;
> >>> +	netif_start_queue(netdev);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> >>> +{
> >>> +	int i;
> >>> +
> >>> +	usb_kill_anchored_urbs(&priv->tx_submitted);
> >>> +	atomic_set(&priv->active_tx_urbs, 0);
> >>> +
> >>> +	for (i = 0; i < MAX_TX_URBS; i++)
> >> ARRAY_SIZE(priv->tx_contexts) instead of MAX_TX_URBS
> > 
> > Ok.
> > 
> >>> +		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> >>> +}
> >>> +
> >>> +static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
> >>> +{
> >>> +	int i;
> >>> +
> >>> +	usb_kill_anchored_urbs(&dev->rx_submitted);
> >>> +
> >>> +	for (i = 0; i < MAX_NET_DEVICES; i++) {
> >> ARRAY_SIZE()
> >>> +		struct kvaser_usb_net_priv *priv = dev->nets[i];
> >>> +
> >>> +		if (priv)
> >>> +			kvaser_usb_unlink_tx_urbs(priv);
> >>> +	}
> >>> +}
> >>> +
> >>> +static int kvaser_usb_stop_chip(struct kvaser_usb_net_priv *priv)
> >>> +{
> >>> +	int err;
> >>> +
> >>> +	init_completion(&priv->start_stop_comp);
> >>> +
> >>> +	err = kvaser_usb_send_simple_msg(priv->dev, CMD_STOP_CHIP,
> >>> +					 priv->channel);
> >>> +	if (err)
> >>> +		return err;
> >>> +
> >>> +	if (!wait_for_completion_timeout(&priv->start_stop_comp,
> >>> +					 msecs_to_jiffies(STOP_TIMEOUT)))
> >>> +		return -ETIMEDOUT;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_flush_queue(struct kvaser_usb_net_priv *priv)
> >>> +{
> >>> +	struct kvaser_msg msg;
> >>> +
> >>> +	memset(&msg, 0x00, sizeof(msg));
> >>> +	msg.id = CMD_FLUSH_QUEUE;
> >>> +	msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_flush_queue);
> >>> +	msg.u.flush_queue.channel = priv->channel;
> >> C99 initialziers, please
> > 
> > Ok.
> > 
> >>> +
> >>> +	return kvaser_usb_send_msg(priv->dev, &msg);
> >>> +}
> >>> +
> >>> +static int kvaser_usb_close(struct net_device *netdev)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> >>> +	int err;
> >>> +
> >>> +	netif_stop_queue(netdev);
> >>> +
> >>> +	err = kvaser_usb_flush_queue(priv);
> >>> +	if (err)
> >>> +		netdev_warn(netdev, "Cannot flush queue, error %d\n", err);
> >>> +
> >>> +	err = kvaser_usb_stop_chip(priv);
> >>> +	if (err) {
> >>> +		netdev_warn(netdev, "Cannot stop device, error %d\n", err);
> >>> +		return err;
> >>> +	}
> >>> +
> >>> +	kvaser_usb_unlink_tx_urbs(priv);
> >>> +
> >>> +	priv->can.state = CAN_STATE_STOPPED;
> >>> +	close_candev(priv->netdev);
> >>> +	priv->open_time = 0;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void kvaser_usb_write_bulk_callback(struct urb *urb)
> >>> +{
> >>> +	struct kvaser_usb_tx_urb_context *context = urb->context;
> >>> +	struct kvaser_usb_net_priv *priv;
> >>> +	struct net_device *netdev;
> >>> +
> >>> +	if (WARN_ON(!context))
> >>> +		return;
> >>> +
> >>> +	priv = context->priv;
> >>> +	netdev = priv->netdev;
> >>> +
> >>> +	usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> >>> +			  urb->transfer_buffer, urb->transfer_dma);
> >>> +
> >>> +	if (!netif_device_present(netdev))
> >>> +		return;
> >>> +
> >>> +	if (urb->status)
> >>> +		netdev_info(netdev, "Tx URB aborted (%d)\n", urb->status);
> >>> +
> >>> +	netdev->trans_start = jiffies;
> >>
> >> Is trans_start needed? at least for non-usb devices it works without.
> > 
> > I don't know, I'll try to figure this out.
> > I see it's used in the two others CAN/USB drivers, 'ems_usb.c' and
> > 'esd_usb2.c'
> > 
> >>
> >>> +}
> >>> +
> >>> +static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> >>> +					 struct net_device *netdev)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> >>> +	struct kvaser_usb *dev = priv->dev;
> >>> +	struct net_device_stats *stats = &netdev->stats;
> >>> +	struct can_frame *cf = (struct can_frame *)skb->data;
> >>> +	struct kvaser_usb_tx_urb_context *context = NULL;
> >>> +	struct urb *urb;
> >>> +	void *buf;
> >>> +	struct kvaser_msg *msg;
> >>> +	int i, err;
> >>> +	int ret = NETDEV_TX_OK;
> >>> +
> >>> +	if (can_dropped_invalid_skb(netdev, skb))
> >>> +		return NETDEV_TX_OK;
> >>> +
> >>> +	urb = usb_alloc_urb(0, GFP_ATOMIC);
> >>> +	if (!urb) {
> >>> +		netdev_err(netdev, "No memory left for URBs\n");
> >>> +		stats->tx_dropped++;
> >>> +		dev_kfree_skb(skb);
> >>> +		goto nourbmem;
> >>> +	}
> >>> +
> >>> +	buf = usb_alloc_coherent(dev->udev, sizeof(struct kvaser_msg),
> >>> +				 GFP_ATOMIC, &urb->transfer_dma);
> >>> +	if (!buf) {
> >>> +		netdev_err(netdev, "No memory left for USB buffer\n");
> >>> +		stats->tx_dropped++;
> >>> +		dev_kfree_skb(skb);
> >>> +		goto nobufmem;
> >>> +	}
> >>> +
> >>> +	msg = (struct kvaser_msg *)buf;
> >>> +	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
> >>> +	msg->u.tx_can.flags = 0;
> >>> +	msg->u.tx_can.channel = priv->channel;
> >>> +
> >>> +	if (cf->can_id & CAN_EFF_FLAG) {
> >>> +		msg->id = CMD_TX_EXT_MESSAGE;
> >>> +		msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
> >>> +		msg->u.tx_can.msg[1] = (cf->can_id >> 18) & 0x3f;
> >>> +		msg->u.tx_can.msg[2] = (cf->can_id >> 14) & 0x0f;
> >>> +		msg->u.tx_can.msg[3] = (cf->can_id >> 6) & 0xff;
> >>> +		msg->u.tx_can.msg[4] = cf->can_id & 0x3f;
> >>> +	} else {
> >>> +		msg->id = CMD_TX_STD_MESSAGE;
> >>> +		msg->u.tx_can.msg[0] = (cf->can_id >> 6) & 0x1f;
> >>> +		msg->u.tx_can.msg[1] = cf->can_id & 0x3f;
> >>> +	}
> >>> +
> >>> +	msg->u.tx_can.msg[5] = cf->can_dlc;
> >>> +	memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
> >>> +
> >>> +	if (cf->can_id & CAN_RTR_FLAG)
> >>> +		msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
> >>> +
> >>> +	for (i = 0; i < MAX_TX_URBS; i++) {
> >> ARRAY_SIZE
> > 
> > Ok.
> > 
> >>> +		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> >>> +			context = &priv->tx_contexts[i];
> >>> +			break;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	if (!context) {
> >>> +		netdev_warn(netdev, "cannot find free context\n");
> >>> +		ret =  NETDEV_TX_BUSY;
> >>> +		goto releasebuf;
> >>> +	}
> >>> +
> >>> +	context->priv = priv;
> >>> +	context->echo_index = i;
> >>> +	context->dlc = cf->can_dlc;
> >>> +
> >>> +	msg->u.tx_can.tid = context->echo_index;
> >>> +
> >>> +	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 1),
> >>> +			  buf, msg->len,
> >>> +			  kvaser_usb_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_urbs);
> >>> +
> >>> +	if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
> >>> +		netif_stop_queue(netdev);
> >>> +
> >>> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> >>> +	if (unlikely(err)) {
> >>> +		can_free_echo_skb(netdev, context->echo_index);
> >>> +
> >>> +		atomic_dec(&priv->active_tx_urbs);
> >>> +		usb_unanchor_urb(urb);
> >>> +
> >>> +		stats->tx_dropped++;
> >>> +
> >>> +		if (err == -ENODEV)
> >>> +			netif_device_detach(netdev);
> >>> +		else
> >>> +			netdev_warn(netdev, "Failed tx_urb %d\n", err);
> >>> +
> >>> +		goto releasebuf;
> >>> +	}
> >>> +
> >>> +	netdev->trans_start = jiffies;
> >>> +
> >>> +	usb_free_urb(urb);
> >>> +
> >>> +	return NETDEV_TX_OK;
> >>> +
> >>> +releasebuf:
> >>> +	usb_free_coherent(dev->udev, sizeof(struct kvaser_msg),
> >>> +			  buf, urb->transfer_dma);
> >>> +nobufmem:
> >>> +	usb_free_urb(urb);
> >>> +nourbmem:
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static const struct net_device_ops kvaser_usb_netdev_ops = {
> >>> +	.ndo_open = kvaser_usb_open,
> >>> +	.ndo_stop = kvaser_usb_close,
> >>> +	.ndo_start_xmit = kvaser_usb_start_xmit,
> >>> +};
> >>> +
> >>> +static struct can_bittiming_const kvaser_usb_bittiming_const = {
> >>> +	.name = "kvaser_usb",
> >>> +	.tseg1_min = KVASER_USB_TSEG1_MIN,
> >>> +	.tseg1_max = KVASER_USB_TSEG1_MAX,
> >>> +	.tseg2_min = KVASER_USB_TSEG2_MIN,
> >>> +	.tseg2_max = KVASER_USB_TSEG2_MAX,
> >>> +	.sjw_max = KVASER_USB_SJW_MAX,
> >>> +	.brp_min = KVASER_USB_BRP_MIN,
> >>> +	.brp_max = KVASER_USB_BRP_MAX,
> >>> +	.brp_inc = KVASER_USB_BRP_INC,
> >>> +};
> >>> +
> >>> +static int kvaser_usb_set_bittiming(struct net_device *netdev)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> >>> +	struct can_bittiming *bt = &priv->can.bittiming;
> >>> +	struct kvaser_usb *dev = priv->dev;
> >>> +	struct kvaser_msg msg;
> >>> +
> >>> +	msg.id = CMD_SET_BUS_PARAMS;
> >>> +	msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_busparams);
> >>> +	msg.u.busparams.channel = priv->channel;
> >>> +	msg.u.busparams.tid = 0xff;
> >>> +	msg.u.busparams.bitrate = bt->bitrate;
> >>
> >> bitrate is le32
> > 
> > Indeed ! I'll fix this.
> > 
> >>
> >>> +	msg.u.busparams.sjw = bt->sjw;
> >>> +	msg.u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1;
> >>> +	msg.u.busparams.tseg2 = bt->phase_seg2;
> >>
> >> C99 initializers, please
> >>
> >>> +
> >>> +	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> >>> +		msg.u.busparams.no_samp = 3;
> >>> +	else
> >>> +		msg.u.busparams.no_samp = 1;
> >>> +
> >>> +	return kvaser_usb_send_msg(dev, &msg);
> >>> +}
> >>> +
> >>> +static int kvaser_usb_set_mode(struct net_device *netdev,
> >>> +			       enum can_mode mode)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> >>> +
> >>> +	if (!priv->open_time)
> >>> +		return -EINVAL;
> >>> +
> >>> +	switch (mode) {
> >>> +	case CAN_MODE_START:
> >>> +		if (netif_queue_stopped(netdev))
> >>> +			netif_wake_queue(netdev);
> >>
> >> No need to restart your USB device?
> > 
> > No. I don't think so.
> > The module continuously tries to transmit the frame and isn't stopped.
> > So there is no need to restart it if it has been explicitely stopped.
> > 
> > When it cannot transmit, the module try again and sends continuously
> > CMD_CAN_ERROR_EVENT frames until it succeeds to transmit the frame.
> > If the device is stopped with the command CMD_STOP_CHIP then it stops
> > sending these CMD_CAN_ERROR_EVENT.
> > Should I handle this in another manner?
> 
> If the firmware automatically recovers from busoff (like the at91 does),
> you should stop the chip it priv->can.restart_ms == 0 and let the chip
> continue working otherwise.
> 
> > 
> >>
> >>> +		break;
> >>> +	default:
> >>> +		return -EOPNOTSUPP;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_get_berr_counter(const struct net_device *netdev,
> >>> +				       struct can_berr_counter *bec)
> >>> +{
> >>> +	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> >>> +
> >>> +	bec->txerr = priv->bec.txerr;
> >>> +	bec->rxerr = priv->bec.rxerr;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_init_one(struct usb_interface *intf,
> >>> +			       const struct usb_device_id *id, int channel)
> >>> +{
> >>> +	struct kvaser_usb *dev = usb_get_intfdata(intf);
> >>> +	struct net_device *netdev;
> >>> +	struct kvaser_usb_net_priv *priv;
> >>> +	int i, err;
> >>> +
> >>> +	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
> >>> +	if (!netdev) {
> >>> +		dev_err(&intf->dev, "Cannot alloc candev\n");
> >>> +		return -ENOMEM;
> >>> +	}
> >>> +
> >>> +	priv = netdev_priv(netdev);
> >>> +
> >>> +	init_usb_anchor(&priv->tx_submitted);
> >>> +	atomic_set(&priv->active_tx_urbs, 0);
> >>> +
> >>> +	for (i = 0; i < MAX_TX_URBS; i++)
> >>> +		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> >>> +
> >>> +	priv->dev = dev;
> >>> +	priv->netdev = netdev;
> >>> +	priv->channel = channel;
> >>> +
> >>> +	priv->can.state = CAN_STATE_STOPPED;
> >>> +	priv->can.clock.freq = CAN_USB_CLOCK;
> >>> +	priv->can.bittiming_const = &kvaser_usb_bittiming_const;
> >>> +	priv->can.do_set_bittiming = kvaser_usb_set_bittiming;
> >>> +	priv->can.do_set_mode = kvaser_usb_set_mode;
> >>> +	priv->can.do_get_berr_counter = kvaser_usb_get_berr_counter;
> >>> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
> >>> +	if (id->driver_info & KVASER_HAS_SILENT_MODE)
> >>> +		priv->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
> >>> +
> >>> +	netdev->flags |= IFF_ECHO;
> >>> +
> >>> +	netdev->netdev_ops = &kvaser_usb_netdev_ops;
> >>> +
> >>> +	SET_NETDEV_DEV(netdev, &intf->dev);
> >>> +
> >>> +	err = register_candev(netdev);
> >>> +	if (err) {
> >>> +		dev_err(&intf->dev, "Failed to register can device\n");
> >>> +		free_candev(netdev);
> >>> +		return err;
> >>> +	}
> >>> +
> >>> +	dev->nets[channel] = priv;
> >>> +	netdev_info(netdev, "device %s registered\n", netdev->name);
> >>
> >> netdev_info should take care of printing the device's name.
> > 
> > Ok.
> > 
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int kvaser_usb_probe(struct usb_interface *intf,
> >>> +			    const struct usb_device_id *id)
> >>> +{
> >>> +	struct kvaser_usb *dev;
> >>> +	int err = -ENOMEM;
> >>> +	int i;
> >>> +
> >>> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >>
> >> Who will free dev on driver unload? Please make use of devm_kzalloc().
> > 
> > Ok. kfree is missing is disconnect().
> > I'll replace it by devm_kzalloc() and devm_free().
> 
> The beauty of devm_kzalloc is you don't have to call *_free, its
> automatically called if probe fails or when remove function has been called.

Cool :-)

> 
> > 
> >>
> >>> +	if (!dev)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	dev->udev = interface_to_usbdev(intf);
> >>> +
> >>> +	init_usb_anchor(&dev->rx_submitted);
> >>> +
> >>> +	usb_set_intfdata(intf, dev);
> >>> +
> >>> +	if (kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, 0)) {
> >>> +		dev_err(&intf->dev, "Cannot reset kvaser\n");
> >>> +		goto error;
> >>> +	}
> >>> +
> >>> +	if (kvaser_usb_get_software_info(dev)) {
> >>> +		dev_err(&intf->dev, "Cannot get software infos\n");
> >>> +		goto error;
> >>> +	}
> >>> +
> >>> +	if (kvaser_usb_get_card_info(dev)) {
> >>> +		dev_err(&intf->dev, "Cannot get card infos\n");
> >>> +		goto error;
> >>> +	}
> >>> +
> >>> +	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
> >>> +		 ((dev->fw_version >> 24) & 0xff),
> >>> +		 ((dev->fw_version >> 16) & 0xff),
> >>> +		 (dev->fw_version & 0xffff));
> >>> +
> >>> +	for (i = 0; i < dev->nchannels; i++)
> >>> +		kvaser_usb_init_one(intf, id, i);
> >>> +
> >>> +	return 0;
> >>> +
> >>> +error:
> >>> +	kfree(dev);
> >>> +	return err;
> >>> +}
> >>> +
> >>> +static void kvaser_usb_disconnect(struct usb_interface *intf)
> >>> +{
> >>> +	struct kvaser_usb *dev = usb_get_intfdata(intf);
> >>> +	int i;
> >>> +
> >>> +	usb_set_intfdata(intf, NULL);
> >>> +
> >>> +	if (!dev)
> >>> +		return;
> >>> +
> >>> +	for (i = 0; i < dev->nchannels; i++) {
> >>> +		if (!dev->nets[i])
> >>> +			continue;
> >>> +
> >>> +		unregister_netdev(dev->nets[i]->netdev);
> >>> +		free_candev(dev->nets[i]->netdev);
> >>> +	}
> >>> +
> >>> +	kvaser_usb_unlink_all_urbs(dev);
> >>> +}
> >>> +
> >>> +static struct usb_driver kvaser_usb_driver = {
> >>> +	.name = "kvaser_usb",
> >>> +	.probe = kvaser_usb_probe,
> >>> +	.disconnect = kvaser_usb_disconnect,
> >>> +	.id_table = kvaser_usb_table
> >>> +};
> >>> +
> >>> +module_usb_driver(kvaser_usb_driver);
> >>> +
> >>> +MODULE_AUTHOR("Olivier Sobrie <[email-masked]>");
> >>> +MODULE_DESCRIPTION("Can driver for Kvaser CAN/USB devices");
> >>> +MODULE_LICENSE("GPL v2");
> >>> diff --git a/drivers/net/can/usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb.h
> >>> new file mode 100644
> >>> index 0000000..8e0b6ab
> >>> --- /dev/null
> >>> +++ b/drivers/net/can/usb/kvaser_usb.h
> >>> @@ -0,0 +1,237 @@
> >>> +#ifndef _KVASER_USB_H_
> >>> +#define _KVASER_USB_H_
> >>> +
> >>> +#define	MAX_TX_URBS			16
> >> Please no tab between define and macro name
> > 
> > Ok I didn't know it was not allowed... checkpatch didn't complain.
> 
> It's allowed, but not used without tab it's more common, at least among
> CAN drivers.
> 
> > 
> >>> +#define	MAX_RX_URBS			4
> >>> +#define	START_TIMEOUT			1000
> >>> +#define	STOP_TIMEOUT			1000
> >>> +#define	USB_SEND_TIMEOUT		1000
> >>> +#define	USB_RECEIVE_TIMEOUT		1000
> >>> +#define	RX_BUFFER_SIZE			3072
> >>> +#define	CAN_USB_CLOCK			8000000
> >>> +#define	MAX_NET_DEVICES			3
> >>> +
> >>> +/* Kvaser USB devices */
> >>> +#define	KVASER_VENDOR_ID		0x0bfd
> >>> +#define	USB_LEAF_DEVEL_PRODUCT_ID	10
> >>> +#define	USB_LEAF_LITE_PRODUCT_ID	11
> >>> +#define	USB_LEAF_PRO_PRODUCT_ID		12
> >>> +#define	USB_LEAF_SPRO_PRODUCT_ID	14
> >>> +#define	USB_LEAF_PRO_LS_PRODUCT_ID	15
> >>> +#define	USB_LEAF_PRO_SWC_PRODUCT_ID	16
> >>> +#define	USB_LEAF_PRO_LIN_PRODUCT_ID	17
> >>> +#define	USB_LEAF_SPRO_LS_PRODUCT_ID	18
> >>> +#define	USB_LEAF_SPRO_SWC_PRODUCT_ID	19
> >>> +#define	USB_MEMO2_DEVEL_PRODUCT_ID	22
> >>> +#define	USB_MEMO2_HSHS_PRODUCT_ID	23
> >>> +#define	USB_UPRO_HSHS_PRODUCT_ID	24
> >>> +#define	USB_LEAF_LITE_GI_PRODUCT_ID	25
> >>> +#define	USB_LEAF_PRO_OBDII_PRODUCT_ID	26
> >>> +#define	USB_MEMO2_HSLS_PRODUCT_ID	27
> >>> +#define	USB_LEAF_LITE_CH_PRODUCT_ID	28
> >>> +#define	USB_BLACKBIRD_SPRO_PRODUCT_ID	29
> >>> +#define	USB_OEM_MERCURY_PRODUCT_ID	34
> >>> +#define	USB_OEM_LEAF_PRODUCT_ID		35
> >>> +#define	USB_CAN_R_PRODUCT_ID		39
> >>> +
> >>> +/* USB devices features */
> >>> +#define	KVASER_HAS_SILENT_MODE		(1 << 0)
> >> pleae use BIT(0)
> >>> +
> >>> +/* Message header size */
> >>> +#define	MSG_HEADER_LEN			2
> >>> +
> >>> +/* Can message flags */
> >>> +#define	MSG_FLAG_ERROR_FRAME		(1 << 0)
> >>> +#define	MSG_FLAG_OVERRUN		(1 << 1)
> >>> +#define	MSG_FLAG_NERR			(1 << 2)
> >>> +#define	MSG_FLAG_WAKEUP			(1 << 3)
> >>> +#define	MSG_FLAG_REMOTE_FRAME		(1 << 4)
> >>> +#define	MSG_FLAG_RESERVED		(1 << 5)
> >>> +#define	MSG_FLAG_TX_ACK			(1 << 6)
> >>> +#define	MSG_FLAG_TX_REQUEST		(1 << 7)
> >>> +
> >>> +/* Can states */
> >>> +#define	M16C_STATE_BUS_RESET		(1 << 0)
> >>> +#define	M16C_STATE_BUS_ERROR		(1 << 4)
> >>> +#define	M16C_STATE_BUS_PASSIVE		(1 << 5)
> >>> +#define	M16C_STATE_BUS_OFF		(1 << 6)
> >>> +
> >>> +/* Can msg ids */
> >>> +#define	CMD_RX_STD_MESSAGE		12
> >>> +#define	CMD_TX_STD_MESSAGE		13
> >>> +#define	CMD_RX_EXT_MESSAGE		14
> >>> +#define	CMD_TX_EXT_MESSAGE		15
> >>> +#define	CMD_SET_BUS_PARAMS		16
> >>> +#define	CMD_GET_BUS_PARAMS		17
> >>> +#define	CMD_GET_BUS_PARAMS_REPLY	18
> >>> +#define	CMD_GET_CHIP_STATE		19
> >>> +#define	CMD_CHIP_STATE_EVENT		20
> >>> +#define	CMD_SET_CTRL_MODE		21
> >>> +#define	CMD_GET_CTRL_MODE		22
> >>> +#define	CMD_GET_CTRL_MODE_REPLY		23
> >>> +#define	CMD_RESET_CHIP			24
> >>> +#define	CMD_RESET_CHIP_REPLY		25
> >>> +#define	CMD_START_CHIP			26
> >>> +#define	CMD_START_CHIP_REPLY		27
> >>> +#define	CMD_STOP_CHIP			28
> >>> +#define	CMD_STOP_CHIP_REPLY		29
> >>> +#define	CMD_GET_CARD_INFO2		32
> >>> +#define	CMD_GET_CARD_INFO		34
> >>> +#define	CMD_GET_CARD_INFO_REPLY		35
> >>> +#define	CMD_GET_SOFTWARE_INFO		38
> >>> +#define	CMD_GET_SOFTWARE_INFO_REPLY	39
> >>> +#define	CMD_ERROR_EVENT			45
> >>> +#define	CMD_FLUSH_QUEUE			48
> >>> +#define	CMD_TX_ACKNOWLEDGE		50
> >>> +#define	CMD_CAN_ERROR_EVENT		51
> >>> +#define	CMD_USB_THROTTLE		77
> >>> +
> >>> +/* error factors */
> >>> +#define	M16C_EF_ACKE			(1 << 0)
> >>> +#define	M16C_EF_CRCE			(1 << 1)
> >>> +#define	M16C_EF_FORME			(1 << 2)
> >>> +#define	M16C_EF_STFE			(1 << 3)
> >>> +#define	M16C_EF_BITE0			(1 << 4)
> >>> +#define	M16C_EF_BITE1			(1 << 5)
> >>> +#define	M16C_EF_RCVE			(1 << 6)
> >>> +#define	M16C_EF_TRE			(1 << 7)
> >>> +
> >>> +/* bittiming parameters */
> >>> +#define	KVASER_USB_TSEG1_MIN		1
> >>> +#define	KVASER_USB_TSEG1_MAX		16
> >>> +#define	KVASER_USB_TSEG2_MIN		1
> >>> +#define	KVASER_USB_TSEG2_MAX		8
> >>> +#define	KVASER_USB_SJW_MAX		4
> >>> +#define	KVASER_USB_BRP_MIN		1
> >>> +#define	KVASER_USB_BRP_MAX		64
> >>> +#define	KVASER_USB_BRP_INC		1
> >>> +
> >>> +/* ctrl modes */
> >>> +#define	KVASER_CTRL_MODE_NORMAL		1
> >>> +#define	KVASER_CTRL_MODE_SILENT		2
> >>> +#define	KVASER_CTRL_MODE_SELFRECEPTION	3
> >>> +#define	KVASER_CTRL_MODE_OFF		4
> >>> +
> >>> +struct kvaser_msg_simple {
> >>> +	u8 tid;
> >>> +	u8 channel;
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_cardinfo {
> >>> +	u8 tid;
> >>> +	u8 nchannels;
> >>> +	__le32 serial_number;
> >>> +	__le32 padding;
> >>> +	__le32 clock_resolution;
> >>> +	__le32 mfgdate;
> >>> +	u8 ean[8];
> >>> +	u8 hw_revision;
> >>> +	u8 usb_hs_mode;
> >>> +	__le16 padding2;
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_cardinfo2 {
> >>> +	u8 tid;
> >>> +	u8 channel;
> >>> +	u8 pcb_id[24];
> >>> +	__le32 oem_unlock_code;
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_softinfo {
> >>> +	u8 tid;
> >>> +	u8 channel;
> >>> +	__le32 sw_options;
> >>> +	__le32 fw_version;
> >>> +	__le16 max_outstanding_tx;
> >>> +	__le16 padding[9];
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_busparams {
> >>> +	u8 tid;
> >>> +	u8 channel;
> >>> +	__le32 bitrate;
> >>> +	u8 tseg1;
> >>> +	u8 tseg2;
> >>> +	u8 sjw;
> >>> +	u8 no_samp;
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_tx_can {
> >>> +	u8 channel;
> >>> +	u8 tid;
> >>> +	u8 msg[14];
> >>> +	u8 padding;
> >>> +	u8 flags;
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_rx_can {
> >>> +	u8 channel;
> >>> +	u8 flag;
> >>> +	__le16 time[3];
> >>> +	u8 msg[14];
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_chip_state_event {
> >>> +	u8 tid;
> >>> +	u8 channel;
> >>> +	__le16 time[3];
> >>> +	u8 tx_errors_count;
> >>> +	u8 rx_errors_count;
> >>> +	u8 status;
> >>> +	u8 padding[3];
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_tx_acknowledge {
> >>> +	u8 channel;
> >>> +	u8 tid;
> >>> +	__le16 time[3];
> >>> +	u8 flags;
> >>> +	u8 time_offset;
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_error_event {
> >>> +	u8 tid;
> >>> +	u8 flags;
> >>> +	__le16 time[3];
> >>> +	u8 channel;
> >>> +	u8 padding;
> >>> +	u8 tx_errors_count;
> >>> +	u8 rx_errors_count;
> >>> +	u8 status;
> >>> +	u8 error_factor;
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_ctrl_mode {
> >>> +	u8 tid;
> >>> +	u8 channel;
> >>> +	u8 ctrl_mode;
> >>> +	u8 padding[3];
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg_flush_queue {
> >>> +	u8 tid;
> >>> +	u8 channel;
> >>> +	u8 flags;
> >>> +	u8 padding[3];
> >>> +} __packed;
> >>> +
> >>> +struct kvaser_msg {
> >>> +	u8 len;
> >>> +	u8 id;
> >>> +	union	{
> >>> +		struct kvaser_msg_simple simple;
> >>> +		struct kvaser_msg_cardinfo cardinfo;
> >>> +		struct kvaser_msg_cardinfo2 cardinfo2;
> >>> +		struct kvaser_msg_softinfo softinfo;
> >>> +		struct kvaser_msg_busparams busparams;
> >>> +		struct kvaser_msg_tx_can tx_can;
> >>> +		struct kvaser_msg_rx_can rx_can;
> >>> +		struct kvaser_msg_chip_state_event chip_state_event;
> >>> +		struct kvaser_msg_tx_acknowledge tx_acknowledge;
> >>> +		struct kvaser_msg_error_event error_event;
> >>> +		struct kvaser_msg_ctrl_mode ctrl_mode;
> >>> +		struct kvaser_msg_ctrl_mode flush_queue;
> >>> +	} u;
> >>> +} __packed;
> >>> +
> >>> +#endif
> >>>
> >>
> >>
> >> -- 
> >> Pengutronix e.K.                  | Marc Kleine-Budde           |
> >> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> >> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> >> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> >>
> > 
> > Thanks for the review.
> np,
> 
> regards, Marc
> 
> -- 
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> 

Thanks,

-- 
Olivier