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?
> +
> + dev = context->dev;
> + netdev = dev->netdev;
> +
> + if (!netif_device_present(netdev))
> + return;
You don't free the buffer in the error case. Memory leak?
> +
> + 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?
> + 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.
> + 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.
> + 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.
> +
> + 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.
> + }
> + }
> +
> + /*
> + * 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()
> + /* 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;
> +}
--
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
