From: Ahmed S. Darwish <[email protected]>
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 <[email protected]>
---
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
From: Ahmed S. Darwish <[email protected]>
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 <[email protected]>
---
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
From: Ahmed S. Darwish <[email protected]>
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 <[email protected]>
---
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
From: Ahmed S. Darwish <[email protected]>
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 <[email protected]>
---
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
From: Ahmed S. Darwish <[email protected]>
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 <[email protected]>
---
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