On Monday 11 January 2010 21:10, Wolfgang Grandegger wrote:
> We are close, just a few more issues.
Uff :-)
> > +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.
Good point. Marc invented some versions before to use __le32 instead of
u32 in these device structures.
As far as I understand this should signal that these are some kind
of exchange types (typically with userspace programs or in this case
a device).
I am using __ types for all structures that are exchanged with the hardware.
All other datatypes stay in the kernel.
So what's correct?
>
> > +#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().
Ok, I will check this.
>
> > + }
> > +
> > + 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.
Of course.
>
> > + * 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.
Ok, this was for readability: 1 (msg header) + ESD_MAX_.... (for 2.0a IDs) and
+ 1 (for 2.0b ID range). But you are rigth it looks a little bit curious.
I will change it and add some more comments about how the device's ID filter
is configured.
>
> > + 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.
>
>
Matthias
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
