On 11/20/2012 09:46 AM, Olivier Sobrie wrote:
[...]
>>> +static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>>> + struct net_device *netdev)
>>> +{
>>> + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>>> + struct kvaser_usb *dev = priv->dev;
>>> + struct net_device_stats *stats = &netdev->stats;
>>> + struct can_frame *cf = (struct can_frame *)skb->data;
>>> + struct kvaser_usb_tx_urb_context *context = NULL;
>>> + struct urb *urb;
>>> + void *buf;
>>> + struct kvaser_msg *msg;
>>> + int i, err;
>>> + int ret = NETDEV_TX_OK;
>>> +
>>> + if (can_dropped_invalid_skb(netdev, skb))
>>> + return NETDEV_TX_OK;
>>> +
>>> + urb = usb_alloc_urb(0, GFP_ATOMIC);
>>> + if (!urb) {
>>> + netdev_err(netdev, "No memory left for URBs\n");
>>> + stats->tx_dropped++;
>>
>> Move the dev_kfree_skb to the end and goto there.
>
> I assume you mean doing something like that at the end of the function:
Yes.
> releasebuf:
> kfree(buf);
> nobufmem:
> usb_free_urb(urb);
> nourbmem:
> dev_kfree_skb(skb);
> return ret;
>
> If I do that it will give problems when the 'releasebuf' condition is
> reached. The skb buffer will be freed twice. The skb is already freed
> by the function can_free_echo_skb().
Okay. dev_kfree_skb(skb) will work with skb == NULL. So just set skb to
NULL after can_free_echo_skb(). Maybe along with a short comment: "set
to NULL to avoid double free in dev_kfree_skb(skb)".
>
>>
>>> + dev_kfree_skb(skb);
>>> + return NETDEV_TX_OK;
>>> + }
>>> +
>>> + buf = kmalloc(sizeof(struct kvaser_msg), GFP_ATOMIC);
>>> + if (!buf) {
>>> + netdev_err(netdev, "No memory left for USB buffer\n");
>>> + stats->tx_dropped++;
>> You cann usb_free_urb twice...here and in the error handling at the end.
>
> Indeed thanks.
>
>>
>>> + dev_kfree_skb(skb);
>>> + usb_free_urb(urb);
>>> + goto nobufmem;
>>> + }
>>> +
>>> + msg = buf;
>>> + msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
>>> + msg->u.tx_can.flags = 0;
>>> + msg->u.tx_can.channel = priv->channel;
>>> +
>>> + if (cf->can_id & CAN_EFF_FLAG) {
>>> + msg->id = CMD_TX_EXT_MESSAGE;
>>> + msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
>>> + msg->u.tx_can.msg[1] = (cf->can_id >> 18) & 0x3f;
>>> + msg->u.tx_can.msg[2] = (cf->can_id >> 14) & 0x0f;
>>> + msg->u.tx_can.msg[3] = (cf->can_id >> 6) & 0xff;
>>> + msg->u.tx_can.msg[4] = cf->can_id & 0x3f;
>>> + } else {
>>> + msg->id = CMD_TX_STD_MESSAGE;
>>> + msg->u.tx_can.msg[0] = (cf->can_id >> 6) & 0x1f;
>>> + msg->u.tx_can.msg[1] = cf->can_id & 0x3f;
>>> + }
>>> +
>>> + msg->u.tx_can.msg[5] = cf->can_dlc;
>>> + memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
>>> +
>>> + if (cf->can_id & CAN_RTR_FLAG)
>>> + msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
>>> + if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
>>> + context = &priv->tx_contexts[i];
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (!context) {
>>> + netdev_warn(netdev, "cannot find free context\n");
>>> + ret = NETDEV_TX_BUSY;
>>> + goto releasebuf;
>>> + }
>>> +
>>> + context->priv = priv;
>>> + context->echo_index = i;
>>> + context->dlc = cf->can_dlc;
>>> +
>>> + msg->u.tx_can.tid = context->echo_index;
>>> +
>>> + usb_fill_bulk_urb(urb, dev->udev,
>>> + usb_sndbulkpipe(dev->udev,
>>> + dev->bulk_out->bEndpointAddress),
>>> + buf, msg->len,
>>> + kvaser_usb_write_bulk_callback, context);
>>> + usb_anchor_urb(urb, &priv->tx_submitted);
>>> +
>>> + can_put_echo_skb(skb, netdev, context->echo_index);
>>> +
>>> + atomic_inc(&priv->active_tx_urbs);
>>> +
>>> + if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
>>> + netif_stop_queue(netdev);
>>> +
>>> + err = usb_submit_urb(urb, GFP_ATOMIC);
>>> + if (unlikely(err)) {
>>> + can_free_echo_skb(netdev, context->echo_index);
>>> +
skb = NULL; /* +comment */
>>> + atomic_dec(&priv->active_tx_urbs);
>>> + usb_unanchor_urb(urb);
>>> +
>>> + stats->tx_dropped++;
>>> +
>>> + if (err == -ENODEV)
>>> + netif_device_detach(netdev);
>>> + else
>>> + netdev_warn(netdev, "Failed tx_urb %d\n", err);
>>> +
>>> + goto releasebuf;
>>> + }
>>> +
>>> + usb_free_urb(urb);
>>> +
>>> + return NETDEV_TX_OK;
>>> +
>>> +releasebuf:
>>> + kfree(buf);
>>> +nobufmem:
>>> + usb_free_urb(urb);
>>> + return ret;
>>> +}
[...]
>>> +static struct usb_driver kvaser_usb_driver = {
>>> + .name = "kvaser_usb",
>>> + .probe = kvaser_usb_probe,
>>> + .disconnect = kvaser_usb_disconnect,
>>> + .id_table = kvaser_usb_table
>> ^^^
>> nitpick, please add a "," there.
>
> Ok.
>
>>> +};
>>>
>> can you please add MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
>
> It is already present just after the kvaser_usb_table structure.
:) You're right.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
