Re: [PATCH] usb2can: Add support for USB2CAN interface from 8 devices

[Wolfgang Grandegger <wg@grandegger.com>]

Hi Bernd,

nice to see this driver being pushed mainline. As Oliver already pointed
out, there are a few general naming and coding style issues:

- usb2can is to general, a more specific name would be nice, e.g.
  usb_8dev, also as prefix for macro, function and variable names.
  [For the latter, the name is not allowed to start with a number and
  therefore the prefix 8dev_usb is not an option].

- Tabs are OK just for aligning macro definition. In all other case
  please use just *one* space, e.g.:

  s/int    err;/int err;/
  s/outmsg.opt1    = USB2CAN/outmsg.opt1 = USB2CAN/

- The preferred multi line comment styles is:

  /*
   * A Comment
   */

- The patch does not yet apply to David Millers "net-next" GIT tree.
  There are problems with Kconfig and Makefile.

- Care about casts. Try to avoid them.

- Sooner than later, you should also add a CC to the Linux USB ml.

More comments inline...

On 12/02/2012 10:25 AM, krumboeck@universalnet.at wrote:
> Add device driver for USB2CAN interface from "8 devices"
> (http://www.8devices.com).
> 
> Signed-off-by: Bernd Krumboeck <krumboeck@universalnet.at>
> ---
>  drivers/net/can/usb/Kconfig   |    6 +
>  drivers/net/can/usb/Makefile  |    1 +
>  drivers/net/can/usb/usb2can.c | 1323
> +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1330 insertions(+)
>  create mode 100644 drivers/net/can/usb/usb2can.c
> 
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index a4e4bee..2068c99 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -48,4 +48,10 @@ config CAN_PEAK_USB
>        This driver supports the PCAN-USB and PCAN-USB Pro adapters
>        from PEAK-System Technik (http://www.peak-system.com).
> 
> +config CAN_USB2CAN
> +    tristate "8 devices USB2CAN interface"
> +    ---help---
> +      This driver supports the USB2CAN interface
> +      from 8 devices (http://www.8devices.com).
> +
>  endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 80a2ee4..3c0a378 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -6,5 +6,6 @@ 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/
> +obj-$(CONFIG_CAN_USB2CAN) += usb2can.o
> 
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/usb/usb2can.c b/drivers/net/can/usb/usb2can.c
> new file mode 100644
> index 0000000..5a09141
> --- /dev/null
> +++ b/drivers/net/can/usb/usb2can.c
> @@ -0,0 +1,1323 @@
> +/*
> + * CAN driver for UAB "8 devices" USB2CAN converter
> + *
> + * Copyright (C) 2012 Bernd Krumboeck (krumboeck@universalnet.at)
> + *
> + * 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.
> + *
> + * This driver is based on the 3.2.0 version of
> drivers/net/can/usb/ems_usb.c
> + * and drivers/net/can/usb/esd_usb2.c

If you say "based on", you should honor the copyrights. "inspired by"
sounds better or just drop these lines.

Please check if there are fixes since 3.2.0.

> + *
> + * Many thanks to Gerhard Bertelsmann (info@gerhard-bertelsmann.de)
> + * for testing and fixing this driver. Also many thanks to "8 devices",
> + * who were very cooperative and answered my questions.
> + *
> + */
> +
> +#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>
> +
> +
> +/* driver constants */
> +#define MAX_RX_URBS            10
> +#define MAX_TX_URBS            10
> +#define RX_BUFFER_SIZE            64
> +
> +/* vendor and product id */
> +#define USB2CAN_VENDOR_ID        0x0483
> +#define USB2CAN_PRODUCT_ID        0x1234
> +
> +/* bittiming constants */
> +#define USB2CAN_ABP_CLOCK        32000000
> +#define USB2CAN_BAUD_MANUAL        0x09
> +#define USB2CAN_TSEG1_MIN        1
> +#define USB2CAN_TSEG1_MAX        16
> +#define USB2CAN_TSEG2_MIN        1
> +#define USB2CAN_TSEG2_MAX        8
> +#define USB2CAN_SJW_MAX            4
> +#define USB2CAN_BRP_MIN            1
> +#define USB2CAN_BRP_MAX            1024
> +#define USB2CAN_BRP_INC            1
> +
> +/* setup flags */
> +#define USB2CAN_SILENT            0x00000001
> +#define USB2CAN_LOOPBACK        0x00000002
> +#define USB2CAN_DISABLE_AUTO_RESTRANS    0x00000004
> +#define USB2CAN_STATUS_FRAME        0x00000008
> +
> +/* commands */
> +#define USB2CAN_RESET            1
> +#define USB2CAN_OPEN            2
> +#define USB2CAN_CLOSE            3
> +#define USB2CAN_SET_SPEED        4
> +#define USB2CAN_SET_MASK_FILTER        5
> +#define USB2CAN_GET_STATUS        6
> +#define USB2CAN_GET_STATISTICS        7
> +#define USB2CAN_GET_SERIAL        8
> +#define USB2CAN_GET_SOFTW_VER        9
> +#define USB2CAN_GET_HARDW_VER        10
> +#define USB2CAN_RESET_TIMESTAMP        11
> +#define USB2CAN_GET_SOFTW_HARDW_VER    12

This looks like an enumeration.

> +#define USB2CAN_CMD_START        0x11
> +#define USB2CAN_CMD_END            0x22
> +
> +#define USB2CAN_CMD_SUCCESS        0
> +#define USB2CAN_CMD_ERROR        255
> +
> +/* statistics */
> +#define USB2CAN_STAT_RX_FRAMES        0
> +#define USB2CAN_STAT_RX_BYTES        1
> +#define USB2CAN_STAT_TX_FRAMES        2
> +#define USB2CAN_STAT_TX_BYTES        3
> +#define USB2CAN_STAT_OVERRUNS        4
> +#define USB2CAN_STAT_WARNINGS        5
> +#define USB2CAN_STAT_BUS_OFF        6
> +#define USB2CAN_STAT_RESET_STAT        7
> +
> +/* frames */
> +#define USB2CAN_DATA_START        0x55
> +#define USB2CAN_DATA_END        0xAA
> +
> +#define USB2CAN_TYPE_CAN_FRAME        0
> +#define USB2CAN_TYPE_ERROR_FRAME    3
> +
> +#define USB2CAN_EXTID            0x01
> +#define USB2CAN_RTR            0x02
> +#define USB2CAN_ERR_FLAG        0x04
> +
> +/* status */
> +#define USB2CAN_STATUSMSG_OK        0x00  /* Normal condition. */
> +#define USB2CAN_STATUSMSG_OVERRUN    0x01  /* Overrun occured when
> sending */
> +#define USB2CAN_STATUSMSG_BUSLIGHT    0x02  /* Error counter has
> reached 96 */
> +#define USB2CAN_STATUSMSG_BUSHEAVY    0x03  /* Error count. has reached
> 128 */
> +#define USB2CAN_STATUSMSG_BUSOFF    0x04  /* Device is in BUSOFF */
> +#define USB2CAN_STATUSMSG_STUFF        0x20  /* Stuff Error */
> +#define USB2CAN_STATUSMSG_FORM        0x21  /* Form Error */
> +#define USB2CAN_STATUSMSG_ACK        0x23  /* Ack Error */
> +#define USB2CAN_STATUSMSG_BIT0        0x24  /* Bit1 Error */
> +#define USB2CAN_STATUSMSG_BIT1        0x25  /* Bit0 Error */
> +#define USB2CAN_STATUSMSG_CRC        0x26  /* CRC Error */
> +
> +#define USB2CAN_RP_MASK            0x7F  /* Mask for Receive Error Bit */
> +
> +
> +/* table of devices that work with this driver */
> +static struct usb_device_id usb2can_table[] = {
> +    { USB_DEVICE(USB2CAN_VENDOR_ID, USB2CAN_PRODUCT_ID) },
> +    { }                    /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, usb2can_table);
> +
> +struct usb2can_tx_urb_context {
> +    struct usb2can *dev;
> +
> +    u32 echo_index;
> +    u8 dlc;
> +};
> +
> +/* Structure to hold all of our device specific stuff */
> +struct usb2can {
> +    struct can_priv can; /* must be the first member */
> +
> +    struct sk_buff *echo_skb[MAX_TX_URBS];
> +
> +    struct usb_device *udev;
> +    struct net_device *netdev;
> +
> +    atomic_t active_tx_urbs;
> +    struct usb_anchor tx_submitted;
> +    struct usb2can_tx_urb_context tx_contexts[MAX_TX_URBS];
> +
> +    struct usb_anchor rx_submitted;
> +
> +    u8 *cmd_msg_buffer;
> +
> +    unsigned int free_slots; /* remember number of available slots */
> +
> +    unsigned int dar; /* disable automatic restransmission */
> +
> +    struct mutex usb2can_cmd_lock;
> +};
> +
> +/* tx frame */
> +struct __packed usb2can_tx_msg {
> +    u8 begin;
> +    u8 flags;    /* RTR and EXT_ID flag */
> +    __be32 id;    /* upper 3 bits not used */
> +    u8 dlc;        /* data length code 0-8 bytes */
> +    u8 data[8];    /* 64-bit data */
> +    u8 end;
> +};
> +
> +/* rx frame */
> +struct __packed usb2can_rx_msg {
> +    u8 begin;
> +    u8 type;        /* frame type */
> +    u8 flags;        /* RTR and EXT_ID flag */
> +    __be32 id;        /* upper 3 bits not used */
> +    u8 dlc;            /* data length code 0-8 bytes */
> +    u8 data[8];        /* 64-bit data */
> +    __be32 timestamp;    /* 32-bit timestamp */
> +    u8 end;
> +};
> +
> +/* command frame */
> +struct __packed usb2can_cmd_msg {
> +    u8 begin;
> +    u8 channel;    /* unkown - always 0 */
> +    u8 command;    /* command to execute */
> +    u8 opt1;    /* optional parameter / return value */
> +    u8 opt2;    /* optional parameter 2 */
> +    u8 data[10];    /* optional parameter and data */
> +    u8 end;
> +};
> +
> +static struct usb_driver usb2can_driver;
> +
> +static int usb2can_send_cmd_msg(struct usb2can *dev, u8 *msg, int size)
> +{
> +    int actual_length;
> +
> +    return usb_bulk_msg(dev->udev,
> +                usb_sndbulkpipe(dev->udev, 4),
> +                msg,
> +                size,
> +                &actual_length,
> +                1000);

Does fit on less lines!

> +}
> +
> +static int usb2can_wait_cmd_msg(struct usb2can *dev, u8 *msg, int size,
> +                int *actual_length)
> +{
> +    return usb_bulk_msg(dev->udev,
> +                usb_rcvbulkpipe(dev->udev, 3),
> +                msg,
> +                size,
> +                actual_length,
> +                1000);

Ditto

> +}
> +
> +/* Send command to device and receive result.
> + * Command was successful When opt1 = 0.
> + */
> +static int usb2can_send_cmd(struct usb2can *dev, struct usb2can_cmd_msg
> *out,
> +                struct usb2can_cmd_msg *in)
> +{
> +    int    err;
> +    int    nBytesRead;

Please use the usual variable name style. All lowercase, eventuell with
"_". Here and in other places as well.

> +    struct net_device *netdev;
> +
> +    netdev = dev->netdev;
> +
> +    out->begin = USB2CAN_CMD_START;
> +    out->end = USB2CAN_CMD_END;
> +
> +    memcpy(&dev->cmd_msg_buffer[0], out,
> +        sizeof(struct usb2can_cmd_msg));
> +
> +    mutex_lock(&dev->usb2can_cmd_lock);
> +
> +    err = usb2can_send_cmd_msg(dev, &dev->cmd_msg_buffer[0],
> +                   sizeof(struct usb2can_cmd_msg));
> +    if (err < 0) {
> +        dev_err(netdev->dev.parent, "sending command message failed\n");
> +        return err;
> +    }
> +
> +    err = usb2can_wait_cmd_msg(dev, &dev->cmd_msg_buffer[0],
> +                   sizeof(struct usb2can_cmd_msg), &nBytesRead);
> +    if (err < 0) {
> +        dev_err(netdev->dev.parent, "no command message answer\n");
> +        return err;
> +    }
> +
> +    mutex_unlock(&dev->usb2can_cmd_lock);
> +
> +    memcpy(in, &dev->cmd_msg_buffer[0],
> +        sizeof(struct usb2can_cmd_msg));
> +
> +    if (in->begin != USB2CAN_CMD_START || in->end != USB2CAN_CMD_END ||
> +            nBytesRead != 16 || in->opt1 != 0)
> +        return -EPROTO;
> +
> +    return 0;
> +}
> +
> +/* Send open command to device */
> +static int usb2can_cmd_open(struct usb2can *dev)
> +{
> +    struct can_bittiming *bt = &dev->can.bittiming;
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +    u32 flags = 0x00000000;
> +    u32 beflags;
> +    u16 bebrp;
> +    u32 ctrlmode = dev->can.ctrlmode;
> +
> +    if (ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +        flags |= USB2CAN_LOOPBACK;
> +    if (ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +        flags |= USB2CAN_SILENT;
> +    if (dev->dar == 1)
> +        flags |= USB2CAN_DISABLE_AUTO_RESTRANS;
> +
> +    flags |= USB2CAN_STATUS_FRAME;
> +
> +    memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +    outmsg.command = USB2CAN_OPEN;
> +    outmsg.opt1    = USB2CAN_BAUD_MANUAL;
> +    outmsg.data[0] = (u8) (bt->prop_seg + bt->phase_seg1);
> +    outmsg.data[1] = (u8) bt->phase_seg2;
> +    outmsg.data[2] = (u8) bt->sjw;

I don't think you need the casts here.

> +    /* BRP */
> +    bebrp = cpu_to_be16((u16) bt->brp);
> +    memcpy(&outmsg.data[3], &bebrp, sizeof(bebrp));
> +
> +    /* flags */
> +    beflags = cpu_to_be32(flags);
> +    memcpy(&outmsg.data[5], &beflags, sizeof(beflags));
> +
> +    return usb2can_send_cmd(dev, &outmsg, &inmsg);
> +}
> +
> +/* Send close command to device */
> +static int usb2can_cmd_close(struct usb2can *dev)
> +{
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +
> +    memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +    outmsg.command = USB2CAN_CLOSE;
> +
> +    return usb2can_send_cmd(dev, &outmsg, &inmsg);
> +}
> +
> +/* Get firmware and hardware version */
> +static int usb2can_cmd_version(struct usb2can *dev, u32 *res)
> +{
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +    int err = 0;
> +    u32 *value;
> +
> +    memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +    outmsg.command = USB2CAN_GET_SOFTW_HARDW_VER;
> +
> +    err = usb2can_send_cmd(dev, &outmsg, &inmsg);
> +    if (err)
> +        return err;
> +
> +    value = (u32 *) inmsg.data;
> +    *res = be32_to_cpu(*value);

I do not see a need for the extra variable "value" here.

> +
> +    return err;
> +}
> +
> +/* Get firmware version */
> +static ssize_t show_firmware(struct device *d, struct device_attribute
> *attr,
> +                 char *buf)
> +{
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +    int err = 0;
> +    u16 *value;
> +    u16 result;
> +    struct usb_interface *intf = to_usb_interface(d);
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +    outmsg.command = USB2CAN_GET_SOFTW_VER;
> +
> +    err = usb2can_send_cmd(dev, &outmsg, &inmsg);
> +    if (err)
> +        return -EIO;
> +
> +    value = (u16 *) inmsg.data;
> +    result = be16_to_cpu(*value);

Ditto.

> +    return sprintf(buf, "%d.%d\n", (u8)(result>>8), (u8)result);
> +}
> +
> +/* Get hardware version */
> +static ssize_t show_hardware(struct device *d, struct device_attribute
> *attr,
> +                 char *buf)
> +{
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +    int err = 0;
> +    u16 *value;
> +    u16 result;
> +    struct usb_interface *intf = to_usb_interface(d);
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +    outmsg.command = USB2CAN_GET_HARDW_VER;
> +
> +    err = usb2can_send_cmd(dev, &outmsg, &inmsg);
> +    if (err)
> +        return -EIO;
> +
> +    value = (u16 *) inmsg.data;
> +    result = be16_to_cpu(*value);

Ditto.

> +
> +    return sprintf(buf, "%d.%d\n", (u8)(result>>8), (u8)result);
> +}
> +
> +/* Get status
> + *
> + * Returns:
> + * STATUS_NONE        0x00000000
> + * STATUS_BUS_OFF    0x80000000
> + * STATUS_PASSIVE    0x40000000
> + * STATUS_BUS_WARN    0x20000000
> + * STATUS_ACTIVE    0x10000000
> + * STATUS_PHY_FAULT    0x08000000
> + * STATUS_PHY_H        0x04000000
> + * STATUS_PHY_L        0x02000000
> + * STATUS_SLEEPING    0x01000000
> + * STATUS_STOPPED    0x00800000
> + */
> +static ssize_t show_status(struct device *d, struct device_attribute
> *attr,
> +               char *buf)
> +{
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +    int err = 0;
> +    u32 *value;
> +    u32 result;
> +    struct usb_interface *intf = to_usb_interface(d);
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +    outmsg.command = USB2CAN_GET_STATUS;
> +
> +    err = usb2can_send_cmd(dev, &outmsg, &inmsg);
> +    if (err)
> +        return -EIO;
> +
> +    value = (u32 *) inmsg.data;
> +    result = be32_to_cpu(*value);

Ditto.

> +    return sprintf(buf, "0x%08x\n", result);
> +}
> +
> +/* Get statistic values */
> +static ssize_t show_statistics(struct device *d, struct
> device_attribute *attr,
> +                   u8 statistic, char *buf)
> +{
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +    int err = 0;
> +    u32 *value;
> +    u32 result;
> +    struct usb_interface *intf = to_usb_interface(d);
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +    outmsg.command = USB2CAN_GET_STATISTICS;
> +    outmsg.opt1 = statistic;
> +
> +    err = usb2can_send_cmd(dev, &outmsg, &inmsg);
> +    if (err)
> +        return -EIO;
> +
> +    value = (u32 *) inmsg.data;
> +    result = be32_to_cpu(*value);
> +
> +    return sprintf(buf, "%d\n", result);
> +}
> +
> +static ssize_t show_rx_frames(struct device *d, struct device_attribute
> *attr,
> +                  char *buf)
> +{
> +    return show_statistics(d, attr, USB2CAN_STAT_RX_FRAMES, buf);
> +}
> +
> +static ssize_t show_rx_bytes(struct device *d, struct device_attribute
> *attr,
> +                 char *buf)
> +{
> +    return show_statistics(d, attr, USB2CAN_STAT_RX_BYTES, buf);
> +}
> +
> +static ssize_t show_tx_frames(struct device *d, struct device_attribute
> *attr,
> +                  char *buf)
> +{
> +    return show_statistics(d, attr, USB2CAN_STAT_TX_FRAMES, buf);
> +}
> +
> +static ssize_t show_tx_bytes(struct device *d, struct device_attribute
> *attr,
> +                 char *buf)
> +{
> +    return show_statistics(d, attr, USB2CAN_STAT_RX_BYTES, buf);
> +}
> +
> +static ssize_t show_overruns(struct device *d, struct device_attribute
> *attr,
> +                 char *buf)
> +{
> +    return show_statistics(d, attr, USB2CAN_STAT_OVERRUNS, buf);
> +}
> +
> +static ssize_t show_warnings(struct device *d, struct device_attribute
> *attr,
> +                 char *buf)
> +{
> +    return show_statistics(d, attr, USB2CAN_STAT_WARNINGS, buf);
> +}
> +
> +static ssize_t show_bus_off(struct device *d, struct device_attribute
> *attr,
> +                char *buf)
> +{
> +    return show_statistics(d, attr, USB2CAN_STAT_BUS_OFF, buf);
> +}
> +
> +/* Reset statistics */
> +static ssize_t reset_statistics(struct device *d, struct
> device_attribute *attr,
> +                const char *buf, size_t count)
> +{
> +    struct usb2can_cmd_msg    outmsg;
> +    struct usb2can_cmd_msg    inmsg;
> +    int err = 0;
> +    struct usb_interface *intf = to_usb_interface(d);
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    if (buf[0] == '1') {
> +        memset(&outmsg, 0, sizeof(struct usb2can_cmd_msg));
> +        outmsg.command = USB2CAN_GET_STATISTICS;
> +        outmsg.opt1 = USB2CAN_STAT_RESET_STAT;
> +
> +        err = usb2can_send_cmd(dev, &outmsg, &inmsg);
> +        if (err)
> +            return -EIO;
> +    }
> +
> +    return count;
> +}
> +
> +/* Get "disable automatic retransmission" flag */
> +static ssize_t show_dar(struct device *d, struct device_attribute *attr,
> +            char *buf)
> +{
> +    struct usb_interface *intf = to_usb_interface(d);
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    return sprintf(buf, "%d\n", dev->dar);
> +}
> +
> +/* Set "disable automatic retransmission" flag */
> +static ssize_t set_dar(struct device *d, struct device_attribute *attr,
> +               const char *buf, size_t count)
> +{
> +    struct usb_interface *intf = to_usb_interface(d);
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    if (dev->can.state != CAN_STATE_STOPPED) {
> +        dev_err(&intf->dev,
> +            "DAR flag can only be set when device is stopped\n");
> +        return -EIO;
> +    }
> +
> +    if (buf[0] == '0')
> +        dev->dar = 0;
> +    else if (buf[0] == '1')
> +        dev->dar = 1;
> +    else
> +        return -EIO;
> +
> +    return count;
> +}

Automatic retransmission means one-shot mode? We handle this usually via
"ctrlmode_supported" flag CAN_CTRLMODE_ONE_SHOT.

> +static DEVICE_ATTR(firmware, S_IRUGO, show_firmware, NULL);
> +static DEVICE_ATTR(hardware, S_IRUGO, show_hardware, NULL);
> +static DEVICE_ATTR(can_state, S_IRUGO, show_status, NULL);
> +static DEVICE_ATTR(can_rx_frames, S_IRUGO, show_rx_frames, NULL);
> +static DEVICE_ATTR(can_rx_bytes, S_IRUGO, show_rx_bytes, NULL);
> +static DEVICE_ATTR(can_tx_frames, S_IRUGO, show_tx_frames, NULL);
> +static DEVICE_ATTR(can_tx_bytes, S_IRUGO, show_tx_bytes, NULL);
> +static DEVICE_ATTR(can_overruns, S_IRUGO, show_overruns, NULL);
> +static DEVICE_ATTR(can_warnings, S_IRUGO, show_warnings, NULL);
> +static DEVICE_ATTR(can_bus_off_counter, S_IRUGO, show_bus_off, NULL);
> +
> +static DEVICE_ATTR(can_reset_statistics, S_IWUSR, NULL, reset_statistics);
> +
> +static DEVICE_ATTR(can_disable_automatic_retransmission, S_IRUGO |
> S_IWUSR,
> +           show_dar, set_dar);
> +
> +/* Set network device mode
> + *
> + * Maybe we should leave this function empty, because the device
> + * set mode variable with open command.
> + */
> +static int usb2can_set_mode(struct net_device *netdev, enum can_mode mode)
> +{
> +    struct usb2can *dev = netdev_priv(netdev);
> +    int err = 0;
> +
> +    switch (mode) {
> +    case CAN_MODE_START:
> +        err = usb2can_cmd_open(dev);
> +        if (err)
> +            dev_warn(netdev->dev.parent, "couldn't start device");
> +
> +        if (netif_queue_stopped(netdev))
> +            netif_wake_queue(netdev);
> +        break;
> +
> +    default:
> +        return -EOPNOTSUPP;
> +    }
> +
> +    return 0;
> +}
> +
> +/* Empty function: We set bittiming when we start the interface.
> + * This is a firmware limitation.
> + */
> +static int usb2can_set_bittiming(struct net_device *netdev)
> +{
> +    return 0;
> +}
> +
> +/* Read data and status frames */
> +static void usb2can_rx_can_msg(struct usb2can *dev, struct
> usb2can_rx_msg *msg)
> +{
> +    struct can_frame *cf;
> +    struct sk_buff *skb;
> +    int i;
> +    struct net_device_stats *stats = &dev->netdev->stats;
> +
> +    skb = alloc_can_skb(dev->netdev, &cf);
> +    if (skb == NULL)

s/skb == NULL/!skb/ is preferred.
Here and in maybe in other places as well.

> +        return;

PLease use alloc_can_skb for real messages ...-

> +    if (msg->type == USB2CAN_TYPE_CAN_FRAME) {
> +        cf->can_id = be32_to_cpu(msg->id);
> +        cf->can_dlc = get_can_dlc(msg->dlc & 0xF);
> +
> +        if (msg->flags & USB2CAN_EXTID)
> +            cf->can_id |= CAN_EFF_FLAG;
> +
> +        if (msg->flags & USB2CAN_RTR)
> +            cf->can_id |= CAN_RTR_FLAG;
> +        else
> +            for (i = 0; i < cf->can_dlc; i++)
> +                cf->data[i] = msg->data[i];
> +    } else if (msg->type == USB2CAN_TYPE_ERROR_FRAME &&
> +           msg->flags == USB2CAN_ERR_FLAG) {

... and alloc_can_err_skb for error messages.

> +
> +        /* Error message:
> +           byte 0: Status
> +           byte 1: bit   7: Receive Passive
> +           byte 1: bit 0-6: Receive Error Counter
> +           byte 2: Transmit Error Counter
> +           byte 3: Always 0 (maybe reserved for future use)
> +        */
> +
> +        u8 state = msg->data[0];
> +        u8 rxerr = msg->data[1] & USB2CAN_RP_MASK;
> +        u8 txerr = msg->data[2];

In principle, you could then also support the "do_get_berr_counter"
callback.

> +        int rx_errors = 0;
> +        int tx_errors = 0;
> +
> +        dev->can.can_stats.bus_error++;
> +
> +        cf->can_id = CAN_ERR_FLAG;
> +        cf->can_dlc = CAN_ERR_DLC;

... making the above two lines obsolete.

> +
> +        switch (state) {
> +        case USB2CAN_STATUSMSG_OK:
> +            dev->can.state = CAN_STATE_ERROR_ACTIVE;
> +            cf->can_id |= CAN_ERR_PROT;
> +            cf->data[2] = CAN_ERR_PROT_ACTIVE;
> +            break;
> +        case USB2CAN_STATUSMSG_BUSOFF:
> +            dev->can.state = CAN_STATE_BUS_OFF;
> +            cf->can_id |= CAN_ERR_BUSOFF;
> +            can_bus_off(dev->netdev);
> +            break;
> +        default:
> +            dev->can.state = CAN_STATE_ERROR_WARNING;
> +            cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +            break;
> +        }
> +
> +        switch (state) {
> +        case USB2CAN_STATUSMSG_ACK:
> +            cf->can_id |= CAN_ERR_ACK;
> +            tx_errors = 1;
> +            break;
> +        case USB2CAN_STATUSMSG_CRC:
> +            cf->data[2] |= CAN_ERR_PROT_BIT;
> +            rx_errors = 1;
> +            break;
> +        case USB2CAN_STATUSMSG_BIT0:
> +            cf->data[2] |= CAN_ERR_PROT_BIT0;
> +            tx_errors = 1;
> +            break;
> +        case USB2CAN_STATUSMSG_BIT1:
> +            cf->data[2] |= CAN_ERR_PROT_BIT1;
> +            tx_errors = 1;
> +            break;
> +        case USB2CAN_STATUSMSG_FORM:
> +            cf->data[2] |= CAN_ERR_PROT_FORM;
> +            rx_errors = 1;
> +            break;
> +        case USB2CAN_STATUSMSG_STUFF:
> +            cf->data[2] |= CAN_ERR_PROT_STUFF;
> +            rx_errors = 1;
> +            break;
> +        case USB2CAN_STATUSMSG_OVERRUN:
> +            cf->can_id |= CAN_ERR_CRTL;
> +            cf->data[1] = (txerr > rxerr) ?
> +                CAN_ERR_CRTL_TX_OVERFLOW :
> +                CAN_ERR_CRTL_RX_OVERFLOW;
> +            cf->data[2] |= CAN_ERR_PROT_OVERLOAD;
> +            stats->rx_over_errors++;
> +            break;
> +        case USB2CAN_STATUSMSG_BUSLIGHT:
> +            cf->can_id |= CAN_ERR_CRTL;
> +            cf->data[1] = (txerr > rxerr) ?
> +                CAN_ERR_CRTL_TX_WARNING :
> +                CAN_ERR_CRTL_RX_WARNING;
> +            dev->can.can_stats.error_warning++;
> +            break;
> +        case USB2CAN_STATUSMSG_BUSHEAVY:
> +            cf->can_id |= CAN_ERR_CRTL;
> +            cf->data[1] = (txerr > rxerr) ?
> +                CAN_ERR_CRTL_TX_PASSIVE :
> +                CAN_ERR_CRTL_RX_PASSIVE;
> +            dev->can.can_stats.error_passive++;
> +            break;
> +        default:
> +            cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> +            break;
> +        }
> +
> +        if (tx_errors) {
> +            cf->data[2] |= CAN_ERR_PROT_TX;
> +            stats->tx_errors++;
> +        }
> +
> +        if (rx_errors)
> +            stats->rx_errors++;
> +
> +        cf->data[6] = txerr;
> +        cf->data[7] = rxerr;
> +
> +    } else {
> +        dev_warn(dev->udev->dev.parent, "frame type %d unknown",
> +             msg->type);
> +    }

Could you please show the output of "candump -e any,0:0,#FFFFFFFF" when
the device goes 1) error passive and 2) bus-off. You could provoke
these states by 1) sending without connection to the bus or 2)
short-circuiting CAN high and low.

Another question: is it possible to switch off bus-error reporting?

> +    netif_rx(skb);
> +
> +    stats->rx_packets++;
> +    stats->rx_bytes += cf->can_dlc;
> +}
> +
> +/* Callback for reading data from device
> + *
> + * Check urb status, call read function and resubmit urb read operation.
> + */
> +static void usb2can_read_bulk_callback(struct urb *urb)
> +{
> +    struct usb2can *dev = urb->context;
> +    struct net_device *netdev;
> +    int retval;
> +    int pos = 0;
> +
> +    netdev = dev->netdev;
> +
> +    if (!netif_device_present(netdev))
> +        return;
> +
> +    switch (urb->status) {
> +    case 0: /* success */
> +        break;
> +
> +    case -ENOENT:
> +    case -ESHUTDOWN:
> +        return;
> +
> +    default:
> +        dev_info(netdev->dev.parent, "Rx URB aborted (%d)\n",
> +             urb->status);
> +        goto resubmit_urb;
> +    }
> +
> +    while (pos < urb->actual_length) {
> +        struct usb2can_rx_msg *msg;
> +
> +        msg = (struct usb2can_rx_msg *)(urb->transfer_buffer + pos);
> +
> +        usb2can_rx_can_msg(dev, msg);
> +
> +        pos += sizeof(struct usb2can_rx_msg);
> +
> +        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,
> +              usb2can_read_bulk_callback, dev);
> +
> +    retval = usb_submit_urb(urb, GFP_ATOMIC);
> +
> +    if (retval == -ENODEV)
> +        netif_device_detach(netdev);
> +    else if (retval)
> +        dev_err(netdev->dev.parent,
> +            "failed resubmitting read bulk urb: %d\n", retval);
> +}
> +
> +/* Callback handler for write operations
> + *
> + * Free allocated buffers, check transmit status and
> + * calculate statistic.
> + */
> +static void usb2can_write_bulk_callback(struct urb *urb)
> +{
> +    struct usb2can_tx_urb_context *context = urb->context;
> +    struct usb2can *dev;
> +    struct net_device *netdev;
> +
> +    BUG_ON(!context);
> +
> +    dev = context->dev;
> +    netdev = dev->netdev;
> +
> +    /* free up our allocated buffer */
> +    usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> +              urb->transfer_buffer, urb->transfer_dma);
> +
> +    atomic_dec(&dev->active_tx_urbs);
> +
> +    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;

Hm, what is the status of trans_start? Some drivers still do set it,
others don't:

  $ find . -name '*.c'|xargs grep -l trans_start
  ./mscan/mscan.c
  ./usb/esd_usb2.c
  ./usb/peak_usb/pcan_usb_core.c
  ./usb/ems_usb.c

There was some cleanup "1ae5dc342ac78d7a42965fd1f323815f6f5ef2c1"
sometime ago.

> +
> +    netdev->stats.tx_packets++;
> +    netdev->stats.tx_bytes += context->dlc;
> +
> +    can_get_echo_skb(netdev, context->echo_index);
> +
> +    /* Release context */
> +    context->echo_index = MAX_TX_URBS;
> +
> +    if (netif_queue_stopped(netdev))
> +        netif_wake_queue(netdev);
> +}
> +
> +/* Send data to device */
> +static netdev_tx_t usb2can_start_xmit(struct sk_buff *skb,
> +                      struct net_device *netdev)
> +{
> +    struct usb2can *dev = netdev_priv(netdev);
> +    struct net_device_stats *stats = &netdev->stats;
> +    struct can_frame *cf = (struct can_frame *)skb->data;

Is the cast needed?

> +    struct usb2can_tx_msg *msg;
> +    struct urb *urb;
> +    struct usb2can_tx_urb_context *context = NULL;
> +    u8 *buf;
> +    int i, err;
> +    size_t size = sizeof(struct usb2can_tx_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");
> +        goto nomem;
> +    }
> +
> +    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");
> +        usb_free_urb(urb);
> +        goto nomem;
> +    }
> +
> +    memset(buf, 0, size);
> +
> +    msg = (struct usb2can_tx_msg *)buf;
> +    msg->begin = USB2CAN_DATA_START;
> +    msg->flags = 0x00;
> +
> +    if (cf->can_id & CAN_RTR_FLAG)
> +        msg->flags |= USB2CAN_RTR;
> +
> +    if (cf->can_id & CAN_EFF_FLAG)
> +        msg->flags |= USB2CAN_EXTID;
> +
> +    msg->id = cpu_to_be32(cf->can_id & CAN_ERR_MASK);
> +
> +    msg->dlc = cf->can_dlc;
> +
> +    for (i = 0; i < cf->can_dlc; i++)
> +        msg->data[i] = cf->data[i];
> +
> +    msg->end = USB2CAN_DATA_END;
> +
> +
> +    for (i = 0; i < MAX_TX_URBS; i++) {
> +        if (dev->tx_contexts[i].echo_index == MAX_TX_URBS) {
> +            context = &dev->tx_contexts[i];
> +            break;
> +        }
> +    }
> +
> +    /* May never happen! When this happens we'd more URBs in flight as
> +     * allowed (MAX_TX_URBS).
> +     */
> +    if (!context) {
> +        usb_unanchor_urb(urb);
> +        usb_free_coherent(dev->udev, size, buf, urb->transfer_dma);
> +
> +        dev_warn(netdev->dev.parent, "couldn't find free context");
> +
> +        return NETDEV_TX_BUSY;
> +    }
> +
> +    context->dev = dev;
> +    context->echo_index = i;
> +    context->dlc = cf->can_dlc;
> +
> +    usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
> +              size, usb2can_write_bulk_callback, context);
> +    urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +    usb_anchor_urb(urb, &dev->tx_submitted);
> +
> +    can_put_echo_skb(skb, netdev, context->echo_index);
> +
> +    atomic_inc(&dev->active_tx_urbs);
> +
> +    err = usb_submit_urb(urb, GFP_ATOMIC);
> +    if (unlikely(err)) {
> +        can_free_echo_skb(netdev, context->echo_index);
> +
> +        usb_unanchor_urb(urb);
> +        usb_free_coherent(dev->udev, size, buf, urb->transfer_dma);
> +        dev_kfree_skb(skb);
> +
> +        atomic_dec(&dev->active_tx_urbs);
> +
> +        if (err == -ENODEV) {
> +            netif_device_detach(netdev);
> +        } else {
> +            dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
> +
> +            stats->tx_dropped++;
> +        }
> +    } else {
> +        netdev->trans_start = jiffies;
> +
> +        /* Slow down tx path */
> +        if (atomic_read(&dev->active_tx_urbs) >= MAX_TX_URBS ||
> +            dev->free_slots < 5) {
> +            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;
> +
> +nomem:
> +    dev_kfree_skb(skb);
> +    stats->tx_dropped++;
> +
> +    return NETDEV_TX_OK;
> +}
> +
> +
> +/* Start USB device */
> +static int usb2can_start(struct usb2can *dev)
> +{
> +    struct net_device *netdev = dev->netdev;
> +    int err, i;
> +
> +    dev->free_slots = 15; /* initial size */
> +
> +    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_err(netdev->dev.parent,
> +                "No memory left for URBs\n");
> +            return -ENOMEM;
> +        }
> +
> +        buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
> +                     &urb->transfer_dma);
> +        if (!buf) {
> +            dev_err(netdev->dev.parent,
> +                "No memory left for USB buffer\n");
> +            usb_free_urb(urb);
> +            return -ENOMEM;
> +        }
> +
> +        usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
> +                  buf, RX_BUFFER_SIZE,
> +                  usb2can_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) {
> +            if (err == -ENODEV)
> +                netif_device_detach(dev->netdev);
> +
> +            usb_unanchor_urb(urb);
> +            usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
> +                      urb->transfer_dma);
> +            break;
> +        }
> +
> +        /* Drop reference, USB core will take care of freeing it */
> +        usb_free_urb(urb);
> +    }
> +
> +    /* Did we submit any URBs */
> +    if (i == 0) {
> +        dev_warn(netdev->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(netdev->dev.parent, "rx performance may be slow\n");
> +
> +    err = usb2can_cmd_open(dev);
> +    if (err)
> +        goto failed;
> +
> +    dev->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +    return 0;
> +
> +failed:
> +    if (err == -ENODEV)
> +        netif_device_detach(dev->netdev);
> +
> +    dev_warn(netdev->dev.parent, "couldn't submit control: %d\n", err);
> +
> +    return err;
> +}
> +
> +/* Open USB device */
> +static int usb2can_open(struct net_device *netdev)
> +{
> +    struct usb2can *dev = netdev_priv(netdev);
> +    int err;
> +
> +    /* common open */
> +    err = open_candev(netdev);
> +    if (err)
> +        return err;
> +
> +    /* finally start device */
> +    err = usb2can_start(dev);
> +    if (err) {
> +        if (err == -ENODEV)
> +            netif_device_detach(dev->netdev);
> +
> +        dev_warn(netdev->dev.parent, "couldn't start device: %d\n",
> +             err);
> +
> +        close_candev(netdev);
> +
> +        return err;
> +    }
> +
> +    netif_start_queue(netdev);
> +
> +    return 0;
> +}
> +
> +static void unlink_all_urbs(struct usb2can *dev)
> +{
> +    int i;
> +
> +    usb_kill_anchored_urbs(&dev->rx_submitted);
> +
> +    usb_kill_anchored_urbs(&dev->tx_submitted);
> +    atomic_set(&dev->active_tx_urbs, 0);
> +
> +    for (i = 0; i < MAX_TX_URBS; i++)
> +        dev->tx_contexts[i].echo_index = MAX_TX_URBS;
> +}
> +
> +/* Close USB device */
> +static int usb2can_close(struct net_device *netdev)
> +{
> +    struct usb2can *dev = netdev_priv(netdev);
> +    int err = 0;
> +
> +    /* Send CLOSE command to CAN controller */
> +    err = usb2can_cmd_close(dev);
> +    if (err)
> +        dev_warn(netdev->dev.parent, "couldn't stop device");
> +
> +    dev->can.state = CAN_STATE_STOPPED;
> +
> +    /* Stop polling */
> +    unlink_all_urbs(dev);
> +
> +    netif_stop_queue(netdev);
> +
> +    close_candev(netdev);
> +
> +    return err;
> +}
> +
> +static const struct net_device_ops usb2can_netdev_ops = {
> +    .ndo_open = usb2can_open,
> +    .ndo_stop = usb2can_close,
> +    .ndo_start_xmit = usb2can_start_xmit,
> +};
> +
> +static struct can_bittiming_const usb2can_bittiming_const = {
> +    .name = "usb2can",
> +    .tseg1_min = USB2CAN_TSEG1_MIN,
> +    .tseg1_max = USB2CAN_TSEG1_MAX,
> +    .tseg2_min = USB2CAN_TSEG2_MIN,
> +    .tseg2_max = USB2CAN_TSEG2_MAX,
> +    .sjw_max = USB2CAN_SJW_MAX,
> +    .brp_min = USB2CAN_BRP_MIN,
> +    .brp_max = USB2CAN_BRP_MAX,
> +    .brp_inc = USB2CAN_BRP_INC,
> +};
> +
> +/* Probe USB device
> + *
> + * Check device and firmware.
> + * Set supported modes and bittiming constants.
> + * Allocate some memory.
> + */
> +static int usb2can_probe(struct usb_interface *intf,
> +             const struct usb_device_id *id)
> +{
> +    struct net_device *netdev;
> +    struct usb2can *dev;
> +    int i, err = -ENOMEM;
> +    u32 version;
> +    char buf[18];
> +    struct usb_device *usbdev = interface_to_usbdev(intf);
> +
> +    /* product id looks strange, better we also check iProdukt string */
> +    if (usb_string(usbdev, usbdev->descriptor.iProduct, buf,
> +               sizeof(buf)) > 0 && strcmp(buf, "USB2CAN converter")) {
> +        dev_info(&usbdev->dev, "ignoring: not an USB2CAN converter\n");
> +        return -ENODEV;
> +    }
> +
> +    netdev = alloc_candev(sizeof(struct usb2can), MAX_TX_URBS);
> +    if (!netdev) {
> +        dev_err(&intf->dev, "Couldn't alloc candev\n");
> +        return -ENOMEM;
> +    }
> +
> +    dev = netdev_priv(netdev);
> +
> +    dev->udev = usbdev;
> +    dev->netdev = netdev;
> +
> +    dev->dar = 0;
> +
> +    dev->can.state = CAN_STATE_STOPPED;
> +    dev->can.clock.freq = USB2CAN_ABP_CLOCK;
> +    dev->can.bittiming_const = &usb2can_bittiming_const;
> +    dev->can.do_set_bittiming = usb2can_set_bittiming;
> +    dev->can.do_set_mode = usb2can_set_mode;
> +    dev->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> +                      CAN_CTRLMODE_LISTENONLY;

This reminds me: in listen-only mode, we should not start the tx-queue
on open. Other drivers do have this issue as well.

> +    netdev->netdev_ops = &usb2can_netdev_ops;
> +
> +    netdev->flags |= IFF_ECHO; /* we support local echo */
> +
> +    init_usb_anchor(&dev->rx_submitted);
> +
> +    init_usb_anchor(&dev->tx_submitted);
> +    atomic_set(&dev->active_tx_urbs, 0);
> +
> +    for (i = 0; i < MAX_TX_URBS; i++)
> +        dev->tx_contexts[i].echo_index = MAX_TX_URBS;
> +
> +    dev->cmd_msg_buffer = kzalloc(sizeof(struct usb2can_cmd_msg),
> +                      GFP_KERNEL);
> +    if (!dev->cmd_msg_buffer) {
> +        dev_err(&intf->dev, "Couldn't alloc Tx buffer\n");
> +        goto cleanup_candev;
> +    }
> +
> +    usb_set_intfdata(intf, dev);
> +
> +    SET_NETDEV_DEV(netdev, &intf->dev);
> +
> +    mutex_init(&dev->usb2can_cmd_lock);
> +
> +    err = usb2can_cmd_version(dev, &version);
> +    if (err) {
> +        dev_err(netdev->dev.parent, "can't get firmware version\n");
> +        goto cleanup_cmd_msg_buffer;
> +    } else {
> +        dev_info(netdev->dev.parent,
> +             "firmware: %d.%d, hardware: %d.%d\n",
> +             (u8)(version>>24), (u8)(version>>16),
> +             (u8)(version>>8), (u8)version);

I would prefer "& 0xff" vs. cast here.

> +    }
> +
> +    err = register_candev(netdev);
> +    if (err) {
> +        dev_err(netdev->dev.parent,
> +            "couldn't register CAN device: %d\n", err);
> +        goto cleanup_cmd_msg_buffer;
> +    }
> +
> +    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_can_state))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_state\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_rx_frames))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_rx_frames\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_rx_bytes))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_rx_bytes\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_tx_frames))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_tx_frames\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_tx_bytes))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_tx_bytes\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_overruns))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_overruns\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_warnings))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_warnings\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_bus_off_counter))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_bus_off_counter\n");
> +
> +    if (device_create_file(&intf->dev, &dev_attr_can_reset_statistics))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for can_reset_statistics\n");
> +
> +    if (device_create_file(&intf->dev,
> +                   &dev_attr_can_disable_automatic_retransmission))
> +        dev_err(&intf->dev,
> +            "Couldn't create device file for
> can_disable_automatic_retransmission\n");
> +
> +    /* let the user know what node this device is now attached to */
> +    dev_info(netdev->dev.parent, "device registered as %s\n",
> netdev->name);
> +    return 0;
> +
> +cleanup_cmd_msg_buffer:
> +    kfree(dev->cmd_msg_buffer);
> +
> +cleanup_candev:
> +    free_candev(netdev);
> +
> +    return err;
> +
> +}
> +
> +/* Called by the usb core when driver is unloaded or device is removed */
> +static void usb2can_disconnect(struct usb_interface *intf)
> +{
> +    struct usb2can *dev = usb_get_intfdata(intf);
> +
> +    device_remove_file(&intf->dev, &dev_attr_firmware);
> +    device_remove_file(&intf->dev, &dev_attr_hardware);
> +    device_remove_file(&intf->dev, &dev_attr_can_state);
> +    device_remove_file(&intf->dev, &dev_attr_can_rx_frames);
> +    device_remove_file(&intf->dev, &dev_attr_can_rx_bytes);
> +    device_remove_file(&intf->dev, &dev_attr_can_tx_frames);
> +    device_remove_file(&intf->dev, &dev_attr_can_tx_bytes);
> +    device_remove_file(&intf->dev, &dev_attr_can_overruns);
> +    device_remove_file(&intf->dev, &dev_attr_can_warnings);
> +    device_remove_file(&intf->dev, &dev_attr_can_bus_off_counter);
> +    device_remove_file(&intf->dev, &dev_attr_can_reset_statistics);
> +    device_remove_file(&intf->dev,
> +               &dev_attr_can_disable_automatic_retransmission);

The driver does collect similar statistics and therefore I do not see a
need for these sysfs files... apart from printing the firmware version
when the device is probed. BTW: what is "show_hardware" good for?

Thanks for your contribution.

Wolfgang.