[PATCH 1/5] can: kvaser_usb: Avoid double free on URB submission failures

[PATCH 1/5] can: kvaser_usb: Avoid double free on URB submission failures

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Upon a URB submission failure, the driver calls usb_free_urb()
but then manually frees the URB buffer by itself.  Meanwhile
usb_free_urb() has alredy freed out that transfer buffer since
we're the only code path holding a reference to this URB.

Remove two of such invalid manual free().

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 2928f70..d986fe8 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -787,7 +787,6 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
 		netdev_err(netdev, "Error transmitting URB\n");
 		usb_unanchor_urb(urb);
 		usb_free_urb(urb);
-		kfree(buf);
 		return err;
 	}
 
@@ -1615,8 +1614,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	struct urb *urb;
 	void *buf;
 	struct kvaser_msg *msg;
-	int i, err;
-	int ret = NETDEV_TX_OK;
+	int i, err, ret = NETDEV_TX_OK;
 	u8 *msg_tx_can_flags = NULL;		/* GCC */
 
 	if (can_dropped_invalid_skb(netdev, skb))
@@ -1634,7 +1632,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	if (!buf) {
 		stats->tx_dropped++;
 		dev_kfree_skb(skb);
-		goto nobufmem;
+		goto freeurb;
 	}
 
 	msg = buf;
@@ -1681,8 +1679,10 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	/* This should never happen; it implies a flow control bug */
 	if (!context) {
 		netdev_warn(netdev, "cannot find free context\n");
+
+		kfree(buf);
 		ret =  NETDEV_TX_BUSY;
-		goto releasebuf;
+		goto freeurb;
 	}
 
 	context->priv = priv;
@@ -1719,16 +1719,12 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		else
 			netdev_warn(netdev, "Failed tx_urb %d\n", err);
 
-		goto releasebuf;
+		goto freeurb;
 	}
 
-	usb_free_urb(urb);
-
-	return NETDEV_TX_OK;
+	ret = NETDEV_TX_OK;
 
-releasebuf:
-	kfree(buf);
-nobufmem:
+freeurb:
 	usb_free_urb(urb);
 	return ret;
 }
-- 
1.9.1

[PATCH 2/5] can: kvaser_usb: Read all messages in a bulk-in URB buffer

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

The Kvaser firmware can only read and write messages that are
not crossing the USB endpoint's wMaxPacketSize boundary. While
receiving commands from the CAN device, if the next command in
the same URB buffer crossed that max packet size boundary, the
firmware puts a zero-length placeholder command in its place
then moves the real command to the next boundary mark.

The driver did not recognize such behavior, leading to missing
a good number of rx events during a heavy rx load session.

Moreover, a tx URB context only gets freed upon receiving its
respective tx ACK event. Over time, the free tx URB contexts
pool gets depleted due to the missing ACK events. Consequently,
the netif transmission queue gets __permanently__ stopped; no
frames could be sent again except after restarting the CAN
newtwork interface.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index d986fe8..a316fa4 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -14,6 +14,7 @@
  * Copyright (C) 2015 Valeo S.A.
  */
 
+#include <linux/kernel.h>
 #include <linux/completion.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -584,8 +585,15 @@ static int kvaser_usb_wait_msg(const struct kvaser_usb *dev, u8 id,
 		while (pos <= actual_len - MSG_HEADER_LEN) {
 			tmp = buf + pos;
 
-			if (!tmp->len)
-				break;
+			/* Handle messages crossing the USB endpoint max packet
+			 * size boundary. Check kvaser_usb_read_bulk_callback()
+			 * for further details.
+			 */
+			if (tmp->len == 0) {
+				pos = round_up(pos,
+					       dev->bulk_in->wMaxPacketSize);
+				continue;
+			}
 
 			if (pos + tmp->len > actual_len) {
 				dev_err(dev->udev->dev.parent,
@@ -1316,8 +1324,19 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
 	while (pos <= urb->actual_length - MSG_HEADER_LEN) {
 		msg = urb->transfer_buffer + pos;
 
-		if (!msg->len)
-			break;
+		/* The Kvaser firmware can only read and write messages that
+		 * does not cross the USB's endpoint wMaxPacketSize boundary.
+		 * If a follow-up command crosses such boundary, firmware puts
+		 * a placeholder zero-length command in its place then aligns
+		 * the real command to the next max packet size.
+		 *
+		 * Handle such cases or we're going to miss a significant
+		 * number of events in case of a heavy rx load on the bus.
+		 */
+		if (msg->len == 0) {
+			pos = round_up(pos, dev->bulk_in->wMaxPacketSize);
+			continue;
+		}
 
 		if (pos + msg->len > urb->actual_length) {
 			dev_err(dev->udev->dev.parent, "Format error\n");
@@ -1325,7 +1344,6 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
 		}
 
 		kvaser_usb_handle_message(dev, msg);
-
 		pos += msg->len;
 	}
 
-- 
1.9.1

[PATCH 3/5] can: kvaser_usb: Utilize all possible tx URBs

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish-PLTpYtXPOU4AvxtiuMwx3w@public.gmane.org>

The driver currently limits the number of outstanding, not yet
ACKed, transfers to 16 URBs. Meanwhile, the Kvaser firmware
provides its actual max supported number of outstanding
transmissions in its reply to the CMD_GET_SOFTWARE_INFO message.

One example is the UsbCan-II HS/LS device which reports support
of up to 48 tx URBs instead of just 16, increasing the driver
throughput by two-fold and reducing the possibility of -ENOBUFs.

Dynamically set the max tx URBs value according to firmware
replies.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish-PLTpYtXPOU4AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/can/usb/kvaser_usb.c | 62 ++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index a316fa4..8f835a1 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -24,7 +24,6 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 
-#define MAX_TX_URBS			16
 #define MAX_RX_URBS			4
 #define START_TIMEOUT			1000 /* msecs */
 #define STOP_TIMEOUT			1000 /* msecs */
@@ -455,8 +454,13 @@ struct kvaser_usb {
 	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
 	struct usb_anchor rx_submitted;
 
+	/* @max_tx_urbs: Firmware-reported maximum number of possible
+	 * outstanding transmissions on this specific Kvaser hardware. The
+	 * value is also used as a sentinel for marking free URB contexts.
+	 */
 	u32 fw_version;
 	unsigned int nchannels;
+	unsigned int max_tx_urbs;
 	enum kvaser_usb_family family;
 
 	bool rxinitdone;
@@ -469,7 +473,7 @@ struct kvaser_usb_net_priv {
 
 	atomic_t active_tx_urbs;
 	struct usb_anchor tx_submitted;
-	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
+	struct kvaser_usb_tx_urb_context *tx_contexts;
 
 	struct completion start_comp, stop_comp;
 
@@ -655,9 +659,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
 	switch (dev->family) {
 	case KVASER_LEAF:
 		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
 		break;
 	case KVASER_USBCAN:
 		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
 		break;
 	}
 
@@ -712,7 +720,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats = &priv->netdev->stats;
 
-	context = &priv->tx_contexts[tid % MAX_TX_URBS];
+	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
 
 	/* Sometimes the state change doesn't come after a bus-off event */
 	if (priv->can.restart_ms &&
@@ -739,7 +747,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	stats->tx_bytes += context->dlc;
 	can_get_echo_skb(priv->netdev, context->echo_index);
 
-	context->echo_index = MAX_TX_URBS;
+	context->echo_index = dev->max_tx_urbs;
 	atomic_dec(&priv->active_tx_urbs);
 
 	netif_wake_queue(priv->netdev);
@@ -805,13 +813,14 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
 
 static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
 {
+	struct kvaser_usb *dev = priv->dev;
 	int i;
 
 	usb_kill_anchored_urbs(&priv->tx_submitted);
 	atomic_set(&priv->active_tx_urbs, 0);
 
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+	for (i = 0; i < dev->max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = dev->max_tx_urbs;
 }
 
 static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
@@ -1687,8 +1696,8 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	if (cf->can_id & CAN_RTR_FLAG)
 		*msg_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) {
+	for (i = 0; i < dev->max_tx_urbs; i++) {
+		if (priv->tx_contexts[i].echo_index ==  dev->max_tx_urbs) {
 			context = &priv->tx_contexts[i];
 			break;
 		}
@@ -1720,7 +1729,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 
 	atomic_inc(&priv->active_tx_urbs);
 
-	if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
+	if (atomic_read(&priv->active_tx_urbs) >= dev->max_tx_urbs)
 		netif_stop_queue(netdev);
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
@@ -1860,7 +1869,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	if (err)
 		return err;
 
-	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
 	if (!netdev) {
 		dev_err(&intf->dev, "Cannot alloc candev\n");
 		return -ENOMEM;
@@ -1868,19 +1877,26 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 
 	priv = netdev_priv(netdev);
 
+	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
+				    sizeof(*priv->tx_contexts), GFP_KERNEL);
+	if (!priv->tx_contexts) {
+		free_candev(netdev);
+		return -ENOMEM;
+	}
+
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
 
-	init_usb_anchor(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-
 	priv->dev = dev;
 	priv->netdev = netdev;
 	priv->channel = channel;
 
+	init_usb_anchor(&priv->tx_submitted);
+	atomic_set(&priv->active_tx_urbs, 0);
+
+	for (i = 0; i < dev->max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = dev->max_tx_urbs;
+
 	priv->can.state = CAN_STATE_STOPPED;
 	priv->can.clock.freq = CAN_USB_CLOCK;
 	priv->can.bittiming_const = &kvaser_usb_bittiming_const;
@@ -1909,7 +1925,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 		return err;
 	}
 
-	netdev_dbg(netdev, "device registered\n");
+	netdev_info(netdev, "device registered\n");
 
 	return 0;
 }
@@ -1990,6 +2006,13 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
+	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
+		((dev->fw_version >> 24) & 0xff),
+		((dev->fw_version >> 16) & 0xff),
+		(dev->fw_version & 0xffff));
+
+	dev_dbg(&intf->dev, "Max oustanding tx = %d URBs\n", dev->max_tx_urbs);
+
 	err = kvaser_usb_get_card_info(dev);
 	if (err) {
 		dev_err(&intf->dev,
@@ -1997,11 +2020,6 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
-	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
-		((dev->fw_version >> 24) & 0xff),
-		((dev->fw_version >> 16) & 0xff),
-		(dev->fw_version & 0xffff));

[PATCH 3/5] can: kvaser_usb: Utilize all possible tx URBs

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

The driver currently limits the number of outstanding, not yet
ACKed, transfers to 16 URBs. Meanwhile, the Kvaser firmware
provides its actual max supported number of outstanding
transmissions in its reply to the CMD_GET_SOFTWARE_INFO message.

One example is the UsbCan-II HS/LS device which reports support
of up to 48 tx URBs instead of just 16, increasing the driver
throughput by two-fold and reducing the possibility of -ENOBUFs.

Dynamically set the max tx URBs value according to firmware
replies.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 62 ++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index a316fa4..8f835a1 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -24,7 +24,6 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 
-#define MAX_TX_URBS			16
 #define MAX_RX_URBS			4
 #define START_TIMEOUT			1000 /* msecs */
 #define STOP_TIMEOUT			1000 /* msecs */
@@ -455,8 +454,13 @@ struct kvaser_usb {
 	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
 	struct usb_anchor rx_submitted;
 
+	/* @max_tx_urbs: Firmware-reported maximum number of possible
+	 * outstanding transmissions on this specific Kvaser hardware. The
+	 * value is also used as a sentinel for marking free URB contexts.
+	 */
 	u32 fw_version;
 	unsigned int nchannels;
+	unsigned int max_tx_urbs;
 	enum kvaser_usb_family family;
 
 	bool rxinitdone;
@@ -469,7 +473,7 @@ struct kvaser_usb_net_priv {
 
 	atomic_t active_tx_urbs;
 	struct usb_anchor tx_submitted;
-	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
+	struct kvaser_usb_tx_urb_context *tx_contexts;
 
 	struct completion start_comp, stop_comp;
 
@@ -655,9 +659,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
 	switch (dev->family) {
 	case KVASER_LEAF:
 		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
 		break;
 	case KVASER_USBCAN:
 		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
 		break;
 	}
 
@@ -712,7 +720,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats = &priv->netdev->stats;
 
-	context = &priv->tx_contexts[tid % MAX_TX_URBS];
+	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
 
 	/* Sometimes the state change doesn't come after a bus-off event */
 	if (priv->can.restart_ms &&
@@ -739,7 +747,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	stats->tx_bytes += context->dlc;
 	can_get_echo_skb(priv->netdev, context->echo_index);
 
-	context->echo_index = MAX_TX_URBS;
+	context->echo_index = dev->max_tx_urbs;
 	atomic_dec(&priv->active_tx_urbs);
 
 	netif_wake_queue(priv->netdev);
@@ -805,13 +813,14 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
 
 static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
 {
+	struct kvaser_usb *dev = priv->dev;
 	int i;
 
 	usb_kill_anchored_urbs(&priv->tx_submitted);
 	atomic_set(&priv->active_tx_urbs, 0);
 
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+	for (i = 0; i < dev->max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = dev->max_tx_urbs;
 }
 
 static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
@@ -1687,8 +1696,8 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	if (cf->can_id & CAN_RTR_FLAG)
 		*msg_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) {
+	for (i = 0; i < dev->max_tx_urbs; i++) {
+		if (priv->tx_contexts[i].echo_index ==  dev->max_tx_urbs) {
 			context = &priv->tx_contexts[i];
 			break;
 		}
@@ -1720,7 +1729,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 
 	atomic_inc(&priv->active_tx_urbs);
 
-	if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
+	if (atomic_read(&priv->active_tx_urbs) >= dev->max_tx_urbs)
 		netif_stop_queue(netdev);
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
@@ -1860,7 +1869,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	if (err)
 		return err;
 
-	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
 	if (!netdev) {
 		dev_err(&intf->dev, "Cannot alloc candev\n");
 		return -ENOMEM;
@@ -1868,19 +1877,26 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 
 	priv = netdev_priv(netdev);
 
+	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
+				    sizeof(*priv->tx_contexts), GFP_KERNEL);
+	if (!priv->tx_contexts) {
+		free_candev(netdev);
+		return -ENOMEM;
+	}
+
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
 
-	init_usb_anchor(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-
 	priv->dev = dev;
 	priv->netdev = netdev;
 	priv->channel = channel;
 
+	init_usb_anchor(&priv->tx_submitted);
+	atomic_set(&priv->active_tx_urbs, 0);
+
+	for (i = 0; i < dev->max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = dev->max_tx_urbs;
+
 	priv->can.state = CAN_STATE_STOPPED;
 	priv->can.clock.freq = CAN_USB_CLOCK;
 	priv->can.bittiming_const = &kvaser_usb_bittiming_const;
@@ -1909,7 +1925,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 		return err;
 	}
 
-	netdev_dbg(netdev, "device registered\n");
+	netdev_info(netdev, "device registered\n");
 
 	return 0;
 }
@@ -1990,6 +2006,13 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
+	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
+		((dev->fw_version >> 24) & 0xff),
+		((dev->fw_version >> 16) & 0xff),
+		(dev->fw_version & 0xffff));
+
+	dev_dbg(&intf->dev, "Max oustanding tx = %d URBs\n", dev->max_tx_urbs);
+
 	err = kvaser_usb_get_card_info(dev);
 	if (err) {
 		dev_err(&intf->dev,
@@ -1997,11 +2020,6 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
-	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
-		((dev->fw_version >> 24) & 0xff),
-		((dev->fw_version >> 16) & 0xff),
-		(dev->fw_version & 0xffff));
-
 	for (i = 0; i < dev->nchannels; i++) {
 		err = kvaser_usb_init_one(intf, id, i);
 		if (err) {
-- 
1.9.1

[PATCH 4/5] can: kvaser_usb: Use can-dev unregistration mechanism

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Use can-dev's unregister_candev() instead of directly calling
networking unregister_netdev(). While both are functionally
equivalent, unregister_candev() might do extra stuff in the
future than just calling networking layer unregistration code.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 8f835a1..13bae86 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1844,7 +1844,7 @@ static void kvaser_usb_remove_interfaces(struct kvaser_usb *dev)
 		if (!dev->nets[i])
 			continue;
 
-		unregister_netdev(dev->nets[i]->netdev);
+		unregister_candev(dev->nets[i]->netdev);
 	}
 
 	kvaser_usb_unlink_all_urbs(dev);
-- 
1.9.1

[PATCH 5/5] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

A number of tx queue wake-up events went missing due to the
outlined scenario below. Start state is a pool of 16 tx URBs,
active tx_urbs count = 15, with the netdev tx queue open.

start_xmit()                             tx_acknowledge()
............                             ................
atomic_inc(&tx_urbs);
if (atomic_read(&tx_urbs) >= 16) {
                        URB completion IRQ!
                        -->
                                         atomic_dec(&tx_urbs);
                                         netif_wake_queue();
                                         return;
                        <--
                        end of IRQ!
    netif_stop_queue();
}

At the end, the correct state expected is a 15 tx_urbs count
value with the tx queue state _open_. Due to the race, we get
the same tx_urbs value but with the tx queue state _stopped_.
The wake-up event is completely lost.

Thus avoid hand-rolled concurrency mechanisms and use a proper
lock for contexts protection.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 82 +++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 13bae86..807ab0c 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -14,6 +14,7 @@
  * Copyright (C) 2015 Valeo S.A.
  */
 
+#include <linux/spinlock.h>
 #include <linux/kernel.h>
 #include <linux/completion.h>
 #include <linux/module.h>
@@ -471,10 +472,12 @@ struct kvaser_usb {
 struct kvaser_usb_net_priv {
 	struct can_priv can;
 
-	atomic_t active_tx_urbs;
-	struct usb_anchor tx_submitted;
+	spinlock_t tx_contexts_lock;
+	int active_tx_contexts;
 	struct kvaser_usb_tx_urb_context *tx_contexts;
 
+	struct usb_anchor tx_submitted;
+
 	struct completion start_comp, stop_comp;
 
 	struct kvaser_usb *dev;
@@ -702,6 +705,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	struct kvaser_usb_net_priv *priv;
 	struct sk_buff *skb;
 	struct can_frame *cf;
+	unsigned long flags;
 	u8 channel, tid;
 
 	channel = msg->u.tx_acknowledge_header.channel;
@@ -745,12 +749,15 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats->tx_packets++;
 	stats->tx_bytes += context->dlc;
-	can_get_echo_skb(priv->netdev, context->echo_index);
 
-	context->echo_index = dev->max_tx_urbs;
-	atomic_dec(&priv->active_tx_urbs);
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
+	can_get_echo_skb(priv->netdev, context->echo_index);
+	context->echo_index = dev->max_tx_urbs;
+	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
+
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 }
 
 static void kvaser_usb_simple_msg_callback(struct urb *urb)
@@ -811,18 +818,6 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
 	return 0;
 }
 
-static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
-{
-	struct kvaser_usb *dev = priv->dev;
-	int i;
-
-	usb_kill_anchored_urbs(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < dev->max_tx_urbs; i++)
-		priv->tx_contexts[i].echo_index = dev->max_tx_urbs;
-}
-
 static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
 						 const struct kvaser_usb_error_summary *es,
 						 struct can_frame *cf)
@@ -1524,6 +1519,26 @@ error:
 	return err;
 }
 
+static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
+{
+	int i, max_tx_urbs;
+
+	max_tx_urbs = priv->dev->max_tx_urbs;
+
+	priv->active_tx_contexts = 0;
+	for (i = 0; i < max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = max_tx_urbs;
+}
+
+/* This method might sleep. Do not call it in the atomic context
+ * of URB completions.
+ */
+static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
+{
+	usb_kill_anchored_urbs(&priv->tx_submitted);
+	kvaser_usb_reset_tx_urb_contexts(priv);
+}
+
 static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
 {
 	int i;
@@ -1643,6 +1658,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	struct kvaser_msg *msg;
 	int i, err, ret = NETDEV_TX_OK;
 	u8 *msg_tx_can_flags = NULL;		/* GCC */
+	unsigned long flags;
 
 	if (can_dropped_invalid_skb(netdev, skb))
 		return NETDEV_TX_OK;
@@ -1696,12 +1712,21 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	if (cf->can_id & CAN_RTR_FLAG)
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 	for (i = 0; i < dev->max_tx_urbs; i++) {
 		if (priv->tx_contexts[i].echo_index ==  dev->max_tx_urbs) {
 			context = &priv->tx_contexts[i];
+
+			context->echo_index = i;
+			can_put_echo_skb(skb, netdev, context->echo_index);
+			++priv->active_tx_contexts;
+			if (priv->active_tx_contexts >= dev->max_tx_urbs)
+				netif_stop_queue(netdev);
+
 			break;
 		}
 	}
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
 	/* This should never happen; it implies a flow control bug */
 	if (!context) {
@@ -1713,7 +1738,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	}
 
 	context->priv = priv;
-	context->echo_index = i;
 	context->dlc = cf->can_dlc;
 
 	msg->u.tx_can.tid = context->echo_index;
@@ -1725,18 +1749,17 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 			  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) >= dev->max_tx_urbs)
-		netif_stop_queue(netdev);
-
 	err = usb_submit_urb(urb, GFP_ATOMIC);
 	if (unlikely(err)) {
+		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
+
 		can_free_echo_skb(netdev, context->echo_index);
+		context->echo_index = dev->max_tx_urbs;
+		--priv->active_tx_contexts;
+		netif_wake_queue(netdev);
+
+		spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
-		atomic_dec(&priv->active_tx_urbs);
 		usb_unanchor_urb(urb);
 
 		stats->tx_dropped++;
@@ -1863,7 +1886,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	struct kvaser_usb *dev = usb_get_intfdata(intf);
 	struct net_device *netdev;
 	struct kvaser_usb_net_priv *priv;
-	int i, err;
+	int err;
 
 	err = kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, channel);
 	if (err)
@@ -1892,10 +1915,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	priv->channel = channel;
 
 	init_usb_anchor(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < dev->max_tx_urbs; i++)
-		priv->tx_contexts[i].echo_index = dev->max_tx_urbs;
+	kvaser_usb_reset_tx_urb_contexts(priv);
 
 	priv->can.state = CAN_STATE_STOPPED;
 	priv->can.clock.freq = CAN_USB_CLOCK;
-- 
1.9.1

Re: [PATCH 1/5] can: kvaser_usb: Avoid double free on URB submission failures

[Marc Kleine-Budde]

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

On 02/26/2015 04:20 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> Upon a URB submission failure, the driver calls usb_free_urb()
> but then manually frees the URB buffer by itself.  Meanwhile
> usb_free_urb() has alredy freed out that transfer buffer since
> we're the only code path holding a reference to this URB.
> 
> Remove two of such invalid manual free().
> 
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Applied 1+2 and added stable on Cc. Can you please shuffle the remaining
patches, so that patch 5 comes first, then 4 and 3 as the last patch. As
5 is a bugfix it should go into stable, while 3 isn't.

You can base your series on the can/testing branch.

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: 819 bytes --]

Re: [PATCH 1/5] can: kvaser_usb: Avoid double free on URB submission failures

[Ahmed S. Darwish]

Hi Marc,

(Sorry for the late reply as I was out of town!)

On Wed, Mar 04, 2015 at 10:15:45AM +0100, Marc Kleine-Budde wrote:
> On 02/26/2015 04:20 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > Upon a URB submission failure, the driver calls usb_free_urb()
> > but then manually frees the URB buffer by itself.  Meanwhile
> > usb_free_urb() has alredy freed out that transfer buffer since
> > we're the only code path holding a reference to this URB.
> > 
> > Remove two of such invalid manual free().
> > 
> > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> Applied 1+2 and added stable on Cc. Can you please shuffle the remaining
> patches, so that patch 5 comes first, then 4 and 3 as the last patch. As
> 5 is a bugfix it should go into stable, while 3 isn't.
>
> You can base your series on the can/testing branch.
> 

Did not care much about the bugfixes order this time as the patches
themselves will not apply cleanly (or at all) to -stable due to the
addition of UsbCAN-II code, which all -stable kernels do not have.
Thus I guess I'll need to submit a different patch series for -stable
with patches 1, 2, and 5 -- rebased.

Nonetheless, you're correct that having the bugfixes (1,2,5), then the
optimization (4), then the janitorial fix (3) is the logical order for
history & bisection sake. So.. I'll re-order the patches, individually
test with the new order, and re-submit over can/testing.

Thanks,
Darwish

Re: [PATCH 1/5] can: kvaser_usb: Avoid double free on URB submission failures

[Marc Kleine-Budde]

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

On 03/09/2015 01:32 PM, Ahmed S. Darwish wrote:
> (Sorry for the late reply as I was out of town!)

np :)

> On Wed, Mar 04, 2015 at 10:15:45AM +0100, Marc Kleine-Budde wrote:
>> On 02/26/2015 04:20 PM, Ahmed S. Darwish wrote:
>>> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>>>
>>> Upon a URB submission failure, the driver calls usb_free_urb()
>>> but then manually frees the URB buffer by itself.  Meanwhile
>>> usb_free_urb() has alredy freed out that transfer buffer since
>>> we're the only code path holding a reference to this URB.
>>>
>>> Remove two of such invalid manual free().
>>>
>>> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>>
>> Applied 1+2 and added stable on Cc. Can you please shuffle the remaining
>> patches, so that patch 5 comes first, then 4 and 3 as the last patch. As
>> 5 is a bugfix it should go into stable, while 3 isn't.
>>
>> You can base your series on the can/testing branch.

> Did not care much about the bugfixes order this time as the patches
> themselves will not apply cleanly (or at all) to -stable due to the
> addition of UsbCAN-II code, which all -stable kernels do not have.
> Thus I guess I'll need to submit a different patch series for -stable
> with patches 1, 2, and 5 -- rebased.

Submitting patches ported to -stable would be a second step. You don't
have to, but I'd appreciate it.

> Nonetheless, you're correct that having the bugfixes (1,2,5), then the
> optimization (4), then the janitorial fix (3) is the logical order for
> history & bisection sake. So.. I'll re-order the patches, individually
> test with the new order, and re-submit over can/testing.

Ack, or bugfix, janitorial then optimization. Please use
linux-can-fixes-for-4.0-20150309 (which include 1 and 2) as your base.

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: 819 bytes --]

[PATCH v2 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

A number of tx queue wake-up events went missing due to the
outlined scenario below. Start state is a pool of 16 tx URBs,
active tx_urbs count = 15, with the netdev tx queue open.

start_xmit()                             tx_acknowledge()
............                             ................
atomic_inc(&tx_urbs);
if (atomic_read(&tx_urbs) >= 16) {
                        URB completion IRQ!
                        -->
                                         atomic_dec(&tx_urbs);
                                         netif_wake_queue();
                                         return;
                        <--
                        end of IRQ!
    netif_stop_queue();
}

At the end, the correct state expected is a 15 tx_urbs count
value with the tx queue state _open_. Due to the race, we get
the same tx_urbs value but with the tx queue state _stopped_.
The wake-up event is completely lost.

Thus avoid hand-rolled concurrency mechanisms and use a proper
lock for contexts protection.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 82 ++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 32 deletions(-)

This new series is just like v1, with the exception of moving
bugfix #5 to the top as suggested earlier.  The patches are
based over linux-can-fixes-for-4.0-20150309. Thanks!

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index a316fa4..0aea8e2 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -14,6 +14,7 @@
  * Copyright (C) 2015 Valeo S.A.
  */
 
+#include <linux/spinlock.h>
 #include <linux/kernel.h>
 #include <linux/completion.h>
 #include <linux/module.h>
@@ -467,10 +468,11 @@ struct kvaser_usb {
 struct kvaser_usb_net_priv {
 	struct can_priv can;
 
-	atomic_t active_tx_urbs;
-	struct usb_anchor tx_submitted;
+	spinlock_t tx_contexts_lock;
+	int active_tx_contexts;
 	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
 
+	struct usb_anchor tx_submitted;
 	struct completion start_comp, stop_comp;
 
 	struct kvaser_usb *dev;
@@ -694,6 +696,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	struct kvaser_usb_net_priv *priv;
 	struct sk_buff *skb;
 	struct can_frame *cf;
+	unsigned long flags;
 	u8 channel, tid;
 
 	channel = msg->u.tx_acknowledge_header.channel;
@@ -737,12 +740,15 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats->tx_packets++;
 	stats->tx_bytes += context->dlc;
-	can_get_echo_skb(priv->netdev, context->echo_index);
 
-	context->echo_index = MAX_TX_URBS;
-	atomic_dec(&priv->active_tx_urbs);
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
+	can_get_echo_skb(priv->netdev, context->echo_index);
+	context->echo_index = MAX_TX_URBS;
+	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
+
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 }
 
 static void kvaser_usb_simple_msg_callback(struct urb *urb)
@@ -803,17 +809,6 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
 	return 0;
 }
 
-static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
-{
-	int i;
-
-	usb_kill_anchored_urbs(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-}
-
 static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
 						 const struct kvaser_usb_error_summary *es,
 						 struct can_frame *cf)
@@ -1515,6 +1510,24 @@ error:
 	return err;
 }
 
+static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
+{
+	int i;
+
+	priv->active_tx_contexts = 0;
+	for (i = 0; i < MAX_TX_URBS; i++)
+		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+}
+
+/* This method might sleep. Do not call it in the atomic context
+ * of URB completions.
+ */
+static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
+{
+	usb_kill_anchored_urbs(&priv->tx_submitted);
+	kvaser_usb_reset_tx_urb_contexts(priv);
+}
+
 static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
 {
 	int i;
@@ -1634,6 +1647,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	struct kvaser_msg *msg;
 	int i, err, ret = NETDEV_TX_OK;
 	u8 *msg_tx_can_flags = NULL;		/* GCC */
+	unsigned long flags;
 
 	if (can_dropped_invalid_skb(netdev, skb))
 		return NETDEV_TX_OK;
@@ -1687,12 +1701,21 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	if (cf->can_id & CAN_RTR_FLAG)
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 	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];
+
+			context->echo_index = i;
+			can_put_echo_skb(skb, netdev, context->echo_index);
+			++priv->active_tx_contexts;
+			if (priv->active_tx_contexts >= MAX_TX_URBS)
+				netif_stop_queue(netdev);
+
 			break;
 		}
 	}
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
 	/* This should never happen; it implies a flow control bug */
 	if (!context) {
@@ -1704,7 +1727,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	}
 
 	context->priv = priv;
-	context->echo_index = i;
 	context->dlc = cf->can_dlc;
 
 	msg->u.tx_can.tid = context->echo_index;
@@ -1716,18 +1738,17 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 			  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)) {
+		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
+
 		can_free_echo_skb(netdev, context->echo_index);
+		context->echo_index = MAX_TX_URBS;
+		--priv->active_tx_contexts;
+		netif_wake_queue(netdev);
+
+		spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
-		atomic_dec(&priv->active_tx_urbs);
 		usb_unanchor_urb(urb);
 
 		stats->tx_dropped++;
@@ -1854,7 +1875,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	struct kvaser_usb *dev = usb_get_intfdata(intf);
 	struct net_device *netdev;
 	struct kvaser_usb_net_priv *priv;
-	int i, err;
+	int err;
 
 	err = kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, channel);
 	if (err)
@@ -1868,19 +1889,16 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 
 	priv = netdev_priv(netdev);
 
+	init_usb_anchor(&priv->tx_submitted);
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
 
-	init_usb_anchor(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-
 	priv->dev = dev;
 	priv->netdev = netdev;
 	priv->channel = channel;
 
+	kvaser_usb_reset_tx_urb_contexts(priv);
+
 	priv->can.state = CAN_STATE_STOPPED;
 	priv->can.clock.freq = CAN_USB_CLOCK;
 	priv->can.bittiming_const = &kvaser_usb_bittiming_const;
-- 
1.9.1

[PATCH v2 2/3] can: kvaser_usb: Utilize all possible tx URBs

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

The driver currently limits the number of outstanding, not yet
ACKed, transfers to 16 URBs. Meanwhile, the Kvaser firmware
provides its actual max supported number of outstanding
transmissions in its reply to the CMD_GET_SOFTWARE_INFO message.

One example is the UsbCan-II HS/LS device which reports support
of up to 48 tx URBs instead of just 16, increasing the driver
throughput by two-fold and reducing the possibility of -ENOBUFs.

Dynamically set the max tx URBs value according to firmware
replies.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 55 +++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 18 deletions(-)

This is a bugfix if the kvaser hardware in question has less
than 16 tx URBs, and a speed optimization if it has more ;-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 0aea8e2..0742d53 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -25,7 +25,6 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 
-#define MAX_TX_URBS			16
 #define MAX_RX_URBS			4
 #define START_TIMEOUT			1000 /* msecs */
 #define STOP_TIMEOUT			1000 /* msecs */
@@ -456,8 +455,13 @@ struct kvaser_usb {
 	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
 	struct usb_anchor rx_submitted;
 
+	/* @max_tx_urbs: Firmware-reported maximum number of possible
+	 * outstanding transmissions on this specific Kvaser hardware. The
+	 * value is also used as a sentinel for marking free URB contexts.
+	 */
 	u32 fw_version;
 	unsigned int nchannels;
+	unsigned int max_tx_urbs;
 	enum kvaser_usb_family family;
 
 	bool rxinitdone;
@@ -470,7 +474,7 @@ struct kvaser_usb_net_priv {
 
 	spinlock_t tx_contexts_lock;
 	int active_tx_contexts;
-	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
+	struct kvaser_usb_tx_urb_context *tx_contexts;
 
 	struct usb_anchor tx_submitted;
 	struct completion start_comp, stop_comp;
@@ -657,9 +661,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
 	switch (dev->family) {
 	case KVASER_LEAF:
 		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
 		break;
 	case KVASER_USBCAN:
 		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
 		break;
 	}
 
@@ -715,7 +723,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats = &priv->netdev->stats;
 
-	context = &priv->tx_contexts[tid % MAX_TX_URBS];
+	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
 
 	/* Sometimes the state change doesn't come after a bus-off event */
 	if (priv->can.restart_ms &&
@@ -744,7 +752,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 	can_get_echo_skb(priv->netdev, context->echo_index);
-	context->echo_index = MAX_TX_URBS;
+	context->echo_index = dev->max_tx_urbs;
 	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
 
@@ -1512,11 +1520,13 @@ error:
 
 static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
 {
-	int i;
+	int i, max_tx_urbs;
+
+	max_tx_urbs = priv->dev->max_tx_urbs;
 
 	priv->active_tx_contexts = 0;
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+	for (i = 0; i < max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = max_tx_urbs;
 }
 
 /* This method might sleep. Do not call it in the atomic context
@@ -1702,14 +1712,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
-		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
+	for (i = 0; i < dev->max_tx_urbs; i++) {
+		if (priv->tx_contexts[i].echo_index == dev->max_tx_urbs) {
 			context = &priv->tx_contexts[i];
 
 			context->echo_index = i;
 			can_put_echo_skb(skb, netdev, context->echo_index);
 			++priv->active_tx_contexts;
-			if (priv->active_tx_contexts >= MAX_TX_URBS)
+			if (priv->active_tx_contexts >= dev->max_tx_urbs)
 				netif_stop_queue(netdev);
 
 			break;
@@ -1743,7 +1753,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 		can_free_echo_skb(netdev, context->echo_index);
-		context->echo_index = MAX_TX_URBS;
+		context->echo_index = dev->max_tx_urbs;
 		--priv->active_tx_contexts;
 		netif_wake_queue(netdev);
 
@@ -1881,7 +1891,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	if (err)
 		return err;
 
-	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
 	if (!netdev) {
 		dev_err(&intf->dev, "Cannot alloc candev\n");
 		return -ENOMEM;
@@ -1889,6 +1899,13 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 
 	priv = netdev_priv(netdev);
 
+	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
+				    sizeof(*priv->tx_contexts), GFP_KERNEL);
+	if (!priv->tx_contexts) {
+		free_candev(netdev);
+		return -ENOMEM;
+	}
+
 	init_usb_anchor(&priv->tx_submitted);
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
@@ -1927,7 +1944,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 		return err;
 	}
 
-	netdev_dbg(netdev, "device registered\n");
+	netdev_info(netdev, "device registered\n");
 
 	return 0;
 }
@@ -2008,6 +2025,13 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
+	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
+		((dev->fw_version >> 24) & 0xff),
+		((dev->fw_version >> 16) & 0xff),
+		(dev->fw_version & 0xffff));
+
+	dev_dbg(&intf->dev, "Max oustanding tx = %d URBs\n", dev->max_tx_urbs);
+
 	err = kvaser_usb_get_card_info(dev);
 	if (err) {
 		dev_err(&intf->dev,
@@ -2015,11 +2039,6 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
-	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
-		((dev->fw_version >> 24) & 0xff),
-		((dev->fw_version >> 16) & 0xff),
-		(dev->fw_version & 0xffff));
-
 	for (i = 0; i < dev->nchannels; i++) {
 		err = kvaser_usb_init_one(intf, id, i);
 		if (err) {
-- 
1.9.1

[PATCH v2 3/3] can: kvaser_usb: Use can-dev unregistration mechanism

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Use can-dev's unregister_candev() instead of directly calling
networking unregister_netdev(). While both are functionally
equivalent, unregister_candev() might do extra stuff in the
future than just calling networking layer unregistration code.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 0742d53..f9c14e8 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1866,7 +1866,7 @@ static void kvaser_usb_remove_interfaces(struct kvaser_usb *dev)
 		if (!dev->nets[i])
 			continue;
 
-		unregister_netdev(dev->nets[i]->netdev);
+		unregister_candev(dev->nets[i]->netdev);
 	}
 
 	kvaser_usb_unlink_all_urbs(dev);
-- 
1.9.1

Re: [PATCH v2 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Marc Kleine-Budde]

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

On 03/11/2015 04:23 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> A number of tx queue wake-up events went missing due to the
> outlined scenario below. Start state is a pool of 16 tx URBs,
> active tx_urbs count = 15, with the netdev tx queue open.
> 
> start_xmit()                             tx_acknowledge()
> ............                             ................
> atomic_inc(&tx_urbs);
> if (atomic_read(&tx_urbs) >= 16) {
>                         URB completion IRQ!
>                         -->
>                                          atomic_dec(&tx_urbs);
>                                          netif_wake_queue();
>                                          return;
>                         <--
>                         end of IRQ!
>     netif_stop_queue();
> }
> 
> At the end, the correct state expected is a 15 tx_urbs count
> value with the tx queue state _open_. Due to the race, we get
> the same tx_urbs value but with the tx queue state _stopped_.
> The wake-up event is completely lost.
> 
> Thus avoid hand-rolled concurrency mechanisms and use a proper
> lock for contexts protection.

I'm missing a spin_lock_init(), right? Please compile and test your code
with everything switch on in Kernel hacking -> Lock Debugging.

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: 819 bytes --]

Re: [PATCH v2 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

On Wed, Mar 11, 2015 at 04:36:52PM +0100, Marc Kleine-Budde wrote:
> On 03/11/2015 04:23 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > A number of tx queue wake-up events went missing due to the
> > outlined scenario below. Start state is a pool of 16 tx URBs,
> > active tx_urbs count = 15, with the netdev tx queue open.
> > 
> > start_xmit()                             tx_acknowledge()
> > ............                             ................
> > atomic_inc(&tx_urbs);
> > if (atomic_read(&tx_urbs) >= 16) {
> >                         URB completion IRQ!
> >                         -->
> >                                          atomic_dec(&tx_urbs);
> >                                          netif_wake_queue();
> >                                          return;
> >                         <--
> >                         end of IRQ!
> >     netif_stop_queue();
> > }
> > 
> > At the end, the correct state expected is a 15 tx_urbs count
> > value with the tx queue state _open_. Due to the race, we get
> > the same tx_urbs value but with the tx queue state _stopped_.
> > The wake-up event is completely lost.
> > 
> > Thus avoid hand-rolled concurrency mechanisms and use a proper
> > lock for contexts protection.
> 
> I'm missing a spin_lock_init(), right? Please compile and test your code
> with everything switch on in Kernel hacking -> Lock Debugging.
> 

Ouch... that passed through it seems since __ARCH_SPIN_LOCK_UNLOCKED
is always zero on x86. Recompiling the kernel and re-iterating another
patch series.

Thanks,
Darwish

[PATCH v3 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

A number of tx queue wake-up events went missing due to the
outlined scenario below. Start state is a pool of 16 tx URBs,
active tx_urbs count = 15, with the netdev tx queue open.

start_xmit()                             tx_acknowledge()
............                             ................
atomic_inc(&tx_urbs);
if (atomic_read(&tx_urbs) >= 16) {
                        URB completion IRQ!
                        -->
                                         atomic_dec(&tx_urbs);
                                         netif_wake_queue();
                                         return;
                        <--
                        end of IRQ!
    netif_stop_queue();
}

At the end, the correct state expected is a 15 tx_urbs count
value with the tx queue state _open_. Due to the race, we get
the same tx_urbs value but with the tx queue state _stopped_.
The wake-up event is completely lost.

Thus avoid hand-rolled concurrency mechanisms and use a proper
lock for contexts protection.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 83 ++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 32 deletions(-)

changelog-v3: Added missing spin_lock_init(). With all kernel
lock debugging options set, I've been running my test suite
for an hour now without apparent problems in dmesg so far.

changelog-v2: Put bugfix patch at the start of the series.

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index a316fa4..e97a08c 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -14,6 +14,7 @@
  * Copyright (C) 2015 Valeo S.A.
  */
 
+#include <linux/spinlock.h>
 #include <linux/kernel.h>
 #include <linux/completion.h>
 #include <linux/module.h>
@@ -467,10 +468,11 @@ struct kvaser_usb {
 struct kvaser_usb_net_priv {
 	struct can_priv can;
 
-	atomic_t active_tx_urbs;
-	struct usb_anchor tx_submitted;
+	spinlock_t tx_contexts_lock;
+	int active_tx_contexts;
 	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
 
+	struct usb_anchor tx_submitted;
 	struct completion start_comp, stop_comp;
 
 	struct kvaser_usb *dev;
@@ -694,6 +696,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	struct kvaser_usb_net_priv *priv;
 	struct sk_buff *skb;
 	struct can_frame *cf;
+	unsigned long flags;
 	u8 channel, tid;
 
 	channel = msg->u.tx_acknowledge_header.channel;
@@ -737,12 +740,15 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats->tx_packets++;
 	stats->tx_bytes += context->dlc;
-	can_get_echo_skb(priv->netdev, context->echo_index);
 
-	context->echo_index = MAX_TX_URBS;
-	atomic_dec(&priv->active_tx_urbs);
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
+	can_get_echo_skb(priv->netdev, context->echo_index);
+	context->echo_index = MAX_TX_URBS;
+	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
+
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 }
 
 static void kvaser_usb_simple_msg_callback(struct urb *urb)
@@ -803,17 +809,6 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
 	return 0;
 }
 
-static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
-{
-	int i;
-
-	usb_kill_anchored_urbs(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-}
-
 static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
 						 const struct kvaser_usb_error_summary *es,
 						 struct can_frame *cf)
@@ -1515,6 +1510,24 @@ error:
 	return err;
 }
 
+static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
+{
+	int i;
+
+	priv->active_tx_contexts = 0;
+	for (i = 0; i < MAX_TX_URBS; i++)
+		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+}
+
+/* This method might sleep. Do not call it in the atomic context
+ * of URB completions.
+ */
+static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
+{
+	usb_kill_anchored_urbs(&priv->tx_submitted);
+	kvaser_usb_reset_tx_urb_contexts(priv);
+}
+
 static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
 {
 	int i;
@@ -1634,6 +1647,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	struct kvaser_msg *msg;
 	int i, err, ret = NETDEV_TX_OK;
 	u8 *msg_tx_can_flags = NULL;		/* GCC */
+	unsigned long flags;
 
 	if (can_dropped_invalid_skb(netdev, skb))
 		return NETDEV_TX_OK;
@@ -1687,12 +1701,21 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	if (cf->can_id & CAN_RTR_FLAG)
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 	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];
+
+			context->echo_index = i;
+			can_put_echo_skb(skb, netdev, context->echo_index);
+			++priv->active_tx_contexts;
+			if (priv->active_tx_contexts >= MAX_TX_URBS)
+				netif_stop_queue(netdev);
+
 			break;
 		}
 	}
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
 	/* This should never happen; it implies a flow control bug */
 	if (!context) {
@@ -1704,7 +1727,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	}
 
 	context->priv = priv;
-	context->echo_index = i;
 	context->dlc = cf->can_dlc;
 
 	msg->u.tx_can.tid = context->echo_index;
@@ -1716,18 +1738,17 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 			  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)) {
+		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
+
 		can_free_echo_skb(netdev, context->echo_index);
+		context->echo_index = MAX_TX_URBS;
+		--priv->active_tx_contexts;
+		netif_wake_queue(netdev);
+
+		spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
-		atomic_dec(&priv->active_tx_urbs);
 		usb_unanchor_urb(urb);
 
 		stats->tx_dropped++;
@@ -1854,7 +1875,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	struct kvaser_usb *dev = usb_get_intfdata(intf);
 	struct net_device *netdev;
 	struct kvaser_usb_net_priv *priv;
-	int i, err;
+	int err;
 
 	err = kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, channel);
 	if (err)
@@ -1868,19 +1889,17 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 
 	priv = netdev_priv(netdev);
 
+	init_usb_anchor(&priv->tx_submitted);
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
 
-	init_usb_anchor(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-
 	priv->dev = dev;
 	priv->netdev = netdev;
 	priv->channel = channel;
 
+	spin_lock_init(&priv->tx_contexts_lock);
+	kvaser_usb_reset_tx_urb_contexts(priv);
+
 	priv->can.state = CAN_STATE_STOPPED;
 	priv->can.clock.freq = CAN_USB_CLOCK;
 	priv->can.bittiming_const = &kvaser_usb_bittiming_const;
-- 
1.9.1

[PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

The driver currently limits the number of outstanding, not yet
ACKed, transfers to 16 URBs. Meanwhile, the Kvaser firmware
provides its actual max supported number of outstanding
transmissions in its reply to the CMD_GET_SOFTWARE_INFO message.

One example is the UsbCan-II HS/LS device which reports support
of up to 48 tx URBs instead of just 16, increasing the driver
throughput by two-fold and reducing the possibility of -ENOBUFs.

Dynamically set the max tx URBs value according to firmware
replies.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 55 +++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 18 deletions(-)

changelog-v3: No changes

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index e97a08c..30b4d47 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -25,7 +25,6 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 
-#define MAX_TX_URBS			16
 #define MAX_RX_URBS			4
 #define START_TIMEOUT			1000 /* msecs */
 #define STOP_TIMEOUT			1000 /* msecs */
@@ -456,8 +455,13 @@ struct kvaser_usb {
 	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
 	struct usb_anchor rx_submitted;
 
+	/* @max_tx_urbs: Firmware-reported maximum number of possible
+	 * outstanding transmissions on this specific Kvaser hardware. The
+	 * value is also used as a sentinel for marking free URB contexts.
+	 */
 	u32 fw_version;
 	unsigned int nchannels;
+	unsigned int max_tx_urbs;
 	enum kvaser_usb_family family;
 
 	bool rxinitdone;
@@ -470,7 +474,7 @@ struct kvaser_usb_net_priv {
 
 	spinlock_t tx_contexts_lock;
 	int active_tx_contexts;
-	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
+	struct kvaser_usb_tx_urb_context *tx_contexts;
 
 	struct usb_anchor tx_submitted;
 	struct completion start_comp, stop_comp;
@@ -657,9 +661,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
 	switch (dev->family) {
 	case KVASER_LEAF:
 		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
 		break;
 	case KVASER_USBCAN:
 		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
 		break;
 	}
 
@@ -715,7 +723,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats = &priv->netdev->stats;
 
-	context = &priv->tx_contexts[tid % MAX_TX_URBS];
+	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
 
 	/* Sometimes the state change doesn't come after a bus-off event */
 	if (priv->can.restart_ms &&
@@ -744,7 +752,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 	can_get_echo_skb(priv->netdev, context->echo_index);
-	context->echo_index = MAX_TX_URBS;
+	context->echo_index = dev->max_tx_urbs;
 	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
 
@@ -1512,11 +1520,13 @@ error:
 
 static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
 {
-	int i;
+	int i, max_tx_urbs;
+
+	max_tx_urbs = priv->dev->max_tx_urbs;
 
 	priv->active_tx_contexts = 0;
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+	for (i = 0; i < max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = max_tx_urbs;
 }
 
 /* This method might sleep. Do not call it in the atomic context
@@ -1702,14 +1712,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
-		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
+	for (i = 0; i < dev->max_tx_urbs; i++) {
+		if (priv->tx_contexts[i].echo_index == dev->max_tx_urbs) {
 			context = &priv->tx_contexts[i];
 
 			context->echo_index = i;
 			can_put_echo_skb(skb, netdev, context->echo_index);
 			++priv->active_tx_contexts;
-			if (priv->active_tx_contexts >= MAX_TX_URBS)
+			if (priv->active_tx_contexts >= dev->max_tx_urbs)
 				netif_stop_queue(netdev);
 
 			break;
@@ -1743,7 +1753,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 		can_free_echo_skb(netdev, context->echo_index);
-		context->echo_index = MAX_TX_URBS;
+		context->echo_index = dev->max_tx_urbs;
 		--priv->active_tx_contexts;
 		netif_wake_queue(netdev);
 
@@ -1881,7 +1891,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	if (err)
 		return err;
 
-	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
 	if (!netdev) {
 		dev_err(&intf->dev, "Cannot alloc candev\n");
 		return -ENOMEM;
@@ -1889,6 +1899,13 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 
 	priv = netdev_priv(netdev);
 
+	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
+				    sizeof(*priv->tx_contexts), GFP_KERNEL);
+	if (!priv->tx_contexts) {
+		free_candev(netdev);
+		return -ENOMEM;
+	}
+
 	init_usb_anchor(&priv->tx_submitted);
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
@@ -1928,7 +1945,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 		return err;
 	}
 
-	netdev_dbg(netdev, "device registered\n");
+	netdev_info(netdev, "device registered\n");
 
 	return 0;
 }
@@ -2009,6 +2026,13 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
+	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
+		((dev->fw_version >> 24) & 0xff),
+		((dev->fw_version >> 16) & 0xff),
+		(dev->fw_version & 0xffff));
+
+	dev_dbg(&intf->dev, "Max oustanding tx = %d URBs\n", dev->max_tx_urbs);
+
 	err = kvaser_usb_get_card_info(dev);
 	if (err) {
 		dev_err(&intf->dev,
@@ -2016,11 +2040,6 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
-	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
-		((dev->fw_version >> 24) & 0xff),
-		((dev->fw_version >> 16) & 0xff),
-		(dev->fw_version & 0xffff));
-
 	for (i = 0; i < dev->nchannels; i++) {
 		err = kvaser_usb_init_one(intf, id, i);
 		if (err) {
-- 
1.9.1

[PATCH v3 3/3] can: kvaser_usb: Use can-dev unregistration mechanism

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Use can-dev's unregister_candev() instead of directly calling
networking unregister_netdev(). While both are functionally
equivalent, unregister_candev() might do extra stuff in the
future than just calling networking layer unregistration code.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 30b4d47..fafcb89 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1866,7 +1866,7 @@ static void kvaser_usb_remove_interfaces(struct kvaser_usb *dev)
 		if (!dev->nets[i])
 			continue;
 
-		unregister_netdev(dev->nets[i]->netdev);
+		unregister_candev(dev->nets[i]->netdev);
 	}
 
 	kvaser_usb_unlink_all_urbs(dev);
-- 
1.9.1

Re: [PATCH v3 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Marc Kleine-Budde]

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

On 03/11/2015 06:37 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> A number of tx queue wake-up events went missing due to the
> outlined scenario below. Start state is a pool of 16 tx URBs,
> active tx_urbs count = 15, with the netdev tx queue open.
> 
> start_xmit()                             tx_acknowledge()
> ............                             ................
> atomic_inc(&tx_urbs);
> if (atomic_read(&tx_urbs) >= 16) {
>                         URB completion IRQ!
>                         -->
>                                          atomic_dec(&tx_urbs);
>                                          netif_wake_queue();
>                                          return;
>                         <--
>                         end of IRQ!
>     netif_stop_queue();
> }
> 
> At the end, the correct state expected is a 15 tx_urbs count
> value with the tx queue state _open_. Due to the race, we get
> the same tx_urbs value but with the tx queue state _stopped_.
> The wake-up event is completely lost.
> 
> Thus avoid hand-rolled concurrency mechanisms and use a proper
> lock for contexts protection.
> 
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
>  drivers/net/can/usb/kvaser_usb.c | 83 ++++++++++++++++++++++++----------------
>  1 file changed, 51 insertions(+), 32 deletions(-)
> 
> changelog-v3: Added missing spin_lock_init(). With all kernel
> lock debugging options set, I've been running my test suite
> for an hour now without apparent problems in dmesg so far.
> 
> changelog-v2: Put bugfix patch at the start of the series.
> 
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index a316fa4..e97a08c 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -14,6 +14,7 @@
>   * Copyright (C) 2015 Valeo S.A.
>   */
>  
> +#include <linux/spinlock.h>
>  #include <linux/kernel.h>
>  #include <linux/completion.h>
>  #include <linux/module.h>
> @@ -467,10 +468,11 @@ struct kvaser_usb {
>  struct kvaser_usb_net_priv {
>  	struct can_priv can;
>  
> -	atomic_t active_tx_urbs;
> -	struct usb_anchor tx_submitted;
> +	spinlock_t tx_contexts_lock;
> +	int active_tx_contexts;
>  	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
>  
> +	struct usb_anchor tx_submitted;
>  	struct completion start_comp, stop_comp;
>  
>  	struct kvaser_usb *dev;
> @@ -694,6 +696,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
>  	struct kvaser_usb_net_priv *priv;
>  	struct sk_buff *skb;
>  	struct can_frame *cf;
> +	unsigned long flags;
>  	u8 channel, tid;
>  
>  	channel = msg->u.tx_acknowledge_header.channel;
> @@ -737,12 +740,15 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
>  
>  	stats->tx_packets++;
>  	stats->tx_bytes += context->dlc;
> -	can_get_echo_skb(priv->netdev, context->echo_index);
>  
> -	context->echo_index = MAX_TX_URBS;
> -	atomic_dec(&priv->active_tx_urbs);
> +	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
>  
> +	can_get_echo_skb(priv->netdev, context->echo_index);
> +	context->echo_index = MAX_TX_URBS;
> +	--priv->active_tx_contexts;
>  	netif_wake_queue(priv->netdev);
> +
> +	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);

I think it should be possible to move tun unlock before waking up the queue?

>  }
>  
>  static void kvaser_usb_simple_msg_callback(struct urb *urb)
> @@ -803,17 +809,6 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
>  	return 0;
>  }
>  
> -static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> -{
> -	int i;
> -
> -	usb_kill_anchored_urbs(&priv->tx_submitted);
> -	atomic_set(&priv->active_tx_urbs, 0);
> -
> -	for (i = 0; i < MAX_TX_URBS; i++)
> -		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> -}
> -
>  static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
>  						 const struct kvaser_usb_error_summary *es,
>  						 struct can_frame *cf)
> @@ -1515,6 +1510,24 @@ error:
>  	return err;
>  }
>  
> +static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
> +{
> +	int i;
> +
> +	priv->active_tx_contexts = 0;
> +	for (i = 0; i < MAX_TX_URBS; i++)
> +		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> +}
> +
> +/* This method might sleep. Do not call it in the atomic context
> + * of URB completions.
> + */
> +static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> +{
> +	usb_kill_anchored_urbs(&priv->tx_submitted);
> +	kvaser_usb_reset_tx_urb_contexts(priv);
> +}
> +
>  static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
>  {
>  	int i;
> @@ -1634,6 +1647,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>  	struct kvaser_msg *msg;
>  	int i, err, ret = NETDEV_TX_OK;
>  	u8 *msg_tx_can_flags = NULL;		/* GCC */
> +	unsigned long flags;
>  
>  	if (can_dropped_invalid_skb(netdev, skb))
>  		return NETDEV_TX_OK;
> @@ -1687,12 +1701,21 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>  	if (cf->can_id & CAN_RTR_FLAG)
>  		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
>  
> +	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
>  	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];
> +
> +			context->echo_index = i;
> +			can_put_echo_skb(skb, netdev, context->echo_index);
> +			++priv->active_tx_contexts;
> +			if (priv->active_tx_contexts >= MAX_TX_URBS)
> +				netif_stop_queue(netdev);
> +
>  			break;
>  		}
>  	}
> +	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
>  
>  	/* This should never happen; it implies a flow control bug */
>  	if (!context) {
> @@ -1704,7 +1727,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>  	}
>  
>  	context->priv = priv;
> -	context->echo_index = i;
>  	context->dlc = cf->can_dlc;
>  
>  	msg->u.tx_can.tid = context->echo_index;
> @@ -1716,18 +1738,17 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>  			  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)) {
> +		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> +
>  		can_free_echo_skb(netdev, context->echo_index);
> +		context->echo_index = MAX_TX_URBS;
> +		--priv->active_tx_contexts;
> +		netif_wake_queue(netdev);
> +
> +		spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);

Same here?

>  
> -		atomic_dec(&priv->active_tx_urbs);
>  		usb_unanchor_urb(urb);
>  
>  		stats->tx_dropped++;
> @@ -1854,7 +1875,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>  	struct kvaser_usb *dev = usb_get_intfdata(intf);
>  	struct net_device *netdev;
>  	struct kvaser_usb_net_priv *priv;
> -	int i, err;
> +	int err;
>  
>  	err = kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, channel);
>  	if (err)
> @@ -1868,19 +1889,17 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>  
>  	priv = netdev_priv(netdev);
>  
> +	init_usb_anchor(&priv->tx_submitted);
>  	init_completion(&priv->start_comp);
>  	init_completion(&priv->stop_comp);
>  
> -	init_usb_anchor(&priv->tx_submitted);
> -	atomic_set(&priv->active_tx_urbs, 0);
> -
> -	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
> -		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> -
>  	priv->dev = dev;
>  	priv->netdev = netdev;
>  	priv->channel = channel;
>  
> +	spin_lock_init(&priv->tx_contexts_lock);
> +	kvaser_usb_reset_tx_urb_contexts(priv);
> +
>  	priv->can.state = CAN_STATE_STOPPED;
>  	priv->can.clock.freq = CAN_USB_CLOCK;
>  	priv->can.bittiming_const = &kvaser_usb_bittiming_const;
> 

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: 819 bytes --]

Re: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs

[Marc Kleine-Budde]

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

On 03/11/2015 06:39 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> The driver currently limits the number of outstanding, not yet
> ACKed, transfers to 16 URBs. Meanwhile, the Kvaser firmware
> provides its actual max supported number of outstanding
> transmissions in its reply to the CMD_GET_SOFTWARE_INFO message.
> 
> One example is the UsbCan-II HS/LS device which reports support
> of up to 48 tx URBs instead of just 16, increasing the driver
> throughput by two-fold and reducing the possibility of -ENOBUFs.
> 
> Dynamically set the max tx URBs value according to firmware
> replies.
> 
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
>  drivers/net/can/usb/kvaser_usb.c | 55 +++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 18 deletions(-)
> 
> changelog-v3: No changes
> 
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index e97a08c..30b4d47 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -25,7 +25,6 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
>  
> -#define MAX_TX_URBS			16
>  #define MAX_RX_URBS			4
>  #define START_TIMEOUT			1000 /* msecs */
>  #define STOP_TIMEOUT			1000 /* msecs */
> @@ -456,8 +455,13 @@ struct kvaser_usb {
>  	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>  	struct usb_anchor rx_submitted;
>  
> +	/* @max_tx_urbs: Firmware-reported maximum number of possible
> +	 * outstanding transmissions on this specific Kvaser hardware. The
> +	 * value is also used as a sentinel for marking free URB contexts.
> +	 */
>  	u32 fw_version;
>  	unsigned int nchannels;
> +	unsigned int max_tx_urbs;
>  	enum kvaser_usb_family family;
>  
>  	bool rxinitdone;
> @@ -470,7 +474,7 @@ struct kvaser_usb_net_priv {
>  
>  	spinlock_t tx_contexts_lock;
>  	int active_tx_contexts;
> -	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
> +	struct kvaser_usb_tx_urb_context *tx_contexts;
>  
>  	struct usb_anchor tx_submitted;
>  	struct completion start_comp, stop_comp;
> @@ -657,9 +661,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
>  	switch (dev->family) {
>  	case KVASER_LEAF:
>  		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
> +		dev->max_tx_urbs =
> +			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
>  		break;
>  	case KVASER_USBCAN:
>  		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
> +		dev->max_tx_urbs =
> +			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
>  		break;
>  	}
>  
> @@ -715,7 +723,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
>  
>  	stats = &priv->netdev->stats;
>  
> -	context = &priv->tx_contexts[tid % MAX_TX_URBS];
> +	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
>  
>  	/* Sometimes the state change doesn't come after a bus-off event */
>  	if (priv->can.restart_ms &&
> @@ -744,7 +752,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
>  	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
>  
>  	can_get_echo_skb(priv->netdev, context->echo_index);
> -	context->echo_index = MAX_TX_URBS;
> +	context->echo_index = dev->max_tx_urbs;
>  	--priv->active_tx_contexts;
>  	netif_wake_queue(priv->netdev);
>  
> @@ -1512,11 +1520,13 @@ error:
>  
>  static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
>  {
> -	int i;
> +	int i, max_tx_urbs;
> +
> +	max_tx_urbs = priv->dev->max_tx_urbs;
>  
>  	priv->active_tx_contexts = 0;
> -	for (i = 0; i < MAX_TX_URBS; i++)
> -		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> +	for (i = 0; i < max_tx_urbs; i++)
> +		priv->tx_contexts[i].echo_index = max_tx_urbs;
>  }
>  
>  /* This method might sleep. Do not call it in the atomic context
> @@ -1702,14 +1712,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>  		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
>  
>  	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> -	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
> -		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> +	for (i = 0; i < dev->max_tx_urbs; i++) {
> +		if (priv->tx_contexts[i].echo_index == dev->max_tx_urbs) {
>  			context = &priv->tx_contexts[i];
>  
>  			context->echo_index = i;
>  			can_put_echo_skb(skb, netdev, context->echo_index);
>  			++priv->active_tx_contexts;
> -			if (priv->active_tx_contexts >= MAX_TX_URBS)
> +			if (priv->active_tx_contexts >= dev->max_tx_urbs)
>  				netif_stop_queue(netdev);
>  
>  			break;
> @@ -1743,7 +1753,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>  		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
>  
>  		can_free_echo_skb(netdev, context->echo_index);
> -		context->echo_index = MAX_TX_URBS;
> +		context->echo_index = dev->max_tx_urbs;
>  		--priv->active_tx_contexts;
>  		netif_wake_queue(netdev);
>  
> @@ -1881,7 +1891,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>  	if (err)
>  		return err;
>  
> -	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
> +	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
>  	if (!netdev) {
>  		dev_err(&intf->dev, "Cannot alloc candev\n");
>  		return -ENOMEM;
> @@ -1889,6 +1899,13 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>  
>  	priv = netdev_priv(netdev);
>  
> +	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
> +				    sizeof(*priv->tx_contexts), GFP_KERNEL);
> +	if (!priv->tx_contexts) {
> +		free_candev(netdev);
> +		return -ENOMEM;
> +	}

I'm missing a free for the priv->tx_contexts. I see two options:

1) use devm_kzalloc(), or
2) move struct kvaser_usb_tx_urb_context tx_contexts[]; to the end of
   struct kvaser_usb_net_priv, see [1] for an example.

   Without further testing, I think the correct alloc for that case
   would be:
       alloc_candev(sizeof(*priv + dev->max_tx_urbs *
               sizeof(struct kvaser_usb_tx_urb_context))

Marc

[1] http://stackoverflow.com/questions/2060974/dynamic-array-in-struct-c

-- 
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: 819 bytes --]

Re: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs

[Ahmed S. Darwish]

On Wed, Mar 11, 2015 at 10:53:28PM +0100, Marc Kleine-Budde wrote:
> On 03/11/2015 06:39 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > The driver currently limits the number of outstanding, not yet
> > ACKed, transfers to 16 URBs. Meanwhile, the Kvaser firmware
> > provides its actual max supported number of outstanding
> > transmissions in its reply to the CMD_GET_SOFTWARE_INFO message.
> > 
> > One example is the UsbCan-II HS/LS device which reports support
> > of up to 48 tx URBs instead of just 16, increasing the driver
> > throughput by two-fold and reducing the possibility of -ENOBUFs.
> > 
> > Dynamically set the max tx URBs value according to firmware
> > replies.
> > 
> > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > ---
> >  drivers/net/can/usb/kvaser_usb.c | 55 +++++++++++++++++++++++++++-------------
> >  1 file changed, 37 insertions(+), 18 deletions(-)
> > 
> > changelog-v3: No changes
> > 
> > diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> > index e97a08c..30b4d47 100644
> > --- a/drivers/net/can/usb/kvaser_usb.c
> > +++ b/drivers/net/can/usb/kvaser_usb.c
> > @@ -25,7 +25,6 @@
> >  #include <linux/can/dev.h>
> >  #include <linux/can/error.h>
> >  
> > -#define MAX_TX_URBS			16
> >  #define MAX_RX_URBS			4
> >  #define START_TIMEOUT			1000 /* msecs */
> >  #define STOP_TIMEOUT			1000 /* msecs */
> > @@ -456,8 +455,13 @@ struct kvaser_usb {
> >  	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> >  	struct usb_anchor rx_submitted;
> >  
> > +	/* @max_tx_urbs: Firmware-reported maximum number of possible
> > +	 * outstanding transmissions on this specific Kvaser hardware. The
> > +	 * value is also used as a sentinel for marking free URB contexts.
> > +	 */
> >  	u32 fw_version;
> >  	unsigned int nchannels;
> > +	unsigned int max_tx_urbs;
> >  	enum kvaser_usb_family family;
> >  
> >  	bool rxinitdone;
> > @@ -470,7 +474,7 @@ struct kvaser_usb_net_priv {
> >  
> >  	spinlock_t tx_contexts_lock;
> >  	int active_tx_contexts;
> > -	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
> > +	struct kvaser_usb_tx_urb_context *tx_contexts;
> >  
> >  	struct usb_anchor tx_submitted;
> >  	struct completion start_comp, stop_comp;
> > @@ -657,9 +661,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
> >  	switch (dev->family) {
> >  	case KVASER_LEAF:
> >  		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
> > +		dev->max_tx_urbs =
> > +			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
> >  		break;
> >  	case KVASER_USBCAN:
> >  		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
> > +		dev->max_tx_urbs =
> > +			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
> >  		break;
> >  	}
> >  
> > @@ -715,7 +723,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> >  
> >  	stats = &priv->netdev->stats;
> >  
> > -	context = &priv->tx_contexts[tid % MAX_TX_URBS];
> > +	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
> >  
> >  	/* Sometimes the state change doesn't come after a bus-off event */
> >  	if (priv->can.restart_ms &&
> > @@ -744,7 +752,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> >  	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> >  
> >  	can_get_echo_skb(priv->netdev, context->echo_index);
> > -	context->echo_index = MAX_TX_URBS;
> > +	context->echo_index = dev->max_tx_urbs;
> >  	--priv->active_tx_contexts;
> >  	netif_wake_queue(priv->netdev);
> >  
> > @@ -1512,11 +1520,13 @@ error:
> >  
> >  static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
> >  {
> > -	int i;
> > +	int i, max_tx_urbs;
> > +
> > +	max_tx_urbs = priv->dev->max_tx_urbs;
> >  
> >  	priv->active_tx_contexts = 0;
> > -	for (i = 0; i < MAX_TX_URBS; i++)
> > -		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> > +	for (i = 0; i < max_tx_urbs; i++)
> > +		priv->tx_contexts[i].echo_index = max_tx_urbs;
> >  }
> >  
> >  /* This method might sleep. Do not call it in the atomic context
> > @@ -1702,14 +1712,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> >  		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
> >  
> >  	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> > -	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
> > -		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> > +	for (i = 0; i < dev->max_tx_urbs; i++) {
> > +		if (priv->tx_contexts[i].echo_index == dev->max_tx_urbs) {
> >  			context = &priv->tx_contexts[i];
> >  
> >  			context->echo_index = i;
> >  			can_put_echo_skb(skb, netdev, context->echo_index);
> >  			++priv->active_tx_contexts;
> > -			if (priv->active_tx_contexts >= MAX_TX_URBS)
> > +			if (priv->active_tx_contexts >= dev->max_tx_urbs)
> >  				netif_stop_queue(netdev);
> >  
> >  			break;
> > @@ -1743,7 +1753,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> >  		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> >  
> >  		can_free_echo_skb(netdev, context->echo_index);
> > -		context->echo_index = MAX_TX_URBS;
> > +		context->echo_index = dev->max_tx_urbs;
> >  		--priv->active_tx_contexts;
> >  		netif_wake_queue(netdev);
> >  
> > @@ -1881,7 +1891,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
> >  	if (err)
> >  		return err;
> >  
> > -	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
> > +	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
> >  	if (!netdev) {
> >  		dev_err(&intf->dev, "Cannot alloc candev\n");
> >  		return -ENOMEM;
> > @@ -1889,6 +1899,13 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
> >  
> >  	priv = netdev_priv(netdev);
> >  
> > +	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
> > +				    sizeof(*priv->tx_contexts), GFP_KERNEL);
> > +	if (!priv->tx_contexts) {
> > +		free_candev(netdev);
> > +		return -ENOMEM;
> > +	}
> 
> I'm missing a free for the priv->tx_contexts. I see two options:
> 

Correct. Should not have missed that.

> 1) use devm_kzalloc(), or
> 2) move struct kvaser_usb_tx_urb_context tx_contexts[]; to the end of
>    struct kvaser_usb_net_priv, see [1] for an example.
> 
>    Without further testing, I think the correct alloc for that case
>    would be:
>        alloc_candev(sizeof(*priv + dev->max_tx_urbs *
>                sizeof(struct kvaser_usb_tx_urb_context))
> 

The first option looks better I guess. I'll have to check though
if the resource handling done by devm_kmalloc() will work even
if the probe() method fails with -ENODEV and the like...

> Marc
> 
> [1] http://stackoverflow.com/questions/2060974/dynamic-array-in-struct-c
> 

Thanks for the link. Didn't know that such a "hack" has gained
official status by C99 :-)

Regards,
Darwish

Re: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs

[Marc Kleine-Budde]

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

On 03/12/2015 11:52 AM, Ahmed S. Darwish wrote:
>>> @@ -1881,7 +1891,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>>>  	if (err)
>>>  		return err;
>>>  
>>> -	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
>>> +	netdev = alloc_candev(sizeof(*priv), dev->max_tx_urbs);
>>>  	if (!netdev) {
>>>  		dev_err(&intf->dev, "Cannot alloc candev\n");
>>>  		return -ENOMEM;
>>> @@ -1889,6 +1899,13 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>>>  
>>>  	priv = netdev_priv(netdev);
>>>  
>>> +	priv->tx_contexts = kzalloc(dev->max_tx_urbs *
>>> +				    sizeof(*priv->tx_contexts), GFP_KERNEL);
>>> +	if (!priv->tx_contexts) {
>>> +		free_candev(netdev);
>>> +		return -ENOMEM;
>>> +	}
>>
>> I'm missing a free for the priv->tx_contexts. I see two options:
>>
> 
> Correct. Should not have missed that.
> 
>> 1) use devm_kzalloc(), or
>> 2) move struct kvaser_usb_tx_urb_context tx_contexts[]; to the end of
>>    struct kvaser_usb_net_priv, see [1] for an example.
>>
>>    Without further testing, I think the correct alloc for that case
>>    would be:
>>        alloc_candev(sizeof(*priv + dev->max_tx_urbs *
>>                sizeof(struct kvaser_usb_tx_urb_context))
>>
> 
> The first option looks better I guess. I'll have to check though
> if the resource handling done by devm_kmalloc() will work even
> if the probe() method fails with -ENODEV and the like...

devm does handle failing probe functions by design, while calling a
manual free() on devm allocated mem is a bug, which will cause a double
free or worse.

YMMV: for option 2 saves a mem allocation and a pointer deref for each
access of the context.

>> [1] http://stackoverflow.com/questions/2060974/dynamic-array-in-struct-c

> Thanks for the link. Didn't know that such a "hack" has gained
> official status by C99 :-)

:D and I think it's used in the kernel.

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: 819 bytes --]

Re: [PATCH v3 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

Hi Marc,

On Wed, Mar 11, 2015 at 10:43:07PM +0100, Marc Kleine-Budde wrote:
> On 03/11/2015 06:37 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > A number of tx queue wake-up events went missing due to the
> > outlined scenario below. Start state is a pool of 16 tx URBs,
> > active tx_urbs count = 15, with the netdev tx queue open.
> > 
> > start_xmit()                             tx_acknowledge()
> > ............                             ................
> > atomic_inc(&tx_urbs);
> > if (atomic_read(&tx_urbs) >= 16) {
> >                         URB completion IRQ!
> >                         -->
> >                                          atomic_dec(&tx_urbs);
> >                                          netif_wake_queue();
> >                                          return;
> >                         <--
> >                         end of IRQ!
> >     netif_stop_queue();
> > }
> > 
> > At the end, the correct state expected is a 15 tx_urbs count
> > value with the tx queue state _open_. Due to the race, we get
> > the same tx_urbs value but with the tx queue state _stopped_.
> > The wake-up event is completely lost.
> > 
> > Thus avoid hand-rolled concurrency mechanisms and use a proper
> > lock for contexts protection.
> > 
> > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > ---
> >  drivers/net/can/usb/kvaser_usb.c | 83 ++++++++++++++++++++++++----------------
> >  1 file changed, 51 insertions(+), 32 deletions(-)
> > 
> > changelog-v3: Added missing spin_lock_init(). With all kernel
> > lock debugging options set, I've been running my test suite
> > for an hour now without apparent problems in dmesg so far.
> > 
> > changelog-v2: Put bugfix patch at the start of the series.
> > 
> > diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> > index a316fa4..e97a08c 100644
> > --- a/drivers/net/can/usb/kvaser_usb.c
> > +++ b/drivers/net/can/usb/kvaser_usb.c
> > @@ -14,6 +14,7 @@
> >   * Copyright (C) 2015 Valeo S.A.
> >   */
> >  
> > +#include <linux/spinlock.h>
> >  #include <linux/kernel.h>
> >  #include <linux/completion.h>
> >  #include <linux/module.h>
> > @@ -467,10 +468,11 @@ struct kvaser_usb {
> >  struct kvaser_usb_net_priv {
> >  	struct can_priv can;
> >  
> > -	atomic_t active_tx_urbs;
> > -	struct usb_anchor tx_submitted;
> > +	spinlock_t tx_contexts_lock;
> > +	int active_tx_contexts;
> >  	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
> >  
> > +	struct usb_anchor tx_submitted;
> >  	struct completion start_comp, stop_comp;
> >  
> >  	struct kvaser_usb *dev;
> > @@ -694,6 +696,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> >  	struct kvaser_usb_net_priv *priv;
> >  	struct sk_buff *skb;
> >  	struct can_frame *cf;
> > +	unsigned long flags;
> >  	u8 channel, tid;
> >  
> >  	channel = msg->u.tx_acknowledge_header.channel;
> > @@ -737,12 +740,15 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> >  
> >  	stats->tx_packets++;
> >  	stats->tx_bytes += context->dlc;
> > -	can_get_echo_skb(priv->netdev, context->echo_index);
> >  
> > -	context->echo_index = MAX_TX_URBS;
> > -	atomic_dec(&priv->active_tx_urbs);
> > +	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> >  
> > +	can_get_echo_skb(priv->netdev, context->echo_index);
> > +	context->echo_index = MAX_TX_URBS;
> > +	--priv->active_tx_contexts;
> >  	netif_wake_queue(priv->netdev);
> > +
> > +	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
> 
> I think it should be possible to move tun unlock before waking up the queue?
> 

Hmmm... I've kept thinking about this today. Here's my
understanding of the whole situation:

1) start_xmit() runs in NET_TX_SOFTIRQ softirq context
2) tx_acknowledge() occurs in URB completion softirq context
3) In an SMP machine, softirqs can run parallel to each other
4) start_xmit is protected from itself by the _xmit_lock spin
5) start_xmit() and tx_acknowledge() can, and do, run in parallel
   to each other

From the above, we can imagine the following:

################################################################
#  Transfer queue length = 16
#  Transfer queue index = 15
#  
#  CPU #1                                    CPU #2
#  start_xmit()                              tx_acknowledge()
#  ------------                              ----------------
#                                            ...
#                                            spin_lock(ctx_lock)
#                                            index--
#                                            spin_unlock(ctx__lock)
#                          <---
#  ...
#  spin_lock(ctx_lock)
#  index++
#  spin_unlock(ctx_lock)
#  return;
#  
#  /* Another invocation of start_xmit */
#  
#  ...
#  spin_lock(ctx_lock)
#  index++
#  /* We've filled the tx queue */
#  if (index == 16)
#      netif_stop_queue()
#  spin_unlock(ctx_lock)
#  return;
#  
#  /* Transfer queue stopped */
#
#                          --->
#                                            /* Queue woken up
#  					     while it's full */
#                                            netif_wake_queue()
#                        
################################################################

I admit that unlike the race in the patch commit message, which
actually appeared in practice in a normal but heavy use case,
the one in the box above is highly theoretical. 

Nonetheless, I was actually able to fully and reliably produce
it by inserting a busy loop as follows:

static void kvaser_usb_tx_acknowledge(...)
{
   ...
   spin_lock_irqsave(&priv->tx_contexts_lock, flags);

   can_get_echo_skb(priv->netdev, context->echo_index);
   context->echo_index = dev->max_tx_urbs;
   --priv->active_tx_contexts;

   spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);

   /* Manual delay; use cpu_relax() not to be optimized out */
   for (n = 0; n < 1000; n++)
       cpu_relax();

   netif_wake_queue(priv->netdev);
   ...
}

Then running a very heavy transmission load:

   $ for i in {1..10}; do (cangen -i -I 111 -g1 -Di -L4 can0 &); done

And as I've expected, dmesg began seeing entries of "cannot find
free context" due to waking up the tx queue while being full:

[  +0.000002] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context
[  +0.000003] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context
[  +0.000002] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context
[  +0.000002] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context

Puting the netif_wake_queue() back inside the critical section,
even while keeping the delay loop in place, avoided such a race
condition.

Under normal conditions, such a heavy delay between the critical
section exit and netif_wake_queue() will not usually occur. This
is even more true since a softirq cannot preempt another softirq
running on the same CPU.

Nonetheless, since the race can be manually triggered as seen
above, I'd be safe and wake the queue inside the critical section
rather than sorry...

What do you think?

Regards,
Darwish

Re: [PATCH v3 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

Hi Marc,

On Wed, Mar 11, 2015 at 10:43:07PM +0100, Marc Kleine-Budde wrote:
> On 03/11/2015 06:37 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > A number of tx queue wake-up events went missing due to the
> > outlined scenario below. Start state is a pool of 16 tx URBs,
> > active tx_urbs count = 15, with the netdev tx queue open.
> > 
> > start_xmit()                             tx_acknowledge()
> > ............                             ................
> > atomic_inc(&tx_urbs);
> > if (atomic_read(&tx_urbs) >= 16) {
> >                         URB completion IRQ!
> >                         -->
> >                                          atomic_dec(&tx_urbs);
> >                                          netif_wake_queue();
> >                                          return;
> >                         <--
> >                         end of IRQ!
> >     netif_stop_queue();
> > }
> > 
> > At the end, the correct state expected is a 15 tx_urbs count
> > value with the tx queue state _open_. Due to the race, we get
> > the same tx_urbs value but with the tx queue state _stopped_.
> > The wake-up event is completely lost.
> > 
> > Thus avoid hand-rolled concurrency mechanisms and use a proper
> > lock for contexts protection.
> > 
> > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > ---
> >  drivers/net/can/usb/kvaser_usb.c | 83 ++++++++++++++++++++++++----------------
> >  1 file changed, 51 insertions(+), 32 deletions(-)
> > 
> > changelog-v3: Added missing spin_lock_init(). With all kernel
> > lock debugging options set, I've been running my test suite
> > for an hour now without apparent problems in dmesg so far.
> > 
> > changelog-v2: Put bugfix patch at the start of the series.
> > 
> > diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> > index a316fa4..e97a08c 100644
> > --- a/drivers/net/can/usb/kvaser_usb.c
> > +++ b/drivers/net/can/usb/kvaser_usb.c
> > @@ -14,6 +14,7 @@
> >   * Copyright (C) 2015 Valeo S.A.
> >   */
> >  
> > +#include <linux/spinlock.h>
> >  #include <linux/kernel.h>
> >  #include <linux/completion.h>
> >  #include <linux/module.h>
> > @@ -467,10 +468,11 @@ struct kvaser_usb {
> >  struct kvaser_usb_net_priv {
> >  	struct can_priv can;
> >  
> > -	atomic_t active_tx_urbs;
> > -	struct usb_anchor tx_submitted;
> > +	spinlock_t tx_contexts_lock;
> > +	int active_tx_contexts;
> >  	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
> >  
> > +	struct usb_anchor tx_submitted;
> >  	struct completion start_comp, stop_comp;
> >  
> >  	struct kvaser_usb *dev;
> > @@ -694,6 +696,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> >  	struct kvaser_usb_net_priv *priv;
> >  	struct sk_buff *skb;
> >  	struct can_frame *cf;
> > +	unsigned long flags;
> >  	u8 channel, tid;
> >  
> >  	channel = msg->u.tx_acknowledge_header.channel;
> > @@ -737,12 +740,15 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> >  
> >  	stats->tx_packets++;
> >  	stats->tx_bytes += context->dlc;
> > -	can_get_echo_skb(priv->netdev, context->echo_index);
> >  
> > -	context->echo_index = MAX_TX_URBS;
> > -	atomic_dec(&priv->active_tx_urbs);
> > +	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> >  
> > +	can_get_echo_skb(priv->netdev, context->echo_index);
> > +	context->echo_index = MAX_TX_URBS;
> > +	--priv->active_tx_contexts;
> >  	netif_wake_queue(priv->netdev);
> > +
> > +	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
> 
> I think it should be possible to move tun unlock before waking up the queue?
> 

Hmmm... I've kept thinking about this today. Here's my
understanding of the whole situation:

1) start_xmit() runs in NET_TX_SOFTIRQ softirq context
2) tx_acknowledge() occurs in URB completion softirq context
3) In an SMP machine, softirqs can run parallel to each other
4) start_xmit is protected from itself by the _xmit_lock spin
5) start_xmit() and tx_acknowledge() can, and do, run in parallel
   to each other

>From the above, we can imagine the following:

################################################################
#  Transfer queue length = 16
#  Transfer queue index = 15
#  
#  CPU #1                                    CPU #2
#  start_xmit()                              tx_acknowledge()
#  ------------                              ----------------
#                                            ...
#                                            spin_lock(ctx_lock)
#                                            index--
#                                            spin_unlock(ctx__lock)
#                          <---
#  ...
#  spin_lock(ctx_lock)
#  index++
#  spin_unlock(ctx_lock)
#  return;
#  
#  /* Another invocation of start_xmit */
#  
#  ...
#  spin_lock(ctx_lock)
#  index++
#  /* We've filled the tx queue */
#  if (index == 16)
#      netif_stop_queue()
#  spin_unlock(ctx_lock)
#  return;
#  
#  /* Transfer queue stopped */
#
#                          --->
#                                            /* Queue woken up
#  					     while it's full */
#                                            netif_wake_queue()
#                        
################################################################

I admit that unlike the race in the patch commit message, which
actually appeared in practice in a normal but heavy use case,
the one in the box above is highly theoretical. 

Nonetheless, I was actually able to fully and reliably produce
it by inserting a busy loop as follows:

static void kvaser_usb_tx_acknowledge(...)
{
   ...
   spin_lock_irqsave(&priv->tx_contexts_lock, flags);

   can_get_echo_skb(priv->netdev, context->echo_index);
   context->echo_index = dev->max_tx_urbs;
   --priv->active_tx_contexts;

   spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);

   /* Manual delay; use cpu_relax() not to be optimized out */
   for (n = 0; n < 1000; n++)
       cpu_relax();

   netif_wake_queue(priv->netdev);
   ...
}

Then running a very heavy transmission load:

   $ for i in {1..10}; do (cangen -i -I 111 -g1 -Di -L4 can0 &); done

And as I've expected, dmesg began seeing entries of "cannot find
free context" due to waking up the tx queue while being full:

[  +0.000002] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context
[  +0.000003] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context
[  +0.000002] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context
[  +0.000002] kvaser_usb 1-1.2.1.1:1.0 can0: cannot find free context

Puting the netif_wake_queue() back inside the critical section,
even while keeping the delay loop in place, avoided such a race
condition.

Under normal conditions, such a heavy delay between the critical
section exit and netif_wake_queue() will not usually occur. This
is even more true since a softirq cannot preempt another softirq
running on the same CPU.

Nonetheless, since the race can be manually triggered as seen
above, I'd be safe and wake the queue inside the critical section
rather than sorry...

What do you think?

Regards,
Darwish

[PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

A number of tx queue wake-up events went missing due to the
outlined scenario below. Start state is a pool of 16 tx URBs,
active tx_urbs count = 15, with the netdev tx queue open.

CPU #1 [softirq]                         CPU #2 [softirq]
start_xmit()                             tx_acknowledge()
................                         ................

atomic_inc(&tx_urbs);
if (atomic_read(&tx_urbs) >= 16) {
                        -->
                                         atomic_dec(&tx_urbs);
                                         netif_wake_queue();
                                         return;
                        <--
    netif_stop_queue();
}

At the end, the correct state expected is a 15 tx_urbs count
value with the tx queue state _open_. Due to the race, we get
the same tx_urbs value but with the tx queue state _stopped_.
The wake-up event is completely lost.

Thus avoid hand-rolled concurrency mechanisms and use a proper
lock for contexts and tx queue protection.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 83 ++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 32 deletions(-)

Changelog v4:
-------------

Improve the commit log not to give the impression of a
softirq preempting another softirq, which can never happen. The
race condition occurs by having the softirqs running in parallel.

For why are we waking up the queue inside the newly created
critical section, kindly check the explanation here:

	http://article.gmane.org/gmane.linux.kernel/1907377
	Archived at: http://www.webcitation.org/6X1SNi708

Meanwhile, I've been running the driver for 30 hours now under
very heavy and ordered "cangen -Di" traffic from both ends.
Analyzing the tens of gigabytes candump traces (generated, in
parallel, using in-kernel CAN ID filters to avoid SO_RXQ_OVFL
overflows) shows that all the frames were sent and received in
the expected sequence.

Changelog v3:
-------------

Add missing spin_lock_init(). Run driver tests with locking
and memory management debugging options on.

Changelog v2:
-------------

Put this bugfix patch at the top of the series

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index a316fa4..e97a08c 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -14,6 +14,7 @@
  * Copyright (C) 2015 Valeo S.A.
  */
 
+#include <linux/spinlock.h>
 #include <linux/kernel.h>
 #include <linux/completion.h>
 #include <linux/module.h>
@@ -467,10 +468,11 @@ struct kvaser_usb {
 struct kvaser_usb_net_priv {
 	struct can_priv can;
 
-	atomic_t active_tx_urbs;
-	struct usb_anchor tx_submitted;
+	spinlock_t tx_contexts_lock;
+	int active_tx_contexts;
 	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
 
+	struct usb_anchor tx_submitted;
 	struct completion start_comp, stop_comp;
 
 	struct kvaser_usb *dev;
@@ -694,6 +696,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	struct kvaser_usb_net_priv *priv;
 	struct sk_buff *skb;
 	struct can_frame *cf;
+	unsigned long flags;
 	u8 channel, tid;
 
 	channel = msg->u.tx_acknowledge_header.channel;
@@ -737,12 +740,15 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats->tx_packets++;
 	stats->tx_bytes += context->dlc;
-	can_get_echo_skb(priv->netdev, context->echo_index);
 
-	context->echo_index = MAX_TX_URBS;
-	atomic_dec(&priv->active_tx_urbs);
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
+	can_get_echo_skb(priv->netdev, context->echo_index);
+	context->echo_index = MAX_TX_URBS;
+	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
+
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 }
 
 static void kvaser_usb_simple_msg_callback(struct urb *urb)
@@ -803,17 +809,6 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
 	return 0;
 }
 
-static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
-{
-	int i;
-
-	usb_kill_anchored_urbs(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-}
-
 static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
 						 const struct kvaser_usb_error_summary *es,
 						 struct can_frame *cf)
@@ -1515,6 +1510,24 @@ error:
 	return err;
 }
 
+static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
+{
+	int i;
+
+	priv->active_tx_contexts = 0;
+	for (i = 0; i < MAX_TX_URBS; i++)
+		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+}
+
+/* This method might sleep. Do not call it in the atomic context
+ * of URB completions.
+ */
+static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
+{
+	usb_kill_anchored_urbs(&priv->tx_submitted);
+	kvaser_usb_reset_tx_urb_contexts(priv);
+}
+
 static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
 {
 	int i;
@@ -1634,6 +1647,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	struct kvaser_msg *msg;
 	int i, err, ret = NETDEV_TX_OK;
 	u8 *msg_tx_can_flags = NULL;		/* GCC */
+	unsigned long flags;
 
 	if (can_dropped_invalid_skb(netdev, skb))
 		return NETDEV_TX_OK;
@@ -1687,12 +1701,21 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	if (cf->can_id & CAN_RTR_FLAG)
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
+	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 	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];
+
+			context->echo_index = i;
+			can_put_echo_skb(skb, netdev, context->echo_index);
+			++priv->active_tx_contexts;
+			if (priv->active_tx_contexts >= MAX_TX_URBS)
+				netif_stop_queue(netdev);
+
 			break;
 		}
 	}
+	spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
 	/* This should never happen; it implies a flow control bug */
 	if (!context) {
@@ -1704,7 +1727,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 	}
 
 	context->priv = priv;
-	context->echo_index = i;
 	context->dlc = cf->can_dlc;
 
 	msg->u.tx_can.tid = context->echo_index;
@@ -1716,18 +1738,17 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 			  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)) {
+		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
+
 		can_free_echo_skb(netdev, context->echo_index);
+		context->echo_index = MAX_TX_URBS;
+		--priv->active_tx_contexts;
+		netif_wake_queue(netdev);
+
+		spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
 
-		atomic_dec(&priv->active_tx_urbs);
 		usb_unanchor_urb(urb);
 
 		stats->tx_dropped++;
@@ -1854,7 +1875,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	struct kvaser_usb *dev = usb_get_intfdata(intf);
 	struct net_device *netdev;
 	struct kvaser_usb_net_priv *priv;
-	int i, err;
+	int err;
 
 	err = kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, channel);
 	if (err)
@@ -1868,19 +1889,17 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 
 	priv = netdev_priv(netdev);
 
+	init_usb_anchor(&priv->tx_submitted);
 	init_completion(&priv->start_comp);
 	init_completion(&priv->stop_comp);
 
-	init_usb_anchor(&priv->tx_submitted);
-	atomic_set(&priv->active_tx_urbs, 0);
-
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
-
 	priv->dev = dev;
 	priv->netdev = netdev;
 	priv->channel = channel;
 
+	spin_lock_init(&priv->tx_contexts_lock);
+	kvaser_usb_reset_tx_urb_contexts(priv);
+
 	priv->can.state = CAN_STATE_STOPPED;
 	priv->can.clock.freq = CAN_USB_CLOCK;
 	priv->can.bittiming_const = &kvaser_usb_bittiming_const;
-- 
1.9.1

[PATCH v4 2/3] can: kvaser_usb: Utilize all possible tx URBs

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

The driver currently limits the number of outstanding, not yet
ACKed, transfers to 16 URBs. Meanwhile, the Kvaser firmware
provides its actual max supported number of outstanding
transmissions in its reply to the CMD_GET_SOFTWARE_INFO message.

One example is the UsbCan-II HS/LS device which reports support
of up to 48 tx URBs instead of just 16, increasing the driver
throughput by two-fold and reducing the possibility of -ENOBUFs.

Dynamically set the max tx URBs value according to firmware
replies.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 64 ++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 25 deletions(-)

Changelog v4: Allocate the now-dynamically-sized tx_contexts[]
array as a C99 "flexible array member", insuring automatic proper
de-allocation on driver exit.

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index e97a08c..60eadf5 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -25,7 +25,6 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 
-#define MAX_TX_URBS			16
 #define MAX_RX_URBS			4
 #define START_TIMEOUT			1000 /* msecs */
 #define STOP_TIMEOUT			1000 /* msecs */
@@ -443,6 +442,7 @@ struct kvaser_usb_error_summary {
 	};
 };
 
+/* Context for an outstanding, not yet ACKed, transmission */
 struct kvaser_usb_tx_urb_context {
 	struct kvaser_usb_net_priv *priv;
 	u32 echo_index;
@@ -456,8 +456,13 @@ struct kvaser_usb {
 	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
 	struct usb_anchor rx_submitted;
 
+	/* @max_tx_urbs: Firmware-reported maximum number of oustanding,
+	 * not yet ACKed, transmissions on this device. This value is
+	 * also used as a sentinel for marking free tx contexts.
+	 */
 	u32 fw_version;
 	unsigned int nchannels;
+	unsigned int max_tx_urbs;
 	enum kvaser_usb_family family;
 
 	bool rxinitdone;
@@ -467,19 +472,18 @@ struct kvaser_usb {
 
 struct kvaser_usb_net_priv {
 	struct can_priv can;
-
-	spinlock_t tx_contexts_lock;
-	int active_tx_contexts;
-	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
-
-	struct usb_anchor tx_submitted;
-	struct completion start_comp, stop_comp;
+	struct can_berr_counter bec;
 
 	struct kvaser_usb *dev;
 	struct net_device *netdev;
 	int channel;
 
-	struct can_berr_counter bec;
+	struct completion start_comp, stop_comp;
+	struct usb_anchor tx_submitted;
+
+	spinlock_t tx_contexts_lock;
+	int active_tx_contexts;
+	struct kvaser_usb_tx_urb_context tx_contexts[];
 };
 
 static const struct usb_device_id kvaser_usb_table[] = {
@@ -657,9 +661,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
 	switch (dev->family) {
 	case KVASER_LEAF:
 		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
 		break;
 	case KVASER_USBCAN:
 		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
 		break;
 	}
 
@@ -715,7 +723,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats = &priv->netdev->stats;
 
-	context = &priv->tx_contexts[tid % MAX_TX_URBS];
+	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
 
 	/* Sometimes the state change doesn't come after a bus-off event */
 	if (priv->can.restart_ms &&
@@ -744,7 +752,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 	can_get_echo_skb(priv->netdev, context->echo_index);
-	context->echo_index = MAX_TX_URBS;
+	context->echo_index = dev->max_tx_urbs;
 	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
 
@@ -1512,11 +1520,13 @@ error:
 
 static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
 {
-	int i;
+	int i, max_tx_urbs;
+
+	max_tx_urbs = priv->dev->max_tx_urbs;
 
 	priv->active_tx_contexts = 0;
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+	for (i = 0; i < max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = max_tx_urbs;
 }
 
 /* This method might sleep. Do not call it in the atomic context
@@ -1702,14 +1712,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
-		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
+	for (i = 0; i < dev->max_tx_urbs; i++) {
+		if (priv->tx_contexts[i].echo_index == dev->max_tx_urbs) {
 			context = &priv->tx_contexts[i];
 
 			context->echo_index = i;
 			can_put_echo_skb(skb, netdev, context->echo_index);
 			++priv->active_tx_contexts;
-			if (priv->active_tx_contexts >= MAX_TX_URBS)
+			if (priv->active_tx_contexts >= dev->max_tx_urbs)
 				netif_stop_queue(netdev);
 
 			break;
@@ -1743,7 +1753,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 		can_free_echo_skb(netdev, context->echo_index);
-		context->echo_index = MAX_TX_URBS;
+		context->echo_index = dev->max_tx_urbs;
 		--priv->active_tx_contexts;
 		netif_wake_queue(netdev);
 
@@ -1881,7 +1891,9 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	if (err)
 		return err;
 
-	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	netdev = alloc_candev(sizeof(*priv) +
+			      dev->max_tx_urbs * sizeof(*priv->tx_contexts),
+			      dev->max_tx_urbs);
 	if (!netdev) {
 		dev_err(&intf->dev, "Cannot alloc candev\n");
 		return -ENOMEM;
@@ -1928,7 +1940,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 		return err;
 	}
 
-	netdev_dbg(netdev, "device registered\n");
+	netdev_info(netdev, "device registered\n");
 
 	return 0;
 }
@@ -2009,6 +2021,13 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
+	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
+		((dev->fw_version >> 24) & 0xff),
+		((dev->fw_version >> 16) & 0xff),
+		(dev->fw_version & 0xffff));
+
+	dev_dbg(&intf->dev, "Max oustanding tx = %d URBs\n", dev->max_tx_urbs);
+
 	err = kvaser_usb_get_card_info(dev);
 	if (err) {
 		dev_err(&intf->dev,
@@ -2016,11 +2035,6 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
-	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
-		((dev->fw_version >> 24) & 0xff),
-		((dev->fw_version >> 16) & 0xff),
-		(dev->fw_version & 0xffff));
-
 	for (i = 0; i < dev->nchannels; i++) {
 		err = kvaser_usb_init_one(intf, id, i);
 		if (err) {
-- 
1.9.1

[PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Use can-dev's unregister_candev() instead of directly calling
networking unregister_netdev(). While both are functionally
equivalent, unregister_candev() might do extra stuff in the
future than just calling networking layer unregistration code.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 60eadf5..d44fb1e 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1866,7 +1866,7 @@ static void kvaser_usb_remove_interfaces(struct kvaser_usb *dev)
 		if (!dev->nets[i])
 			continue;
 
-		unregister_netdev(dev->nets[i]->netdev);
+		unregister_candev(dev->nets[i]->netdev);
 	}
 
 	kvaser_usb_unlink_all_urbs(dev);
-- 
1.9.1

Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Marc Kleine-Budde]

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

On 03/14/2015 02:02 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> A number of tx queue wake-up events went missing due to the
> outlined scenario below. Start state is a pool of 16 tx URBs,
> active tx_urbs count = 15, with the netdev tx queue open.
> 
> CPU #1 [softirq]                         CPU #2 [softirq]
> start_xmit()                             tx_acknowledge()
> ................                         ................
> 
> atomic_inc(&tx_urbs);
> if (atomic_read(&tx_urbs) >= 16) {
>                         -->
>                                          atomic_dec(&tx_urbs);
>                                          netif_wake_queue();
>                                          return;
>                         <--
>     netif_stop_queue();
> }
> 
> At the end, the correct state expected is a 15 tx_urbs count
> value with the tx queue state _open_. Due to the race, we get
> the same tx_urbs value but with the tx queue state _stopped_.
> The wake-up event is completely lost.
> 
> Thus avoid hand-rolled concurrency mechanisms and use a proper
> lock for contexts and tx queue protection.
> 
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Applied to can. This will go into David's net tree and finally into
net-next. Then I'll apply patches 2+3. Nag me, if I forget about them ;)

Thanks,
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: 819 bytes --]

according net pull-request - was Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Oliver Hartkopp]

Hi Marc,

On 03/14/2015 02:41 PM, Marc Kleine-Budde wrote:
> Applied to can. This will go into David's net tree and finally into
> net-next. Then I'll apply patches 2+3. Nag me, if I forget about them ;)

Stephane sent an oot-driver to me for the new CAN FD USB adapter driver.
I splitted up this driver into three patches for a review from Stephane.

I would suggest to wait until Tuesday so that we can add these three patches.

I tested the patches and they work as expected. But Stephane should take a
final look at them.

Would it make sense to post the splitted patches on linux-can right now?

Regards,
Oliver

Re: according net pull-request - was Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Marc Kleine-Budde]

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

On 03/14/2015 03:02 PM, Oliver Hartkopp wrote:
> Hi Marc,
> 
> On 03/14/2015 02:41 PM, Marc Kleine-Budde wrote:
>> Applied to can. This will go into David's net tree and finally into
>> net-next. Then I'll apply patches 2+3. Nag me, if I forget about them ;)
> 
> Stephane sent an oot-driver to me for the new CAN FD USB adapter driver.
> I splitted up this driver into three patches for a review from Stephane.
> 
> I would suggest to wait until Tuesday so that we can add these three patches.

You mean for the pull request? I'll be quite busy until Thursday, for
#reasons, maybe the whole coming week. So I'll send this pull request
and make another one after Stephane's review and of next week.

> I tested the patches and they work as expected. But Stephane should take a
> final look at them.
> 
> Would it make sense to post the splitted patches on linux-can right now?

Sure - Fire at will.

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: 819 bytes --]

Re: [PATCH 2/5] can: kvaser_usb: Read all messages in a bulk-in URB buffer

[Marc Kleine-Budde]

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

On 02/26/2015 04:22 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> The Kvaser firmware can only read and write messages that are
> not crossing the USB endpoint's wMaxPacketSize boundary. While
> receiving commands from the CAN device, if the next command in
> the same URB buffer crossed that max packet size boundary, the
> firmware puts a zero-length placeholder command in its place
> then moves the real command to the next boundary mark.
> 
> The driver did not recognize such behavior, leading to missing
> a good number of rx events during a heavy rx load session.
> 
> Moreover, a tx URB context only gets freed upon receiving its
> respective tx ACK event. Over time, the free tx URB contexts
> pool gets depleted due to the missing ACK events. Consequently,
> the netif transmission queue gets __permanently__ stopped; no
> frames could be sent again except after restarting the CAN
> newtwork interface.
> 
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Can you please send a fix for this endianess errors (against
linux-can-fixes-for-4.0-20150314):

> drivers/net/can/usb/kvaser_usb.c:595:39: warning: restricted __le16 degrades to integer
> drivers/net/can/usb/kvaser_usb.c:1332:31: warning: restricted __le16 degrades to integer

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: 819 bytes --]

Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

Hi Marc,

On Sat, Mar 14, 2015 at 02:41:18PM +0100, Marc Kleine-Budde wrote:
> On 03/14/2015 02:02 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > A number of tx queue wake-up events went missing due to the
> > outlined scenario below. Start state is a pool of 16 tx URBs,
> > active tx_urbs count = 15, with the netdev tx queue open.
> > 
> > CPU #1 [softirq]                         CPU #2 [softirq]
> > start_xmit()                             tx_acknowledge()
> > ................                         ................
> > 
> > atomic_inc(&tx_urbs);
> > if (atomic_read(&tx_urbs) >= 16) {
> >                         -->
> >                                          atomic_dec(&tx_urbs);
> >                                          netif_wake_queue();
> >                                          return;
> >                         <--
> >     netif_stop_queue();
> > }
> > 
> > At the end, the correct state expected is a 15 tx_urbs count
> > value with the tx queue state _open_. Due to the race, we get
> > the same tx_urbs value but with the tx queue state _stopped_.
> > The wake-up event is completely lost.
> > 
> > Thus avoid hand-rolled concurrency mechanisms and use a proper
> > lock for contexts and tx queue protection.
> > 
> > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> Applied to can. This will go into David's net tree and finally into
> net-next. Then I'll apply patches 2+3. Nag me, if I forget about them ;)
> 

Thanks! :-)

So if I've understood correctly, this patch will go to -rc5 and
the rest will go into net-next?

If so, IMHO patch #2 should also go to -rc5. I know it's usually
frowned up on to add further patches at this late -rc stage, but
here's my logic:

The original driver code just _arbitrarily_ assumed a MAX_TX_URB
value of 16 parallel transmissions. This value was chosen, it seems,
because the driver was heavily based on esd_usb2.c and the esd code
just did so :-(

Meanwhile, in the Kvaser hardware at hand, if I've increased the
driver's max parallel transmissions little above the recommended
limit reported by firmware, the firmware breaks up badly, reports a
massive list of internal errors, and the candump traces becomes
sort of an internal mess hardly related to the actual frames sent
and received.

In my case, I was lucky that the brand we own here (*) had a higher
max outstanding transmissions value than 16. But if there is hardware
out there with a max oustanding tx support < 16 (#), such hardware
will break badly under a heavy transmission load :-(

(*) http://www.kvaser.com/products/kvaser-usb-hs-ii-hsls/

(#) There are a huge list of Kvaser products having the same controller
    but with different performance metrics, so this is quite a
    possiblity.

Thanks,
Darwish

Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Marc Kleine-Budde]

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

On 03/14/2015 03:38 PM, Ahmed S. Darwish wrote:
>> Applied to can. This will go into David's net tree and finally into
>> net-next. Then I'll apply patches 2+3. Nag me, if I forget about them ;)
>>
> 
> Thanks! :-)
> 
> So if I've understood correctly, this patch will go to -rc5 and
> the rest will go into net-next?
> 
> If so, IMHO patch #2 should also go to -rc5. I know it's usually
> frowned up on to add further patches at this late -rc stage, but
> here's my logic:
> 
> The original driver code just _arbitrarily_ assumed a MAX_TX_URB
> value of 16 parallel transmissions. This value was chosen, it seems,
> because the driver was heavily based on esd_usb2.c and the esd code
> just did so :-(
> 
> Meanwhile, in the Kvaser hardware at hand, if I've increased the
> driver's max parallel transmissions little above the recommended
> limit reported by firmware, the firmware breaks up badly, reports a
> massive list of internal errors, and the candump traces becomes
> sort of an internal mess hardly related to the actual frames sent
> and received.

In this particular hardware, what's the limit as reported by the firmware?

> In my case, I was lucky that the brand we own here (*) had a higher
> max outstanding transmissions value than 16. But if there is hardware

Okay - higher.

> out there with a max oustanding tx support < 16 (#), such hardware
> will break badly under a heavy transmission load :-(

I see.

If you add this motivation to the patch description and let the subject
reflect that this is a "fix" or safety measure rather than a possible
performance improvement, no-one will say anything against this patch :D

> (*) http://www.kvaser.com/products/kvaser-usb-hs-ii-hsls/
> 
> (#) There are a huge list of Kvaser products having the same controller
>     but with different performance metrics, so this is quite a
>     possiblity.

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: 819 bytes --]

Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions

[Ahmed S. Darwish]

On Sat, Mar 14, 2015 at 03:58:39PM +0100, Marc Kleine-Budde wrote:
> On 03/14/2015 03:38 PM, Ahmed S. Darwish wrote:
> >> Applied to can. This will go into David's net tree and finally into
> >> net-next. Then I'll apply patches 2+3. Nag me, if I forget about them ;)
> >>
> > 
> > Thanks! :-)
> > 
> > So if I've understood correctly, this patch will go to -rc5 and
> > the rest will go into net-next?
> > 
> > If so, IMHO patch #2 should also go to -rc5. I know it's usually
> > frowned up on to add further patches at this late -rc stage, but
> > here's my logic:
> > 
> > The original driver code just _arbitrarily_ assumed a MAX_TX_URB
> > value of 16 parallel transmissions. This value was chosen, it seems,
> > because the driver was heavily based on esd_usb2.c and the esd code
> > just did so :-(
> > 
> > Meanwhile, in the Kvaser hardware at hand, if I've increased the
> > driver's max parallel transmissions little above the recommended
> > limit reported by firmware, the firmware breaks up badly, reports a
> > massive list of internal errors, and the candump traces becomes
> > sort of an internal mess hardly related to the actual frames sent
> > and received.
> 
> In this particular hardware, what's the limit as reported by the firmware?
> 

48 max oustanding tx, which is quite big in itself it seems.

Other drivers in the tree range between 10 (Peak) and 20 tx (8dev).

> > In my case, I was lucky that the brand we own here (*) had a higher
> > max outstanding transmissions value than 16. But if there is hardware
> 
> Okay - higher.
> 
> > out there with a max oustanding tx support < 16 (#), such hardware
> > will break badly under a heavy transmission load :-(
> 
> I see.
> 
> If you add this motivation to the patch description and let the subject
> reflect that this is a "fix" or safety measure rather than a possible
> performance improvement, no-one will say anything against this patch :D
> 

True; I admit the "fix" part should've been clearer :-)

Will send a better worded commit message then.

Thanks a lot,
Darwish

Re: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism

[Marc Kleine-Budde]

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

On 03/14/2015 02:11 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> Use can-dev's unregister_candev() instead of directly calling
> networking unregister_netdev(). While both are functionally
> equivalent, unregister_candev() might do extra stuff in the
> future than just calling networking layer unregistration code.

Since 2 goes into can, I've applied this into can-next.

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: 819 bytes --]

Re: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism

[Ahmed S. Darwish]

On Sat, Mar 14, 2015 at 04:26:56PM +0100, Marc Kleine-Budde wrote:
> On 03/14/2015 02:11 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > Use can-dev's unregister_candev() instead of directly calling
> > networking unregister_netdev(). While both are functionally
> > equivalent, unregister_candev() might do extra stuff in the
> > future than just calling networking layer unregistration code.
> 
> Since 2 goes into can, I've applied this into can-next.
> 

Merci.

Was this a cherry-pick? Because I was going to send a new
series with patch #2 better worded, and with a new patch for
the endiannes issue.

Regards,
Darwish

Re: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism

[Marc Kleine-Budde]

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

On 03/14/2015 04:41 PM, Ahmed S. Darwish wrote:
> On Sat, Mar 14, 2015 at 04:26:56PM +0100, Marc Kleine-Budde wrote:
>> On 03/14/2015 02:11 PM, Ahmed S. Darwish wrote:
>>> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>>>
>>> Use can-dev's unregister_candev() instead of directly calling
>>> networking unregister_netdev(). While both are functionally
>>> equivalent, unregister_candev() might do extra stuff in the
>>> future than just calling networking layer unregistration code.
>>
>> Since 2 goes into can, I've applied this into can-next.

> Was this a cherry-pick? Because I was going to send a new
> series with patch #2 better worded, and with a new patch for
> the endiannes issue.

Yes, no need to resend patch #3, as it's already applied to can-next.

regards,
Marc

1 = can: kvaser_usb: Fix tx queue start/stop race conditions
2 = can: kvaser_usb: Utilize all possible tx URBs
3 = can: kvaser_usb: Use can-dev unregistration mechanism
4 = the endianess issue

1 = is in linux-can and included in linux-can-fixes-for-4.0-20150314
2 = will go into linux-can with a better commit message
    which is currently prepare by you
    will be in the next pull request for net
3 = is in linux-can-next and will be included in the next pull request
    for net-next
4 = is currently prepared by you
    will be in the next pull request for net

This means, you'll send me patches 2 and 4 in a new v5 series. (This
patches will of course have new numbers, 1 and 2.)

-- 
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: 819 bytes --]

Re: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism

[Ahmed S. Darwish]

On Sat, Mar 14, 2015 at 04:55:11PM +0100, Marc Kleine-Budde wrote:
> On 03/14/2015 04:41 PM, Ahmed S. Darwish wrote:
> > On Sat, Mar 14, 2015 at 04:26:56PM +0100, Marc Kleine-Budde wrote:
> >> On 03/14/2015 02:11 PM, Ahmed S. Darwish wrote:
> >>> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> >>>
> >>> Use can-dev's unregister_candev() instead of directly calling
> >>> networking unregister_netdev(). While both are functionally
> >>> equivalent, unregister_candev() might do extra stuff in the
> >>> future than just calling networking layer unregistration code.
> >>
> >> Since 2 goes into can, I've applied this into can-next.
> 
> > Was this a cherry-pick? Because I was going to send a new
> > series with patch #2 better worded, and with a new patch for
> > the endiannes issue.
> 
> Yes, no need to resend patch #3, as it's already applied to can-next.
> 
> regards,
> Marc
> 
> 1 = can: kvaser_usb: Fix tx queue start/stop race conditions
> 2 = can: kvaser_usb: Utilize all possible tx URBs
> 3 = can: kvaser_usb: Use can-dev unregistration mechanism
> 4 = the endianess issue
> 
> 1 = is in linux-can and included in linux-can-fixes-for-4.0-20150314
> 2 = will go into linux-can with a better commit message
>     which is currently prepare by you
>     will be in the next pull request for net
> 3 = is in linux-can-next and will be included in the next pull request
>     for net-next
> 4 = is currently prepared by you
>     will be in the next pull request for net
> 
> This means, you'll send me patches 2 and 4 in a new v5 series. (This
> patches will of course have new numbers, 1 and 2.)
> 

A piece of cake :D

[PATCH v5 1/2] can: kvaser_usb: Comply with firmware max tx URBs value

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

Current driver code arbitrarily assumes a max outstanding tx
value of 16 parallel transmissions. Meanwhile, the device
firmware provides its actual maximum inside its reply to the
CMD_GET_SOFTWARE_INFO message.

Under heavy tx traffic, if the interleaved transmissions count
increases above the limit reported by firmware, the firmware
breaks up badly, reports a massive list of internal errors, and
the candump traces hardly matches the actual frames sent and
received.

On the other hand, in certain models, the firmware can support
up to 48 tx URBs instead of just 16, increasing the driver
throughput by two-fold and reducing the possibility of -ENOBUFs.

Thus dynamically set the driver's max tx URBs value according
to firmware replies.

Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 64 ++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index e97a08c..60eadf5 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -25,7 +25,6 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 
-#define MAX_TX_URBS			16
 #define MAX_RX_URBS			4
 #define START_TIMEOUT			1000 /* msecs */
 #define STOP_TIMEOUT			1000 /* msecs */
@@ -443,6 +442,7 @@ struct kvaser_usb_error_summary {
 	};
 };
 
+/* Context for an outstanding, not yet ACKed, transmission */
 struct kvaser_usb_tx_urb_context {
 	struct kvaser_usb_net_priv *priv;
 	u32 echo_index;
@@ -456,8 +456,13 @@ struct kvaser_usb {
 	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
 	struct usb_anchor rx_submitted;
 
+	/* @max_tx_urbs: Firmware-reported maximum number of oustanding,
+	 * not yet ACKed, transmissions on this device. This value is
+	 * also used as a sentinel for marking free tx contexts.
+	 */
 	u32 fw_version;
 	unsigned int nchannels;
+	unsigned int max_tx_urbs;
 	enum kvaser_usb_family family;
 
 	bool rxinitdone;
@@ -467,19 +472,18 @@ struct kvaser_usb {
 
 struct kvaser_usb_net_priv {
 	struct can_priv can;
-
-	spinlock_t tx_contexts_lock;
-	int active_tx_contexts;
-	struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
-
-	struct usb_anchor tx_submitted;
-	struct completion start_comp, stop_comp;
+	struct can_berr_counter bec;
 
 	struct kvaser_usb *dev;
 	struct net_device *netdev;
 	int channel;
 
-	struct can_berr_counter bec;
+	struct completion start_comp, stop_comp;
+	struct usb_anchor tx_submitted;
+
+	spinlock_t tx_contexts_lock;
+	int active_tx_contexts;
+	struct kvaser_usb_tx_urb_context tx_contexts[];
 };
 
 static const struct usb_device_id kvaser_usb_table[] = {
@@ -657,9 +661,13 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
 	switch (dev->family) {
 	case KVASER_LEAF:
 		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.leaf.softinfo.max_outstanding_tx);
 		break;
 	case KVASER_USBCAN:
 		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+		dev->max_tx_urbs =
+			le16_to_cpu(msg.u.usbcan.softinfo.max_outstanding_tx);
 		break;
 	}
 
@@ -715,7 +723,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 
 	stats = &priv->netdev->stats;
 
-	context = &priv->tx_contexts[tid % MAX_TX_URBS];
+	context = &priv->tx_contexts[tid % dev->max_tx_urbs];
 
 	/* Sometimes the state change doesn't come after a bus-off event */
 	if (priv->can.restart_ms &&
@@ -744,7 +752,7 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 	can_get_echo_skb(priv->netdev, context->echo_index);
-	context->echo_index = MAX_TX_URBS;
+	context->echo_index = dev->max_tx_urbs;
 	--priv->active_tx_contexts;
 	netif_wake_queue(priv->netdev);
 
@@ -1512,11 +1520,13 @@ error:
 
 static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)
 {
-	int i;
+	int i, max_tx_urbs;
+
+	max_tx_urbs = priv->dev->max_tx_urbs;
 
 	priv->active_tx_contexts = 0;
-	for (i = 0; i < MAX_TX_URBS; i++)
-		priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+	for (i = 0; i < max_tx_urbs; i++)
+		priv->tx_contexts[i].echo_index = max_tx_urbs;
 }
 
 /* This method might sleep. Do not call it in the atomic context
@@ -1702,14 +1712,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		*msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
 
 	spin_lock_irqsave(&priv->tx_contexts_lock, flags);
-	for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
-		if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
+	for (i = 0; i < dev->max_tx_urbs; i++) {
+		if (priv->tx_contexts[i].echo_index == dev->max_tx_urbs) {
 			context = &priv->tx_contexts[i];
 
 			context->echo_index = i;
 			can_put_echo_skb(skb, netdev, context->echo_index);
 			++priv->active_tx_contexts;
-			if (priv->active_tx_contexts >= MAX_TX_URBS)
+			if (priv->active_tx_contexts >= dev->max_tx_urbs)
 				netif_stop_queue(netdev);
 
 			break;
@@ -1743,7 +1753,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
 		spin_lock_irqsave(&priv->tx_contexts_lock, flags);
 
 		can_free_echo_skb(netdev, context->echo_index);
-		context->echo_index = MAX_TX_URBS;
+		context->echo_index = dev->max_tx_urbs;
 		--priv->active_tx_contexts;
 		netif_wake_queue(netdev);
 
@@ -1881,7 +1891,9 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 	if (err)
 		return err;
 
-	netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+	netdev = alloc_candev(sizeof(*priv) +
+			      dev->max_tx_urbs * sizeof(*priv->tx_contexts),
+			      dev->max_tx_urbs);
 	if (!netdev) {
 		dev_err(&intf->dev, "Cannot alloc candev\n");
 		return -ENOMEM;
@@ -1928,7 +1940,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
 		return err;
 	}
 
-	netdev_dbg(netdev, "device registered\n");
+	netdev_info(netdev, "device registered\n");
 
 	return 0;
 }
@@ -2009,6 +2021,13 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
+	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
+		((dev->fw_version >> 24) & 0xff),
+		((dev->fw_version >> 16) & 0xff),
+		(dev->fw_version & 0xffff));
+
+	dev_dbg(&intf->dev, "Max oustanding tx = %d URBs\n", dev->max_tx_urbs);
+
 	err = kvaser_usb_get_card_info(dev);
 	if (err) {
 		dev_err(&intf->dev,
@@ -2016,11 +2035,6 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		return err;
 	}
 
-	dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
-		((dev->fw_version >> 24) & 0xff),
-		((dev->fw_version >> 16) & 0xff),
-		(dev->fw_version & 0xffff));
-
 	for (i = 0; i < dev->nchannels; i++) {
 		err = kvaser_usb_init_one(intf, id, i);
 		if (err) {
-- 
1.9.1

[PATCH v5 2/2] can: kvaser_usb: Fix sparse warning __le16 degrades to integer

[Ahmed S. Darwish]

From: Ahmed S. Darwish <ahmed.darwish@valeo.com>

USB endpoint's wMaxPacketSize field is an le16 entity. Use
appropriate le16_to_cpu macros to maintain endian independence.

Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
 drivers/net/can/usb/kvaser_usb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

I wasn't successful in making sparse trigger the warning, even
while using the latest version from the git repos and doing:

make C=2 M=drivers/net/can/usb

So, Marc, I hope this patch fixes the issue reported; it should.

thanks,

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 60eadf5..d924016 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -596,8 +596,8 @@ static int kvaser_usb_wait_msg(const struct kvaser_usb *dev, u8 id,
 			 * for further details.
 			 */
 			if (tmp->len == 0) {
-				pos = round_up(pos,
-					       dev->bulk_in->wMaxPacketSize);
+				pos = round_up(pos, le16_to_cpu(dev->bulk_in->
+								wMaxPacketSize));
 				continue;
 			}
 
@@ -1337,7 +1337,8 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
 		 * number of events in case of a heavy rx load on the bus.
 		 */
 		if (msg->len == 0) {
-			pos = round_up(pos, dev->bulk_in->wMaxPacketSize);
+			pos = round_up(pos, le16_to_cpu(dev->bulk_in->
+							wMaxPacketSize));
 			continue;
 		}
 
-- 
1.9.1

Re: [PATCH v5 1/2] can: kvaser_usb: Comply with firmware max tx URBs value

[Marc Kleine-Budde]

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

On 03/15/2015 04:03 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> Current driver code arbitrarily assumes a max outstanding tx
> value of 16 parallel transmissions. Meanwhile, the device
> firmware provides its actual maximum inside its reply to the
> CMD_GET_SOFTWARE_INFO message.
> 
> Under heavy tx traffic, if the interleaved transmissions count
> increases above the limit reported by firmware, the firmware
> breaks up badly, reports a massive list of internal errors, and
> the candump traces hardly matches the actual frames sent and
> received.
> 
> On the other hand, in certain models, the firmware can support
> up to 48 tx URBs instead of just 16, increasing the driver
> throughput by two-fold and reducing the possibility of -ENOBUFs.
> 
> Thus dynamically set the driver's max tx URBs value according
> to firmware replies.
> 
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>

> @@ -1928,7 +1940,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
>  		return err;
>  	}
>  
> -	netdev_dbg(netdev, "device registered\n");
> +	netdev_info(netdev, "device registered\n");

This makes the driver more noisy, I'd like to drop that hunk, okay? No
need to resend.

regards,
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: 819 bytes --]

Re: [PATCH v5 1/2] can: kvaser_usb: Comply with firmware max tx URBs value

[Ahmed S. Darwish]

On Sun, Mar 15, 2015 at 07:08:23PM +0100, Marc Kleine-Budde wrote:
> On 03/15/2015 04:03 PM, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > 
> > Current driver code arbitrarily assumes a max outstanding tx
> > value of 16 parallel transmissions. Meanwhile, the device
> > firmware provides its actual maximum inside its reply to the
> > CMD_GET_SOFTWARE_INFO message.
> > 
> > Under heavy tx traffic, if the interleaved transmissions count
> > increases above the limit reported by firmware, the firmware
> > breaks up badly, reports a massive list of internal errors, and
> > the candump traces hardly matches the actual frames sent and
> > received.
> > 
> > On the other hand, in certain models, the firmware can support
> > up to 48 tx URBs instead of just 16, increasing the driver
> > throughput by two-fold and reducing the possibility of -ENOBUFs.
> > 
> > Thus dynamically set the driver's max tx URBs value according
> > to firmware replies.
> > 
> > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> 
> > @@ -1928,7 +1940,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
> >  		return err;
> >  	}
> >  
> > -	netdev_dbg(netdev, "device registered\n");
> > +	netdev_info(netdev, "device registered\n");
> 
> This makes the driver more noisy, I'd like to drop that hunk, okay? No
> need to resend.
> 

Sure, go ahead.

I have my reasons for that hunk above, but we can always discuss
this in another separate patch ;-)

Thanks,
Darwish

Re: [PATCH v5 1/2] can: kvaser_usb: Comply with firmware max tx URBs value

[Marc Kleine-Budde]

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

On 03/16/2015 01:16 PM, Ahmed S. Darwish wrote:
>>> -	netdev_dbg(netdev, "device registered\n");
>>> +	netdev_info(netdev, "device registered\n");
>>
>> This makes the driver more noisy, I'd like to drop that hunk, okay? No
>> need to resend.

> Sure, go ahead.

Thanks.

> I have my reasons for that hunk above, but we can always discuss
> this in another separate patch ;-)

Don't try to sneak in unrelated changes :) I'd like to have the drivers
more quiet these days, even when doing ifup, ifdown.

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: 819 bytes --]