Hi Wolfgang,
On Thursday 14 January 2010 20:48, Wolfgang Grandegger wrote:
> Matthias Fuchs wrote:
> > This patch adds support for the esd USB2 CAN interface.
>
> [snip]
> > +#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");
> > + stats->tx_dropped++;
> > + if (skb)
> > + kfree_skb(skb);
>
> skb cannot be zero.
Of course. Checking skb also would be to late here because it's already
references above in the initialisation of cf.
I must admit that I took that check from a related driver :-)
>
> > + 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");
> > + stats->tx_dropped++;
> > + if (skb)
> > + kfree_skb(skb);
>
> Ditto.
Copy'n'paste ...
>
> > + 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;
> > + if (skb)
> > + kfree_skb(skb);
>
> If you return with NETDEV_TX_BUSY, you should *not* free the skb as the
> upper layer will retry later.
... and finally ne paste to much without thinking. Urrg.
>
> > + 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);
> > +
> > + stats->tx_dropped++;
> > +
> > + if (err == -ENODEV)
> > + netif_device_detach(netdev);
> > + else
> > + dev_warn(ND2D(netdev), "failed tx_urb %d\n", err);
> > +
> > + goto releasebuf;
> > + }
> > +
> > + 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:
> > + return ret;
> > +}
> > +
>
> Puh, to avoid another round, I fixed the issues above and another one
> reported by checkpatch and commited the patch with your signed-of-by
> line. Please check.
Thanks. I will checkout and test it.
>
> Thanks for your contribution.
Thanks for review and comments.
BTW, how do we keep SVN and kernel/linux-2.6-net more or less in sync?
Should I post a proper patch to the netdev list?
Matthias
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
