-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Oliver Neukum schrieb:
> Am Samstag, 12. September 2009 12:28:40 schrieb Sebastian Haas:
>> The following patch adds a driver to support the CAN\USB interface
>> CPC-USB/ARM7. The patch was already posted on netdev for inclusion, but as
>> the driver is also a USB driver it would be nice if someone of the Linux
>> USB folks could take a look at the USB side of the driver. Thanks.
>>
>
> I put comments among the code. I hope you find them useful.
>
> Regards
> Oliver
>
>> +
>> +/*
>> + * callback for bulk IN urb
>> + */
>> +static void ems_usb_write_bulk_callback(struct urb *urb)
>> +{
>> + struct ems_tx_urb_context *context = urb->context;
>> + struct ems_usb *dev;
>> + struct net_device *netdev;
>> +
>> + if (!context)
>> + return;
> Can this happen?
No. Would a BUG_ON too paranoid?
>> +
>> + dev = context->dev;
>> + netdev = dev->netdev;
>> +
>> + if (!netif_device_present(netdev))
>> + return;
>
> You don't free the buffer in the error case. Memory leak?
Oh, yes.
>> +
>> + if (urb->status)
>> + dev_info(netdev->dev.parent, "Tx URB aborted (%d)\n",
>> + urb->status);
>> +
>> + /* free up our allocated buffer */
>> + usb_buffer_free(urb->dev, urb->transfer_buffer_length,
>> + urb->transfer_buffer, urb->transfer_dma);
>> +
>> + netdev->trans_start = jiffies;
>> +
>> + /* transmission complete interrupt */
>> + 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;
>> +
>> + atomic_dec(&dev->active_tx_urbs);
>> +
>> + if (netif_queue_stopped(netdev))
>> + netif_wake_queue(netdev);
>> +}
>> +
> [..]
>> +
>> +/*
>> + * Start interface
>> + */
>> +static int ems_usb_start(struct ems_usb *dev)
>> +{
>> + struct net_device *netdev = dev->netdev;
>> + int err, i;
>> +
>> + dev->intr_in_buffer[0] = 0;
>> + 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_ATOMIC);
>
> Why GFP_ATOMIC?
No reason, will change to GFP_KERNEL.
>> + if (!urb) {
>> + dev_err(netdev->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(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, 2),
>> + buf, RX_BUFFER_SIZE,
>> + ems_usb_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_buffer_free(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");
>> +
>> + /* Setup and start interrupt URB */
>> + usb_fill_int_urb(dev->intr_urb, dev->udev,
>> + usb_rcvintpipe(dev->udev, 1),
>> + dev->intr_in_buffer,
>
> This is a DMA coherency bug. You need to allocate a separate buffer.
Okay, good hint. Didn't know anything about it. Will fix it.
>> + sizeof(dev->intr_in_buffer),
>> + ems_usb_read_interrupt_callback, dev, 1);
>> +
>> + err = usb_submit_urb(dev->intr_urb, GFP_KERNEL);
>> + if (err) {
>> + if (err == -ENODEV)
>> + netif_device_detach(dev->netdev);
>> +
>> + dev_warn(netdev->dev.parent, "intr URB submit failed: %d\n",
>> + err);
>> +
>> + return err;
>> + }
>> +
>> + /* CPC-USB will transfer received message to host */
>> + err = ems_usb_control_cmd(dev, CONTR_CAN_MESSAGE | CONTR_CONT_ON);
>> + if (err)
>> + goto failed;
>> +
>> + /* CPC-USB will transfer CAN state changes to host */
>> + err = ems_usb_control_cmd(dev, CONTR_CAN_STATE | CONTR_CONT_ON);
>> + if (err)
>> + goto failed;
>> +
>> + /* CPC-USB will transfer bus errors to host */
>> + err = ems_usb_control_cmd(dev, CONTR_BUS_ERROR | CONTR_CONT_ON);
>> + if (err)
>> + goto failed;
>> +
>> + err = ems_usb_write_mode(dev, SJA1000_MOD_NORMAL);
>> + 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;
>> +}
> [..]
>> +
>> +static int ems_usb_start_xmit(struct sk_buff *skb, struct net_device
>> *netdev) +{
>> + struct ems_usb *dev = netdev_priv(netdev);
>> + struct ems_tx_urb_context *context = NULL;
>> + struct net_device_stats *stats = &netdev->stats;
>> + struct can_frame *cf = (struct can_frame *)skb->data;
>> + struct ems_cpc_msg *msg;
>> + struct urb *urb;
>> + u8 *buf;
>> + int i, err;
>> + size_t size = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN
>> + + sizeof(struct cpc_can_msg);
>> +
>> + /* create a URB, and a buffer for it, and copy the data to the URB */
>> + urb = usb_alloc_urb(0, GFP_ATOMIC);
>
> GFP_ATOMIC
>
>> + if (!urb) {
>> + dev_err(netdev->dev.parent, "No memory left for URBs\n");
>> + goto nomem;
>> + }
>> +
>> + buf = usb_buffer_alloc(dev->udev, size, GFP_KERNEL, &urb->transfer_dma);
>
> GFP_KERNEL. Only one of these flags can be right.
GFP_ATOMIC should be right, as seen in other USB network drivers.
>> + if (!buf) {
>> + dev_err(netdev->dev.parent, "No memory left for USB buffer\n");
>> + usb_free_urb(urb);
>> + goto nomem;
>> + }
>> +
>> + msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
>> +
>> + msg->msg.can_msg.id = cf->can_id & 0x1FFFFFFFU;
>> + msg->msg.can_msg.length = cf->can_dlc;
>> +
>> + if (cf->can_id & CAN_RTR_FLAG) {
>> + msg->type = cf->can_id & CAN_EFF_FLAG ?
>> + CPC_CMD_TYPE_EXT_RTR_FRAME : CPC_CMD_TYPE_RTR_FRAME;
>> +
>> + msg->length = CPC_CAN_MSG_MIN_SIZE;
>> + } else {
>> + msg->type = cf->can_id & CAN_EFF_FLAG ?
>> + CPC_CMD_TYPE_EXT_CAN_FRAME : CPC_CMD_TYPE_CAN_FRAME;
>> +
>> + for (i = 0; i < cf->can_dlc; i++)
>> + msg->msg.can_msg.msg[i] = cf->data[i];
>> +
>> + msg->length = CPC_CAN_MSG_MIN_SIZE + cf->can_dlc;
>> + }
>> +
>> + 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_buffer_free(dev->udev, size, buf, urb->transfer_dma);
>> +
>> + dev_warn(netdev->dev.parent, "couldn't find free context\n");
>> +
>> + 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, ems_usb_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);
>> +
>> + err = usb_submit_urb(urb, GFP_ATOMIC);
>> + if (unlikely(err)) {
>> + 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);
>> +
>> + if (err == -ENODEV) {
>> + netif_device_detach(netdev);
>> + } else {
>> + dev_warn(netdev->dev.parent, "failed tx_urb %d\n", err);
>> +
>> + stats->tx_dropped++;
>> + }
>> + } else {
>> + atomic_inc(&dev->active_tx_urbs);
>
> The URB may have finished, thus dropping active_tx_urbs below zero.
Good tip. Will increment active_tx_urbs before submitting urb and decrement it
on error.
>> +
>> + 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);
>
> You don't call usb_free_urb()
>
>> + return NETDEV_TX_OK;
>
> I'd simply drop the return.
Will fix it.
>> + }
>> + }
>> +
>> + /*
>> + * 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 ems_usb_close(struct net_device *netdev)
>> +{
>> + struct ems_usb *dev = netdev_priv(netdev);
>> +
>> + netif_stop_queue(netdev);
>> +
>> + /* Stop polling */
>> + unlink_all_urbs(dev);
>
> Looks like a race. A completing urb can call netif_wake_queue()
Swapped it.
>> + /* Set CAN controller to reset mode */
>> + if (ems_usb_write_mode(dev, SJA1000_MOD_RM))
>> + dev_warn(netdev->dev.parent, "couldn't stop device");
>> +
>> + close_candev(netdev);
>> +
>> + dev->open_time = 0;
>> +
>> + return 0;
>> +}
Thanks for your quick review, your comments where very helpful.
Cheers, Sebastian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkqr8+IACgkQpqRB8PJG7XwRCwCgk98+LoIJoyMVBCzGbZgV9Alp
+jkAn1EvwKA7kFbePmkMP0e0XoQwqLR9
=zQCq
-----END PGP SIGNATURE-----
--
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HRA Neuburg a.d. Donau, HR-Nr. 70.106
Phone: +49-8441-490260
Fax : +49-8441-81860
http://www.ems-wuensche.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
