Wolfgang and Marc, thanks for your detailed review. I will try to put all of your comments in my V2 patch.
Happy new year Matthias On Monday 28 December 2009 09:54, Wolfgang Grandegger wrote: > Hi Matthias, > > I agree with most of Marc's comments and I will not address all of > them here. > > Matthias Fuchs wrote: > > This patch adds support for esd's CAN/USB2 high speed USB 2.0 > > CAN interface module. Multiple CAN interfaces per module are supported. > > > > This driver has been tested on x86 and PowerPC. > > > > Q: I am not absolutely sure if some locking is required inside > > the driver code. Please have a look. > > What do you have in mind. I suspect a race with "active_tx_urbs" but so > far I did not spot something critical. In general, please remove the > dev_dbg's mainly aiding in debugging. Also, use ND2D() in all can_dbg(), > can_info(), etc. > > > Signed-off-by: Matthias Fuchs <[email protected]> > > > > Index: kernel/2.6/Makefile > > =================================================================== > > --- kernel/2.6/Makefile (revision 1095) > > +++ kernel/2.6/Makefile (working copy) > > @@ -24,6 +24,7 @@ > > export CONFIG_CAN_EMS_104M=m > > export CONFIG_CAN_ESD_PCI=m > > export CONFIG_CAN_ESD_331=m > > +export CONFIG_CAN_ESD_USB2=m > > export CONFIG_CAN_PIPCAN=m > > export CONFIG_CAN_SOFTING=m > > export CONFIG_CAN_SOFTING_CS=m > > Index: kernel/2.6/drivers/net/can/usb/Kconfig > > =================================================================== > > --- kernel/2.6/drivers/net/can/usb/Kconfig (revision 1095) > > +++ kernel/2.6/drivers/net/can/usb/Kconfig (working copy) > > @@ -7,4 +7,10 @@ > > This driver is for the one channel CPC-USB/ARM7 CAN/USB interface > > from from EMS Dr. Thomas Wuensche (http://www.ems-wuensche.de). > > > > +config CAN_ESD_USB2 > > + tristate "ESD USB/2 CAN/USB interface" > > + ---help--- > > + This driver supports the CAN-USB/2 interface > > + from esd electronic system design gmbh (http://www.esd.eu). > > + > > endmenu > > Index: kernel/2.6/drivers/net/can/usb/esd_usb2.c > > =================================================================== > > --- kernel/2.6/drivers/net/can/usb/esd_usb2.c (revision 0) > > +++ kernel/2.6/drivers/net/can/usb/esd_usb2.c (revision 0) > > @@ -0,0 +1,1113 @@ > > +/* > > + * CAN driver for esd CAN-USB/2 > > + * > > + * Copyright (C) 2009 Matthias Fuchs <[email protected]>, esd gmbh > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published > > + * by the Free Software Foundation; version 2 of the License. > > + * > > + * This program is distributed in the hope that it will be useful, but > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License along > > + * with this program; if not, write to the Free Software Foundation, Inc., > > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > > + */ > > +#undef DEBUG > > Please remove. > > > +#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 <socketcan/can.h> > > +#include <socketcan/can/dev.h> > > +#include <socketcan/can/error.h> > > + > > +MODULE_AUTHOR("Matthias Fuchs <[email protected]>"); > > +MODULE_DESCRIPTION("CAN driver for esd CAN-USB/2 interfaces"); > > +MODULE_LICENSE("GPL v2"); > > + > > +/* Define these values to match your devices */ > > +#define USB_ESDGMBH_VENDOR_ID 0x0ab4 > > +#define USB_CANUSB2_PRODUCT_ID 0x0010 > > + > > +#define ESD_USB2_CAN_CLOCK 60000000 > > +#define ESD_USB2_MAX_NETS 2 > > + > > +/* USB2 commands */ > > +#define CMD_VERSION 1 /* also used for VERSION_REPLY */ > > +#define CMD_CAN_RX 2 /* device to host only */ > > +#define CMD_CAN_TX 3 /* also used for TX_DONE */ > > +#define CMD_SETBAUD 4 /* also used for SETBAUD_REPLY */ > > +#define CMD_TS 5 /* also used for TS_REPLY */ > > +#define CMD_IDADD 6 /* also used for IDADD_REPLY */ > > + > > +/* esd CAN message flags - dlc field */ > > +#define ESD_RTR 0x10 > > + > > +/* esd CAN message flags - id field */ > > +#define ESD_EXTID 0x20000000 > > +#define ESD_EVENT 0x40000000 > > +#define ESD_IDMASK 0x1fffffff > > + > > +/* esd CAN event ids used by this driver */ > > +#define ESD_EV_CAN_ERROR_EXT 2 > > + > > +/* baudrate message flags */ > > +#define ESD_UBR 0x80000000 > > +#define ESD_LOM 0x40000000 > > +#define ESD_NO_BAUDRATE 0x7fffffff > > + > > +/* esd IDADD message */ > > +#define ESD_ID_ENABLE 0x80 > > + > > +struct header_msg { > > + u8 len; > > + u8 cmd; > > + u8 rsvd[2]; > > +}; > > + > > +struct version_msg { > > + u8 len; > > + u8 cmd; > > + u8 rsvd; > > + u8 flags; > > + u32 drv_version; > > +}; > > + > > +struct version_reply_msg { > > + u8 len; > > + u8 cmd; > > + u8 nets; > > + u8 features; > > + u32 version; > > + u8 name[16]; > > + u32 rsvd; > > + u32 ts; > > +}; > > + > > +struct rx_msg { > > + u8 len; > > + u8 cmd; > > + u8 net; > > + u8 dlc; > > + u32 ts; > > + u32 id; /* upper 3 bits contain flags */ > > + u8 data[8]; > > +}; > > + > > +struct tx_msg { > > + u8 len; > > + u8 cmd; > > + u8 net; > > + u8 dlc; > > + u32 hnd; > > + u32 id; /* upper 3 bits contain flags */ > > + u8 data[8]; > > +}; > > + > > +struct tx_done_msg { > > + u8 len; > > + u8 cmd; > > + u8 net; > > + u8 status; > > + u32 hnd; > > + u32 ts; > > +}; > > + > > +struct id_filter_msg { > > + u8 len; > > + u8 cmd; > > + u8 net; > > + u8 option; > > + u32 mask[65]; > > +}; > > + > > +struct id_filter_reply_msg { > > + u8 len; > > + u8 cmd; > > + u8 net; > > + u8 option; > > + u16 added; > > + u16 removed; > > + u16 error; > > + u16 active; > > +}; > > + > > +struct set_baudrate_msg { > > + u8 len; > > + u8 cmd; > > + u8 net; > > + u8 rsvd; > > + u32 baud; > > +}; > > + > > +struct set_baudrate_reply_msg { > > + u8 len; > > + u8 cmd; > > + u8 net; > > + u8 rsvd; > > + u32 baud; > > + u32 ts; > > +}; > > Either align the members of structs *consequently* (and not just here) > *or* just don't align. That's what I told Marc for the at91_can driver > as well. I personally prefer the latter (*no* member alignment) like it > is required for expressions by the coding style. > > > + > > +/* Main message type used between library and application */ > > +struct __attribute__ ((packed)) esd_usb2_msg { > > + union { > > + struct header_msg hdr; > > + struct version_msg version; > > + struct version_reply_msg version_reply; > > + struct rx_msg rx; > > + struct tx_msg tx; > > + struct tx_done_msg txdone; > > + struct set_baudrate_msg setbaud; > > + struct set_baudrate_reply_msg setbaud_reply; > > + struct id_filter_msg filter; > > + struct id_filter_reply_msg filter_reply; > > + } msg; > > +}; > > + > > +static struct usb_device_id esd_usb2_table[] = { > > + {USB_DEVICE(USB_ESDGMBH_VENDOR_ID, USB_CANUSB2_PRODUCT_ID)}, > > + {} > > +}; > > + > > Please remove the empty line above. > > > +MODULE_DEVICE_TABLE(usb, esd_usb2_table); > > + > > +#define RX_BUFFER_SIZE 1024 > > +#define MAX_RX_URBS 4 > > +#define MAX_TX_URBS 10 > > See below. > > > +struct esd_usb2_net; > > + > > +struct esd_tx_urb_context { > > + struct esd_usb2_net *net; > > + u32 echo_index; > > +}; > > + > > +struct esd_usb2 { > > + struct usb_device *udev; > > + struct esd_usb2_net *nets[ESD_USB2_MAX_NETS]; > > + > > + struct usb_anchor rx_submitted; > > + > > + int no_nets; > > + u32 version; > > Tabs? > > > + int rxinitdone; > > +}; > > + > > +struct esd_usb2_net { > > + struct can_priv can; /* must be the first member */ > > + > > + atomic_t active_tx_urbs; > > + struct usb_anchor tx_submitted; > > + struct esd_tx_urb_context tx_contexts[MAX_TX_URBS]; > > + > > + int open_time; > > + struct esd_usb2 *usb2; > > + struct net_device *netdev; > > + int index; > > + u8 old_state; > > +}; > > + > > +/* SJA1000 ECC register (emulated by usb2 firmware) */ > > +#define SJA1000_ECC_SEG 0x1F > > +#define SJA1000_ECC_DIR 0x20 > > +#define SJA1000_ECC_ERR 0x06 > > +#define SJA1000_ECC_BIT 0x00 > > +#define SJA1000_ECC_FORM 0x40 > > +#define SJA1000_ECC_STUFF 0x80 > > +#define SJA1000_ECC_MASK 0xc0 > > You are adding #define's, struct declarations and struct assignments > randomly. Could you please order them more logically. > > > +static void esd_usb2_rx_event(struct esd_usb2_net *net, > > + struct esd_usb2_msg *msg) > > +{ > > + struct can_frame *cf; > > + struct sk_buff *skb; > > + struct net_device_stats *stats = &net->netdev->stats; > > + u32 id; > > + > > + id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK; > > + > > + dev_dbg(net->usb2->udev->dev.parent, > > + "event: id=%08x, len=%d, data=%02x %02x %02x %02x\n", > > + id, get_can_dlc(msg->msg.rx.dlc), > > + msg->msg.rx.data[0], msg->msg.rx.data[1], > > + msg->msg.rx.data[2], msg->msg.rx.data[3]); > > For debugging? > > > + switch (id) { > > + case ESD_EV_CAN_ERROR_EXT: > > + { > > The bracket should go one line up. > > > + u8 state = msg->msg.rx.data[0]; > > + u8 ecc = msg->msg.rx.data[1]; > > + u8 txerr = msg->msg.rx.data[2]; > > + u8 rxerr = msg->msg.rx.data[3]; > > + > > + skb = alloc_can_err_skb(net->netdev, &cf); > > + if (skb == NULL) > > + return; > > Hm, not sure if we should accept the failure silently. > > > + > > + if (state != net->old_state) { > > + net->old_state = state; > > + > > + switch (state & 0xc0) { > > + case 0xc0: > > + net->can.state = CAN_STATE_BUS_OFF; > > + cf->can_id |= CAN_ERR_BUSOFF; > > + can_bus_off(net->netdev); > > + break; > > + case 0x40: > > + net->can.state = CAN_STATE_ERROR_WARNING; > > + net->can.can_stats.error_warning++; > > + break; > > + case 0x80: > > + net->can.state = CAN_STATE_ERROR_PASSIVE; > > + net->can.can_stats.error_passive++; > > + break; > > + default: > > + net->can.state = CAN_STATE_ERROR_ACTIVE; > > + break; > > + } > > Please use #define's for these switch cases as well. > > > + } else { > > + net->can.can_stats.bus_error++; > > + stats->rx_errors++; > > + > > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > > + > > + switch (ecc & SJA1000_ECC_MASK) { > > + case SJA1000_ECC_BIT: > > + cf->data[2] |= CAN_ERR_PROT_BIT; > > + break; > > + case SJA1000_ECC_FORM: > > + cf->data[2] |= CAN_ERR_PROT_FORM; > > + break; > > + case SJA1000_ECC_STUFF: > > + cf->data[2] |= CAN_ERR_PROT_STUFF; > > + break; > > + default: > > + cf->data[2] |= CAN_ERR_PROT_UNSPEC; > > + cf->data[3] = ecc & SJA1000_ECC_SEG; > > + break; > > + } > > + > > + /* Error occured during transmission? */ > > + if ((ecc & SJA1000_ECC_DIR) == 0) > > + cf->data[2] |= CAN_ERR_PROT_TX; > > + > > + if (net->can.state == CAN_STATE_ERROR_WARNING || > > + net->can.state == CAN_STATE_ERROR_PASSIVE) { > > + cf->data[1] = (txerr > rxerr) ? > > + CAN_ERR_CRTL_TX_PASSIVE : > > + CAN_ERR_CRTL_RX_PASSIVE; > > + } > > + } > > + > > + netif_rx(skb); > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32) > > + net->netdev->last_rx = jiffies; > > +#endif > > + stats->rx_packets++; > > + stats->rx_bytes += cf->can_dlc; > > + > > + break; > > + } > > + > > + default: > > + break; > > + > > + } > > There is only *one* switch case. An "if () {}" would just be fine. > > > +} > > + > > +static void esd_usb2_rx_can_msg(struct esd_usb2_net *net, > > + struct esd_usb2_msg *msg) > > +{ > > + struct can_frame *cf; > > + struct sk_buff *skb; > > + int i; > > + struct net_device_stats *stats = &net->netdev->stats; > > + u32 id; > > Heavy nitpicking: I think it's common practice to order the declarations > somehow. This is also true for other functions. > > > + > > + if (!netif_device_present(net->netdev)) > > + return; > > + > > + id = le32_to_cpu(msg->msg.rx.id); > > + > > + if (id & ESD_EVENT) { > > + esd_usb2_rx_event(net, msg); > > + } else { > > + skb = alloc_can_skb(net->netdev, &cf); > > + if (skb == NULL) > > + return; > > See comment above. > > > + cf->can_id = id & ESD_IDMASK; > > + cf->can_dlc = get_can_dlc(msg->msg.rx.dlc); > > + > > + if (id & ESD_EXTID) > > + cf->can_id |= CAN_EFF_FLAG; > > + > > + if (msg->msg.rx.dlc & ESD_RTR) { > > + cf->can_id |= CAN_RTR_FLAG; > > + } else { > > + for (i = 0; i < cf->can_dlc; i++) > > + cf->data[i] = msg->msg.rx.data[i]; > > + } > > + > > + netif_rx(skb); > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32) > > + net->netdev->last_rx = jiffies; > > +#endif > > + stats->rx_packets++; > > + stats->rx_bytes += cf->can_dlc; > > + } > > + > > + return; > > +} > > + > > +static void esd_usb2_tx_done_msg(struct esd_usb2_net *net, > > + struct esd_usb2_msg *msg) > > +{ > > + struct net_device_stats *stats = &net->netdev->stats; > > + > > + if (!netif_device_present(net->netdev)) > > + return; > > + > > + if (msg->msg.txdone.status == 0) { > > + stats->tx_packets++; > > + stats->tx_bytes += msg->msg.txdone.hnd & 0xf; > > + } > > +} > > + > > +static void esd_usb2_read_bulk_callback(struct urb *urb) > > +{ > > + struct esd_usb2 *dev = urb->context; > > + int retval; > > + int pos = 0; > > + int i; > > + > > + switch (urb->status) { > > + case 0: /* success */ > > + break; > > + > > + case -ENOENT: > > + case -ESHUTDOWN: > > + return; > > + > > + default: > > + dev_info(dev->udev->dev.parent, > > + "Rx URB aborted (%d)\n", urb->status); > > + goto resubmit_urb; > > + } > > + > > + while (pos < urb->actual_length) { > > + struct esd_usb2_msg *msg; > > + u8 *ibuf = urb->transfer_buffer; > > + > > + msg = (struct esd_usb2_msg *)&ibuf[pos]; > > I don't see a need for an extra variable. "urb->transfer_buffer + pos" > is pointing to the required location. > > > + > > + dev_dbg(dev->udev->dev.parent, "%d/%d msg=%02x\n", > > + pos, urb->actual_length, msg->msg.hdr.cmd); > > For debugging? > > > + switch (msg->msg.hdr.cmd) { > > + case CMD_CAN_RX: > > + esd_usb2_rx_can_msg(dev->nets[msg->msg.rx.net], msg); > > + break; > > + > > + case CMD_CAN_TX: > > + esd_usb2_tx_done_msg(dev->nets[msg->msg.txdone.net], > > + msg); > > + break; > > + > > + default: > > + break; > > The above two lines can be removed savely, I think. > > > + } > > + > > + pos += msg->msg.hdr.len << 2; > > + > > + if (pos > urb->actual_length) { > > + dev_err(dev->udev->dev.parent, "format error\n"); > > + break; > > + } > > + } > > + > > +resubmit_urb: > > + usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1), > > + urb->transfer_buffer, RX_BUFFER_SIZE, > > + esd_usb2_read_bulk_callback, dev); > > + > > + retval = usb_submit_urb(urb, GFP_ATOMIC); > > + if (retval == -ENODEV) { > > + for (i = 0; i < dev->no_nets; i++) > > "no_nets" sounds like "No nets" to me. "no_of_nets" of "net_count" is > more understandable, I believe. > > > + netif_device_detach(dev->nets[i]->netdev); > > + } else if (retval) { > > + dev_err(dev->udev->dev.parent, > > + "failed resubmitting read bulk urb: %d\n", retval); > > + } > > + > > + return; > > +} > > + > > +/* > > + * callback for bulk IN urb > > + */ > > +static void esd_usb2_write_bulk_callback(struct urb *urb) > > +{ > > + struct esd_tx_urb_context *context = urb->context; > > + struct esd_usb2_net *net; > > + struct esd_usb2 *dev; > > + struct net_device *netdev; > > + size_t size = sizeof(struct esd_usb2_msg); > > + > > + BUG_ON(!context); > > + > > + net = context->net; > > + netdev = net->netdev; > > + dev = net->usb2; > > + > > + /* free up our allocated buffer */ > > + usb_buffer_free(urb->dev, size, > > + urb->transfer_buffer, urb->transfer_dma); > > + > > + atomic_dec(&net->active_tx_urbs); > > + > > + if (!netif_device_present(netdev)) > > + return; > > + > > + if (urb->status) > > + dev_info(ND2D(netdev), "Tx URB aborted (%d)\n", > > + urb->status); > > + > > + netdev->trans_start = jiffies; > > + > > + can_get_echo_skb(netdev, context->echo_index); > > This functions is called when the USB transfer has completed, right? > can_get_echo_skb() should be called when the TX done notification > arrives (in esd_usb2_tx_done_msg). This is *wrong* in the ems_usb driver > as well and should be fixed. > > > + > > + /* Release context */ > > + context->echo_index = MAX_TX_URBS; > > + > > + if (netif_queue_stopped(netdev)) > > + netif_wake_queue(netdev); > > +} > > + > > +#ifdef CONFIG_SYSFS > > +static ssize_t show_firmware(struct device *d, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct usb_interface *intf = to_usb_interface(d); > > + struct esd_usb2 *dev = usb_get_intfdata(intf); > > + > > + return sprintf(buf, "%d.%d.%d\n", > > + (dev->version >> 12) & 0xf, > > + (dev->version >> 8) & 0xf, > > + dev->version & 0xff); > > +} > > +static DEVICE_ATTR(firmware, S_IRUGO, show_firmware, NULL); > > + > > +static ssize_t show_hardware(struct device *d, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct usb_interface *intf = to_usb_interface(d); > > + struct esd_usb2 *dev = usb_get_intfdata(intf); > > + > > + return sprintf(buf, "%d.%d.%d\n", > > + (dev->version >> 28) & 0xf, > > + (dev->version >> 24) & 0xf, > > + (dev->version >> 16) & 0xff); > > +} > > +static DEVICE_ATTR(hardware, S_IRUGO, show_hardware, NULL); > > + > > +static ssize_t show_nets(struct device *d, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct usb_interface *intf = to_usb_interface(d); > > + struct esd_usb2 *dev = usb_get_intfdata(intf); > > + > > + return sprintf(buf, "%d", dev->no_nets); > > +} > > +static DEVICE_ATTR(nets, S_IRUGO, show_nets, NULL); > > +#endif > > + > > +static int esd_usb2_send_msg(struct esd_usb2 *dev, struct esd_usb2_msg > > *msg) > > +{ > > + int actual_length; > > + > > + return usb_bulk_msg(dev->udev, > > + usb_sndbulkpipe(dev->udev, 2), > > + msg, > > + msg->msg.hdr.len << 2, > > + &actual_length, > > + 1000); > > +} > > + > > +static int esd_usb2_wait_msg(struct esd_usb2 *dev, > > + struct esd_usb2_msg *msg) > > +{ > > + int actual_length; > > + > > + return usb_bulk_msg(dev->udev, > > + usb_rcvbulkpipe(dev->udev, 1), > > + msg, > > + sizeof(*msg), > > + &actual_length, > > + 1000); > > +} > > + > > +static int esd_usb2_setup_rx_urbs(struct esd_usb2 *dev) > > +{ > > + int i, err; > > + > > + if (dev->rxinitdone) > > + return 0; > > + > > + for (i = 0; i < MAX_RX_URBS; i++) { > > + struct urb *urb = NULL; > > + u8 *buf = NULL; > > + > > + /* create a URB, and a buffer for it */ > > + urb = usb_alloc_urb(0, GFP_KERNEL); > > + if (!urb) { > > + dev_err(dev->udev->dev.parent, > > + "No memory left for URBs\n"); > > + return -ENOMEM; > > + } > > + > > + buf = usb_buffer_alloc(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL, > > + &urb->transfer_dma); > > + if (!buf) { > > + dev_err(dev->udev->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, > > + esd_usb2_read_bulk_callback, dev); > > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > + usb_anchor_urb(urb, &dev->rx_submitted); > > + > > + err = usb_submit_urb(urb, GFP_KERNEL); > > + if (err) { > > + usb_unanchor_urb(urb); > > + usb_buffer_free(dev->udev, RX_BUFFER_SIZE, buf, > > + urb->transfer_dma); > > I wonder if skb_free_urb(urb); is missing here. > > > + 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(dev->udev->dev.parent, "couldn't setup read URBs\n"); > > + return err; > > + } > > + > > + /* Warn if we've couldn't transmit all the URBs */ > > + if (i < MAX_RX_URBS) { > > + dev_warn(dev->udev->dev.parent, > > + "rx performance may be slow\n"); > > + } > > + > > + dev->rxinitdone = 1; > > + > > + return 0; > > +} > > + > > + > > +/* > > + * Start interface > > + */ > > +static int esd_usb2_start(struct esd_usb2_net *net) > > +{ > > + struct esd_usb2 *dev = net->usb2; > > + struct net_device *netdev = net->netdev; > > + int err, i; > > + struct esd_usb2_msg msg; > > + > > + /* enable all (!) IDs */ > > + msg.msg.hdr.cmd = CMD_IDADD; > > + msg.msg.hdr.len = 1 + 64 + 1; > > Hm, I think you understand what it means, but for me "1 + 64 + 1" is > just magic. Please use a proper #define for 65 (or 64), also in the > struct above and problably add also some comment. > > > + msg.msg.filter.net = net->index; > > + msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */ > > + for (i = 0; i < 64; i++) > > + msg.msg.filter.mask[i] = cpu_to_le32(0xffffffff); > > + msg.msg.filter.mask[64] = cpu_to_le32(0x00000001); > > + > > + err = esd_usb2_send_msg(dev, &msg); > > + if (err) > > + goto failed; > > + > > + esd_usb2_setup_rx_urbs(dev); > > + > > + net->can.state = CAN_STATE_ERROR_ACTIVE; > > + > > + return 0; > > + > > +failed: > > + if (err == -ENODEV) > > + netif_device_detach(netdev); > > + > > + dev_warn(ND2D(netdev), "couldn't submit control: %d\n", err); > > + > > + return err; > > +} > > + > > +static void unlink_all_urbs(struct esd_usb2 *dev) > > +{ > > + int i; > > + struct esd_usb2_net *net; > > + > > + usb_kill_anchored_urbs(&dev->rx_submitted); > > + > > + for (i = 0; i < dev->no_nets; i++) { > > + net = dev->nets[i]; > > + if (net) { > > + usb_kill_anchored_urbs(&net->tx_submitted); > > + atomic_set(&net->active_tx_urbs, 0); > > + > > + for (i = 0; i < MAX_TX_URBS; i++) > > + net->tx_contexts[i].echo_index = MAX_TX_URBS; > > + } > > + } > > +} > > + > > + > > +static int esd_usb2_open(struct net_device *netdev) > > +{ > > + struct esd_usb2_net *net = netdev_priv(netdev); > > + int err; > > + > > + /* common open */ > > + err = open_candev(netdev); > > + if (err) > > + return err; > > + > > + /* finally start device */ > > + err = esd_usb2_start(net); > > + if (err) { > > + if (err == -ENODEV) > > + netif_device_detach(netdev); > > + > > + dev_warn(ND2D(netdev), "couldn't start device: %d\n", err); > > + > > + close_candev(netdev); > > + > > + return err; > > + } > > + > > + net->open_time = jiffies; > > + > > + netif_start_queue(netdev); > > + > > + return 0; > > +} > > + > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32) > > +static int esd_usb2_start_xmit(struct sk_buff *skb, struct net_device > > *netdev) > > +#else > > +static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb, > > + struct net_device *netdev) > > +#endif > > +{ > > + struct esd_usb2_net *net = netdev_priv(netdev); > > + struct esd_usb2 *dev = net->usb2; > > + struct esd_tx_urb_context *context = NULL; > > + struct net_device_stats *stats = &netdev->stats; > > + struct can_frame *cf = (struct can_frame *)skb->data; > > + struct esd_usb2_msg *msg; > > + struct urb *urb; > > + u8 *buf; > > + int i, err; > > + size_t size = sizeof(struct esd_usb2_msg); > > + > > + /* 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(ND2D(netdev), "No memory left for URBs\n"); > > + goto nomem; > > + } > > + > > + buf = usb_buffer_alloc(dev->udev, size, GFP_ATOMIC, &urb->transfer_dma); > > + if (!buf) { > > + dev_err(ND2D(netdev), "No memory left for USB buffer\n"); > > + usb_free_urb(urb); > > + goto nomem; > > + } > > + > > + msg = (struct esd_usb2_msg *)buf; > > + > > + msg->msg.hdr.len = 3; /* minimal lenght */ > > + msg->msg.hdr.cmd = CMD_CAN_TX; > > + msg->msg.tx.net = net->index; > > + msg->msg.tx.dlc = cf->can_dlc; > > + /* hnd must not be 0 */ > > + msg->msg.tx.hnd = 0x80000000 | cf->can_dlc; > > + msg->msg.tx.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK); > > + > > + if (cf->can_id & CAN_RTR_FLAG) > > + msg->msg.tx.dlc |= ESD_RTR; > > + > > + if (cf->can_id & CAN_EFF_FLAG) > > + msg->msg.tx.id |= cpu_to_le32(ESD_EXTID); > > + > > + for (i = 0; i < cf->can_dlc; i++) > > + msg->msg.tx.data[i] = cf->data[i]; > > + > > + msg->msg.hdr.len += (cf->can_dlc + 3) >> 2; > > + > > + for (i = 0; i < MAX_TX_URBS; i++) { > > + if (net->tx_contexts[i].echo_index == MAX_TX_URBS) { > > + context = &net->tx_contexts[i]; > > + break; > > + } > > + } > > + > > + if (!context) { > > + usb_buffer_free(dev->udev, size, buf, urb->transfer_dma); > > + usb_free_urb(urb); > > + dev_warn(ND2D(netdev), "couldn't find free context\n"); > > + return NETDEV_TX_BUSY; > > + } > > + > > + context->net = net; > > + context->echo_index = i; > > + > > + usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf, > > + msg->msg.hdr.len << 2, > > + esd_usb2_write_bulk_callback, context); > > + > > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > + > > + usb_anchor_urb(urb, &net->tx_submitted); > > + > > + can_put_echo_skb(skb, netdev, context->echo_index); > > + > > + atomic_inc(&net->active_tx_urbs); > > + > > + err = usb_submit_urb(urb, GFP_ATOMIC); > > + if (unlikely(err)) { > > That's the only place where you use "unlikely". Either drop it or use it > consequently for frequently executed branches throughout this file. It's > just copied from ems_usb, I think. > > > + can_free_echo_skb(netdev, context->echo_index); > > + > > + usb_unanchor_urb(urb); > > + usb_buffer_free(dev->udev, size, buf, urb->transfer_dma); > > + dev_kfree_skb(skb); > > + > > + atomic_dec(&net->active_tx_urbs); > > + > > + if (err == -ENODEV) { > > + netif_device_detach(netdev); > > + } else { > > + dev_warn(ND2D(netdev), "failed tx_urb %d\n", err); > > + stats->tx_dropped++; > > + } > > + } else { > > + netdev->trans_start = jiffies; > > + > > + /* Slow down tx path */ > > + if (atomic_read(&net->active_tx_urbs) >= MAX_TX_URBS) > > + netif_stop_queue(netdev); > > + } > > + > > + /* > > + * Release our reference to this URB, the USB core will eventually free > > + * it entirely. > > + */ > > + usb_free_urb(urb); > > + > > + return NETDEV_TX_OK; > > + > > +nomem: > > + if (skb) > > + dev_kfree_skb(skb); > > + > > + stats->tx_dropped++; > > + > > + return NETDEV_TX_OK; > > +} > > + > > +static int esd_usb2_close(struct net_device *netdev) > > +{ > > + struct esd_usb2_net *net = netdev_priv(netdev); > > + int i; > > + struct esd_usb2_msg msg; > > + > > + /* Disable all IDs */ > > + msg.msg.hdr.cmd = CMD_IDADD; > > + msg.msg.hdr.len = 1 + 64 + 1; > > + msg.msg.filter.net = net->index; > > + msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */ > > + for (i = 0; i < 64; i++) > > + msg.msg.filter.mask[i] = 0; > > + msg.msg.filter.mask[64] = 0; > > Hm, no need to assign mask[64] separately. Also, as commented above, > please use a macro definition for the maximum number. > > > + esd_usb2_send_msg(net->usb2, &msg); > > + > > + /* set CAN controller to reset mode */ > > + msg.msg.setbaud.len = 2; > > + msg.msg.setbaud.cmd = CMD_SETBAUD; > > + msg.msg.setbaud.net = net->index; > > + msg.msg.setbaud.rsvd = 0; > > + msg.msg.setbaud.baud = cpu_to_le32(ESD_NO_BAUDRATE); > > + esd_usb2_send_msg(net->usb2, &msg); > > + > > + netif_stop_queue(netdev); > > + > > + close_candev(netdev); > > + > > + net->open_time = 0; > > + > > + return 0; > > +} > > + > > +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,28) > > +static const struct net_device_ops esd_usb2_netdev_ops = { > > + .ndo_open = esd_usb2_open, > > + .ndo_stop = esd_usb2_close, > > + .ndo_start_xmit = esd_usb2_start_xmit, > > +}; > > +#endif > > + > > +static struct can_bittiming_const esd_usb2_bittiming_const = { > > + .name = "esd_usb2", > > + .tseg1_min = 1, > > + .tseg1_max = 16, > > + .tseg2_min = 1, > > + .tseg2_max = 8, > > + .sjw_max = 4, > > + .brp_min = 1, > > + .brp_max = 1024, > > + .brp_inc = 1, > > +}; > > Randomly placed? Consider moving it to the file header. > > > +static int esd_usb2_set_mode(struct net_device *netdev, enum can_mode mode) > > +{ > > + struct esd_usb2_net *net = netdev_priv(netdev); > > + > > + if (!net->open_time) > > + return -EINVAL; > > + > > + switch (mode) { > > + case CAN_MODE_START: > > + if (netif_queue_stopped(netdev)) > > + netif_wake_queue(netdev); > > + break; > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int esd_usb2_set_bittiming(struct net_device *netdev) > > +{ > > + struct esd_usb2_net *net = netdev_priv(netdev); > > + struct can_bittiming *bt = &net->can.bittiming; > > + struct esd_usb2_msg msg; > > + u32 canbtr; > > + > > + canbtr = ESD_UBR; > > + canbtr |= (bt->brp - 1) & 0x3ff; > > + canbtr |= ((bt->sjw - 1) & 0x3) << 14; > > + canbtr |= ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) << 16; > > + canbtr |= ((bt->phase_seg2 - 1) & 0x7) << 20; > > + > > + if (net->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > > + canbtr |= 0x00800000; > > + > > + msg.msg.setbaud.len = 2; > > + msg.msg.setbaud.cmd = CMD_SETBAUD; > > + msg.msg.setbaud.net = net->index; > > + msg.msg.setbaud.rsvd = 0; > > + msg.msg.setbaud.baud = cpu_to_le32(canbtr); > > It's common practice to add here: > > dev_info(ND2D(netdev), "Setting canbtr=%#x\n", ...); > > > + > > + return esd_usb2_send_msg(net->usb2, &msg); > > +} > > + > > +static int esd_usb2_probe_one_net(struct usb_interface *intf, int index) > > +{ > > + struct esd_usb2 *dev = usb_get_intfdata(intf); > > + struct net_device *netdev; > > + struct esd_usb2_net *net; > > + int err; > > + int i; > > + > > + netdev = alloc_candev(sizeof(struct esd_usb2_net), MAX_TX_URBS); > > netdev = alloc_candev(sizeof(*net), MAX_TX_URBS); > > is clearer, IMHO. > > > + if (!netdev) { > > + dev_err(&intf->dev, "couldn't alloc candev\n"); > > + return -ENOMEM; > > + } > > + > > + net = netdev_priv(netdev); > > I personally don't find the name "net" well suited for the private data > of netdev. What about "struct esd_usb2_priv" and "priv"? > > > + > > + init_usb_anchor(&net->tx_submitted); > > + atomic_set(&net->active_tx_urbs, 0); > > + > > + for (i = 0; i < MAX_TX_URBS; i++) > > + net->tx_contexts[i].echo_index = MAX_TX_URBS; > > + > > + net->usb2 = dev; > > + net->netdev = netdev; > > + net->index = index; > > + > > + net->can.state = CAN_STATE_STOPPED; > > + net->can.clock.freq = ESD_USB2_CAN_CLOCK; > > + net->can.bittiming_const = &esd_usb2_bittiming_const; > > + net->can.do_set_bittiming = esd_usb2_set_bittiming; > > + net->can.do_set_mode = esd_usb2_set_mode; > > + > > + netdev->flags |= IFF_ECHO; /* we support local echo */ > > + > > +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,28) > > + netdev->netdev_ops = &esd_usb2_netdev_ops; > > +#else > > + netdev->open = esd_usb2_open; > > + netdev->stop = esd_usb2_close; > > + netdev->hard_start_xmit = esd_usb2_start_xmit; > > +#endif > > + > > + netdev->flags |= IFF_ECHO; /* we support local echo */ > > + > > + SET_NETDEV_DEV(netdev, &intf->dev); > > + > > + err = register_candev(netdev); > > + if (err) { > > + dev_err(netdev->dev.parent, > > + "couldn't register CAN device: %d\n", err); > > + free_candev(netdev); > > + return -ENOMEM; > > + } > > + > > + dev->nets[index] = net; > > + return 0; > > +} > > + > > +/* > > + * probe function for new USB2 devices > > + * > > + * check version information and number of available > > + * CAN interfaces > > + */ > > +static int esd_usb2_probe(struct usb_interface *intf, > > + const struct usb_device_id *id) > > +{ > > + struct esd_usb2 *dev; > > + struct esd_usb2_msg msg; > > + int i, err = -ENOMEM; > > + > > + dev = kzalloc(sizeof(struct esd_usb2), GFP_KERNEL); > > sizeof(*dev)? > > > + if (!dev) > > + return -ENOMEM; > > + > > + dev->udev = interface_to_usbdev(intf); > > + > > + init_usb_anchor(&dev->rx_submitted); > > + > > + usb_set_intfdata(intf, dev); > > + > > + /* query number of CAN interfaces (nets) */ > > + msg.msg.hdr.cmd = CMD_VERSION; > > + msg.msg.hdr.len = 2; > > + msg.msg.version.rsvd = 0; > > + msg.msg.version.flags = 0; > > + msg.msg.version.drv_version = 0; > > + > > + if (esd_usb2_send_msg(dev, &msg) < 0) { > > + dev_err(&intf->dev, "sending version message failed\n"); > > + goto free_dev; > > + } > > + > > + if (esd_usb2_wait_msg(dev, &msg) < 0) { > > + dev_err(&intf->dev, "no version message answer\n"); > > + goto free_dev; > > + } > > + > > + dev->no_nets = (int)msg.msg.version_reply.nets; > > + dev->version = le32_to_cpu(msg.msg.version_reply.version); > > + > > +#ifdef CONFIG_SYSFS > > + device_create_file(&intf->dev, &dev_attr_firmware); > > + device_create_file(&intf->dev, &dev_attr_hardware); > > + device_create_file(&intf->dev, &dev_attr_nets); > > +#endif > > + > > + /* do per device probing */ > > + for (i = 0; i < dev->no_nets; i++) > > + esd_usb2_probe_one_net(intf, i); > > + > > + return 0; > > + > > +free_dev: > > + kfree(dev); > > + return err; > > +} > > + > > +/* > > + * called by the usb core when the device is removed from the system > > + */ > > +static void esd_usb2_disconnect(struct usb_interface *intf) > > +{ > > + struct esd_usb2 *dev = usb_get_intfdata(intf); > > + struct net_device *netdev; > > + int i; > > + > > +#ifdef CONFIG_SYSFS > > + device_remove_file(&intf->dev, &dev_attr_firmware); > > + device_remove_file(&intf->dev, &dev_attr_hardware); > > + device_remove_file(&intf->dev, &dev_attr_nets); > > +#endif > > + usb_set_intfdata(intf, NULL); > > + > > + if (dev) { > > + for (i = 0; i < dev->no_nets; i++) { > > + if (dev->nets[i]) { > > + netdev = dev->nets[i]->netdev; > > + unregister_netdev(netdev); > > + free_candev(netdev); > > + } > > + } > > + unlink_all_urbs(dev); > > + } > > +} > > + > > +/* usb specific object needed to register this driver with the usb > > subsystem */ > > +static struct usb_driver esd_usb2_driver = { > > + .name = "esd_usb2", > > + .probe = esd_usb2_probe, > > + .disconnect = esd_usb2_disconnect, > > + .id_table = esd_usb2_table, > > +}; > > + > > +static int __init esd_usb2_init(void) > > +{ > > + int err; > > + > > + printk(KERN_INFO "esd USB Mini/2 kernel driver\n"); > > + > > + /* register this driver with the USB subsystem */ > > + err = usb_register(&esd_usb2_driver); > > + > > + if (err) { > > + err("usb_register failed. Error number %d\n", err); > > + return err; > > + } > > + > > + return 0; > > +} > > + > > +static void __exit esd_usb2_exit(void) > > +{ > > + /* deregister this driver with the USB subsystem */ > > + usb_deregister(&esd_usb2_driver); > > +} > > + > > +module_init(esd_usb2_init); > > +module_exit(esd_usb2_exit); > > Index: kernel/2.6/drivers/net/can/usb/Makefile > > =================================================================== > > --- kernel/2.6/drivers/net/can/usb/Makefile (revision 1095) > > +++ kernel/2.6/drivers/net/can/usb/Makefile (working copy) > > @@ -16,6 +16,7 @@ > > -include $(TOPDIR)/Makefile.common > > > > obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o > > +obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o > > > > ifeq ($(CONFIG_CAN_DEBUG_DEVICES),y) > > EXTRA_CFLAGS += -DDEBUG > > Index: kernel/2.6/drivers/net/can/Makefile > > =================================================================== > > --- kernel/2.6/drivers/net/can/Makefile (revision 1095) > > +++ kernel/2.6/drivers/net/can/Makefile (working copy) > > @@ -19,6 +19,7 @@ > > export CONFIG_CAN_EMS_PCI=m > > export CONFIG_CAN_EMS_PCMCIA=m > > export CONFIG_CAN_ESD_PCI331=m > > +export CONFIG_CAN_ESD_USB2=m > > export CONFIG_CAN_PIPCAN=m > > export CONFIG_CAN_SOFTING=m > > export CONFIG_CAN_SOFTING_CS=m > > _______________________________________________ > > Wolfgang. > > -- ------------------------------------------------------------------------- Dipl.-Ing. Matthias Fuchs Head of System Design esd electronic system design gmbh Vahrenwalder Str. 207 - 30165 Hannover - GERMANY Phone: +49-511-37298-0 - Fax: +49-511-37298-68 Please visit our homepage http://www.esd.eu Quality Products - Made in Germany ------------------------------------------------------------------------- Geschäftsführer: Klaus Detering, Dr. Werner Schulze Amtsgericht Hannover HRB 51373 - VAT-ID DE 115672832 ------------------------------------------------------------------------- _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
