Hi, Matthias Fuchs wrote: > Hi, > > here comes my updated V3 patch. tx_context handling is now working fine > and the implementation makes much more sense. No more crashes. > This seems to be a good candidate.
We are close, just a few more issues. > Matthias > > Changelog: > V3: > - handle msg->msg.txdone.status != 0 in esd_usb2_tx_done_msg() > - move active_tx_urbs-- to esd_usb2_tx_done_msg() from > ...write_bulk_callback > - move netif_wake_queue(netdev) to esd_usb2_tx_done_msg() > - add BTR macros > - rename active_tx_urbs into active_tx_jobs > > V2: > - remove duplicate line "netdev->flags |= IFF_ECHO; ..." > in esd_usb2_probe_one_net() > - set can.state back to CAN_STATE_STOPPED in esd_usb2_close() > Without this setting the bitrate via sysfs after an interface > up/down cycle does not work. > - remove "#undef DEBUG" line > - codingstyle clean (tabs, empty lines, ...) > - don't check against 0, but use !() > - remove netif_queue_stopped() check before calling netif_wake_queue() > - remove double netif_device_detach() from esd_usb2_open() > - print canbtr in esd_usb2_set_bittiming() > - use __types in structures that a exchanged with the device > - add ESD_MAX_ID_SEGMENT macro to get rid of the magic hardcoded "64" > when setting up the device's ID filter. > - Add some comments about the IDADD message. > - move esd_usb2_bittiming_const close to esd_usb2_set_bittiming() > - move macro definitions to top of file > - use if() for single case switch statements > - add macros for esd bus state event codes > - order declarations somehow: move structs to top > - get rid of ibuf variable in esd_usb2_read_bulk_callback() > - rename no_nets to net_count :-) > - consequently do not use unlikely() > - call can_get_echo_skb() from esd_usb2_tx_done_msg() to loopback > only when message has been sent successfully > - increase MAX_TX_URBS because releasing tx_contexts is now done > much later and we easily run out of free tx_contexts > - check dev->nets[i] before netif_device_detach() in ...read_bulk_callback() > - make esd_usb2_setup_rx_urbs() succeed when we got at least one urb > setup correctly > - implement common error handling in esd_usb2_start_xmit() > - handle failure of alloc_can_(err_)sbk: stats->rx_dropped++ > - rename netdev's private data pointer from "net" to "priv" > - add "device %s registered" message on successful net creation > > Index: Makefile > =================================================================== > --- Makefile (revision 1095) > +++ 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: drivers/net/can/usb/Kconfig > =================================================================== > --- drivers/net/can/usb/Kconfig (revision 1095) > +++ 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: drivers/net/can/usb/esd_usb2.c > =================================================================== > --- drivers/net/can/usb/esd_usb2.c (revision 0) > +++ drivers/net/can/usb/esd_usb2.c (revision 0) > @@ -0,0 +1,1144 @@ > +/* > + * CAN driver for esd CAN-USB/2 > + * > + * Copyright (C) 2010 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. > + */ > +#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_USB2_UBR 0x80000000 > +#define ESD_USB2_LOM 0x40000000 > +#define ESD_USB2_NO_BAUDRATE 0x7fffffff > +#define ESD_USB2_TSEG1_MIN 1 > +#define ESD_USB2_TSEG1_MAX 16 > +#define ESD_USB2_TSEG1_SHIFT 16 > +#define ESD_USB2_TSEG2_MIN 1 > +#define ESD_USB2_TSEG2_MAX 8 > +#define ESD_USB2_TSEG2_SHIFT 20 > +#define ESD_USB2_SJW_MAX 4 > +#define ESD_USB2_SJW_SHIFT 14 > +#define ESD_USB2_BRP_MIN 1 > +#define ESD_USB2_BRP_MAX 1024 > +#define ESD_USB2_BRP_INC 1 > +#define ESD_USB2_3_SAMPLES 0x00800000 > + > +/* esd IDADD message */ > +#define ESD_ID_ENABLE 0x80 > +#define ESD_MAX_ID_SEGMENT 64 > + > +/* SJA1000 ECC register (emulated by usb2 firmware) */ > +#define SJA1000_ECC_SEG 0x1F > +#define SJA1000_ECC_DIR 0x20 > +#define SJA1000_ECC_ERR 0x06 > +#define SJA1000_ECC_BIT 0x00 > +#define SJA1000_ECC_FORM 0x40 > +#define SJA1000_ECC_STUFF 0x80 > +#define SJA1000_ECC_MASK 0xc0 > + > +/* esd bus state event codes */ > +#define ESD_BUSSTATE_MASK 0xc0 > +#define ESD_BUSSTATE_WARN 0x40 > +#define ESD_BUSSTATE_ERRPASSIVE 0x80 > +#define ESD_BUSSTATE_BUSOFF 0xc0 > + > +#define RX_BUFFER_SIZE 1024 > +#define MAX_RX_URBS 4 > +#define MAX_TX_URBS 10 > + > +struct header_msg { > + __u8 len; /* len is always the total message length in 32bit words */ > + __u8 cmd; > + __u8 rsvd[2]; > +}; Please use just one type set. Either u8 or __u8 but not both. The same for __u32, __le32, etc. > +#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_priv *priv = netdev_priv(netdev); > + struct esd_usb2 *dev = priv->usb2; > + struct esd_tx_urb_context *context = NULL; > + struct net_device_stats *stats = &netdev->stats; > + struct can_frame *cf = (struct can_frame *)skb->data; > + struct esd_usb2_msg *msg; > + struct urb *urb; > + u8 *buf; > + int i, err; > + int ret = NETDEV_TX_OK; > + size_t size = sizeof(struct esd_usb2_msg); > + > + /* 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 nourbmem; > + } > + > + 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"); > + goto nobufmem; > + } > + > + msg = (struct esd_usb2_msg *)buf; > + > + msg->msg.hdr.len = 3; /* minimal length */ > + msg->msg.hdr.cmd = CMD_CAN_TX; > + msg->msg.tx.net = priv->index; > + msg->msg.tx.dlc = cf->can_dlc; > + msg->msg.tx.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK); > + > + if (cf->can_id & CAN_RTR_FLAG) > + msg->msg.tx.dlc |= ESD_RTR; > + > + if (cf->can_id & CAN_EFF_FLAG) > + msg->msg.tx.id |= cpu_to_le32(ESD_EXTID); > + > + for (i = 0; i < cf->can_dlc; i++) > + msg->msg.tx.data[i] = cf->data[i]; > + > + msg->msg.hdr.len += (cf->can_dlc + 3) >> 2; > + > + for (i = 0; i < MAX_TX_URBS; i++) { > + if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) { > + context = &priv->tx_contexts[i]; > + break; > + } > + } > + > + /* > + * This may never happen. > + */ > + if (!context) { > + dev_warn(ND2D(netdev), "couldn't find free context\n"); > + ret = NETDEV_TX_BUSY; > + goto releasebuf; > + } > + > + context->priv = priv; > + context->echo_index = i; > + context->dlc = cf->can_dlc; > + > + /* hnd must not be 0 */ > + msg->msg.tx.hnd = 0x80000000 | i; /* returned in TX done message */ > + > + usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf, > + msg->msg.hdr.len << 2, > + esd_usb2_write_bulk_callback, context); > + > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > + > + usb_anchor_urb(urb, &priv->tx_submitted); > + > + can_put_echo_skb(skb, netdev, context->echo_index); > + > + atomic_inc(&priv->active_tx_jobs); > + > + err = usb_submit_urb(urb, GFP_ATOMIC); > + if (err) { > + can_free_echo_skb(netdev, context->echo_index); > + > + atomic_dec(&priv->active_tx_jobs); > + usb_unanchor_urb(urb); > + > + if (err == -ENODEV) > + netif_device_detach(netdev); > + else > + dev_warn(ND2D(netdev), "failed tx_urb %d\n", err); > + > + goto releasebuf; > + } else { > + netdev->trans_start = jiffies; > + > + /* Slow down tx path */ > + if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS) > + netif_stop_queue(netdev); > + } > + > + /* > + * Release our reference to this URB, the USB core will eventually free > + * it entirely. > + */ > + usb_free_urb(urb); > + > + return NETDEV_TX_OK; > + > +releasebuf: > + usb_buffer_free(dev->udev, size, buf, urb->transfer_dma); > + > +nobufmem: > + usb_free_urb(urb); > + > +nourbmem: > + if (ret != NETDEV_TX_BUSY) { > + if (skb) > + dev_kfree_skb(skb); > + > + stats->tx_dropped++; Be aware that can_put_echo_skb() of can_free_echo_skb() above might have already freed the skb, also use kfree_skb() instead of dev_kfree_skb(). > + } > + > + return ret; > +} > + > +static int esd_usb2_close(struct net_device *netdev) > +{ > + struct esd_usb2_net_priv *priv = netdev_priv(netdev); > + struct esd_usb2_msg msg; > + int i; > + > + /* Disable all IDs /* * Disable all IDs Please. > + * The IDADD message takes up to ESD_MAX_ID_SEGMENT 32 bit > + * bitmasks = 2048 bits. A cleared bit disables reception > + * of the corresponding CAN identifier. > + * The next bitmask value following the CAN 2.0A > + * bits is used to disable reception of extended CAN frames > + * at all. > + */ > + msg.msg.hdr.cmd = CMD_IDADD; > + msg.msg.hdr.len = 1 + ESD_MAX_ID_SEGMENT + 1; msg.msg.hdr.len = ESD_MAX_ID_SEGMENT + 2; Please. > + msg.msg.filter.net = priv->index; > + msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */ > + for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++) > + msg.msg.filter.mask[i] = 0; > + esd_usb2_send_msg(priv->usb2, &msg); > + > + /* set CAN controller to reset mode */ > + msg.msg.hdr.len = 2; > + msg.msg.hdr.cmd = CMD_SETBAUD; > + msg.msg.setbaud.net = priv->index; > + msg.msg.setbaud.rsvd = 0; > + msg.msg.setbaud.baud = cpu_to_le32(ESD_USB2_NO_BAUDRATE); > + esd_usb2_send_msg(priv->usb2, &msg); > + > + priv->can.state = CAN_STATE_STOPPED; > + > + netif_stop_queue(netdev); > + > + close_candev(netdev); > + > + priv->open_time = 0; > + > + return 0; > +} Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
