can: kvaser_usb: Avoid double free on URB submission failures
Subsystems (3)
PATCH 1/5 can: kvaser_usb: Avoid double free on URB submission failures
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().
--- drivers/net/can/usb/kvaser_usb.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/net/can/usb/kvaser_usb.c +8 -12
@@ -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; }
Signature · 1 line
1.9.1
PATCH 2/5 can: kvaser_usb: Read all messages in a bulk-in URB buffer
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.
--- drivers/net/can/usb/kvaser_usb.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
drivers/net/can/usb/kvaser_usb.c +23 -5
@@ -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; }
Signature · 1 line
1.9.1
PATCH 3/5 can: kvaser_usb: Utilize all possible tx URBs
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.
--- drivers/net/can/usb/kvaser_usb.c | 62 ++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 22 deletions(-)
drivers/net/can/usb/kvaser_usb.c +40 -21
@@ -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 4/5 can: kvaser_usb: Use can-dev unregistration mechanism
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.
--- drivers/net/can/usb/kvaser_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb.c +1 -1
@@ -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);
Signature · 1 line
1.9.1
PATCH 5/5 can: kvaser_usb: Fix tx queue start/stop race conditions
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.
--- drivers/net/can/usb/kvaser_usb.c | 82 +++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 31 deletions(-)
drivers/net/can/usb/kvaser_usb.c +51 -31
@@ -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 ;
Signature · 1 line
1.9.1
Marc Kleine-Budde
maintainerRe: [PATCH 2/5] can: kvaser_usb: Read all messages in a bulk-in URB buffer
In reply to Ahmed S. DarwishQuoted 12 lines · Ahmed S. Darwish
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.
Quoted 8 lines
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]>
Can you please send a fix for this endianess errors (against linux-can-fixes-for-4.0-20150314):
Quoted 2 lines
drivers/net/can/usb/kvaser_usb.c:595:39: warning: restricted__le16degrades to integerdrivers/net/can/usb/kvaser_usb.c:1332:31: warning: restricted__le16degrades to integer
Marc
Signature · 4 lines
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 |
Marc Kleine-Budde
maintainerQuoted 10 lines · Ahmed S. Darwish
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. Meanwhileusb_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]>
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
Signature · 4 lines
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 |
Hi Marc,
(Sorry for the late reply as I was out of town!)
Quoted 10 lines · Marc Kleine-Budde
Quoted 9 lines · Ahmed S. Darwish
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. Meanwhileusb_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().
Quoted 8 lines
Quoted 1 line
Signed-off-by Ahmed S. Darwish <[email protected]>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
Marc Kleine-Budde
maintainerQuoted 1 line · Ahmed S. Darwish
(Sorry for the late reply as I was out of town!)
np :)
Quoted 17 lines · Marc Kleine-Budde
Quoted 16 lines · Ahmed S. Darwish
Quoted 10 lines
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. Meanwhileusb_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]>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.
Quoted 7 lines
Quoted 1 line
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.
Quoted 4 lines
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
Signature · 4 lines
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 |
PATCH v2 1/3 can: kvaser_usb: Fix tx queue start/stop race conditions
In reply to Ahmed S. DarwishFrom: 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.
--- 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!
drivers/net/can/usb/kvaser_usb.c +50 -32
@@ -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;
Signature · 1 line
1.9.1
PATCH v2 2/3 can: kvaser_usb: Utilize all possible tx URBs
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.
--- 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 ;-)
drivers/net/can/usb/kvaser_usb.c +37 -18
@@ -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) {
Signature · 1 line
1.9.1
PATCH v2 3/3 can: kvaser_usb: Use can-dev unregistration mechanism
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.
--- drivers/net/can/usb/kvaser_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb.c +1 -1
@@ -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);
Signature · 1 line
1.9.1
Marc Kleine-Budde
maintainerRe: [PATCH v2 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
In reply to Ahmed S. DarwishQuoted 20 lines · Ahmed S. Darwish
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_urbscount = 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(); }
Quoted 7 lines
At the end, the correct state expected is a 15
tx_urbscount value with the tx queue state _open_. Due to the race, we get the sametx_urbsvalue 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
Signature · 4 lines
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 |
Re: [PATCH v2 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
Quoted 26 lines · Marc Kleine-Budde
Quoted 25 lines · Ahmed S. Darwish
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_urbscount = 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_urbscount value with the tx queue state _open_. Due to the race, we get the sametx_urbsvalue but with the tx queue state _stopped_. The wake-up event is completely lost.
Quoted 6 lines
Quoted 2 lines
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
In reply to Ahmed S. DarwishFrom: 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.
--- 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.
drivers/net/can/usb/kvaser_usb.c +51 -32
@@ -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;
Signature · 1 line
1.9.1
PATCH v3 2/3 can: kvaser_usb: Utilize all possible tx URBs
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.
--- drivers/net/can/usb/kvaser_usb.c | 55 +++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 18 deletions(-)
changelog-v3: No changes
drivers/net/can/usb/kvaser_usb.c +37 -18
@@ -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) {
Signature · 1 line
1.9.1
PATCH v3 3/3 can: kvaser_usb: Use can-dev unregistration mechanism
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.
--- drivers/net/can/usb/kvaser_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb.c +1 -1
@@ -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);
Signature · 1 line
1.9.1
Marc Kleine-Budde
maintainerRe: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs
In reply to Ahmed S. DarwishQuoted 142 lines · Ahmed S. Darwish
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_INFOmessage.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 | 55 +++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 18 deletions(-)changelog-v3: No changes
drivers/net/can/usb/kvaser_usb.c +22
-12@@ -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 ;
drivers/net/can/usb/kvaser_usb.c static int kvaser_usb_init_one(struct usb_interface *intf, · line 1907
Quoted 9 lines
drivers/net/can/usb/kvaser_usb.c +6
-0@@ -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
Signature · 4 lines
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 |
Re: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs
Quoted 146 lines · Marc Kleine-Budde
Quoted 145 lines · Ahmed S. Darwish
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_INFOmessage.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 | 55 +++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 18 deletions(-)changelog-v3: No changes
drivers/net/can/usb/kvaser_usb.c +22
-12@@ -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);
Quoted 9 lines
drivers/net/can/usb/kvaser_usb.c static int kvaser_usb_init_one(struct usb_interface *intf, · line 1907
Quoted 6 lines
drivers/net/can/usb/kvaser_usb.c +6
-0+ 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.
Quoted 9 lines
1) use
devm_kzalloc(), or
2) move structkvaser_usb_tx_urb_contexttx_contexts[]; to the end of structkvaser_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...
Quoted 4 lines
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
Marc Kleine-Budde
maintainerRe: [PATCH v3 2/3] can: kvaser_usb: Utilize all possible tx URBs
Quoted 35 lines · Ahmed S. Darwish
Quoted 13 lines
Quoted 13 lines
drivers/net/can/usb/kvaser_usb.c +1
-1@@ -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);Quoted 9 lines
drivers/net/can/usb/kvaser_usb.c static int kvaser_usb_init_one(struct usb_interface *intf, · line 1907
Quoted 6 lines
drivers/net/can/usb/kvaser_usb.c +6
-0+ 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.
Quoted 9 lines
1) use
devm_kzalloc(), or
2) move structkvaser_usb_tx_urb_contexttx_contexts[]; to the end of structkvaser_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))
Quoted 3 lines
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 theprobe()method fails with-ENODEVand 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.
Quoted 4 lines
Quoted 1 line
[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
Signature · 4 lines
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 |
Marc Kleine-Budde
maintainerRe: [PATCH v3 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
In reply to Ahmed S. DarwishQuoted 79 lines · Ahmed S. Darwish
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_urbscount = 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_urbscount value with the tx queue state _open_. Due to the race, we get the sametx_urbsvalue 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 | 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.
drivers/net/can/usb/kvaser_usb.c +5
-3@@ -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);
drivers/net/can/usb/kvaser_usb.c static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev, · line 751
Quoted 10 lines
drivers/net/can/usb/kvaser_usb.c +6
-2- 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?
Quoted 95 lines
drivers/net/can/usb/kvaser_usb.c +28
-19}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); -
drivers/net/can/usb/kvaser_usb.c static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, · line 1750
Quoted 10 lines
drivers/net/can/usb/kvaser_usb.c +7
-0err =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?
Quoted 29 lines
drivers/net/can/usb/kvaser_usb.c +2
-8-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 ; -
drivers/net/can/usb/kvaser_usb.c static int kvaser_usb_init_one(struct usb_interface *intf, · line 1905
Quoted 10 lines
drivers/net/can/usb/kvaser_usb.c +3
-0priv->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
Signature · 4 lines
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 |
Re: [PATCH v3 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
Hi Marc,
Quoted 84 lines · Marc Kleine-Budde
Quoted 83 lines · Ahmed S. Darwish
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_urbscount = 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_urbscount value with the tx queue state _open_. Due to the race, we get the sametx_urbsvalue 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 | 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.
drivers/net/can/usb/kvaser_usb.c +6
-5@@ -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);
Quoted 9 lines
drivers/net/can/usb/kvaser_usb.c static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev, · line 751
Quoted 6 lines
drivers/net/can/usb/kvaser_usb.c +5
-0+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 &); doneAnd 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
In reply to Ahmed S. DarwishFrom: 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.
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.
--- 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
drivers/net/can/usb/kvaser_usb.c +51 -32
@@ -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;
Signature · 1 line
1.9.1
PATCH v4 2/3 can: kvaser_usb: Utilize all possible tx URBs
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.
--- 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.
drivers/net/can/usb/kvaser_usb.c +39 -25
@@ -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) {
Signature · 1 line
1.9.1
PATCH v4 3/3 can: kvaser_usb: Use can-dev unregistration mechanism
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.
--- drivers/net/can/usb/kvaser_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb.c +1 -1
@@ -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);
Signature · 1 line
1.9.1
Marc Kleine-Budde
maintainerRe: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism
Quoted 6 lines · Ahmed S. Darwish
From: Ahmed S. Darwish <[email protected]>
Use can-dev's
unregister_candev()instead of directly calling networkingunregister_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
Signature · 4 lines
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 |
Re: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism
Quoted 10 lines · Marc Kleine-Budde
Quoted 6 lines · Ahmed S. Darwish
From: Ahmed S. Darwish <[email protected]>
Use can-dev's
unregister_candev()instead of directly calling networkingunregister_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
Marc Kleine-Budde
maintainerRe: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism
Quoted 9 lines · Ahmed S. Darwish
Quoted 8 lines · Marc Kleine-Budde
Quoted 6 lines · Ahmed S. Darwish
From: Ahmed S. Darwish <[email protected]>
Use can-dev's
unregister_candev()instead of directly calling networkingunregister_netdev(). While both are functionally equivalent,unregister_candev()might do extra stuff in the future than just calling networking layer unregistration code.
Quoted 5 lines
Quoted 1 line
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.)
Signature · 4 lines
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 |
Re: [PATCH v4 3/3] can: kvaser_usb: Use can-dev unregistration mechanism
Quoted 35 lines · Marc Kleine-Budde
Quoted 9 lines · Ahmed S. Darwish
Quoted 8 lines · Marc Kleine-Budde
Quoted 6 lines · Ahmed S. Darwish
From: Ahmed S. Darwish <[email protected]>
Use can-dev's
unregister_candev()instead of directly calling networkingunregister_netdev(). While both are functionally equivalent,unregister_candev()might do extra stuff in the future than just calling networking layer unregistration code.Quoted 5 lines
Quoted 1 line
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,
Marc1 = 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 issue1 = 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
Quoted 3 lines
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
Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
In reply to Ahmed S. DarwishQuoted 25 lines · Ahmed S. Darwish
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_urbscount = 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_urbscount value with the tx queue state _open_. Due to the race, we get the sametx_urbsvalue but with the tx queue state _stopped_. The wake-up event is completely lost.
Quoted 4 lines
Thus avoid hand-rolled concurrency mechanisms and use a proper lock for contexts and tx queue protection.
Signed-off-by Ahmed S. Darwish <[email protected]>
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
Signature · 4 lines
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 |
according net pull-request - was Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
Hi Marc,
Quoted 2 lines · Marc Kleine-Budde
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
Marc Kleine-Budde
maintainerRe: according net pull-request - was Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
Quoted 10 lines · Oliver Hartkopp
Hi Marc,
Quoted 2 lines · Marc Kleine-Budde
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.
Quoted 4 lines
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
Signature · 4 lines
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 |
Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
In reply to Marc Kleine-BuddeHi Marc,
Quoted 26 lines · Marc Kleine-Budde
Quoted 25 lines · Ahmed S. Darwish
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_urbscount = 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_urbscount value with the tx queue state _open_. Due to the race, we get the sametx_urbsvalue but with the tx queue state _stopped_. The wake-up event is completely lost.
Quoted 8 lines
Quoted 4 lines
Thus avoid hand-rolled concurrency mechanisms and use a proper lock for contexts and tx queue protection.
Signed-off-by Ahmed S. Darwish <[email protected]>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
Marc Kleine-Budde
maintainerRe: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
Quoted 18 lines · Ahmed S. Darwish
Quoted 3 lines
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_URBvalue of 16 parallel transmissions. This value was chosen, it seems, because the driver was heavily based onesd_usb2.cand the esd code just did so :-(
Quoted 6 lines
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?
Quoted 2 lines
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.
Quoted 2 lines
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
Quoted 5 lines
(*) 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
Signature · 4 lines
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 |
Re: [PATCH v4 1/3] can: kvaser_usb: Fix tx queue start/stop race conditions
Quoted 19 lines · Marc Kleine-Budde
Quoted 18 lines · Ahmed S. Darwish
Quoted 3 lines
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_URBvalue of 16 parallel transmissions. This value was chosen, it seems, because the driver was heavily based onesd_usb2.cand the esd code just did so :-(
Quoted 9 lines
Quoted 6 lines
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).
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 2 lines
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
PATCH v5 1/2 can: kvaser_usb: Comply with firmware max tx URBs value
In reply to Ahmed S. DarwishFrom: Ahmed S. Darwish <[email protected]>
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.
--- drivers/net/can/usb/kvaser_usb.c | 64 ++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 25 deletions(-)
drivers/net/can/usb/kvaser_usb.c +39 -25
@@ -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) {
Signature · 1 line
1.9.1
PATCH v5 2/2 can: kvaser_usb: Fix sparse warning __le16 degrades to integer
From: Ahmed S. Darwish <[email protected]>
USB endpoint's wMaxPacketSize field is an le16 entity. Use appropriate le16_to_cpu macros to maintain endian independence.
--- 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,
drivers/net/can/usb/kvaser_usb.c +4 -3
@@ -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 ; }
Signature · 1 line
1.9.1
Marc Kleine-Budde
maintainerRe: [PATCH v5 1/2] can: kvaser_usb: Comply with firmware max tx URBs value
In reply to Ahmed S. DarwishQuoted 23 lines · Ahmed S. Darwish
From: Ahmed S. Darwish <[email protected]>
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_INFOmessage.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 <[email protected]>
drivers/net/can/usb/kvaser_usb.c static int kvaser_usb_init_one(struct usb_interface *intf, · line 1943
Quoted 5 lines
drivers/net/can/usb/kvaser_usb.c +1
-1@@ -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
Signature · 4 lines
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 |
Re: [PATCH v5 1/2] can: kvaser_usb: Comply with firmware max tx URBs value
Quoted 27 lines · Marc Kleine-Budde
Quoted 26 lines · Ahmed S. Darwish
From: Ahmed S. Darwish <[email protected]>
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_INFOmessage.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 <[email protected]>drivers/net/can/usb/kvaser_usb.c +0
-0@@ -1928,7 +1940,7 @@ static int kvaser_usb_init_one(struct usb_interface *intf,return err; }
Quoted 6 lines
drivers/net/can/usb/kvaser_usb.c static int kvaser_usb_init_one(struct usb_interface *intf, · line 1943
Quoted 2 lines
drivers/net/can/usb/kvaser_usb.c +1
-1-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
Marc Kleine-Budde
maintainerRe: [PATCH v5 1/2] can: kvaser_usb: Comply with firmware max tx URBs value
Quoted 7 lines · Ahmed S. Darwish
Quoted 5 lines
drivers/net/can/usb/kvaser_usb.c static int kvaser_usb_init_one(struct usb_interface *intf, · line 1943
Quoted 2 lines
drivers/net/can/usb/kvaser_usb.c +1
-1-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.
Quoted 2 lines
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
Signature · 4 lines
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 |