Re: [PATCH v5] can: kvaser_usb: Add support for Kvaser CAN/USB devices

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5252 bytes --]

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]