From 636d0375e7a850c95a0abc2c214295ce06cd2ab4 Mon Sep 17 00:00:00 2001 From: Bjorn Van Tilt Date: Mon, 24 Mar 2014 15:32:08 +0100 Subject: [PATCH 01/16] can: usb_8dev: Fix memory leak in usb_8dev_start_xmit Fixed a memory leak when an error occurred in the transmit function. In the error handling the urb wasn't freed before returning. There was also a call to the usb_unanchor_urb() function but the urb wasn't anchored. Signed-off-by: Bjorn Van Tilt Signed-off-by: Marc Kleine-Budde --- drivers/net/can/usb/usb_8dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/usb/usb_8dev.c b/drivers/net/can/usb/usb_8dev.c index a0fa1fd5092b..e7247a506586 100644 --- a/drivers/net/can/usb/usb_8dev.c +++ b/drivers/net/can/usb/usb_8dev.c @@ -697,8 +697,8 @@ static netdev_tx_t usb_8dev_start_xmit(struct sk_buff *skb, return NETDEV_TX_OK; nofreecontext: - usb_unanchor_urb(urb); usb_free_coherent(priv->udev, size, buf, urb->transfer_dma); + usb_free_urb(urb); netdev_warn(netdev, "couldn't find free context"); From 5fb7639dee74496107f269a987933e2f0465692c Mon Sep 17 00:00:00 2001 From: Robert Schwebel Date: Wed, 19 Mar 2014 09:49:42 +0100 Subject: [PATCH 02/16] can: Documentation: fix parameter name "sample-point" This patch fixes the name of the parameter to configure the sample point used in iproute2's ip command. The correct writing is "sample-point" not "sample_point". Signed-off-by: Robert Schwebel Signed-off-by: Marc Kleine-Budde --- Documentation/networking/can.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/networking/can.txt b/Documentation/networking/can.txt index 0cbe6ec22d6f..2fa44cbe81b7 100644 --- a/Documentation/networking/can.txt +++ b/Documentation/networking/can.txt @@ -1017,7 +1017,7 @@ solution for a couple of reasons: in case of a bus-off condition after the specified delay time in milliseconds. By default it's off. - "bitrate 125000 sample_point 0.875" + "bitrate 125000 sample-point 0.875" Shows the real bit-rate in bits/sec and the sample-point in the range 0.000..0.999. If the calculation of bit-timing parameters is enabled in the kernel (CONFIG_CAN_CALC_BITTIMING=y), the From f29b423834be812b736bf5e804290c3e14b1dd67 Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Tue, 18 Mar 2014 19:13:59 +0100 Subject: [PATCH 03/16] can: c_can: free_c_can_dev(): add missing netif_napi_del() This patch adds the missing netif_napi_del() to the free_c_can_dev() function. Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 951bfede8f3d..6c03731d7ee7 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -1269,6 +1269,9 @@ EXPORT_SYMBOL_GPL(c_can_power_up); void free_c_can_dev(struct net_device *dev) { + struct c_can_priv *priv = netdev_priv(dev); + + netif_napi_del(&priv->napi); free_candev(dev); } EXPORT_SYMBOL_GPL(free_c_can_dev); From 130a5171dad06c6d89fd5568260fbb0c4b34bd74 Mon Sep 17 00:00:00 2001 From: Marc Kleine-Budde Date: Tue, 18 Mar 2014 19:06:01 +0100 Subject: [PATCH 04/16] can: c_can: check return value to users of c_can_set_bittiming() This patch adds return value checking to all direct and indirect users of c_can_set_bittiming(). Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 6c03731d7ee7..68839380086d 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -631,7 +631,7 @@ static void c_can_configure_msg_objects(struct net_device *dev) * - set operating mode * - configure message objects */ -static void c_can_chip_config(struct net_device *dev) +static int c_can_chip_config(struct net_device *dev) { struct c_can_priv *priv = netdev_priv(dev); @@ -668,15 +668,18 @@ static void c_can_chip_config(struct net_device *dev) priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED); /* set bittiming params */ - c_can_set_bittiming(dev); + return c_can_set_bittiming(dev); } -static void c_can_start(struct net_device *dev) +static int c_can_start(struct net_device *dev) { struct c_can_priv *priv = netdev_priv(dev); + int err; /* basic c_can configuration */ - c_can_chip_config(dev); + err = c_can_chip_config(dev); + if (err) + return err; priv->can.state = CAN_STATE_ERROR_ACTIVE; @@ -685,6 +688,8 @@ static void c_can_start(struct net_device *dev) /* enable status change, error and module interrupts */ c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS); + + return 0; } static void c_can_stop(struct net_device *dev) @@ -700,9 +705,13 @@ static void c_can_stop(struct net_device *dev) static int c_can_set_mode(struct net_device *dev, enum can_mode mode) { + int err; + switch (mode) { case CAN_MODE_START: - c_can_start(dev); + err = c_can_start(dev); + if (err) + return err; netif_wake_queue(dev); break; default: @@ -1133,17 +1142,20 @@ static int c_can_open(struct net_device *dev) goto exit_irq_fail; } - napi_enable(&priv->napi); + /* start the c_can controller */ + err = c_can_start(dev); + if (err) + goto exit_start_fail; can_led_event(dev, CAN_LED_EVENT_OPEN); - /* start the c_can controller */ - c_can_start(dev); - + napi_enable(&priv->napi); netif_start_queue(dev); return 0; +exit_start_fail: + free_irq(dev->irq, dev); exit_irq_fail: close_candev(dev); exit_open_fail: @@ -1260,9 +1272,7 @@ int c_can_power_up(struct net_device *dev) if (time_after(jiffies, time_out)) return -ETIMEDOUT; - c_can_start(dev); - - return 0; + return c_can_start(dev); } EXPORT_SYMBOL_GPL(c_can_power_up); #endif From 9fac1d1ab8e66816c40a235a238357b1f1fc4dee Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:08 +0000 Subject: [PATCH 05/16] can: c_can: Wait for CONTROL_INIT to be cleared According to the documentation the CPU must wait for CONTROL_INIT to be cleared before writing to the baudrate registers. Signed-off-by: Benedikt Spranger Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 68839380086d..4d08a32f27ac 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -566,6 +566,21 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb, return NETDEV_TX_OK; } +static int c_can_wait_for_ctrl_init(struct net_device *dev, + struct c_can_priv *priv, u32 init) +{ + int retry = 0; + + while (init != (priv->read_reg(priv, C_CAN_CTRL_REG) & CONTROL_INIT)) { + udelay(10); + if (retry++ > 1000) { + netdev_err(dev, "CCTRL: set CONTROL_INIT failed\n"); + return -EIO; + } + } + return 0; +} + static int c_can_set_bittiming(struct net_device *dev) { unsigned int reg_btr, reg_brpe, ctrl_save; @@ -573,6 +588,7 @@ static int c_can_set_bittiming(struct net_device *dev) u32 ten_bit_brp; struct c_can_priv *priv = netdev_priv(dev); const struct can_bittiming *bt = &priv->can.bittiming; + int res; /* c_can provides a 6-bit brp and 4-bit brpe fields */ ten_bit_brp = bt->brp - 1; @@ -590,13 +606,17 @@ static int c_can_set_bittiming(struct net_device *dev) "setting BTR=%04x BRPE=%04x\n", reg_btr, reg_brpe); ctrl_save = priv->read_reg(priv, C_CAN_CTRL_REG); - priv->write_reg(priv, C_CAN_CTRL_REG, - ctrl_save | CONTROL_CCE | CONTROL_INIT); + ctrl_save &= ~CONTROL_INIT; + priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_CCE | CONTROL_INIT); + res = c_can_wait_for_ctrl_init(dev, priv, CONTROL_INIT); + if (res) + return res; + priv->write_reg(priv, C_CAN_BTR_REG, reg_btr); priv->write_reg(priv, C_CAN_BRPEXT_REG, reg_brpe); priv->write_reg(priv, C_CAN_CTRL_REG, ctrl_save); - return 0; + return c_can_wait_for_ctrl_init(dev, priv, 0); } /* From 5bb9cbaa622a2bbde8e307d4e0528dd2c8212a6a Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:08 +0000 Subject: [PATCH 06/16] can: c_can: Fix hardware raminit function The function is broken in several ways: - The function does not wait for the init to complete. That can take quite some microseconds. - No protection against being called for two chips at the same time. SMP is such a new thing, right? Clear the start and the init done bit unconditionally and wait for both bits to be clear. In the enable path set the init bit and wait for the init done bit. Add proper locking. Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can_platform.c | 45 ++++++++++++++++++++------ 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c index d66ac265269c..806d92753427 100644 --- a/drivers/net/can/c_can/c_can_platform.c +++ b/drivers/net/can/c_can/c_can_platform.c @@ -37,8 +37,10 @@ #include "c_can.h" -#define CAN_RAMINIT_START_MASK(i) (1 << (i)) - +#define CAN_RAMINIT_START_MASK(i) (0x001 << (i)) +#define CAN_RAMINIT_DONE_MASK(i) (0x100 << (i)) +#define CAN_RAMINIT_ALL_MASK(i) (0x101 << (i)) +static DEFINE_SPINLOCK(raminit_lock); /* * 16-bit c_can registers can be arranged differently in the memory * architecture of different implementations. For example: 16-bit @@ -69,16 +71,41 @@ static void c_can_plat_write_reg_aligned_to_32bit(struct c_can_priv *priv, writew(val, priv->base + 2 * priv->regs[index]); } +static void c_can_hw_raminit_wait(const struct c_can_priv *priv, u32 mask, + u32 val) +{ + /* We look only at the bits of our instance. */ + val &= mask; + while ((readl(priv->raminit_ctrlreg) & mask) != val) + udelay(1); +} + static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) { - u32 val; + u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance); + u32 ctrl; - val = readl(priv->raminit_ctrlreg); - if (enable) - val |= CAN_RAMINIT_START_MASK(priv->instance); - else - val &= ~CAN_RAMINIT_START_MASK(priv->instance); - writel(val, priv->raminit_ctrlreg); + spin_lock(&raminit_lock); + + ctrl = readl(priv->raminit_ctrlreg); + /* We clear the done and start bit first. The start bit is + * looking at the 0 -> transition, but is not self clearing; + * And we clear the init done bit as well. + */ + ctrl &= ~CAN_RAMINIT_START_MASK(priv->instance); + ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance); + writel(ctrl, priv->raminit_ctrlreg); + ctrl &= ~CAN_RAMINIT_DONE_MASK(priv->instance); + c_can_hw_raminit_wait(priv, ctrl, mask); + + if (enable) { + /* Set start bit and wait for the done bit. */ + ctrl |= CAN_RAMINIT_START_MASK(priv->instance); + writel(ctrl, priv->raminit_ctrlreg); + ctrl |= CAN_RAMINIT_DONE_MASK(priv->instance); + c_can_hw_raminit_wait(priv, ctrl, mask); + } + spin_unlock(&raminit_lock); } static struct platform_device_id c_can_id_table[] = { From 640916db2bf7de7c5ac13a1f470fa959459ccf6d Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:09 +0000 Subject: [PATCH 07/16] can: c_can: Make it SMP safe The hardware has two message control interfaces, but the code only uses the first one. So on SMP the following can be observed: CPU0 CPU1 rx_poll() write IF1 xmit() write IF1 write IF1 That results in corrupted message object configurations. The TX/RX is not globally serialized it's only serialized on a core. Simple solution: Let RX use IF1 and TX use IF2 and all is good. Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 36 ++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 4d08a32f27ac..38f9adaf15ac 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -132,6 +132,12 @@ #define IF_MCONT_EOB BIT(7) #define IF_MCONT_DLC_MASK 0xf +/* + * Use IF1 for RX and IF2 for TX + */ +#define IF_RX 0 +#define IF_TX 1 + /* * IFx register masks: * allow easy operation on 16-bit registers when the @@ -420,7 +426,7 @@ static void c_can_handle_lost_msg_obj(struct net_device *dev, priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), IF_MCONT_CLR_MSGLST); - c_can_object_put(dev, 0, objno, IF_COMM_CONTROL); + c_can_object_put(dev, iface, objno, IF_COMM_CONTROL); /* create an error msg */ skb = alloc_can_err_skb(dev, &frame); @@ -551,7 +557,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb, msg_obj_no = get_tx_next_msg_obj(priv); /* prepare message object for transmission */ - c_can_write_msg_object(dev, 0, frame, msg_obj_no); + c_can_write_msg_object(dev, IF_TX, frame, msg_obj_no); can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST); /* @@ -634,14 +640,14 @@ static void c_can_configure_msg_objects(struct net_device *dev) /* first invalidate all message objects */ for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++) - c_can_inval_msg_object(dev, 0, i); + c_can_inval_msg_object(dev, IF_RX, i); /* setup receive message objects */ for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++) - c_can_setup_receive_object(dev, 0, i, 0, 0, + c_can_setup_receive_object(dev, IF_RX, i, 0, 0, (IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB); - c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0, + c_can_setup_receive_object(dev, IF_RX, C_CAN_MSG_OBJ_RX_LAST, 0, 0, IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK); } @@ -792,13 +798,13 @@ static void c_can_do_tx(struct net_device *dev) if (!(val & (1 << (msg_obj_no - 1)))) { can_get_echo_skb(dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST); - c_can_object_get(dev, 0, msg_obj_no, IF_COMM_ALL); + c_can_object_get(dev, IF_TX, msg_obj_no, IF_COMM_ALL); stats->tx_bytes += priv->read_reg(priv, - C_CAN_IFACE(MSGCTRL_REG, 0)) + C_CAN_IFACE(MSGCTRL_REG, IF_TX)) & IF_MCONT_DLC_MASK; stats->tx_packets++; can_led_event(dev, CAN_LED_EVENT_TX); - c_can_inval_msg_object(dev, 0, msg_obj_no); + c_can_inval_msg_object(dev, IF_TX, msg_obj_no); } else { break; } @@ -850,13 +856,13 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) while ((msg_obj = ffs(val)) && quota > 0) { val &= ~BIT(msg_obj - 1); - c_can_object_get(dev, 0, msg_obj, IF_COMM_ALL & + c_can_object_get(dev, IF_RX, msg_obj, IF_COMM_ALL & ~IF_COMM_TXRQST); msg_ctrl_save = priv->read_reg(priv, - C_CAN_IFACE(MSGCTRL_REG, 0)); + C_CAN_IFACE(MSGCTRL_REG, IF_RX)); if (msg_ctrl_save & IF_MCONT_MSGLST) { - c_can_handle_lost_msg_obj(dev, 0, msg_obj); + c_can_handle_lost_msg_obj(dev, IF_RX, msg_obj); num_rx_pkts++; quota--; continue; @@ -869,19 +875,19 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) continue; /* read the data from the message object */ - c_can_read_msg_object(dev, 0, msg_ctrl_save); + c_can_read_msg_object(dev, IF_RX, msg_ctrl_save); if (msg_obj < C_CAN_MSG_RX_LOW_LAST) - c_can_mark_rx_msg_obj(dev, 0, + c_can_mark_rx_msg_obj(dev, IF_RX, msg_ctrl_save, msg_obj); else if (msg_obj > C_CAN_MSG_RX_LOW_LAST) /* activate this msg obj */ - c_can_activate_rx_msg_obj(dev, 0, + c_can_activate_rx_msg_obj(dev, IF_RX, msg_ctrl_save, msg_obj); else if (msg_obj == C_CAN_MSG_RX_LOW_LAST) /* activate all lower message objects */ c_can_activate_all_lower_rx_msg_obj(dev, - 0, msg_ctrl_save); + IF_RX, msg_ctrl_save); num_rx_pkts++; quota--; From 64f08f2f3544eb8b6b14fd35e6087d7d3ede77cd Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:10 +0000 Subject: [PATCH 08/16] can: c_can: Fix buffer ordering The buffer handling of c_can has been broken forever. That leads to message reordering: ksoftirqd/0-3 [000] ..s. 79.123776: c_can_poll: rx_poll: val: 00007fff ksoftirqd/0-3 [000] ..s. 79.124101: c_can_poll: rx_poll: val: 00008001 What happens is: CPU HW queue new packet into obj 16 (0-15 are busy) read obj 1-15 return because pending is 0 set pending obj 16 -> pending reg 8000 queue new packet into obj 1 set pending obj 1 -> pending reg 8001 So the current algorithmus reads the newest message first, which violates the ordering rules of CAN. Add proper handling of that situation by analyzing the contents of the pending register for gaps. This does NOT fix the message object corruption which can lead to interrupt storms. Thats addressed in the next patches. Signed-off-by: Thomas Gleixner [mkl: adjusted subject] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 52 +++++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 38f9adaf15ac..cef9967eff93 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -816,6 +816,38 @@ static void c_can_do_tx(struct net_device *dev) netif_wake_queue(dev); } +/* + * If we have a gap in the pending bits, that means we either + * raced with the hardware or failed to readout all upper + * objects in the last run due to quota limit. + */ +static u32 c_can_adjust_pending(u32 pend) +{ + u32 weight, lasts; + + if (pend == RECEIVE_OBJECT_BITS) + return pend; + + /* + * If the last set bit is larger than the number of pending + * bits we have a gap. + */ + weight = hweight32(pend); + lasts = fls(pend); + + /* If the bits are linear, nothing to do */ + if (lasts == weight) + return pend; + + /* + * Find the first set bit after the gap. We walk backwards + * from the last set bit. + */ + for (lasts--; pend & (1 << (lasts - 1)); lasts--); + + return pend & ~((1 << lasts) - 1); +} + /* * theory of operation: * @@ -843,7 +875,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) u32 num_rx_pkts = 0; unsigned int msg_obj, msg_ctrl_save; struct c_can_priv *priv = netdev_priv(dev); - u16 val; + u32 val, pend = 0; /* * It is faster to read only one 16bit register. This is only possible @@ -852,7 +884,23 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) BUILD_BUG_ON_MSG(C_CAN_MSG_OBJ_RX_LAST > 16, "Implementation does not support more message objects than 16"); - while (quota > 0 && (val = priv->read_reg(priv, C_CAN_INTPND1_REG))) { + while (quota > 0) { + + if (!pend) { + pend = priv->read_reg(priv, C_CAN_INTPND1_REG); + if (!pend) + return num_rx_pkts; + /* + * If the pending field has a gap, handle the + * bits above the gap first. + */ + val = c_can_adjust_pending(pend); + } else { + val = pend; + } + /* Remove the bits from pend */ + pend &= ~val; + while ((msg_obj = ffs(val)) && quota > 0) { val &= ~BIT(msg_obj - 1); From 07c7b6f6161be52b8ab6bca70ed6a7140708c94e Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:10 +0000 Subject: [PATCH 09/16] can: c_can: Fix the lost message handling The lost message handling is broken in several ways. 1) Clearing the message lost flag is done by writing 0 to the message control register of the object. #define IF_MCONT_CLR_MSGLST (0 << 14) That clears the object buffer configuration in the worst case, which results in a loss of the EOB flag. That leaves the FIFO chain without a limit and causes a complete lockup of the HW 2) In case that the error skb allocation fails, the code happily claims that it handed down a packet. Just an accounting bug, but .... 3) The code adds a lot of pointless overhead to that error case, where we need to get stuff done as fast as possible to avoid more packet loss. - printk an annoying error message - reread the object buffer for nothing Fix is simple again: - Use the already known MSGCTRL content and only clear the MSGLST bit - Fix the buffer accounting by adding a proper return code - Remove the pointless operations Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index cef9967eff93..ef5f3b8099f4 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -122,7 +122,6 @@ /* IFx message control */ #define IF_MCONT_NEWDAT BIT(15) #define IF_MCONT_MSGLST BIT(14) -#define IF_MCONT_CLR_MSGLST (0 << 14) #define IF_MCONT_INTPND BIT(13) #define IF_MCONT_UMASK BIT(12) #define IF_MCONT_TXIE BIT(11) @@ -411,27 +410,22 @@ static inline void c_can_activate_rx_msg_obj(struct net_device *dev, c_can_object_put(dev, iface, obj, IF_COMM_CONTROL); } -static void c_can_handle_lost_msg_obj(struct net_device *dev, - int iface, int objno) +static int c_can_handle_lost_msg_obj(struct net_device *dev, + int iface, int objno, u32 ctrl) { - struct c_can_priv *priv = netdev_priv(dev); struct net_device_stats *stats = &dev->stats; - struct sk_buff *skb; + struct c_can_priv *priv = netdev_priv(dev); struct can_frame *frame; + struct sk_buff *skb; - netdev_err(dev, "msg lost in buffer %d\n", objno); - - c_can_object_get(dev, iface, objno, IF_COMM_ALL & ~IF_COMM_TXRQST); - - priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), - IF_MCONT_CLR_MSGLST); - + ctrl &= ~(IF_MCONT_MSGLST | IF_MCONT_INTPND | IF_MCONT_NEWDAT); + priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl); c_can_object_put(dev, iface, objno, IF_COMM_CONTROL); /* create an error msg */ skb = alloc_can_err_skb(dev, &frame); if (unlikely(!skb)) - return; + return 0; frame->can_id |= CAN_ERR_CRTL; frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; @@ -439,6 +433,7 @@ static void c_can_handle_lost_msg_obj(struct net_device *dev, stats->rx_over_errors++; netif_receive_skb(skb); + return 1; } static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl) @@ -910,9 +905,13 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) C_CAN_IFACE(MSGCTRL_REG, IF_RX)); if (msg_ctrl_save & IF_MCONT_MSGLST) { - c_can_handle_lost_msg_obj(dev, IF_RX, msg_obj); - num_rx_pkts++; - quota--; + int n; + + n = c_can_handle_lost_msg_obj(dev, IF_RX, + msg_obj, + msg_ctrl_save); + num_rx_pkts += n; + quota -=n; continue; } From 710c56105dfd10e32a89086cf78cc1c8433f6a7a Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:11 +0000 Subject: [PATCH 10/16] can: c_can: Remove EOB exit The rx_poll code has the following gem: if (msg_ctrl_save & IF_MCONT_EOB) return num_rx_pkts; The EOB bit is the indicator for the hardware that this is the last configured FIFO object. But this object can contain valid data, if we manage to free up objects before the overrun case hits. Now if the code exits due to the EOB bit set, then this buffer is stale and the interrupt bit and NewDat bit of the buffer are still set. Results in a nice interrupt storm unless we come into an overrun situation where the MSGLST bit gets set. ksoftirqd/0-3 [000] ..s. 79.124101: c_can_poll: rx_poll: val: 00008001 pend 00008001 ksoftirqd/0-3 [000] ..s. 79.124176: c_can_poll: rx_poll: val: 00008000 pend 00008000 ksoftirqd/0-3 [000] ..s. 79.124187: c_can_poll: rx_poll: val: 00008002 pend 00008002 ksoftirqd/0-3 [000] ..s. 79.124256: c_can_poll: rx_poll: val: 00008000 pend 00008000 ksoftirqd/0-3 [000] ..s. 79.124267: c_can_poll: rx_poll: val: 00008000 pend 00008000 The amazing thing is that the check of the MSGLST (aka overrun bit) used to be after the check of the EOB bit. That was "fixed" in commit 5d0f801a2c(can: c_can: Fix RX message handling, handle lost message before EOB). But the author of this "fix" did not even understand that the EOB check is broken as well. Again a simple solution: Remove Signed-off-by: Thomas Gleixner [mkl: adjusted subject and commit message] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index ef5f3b8099f4..30a85aa81d45 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -915,9 +915,6 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) continue; } - if (msg_ctrl_save & IF_MCONT_EOB) - return num_rx_pkts; - if (!(msg_ctrl_save & IF_MCONT_NEWDAT)) continue; From bf88a20611d5a62119a02da4eba95b461ec692c8 Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:12 +0000 Subject: [PATCH 11/16] can: c_can: Provide protection in the xmit path The network core does not serialize the access to the hardware. The xmit related code lets the following happen: CPU0 CPU1 interrupt() do_poll() c_can_do_tx() Fiddle with HW and xmit() internal data Fiddle with HW and internal data due the complete lack of serialization. Add proper locking. Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 9 ++++++++- drivers/net/can/c_can/c_can.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 30a85aa81d45..1fe79ce50c5a 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -549,6 +549,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb, if (can_dropped_invalid_skb(dev, skb)) return NETDEV_TX_OK; + spin_lock_bh(&priv->xmit_lock); msg_obj_no = get_tx_next_msg_obj(priv); /* prepare message object for transmission */ @@ -563,6 +564,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb, if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv)) || (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0) netif_stop_queue(dev); + spin_unlock_bh(&priv->xmit_lock); return NETDEV_TX_OK; } @@ -787,7 +789,9 @@ static void c_can_do_tx(struct net_device *dev) struct c_can_priv *priv = netdev_priv(dev); struct net_device_stats *stats = &dev->stats; - for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) { + spin_lock_bh(&priv->xmit_lock); + + for (; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) { msg_obj_no = get_tx_echo_msg_obj(priv); val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG); if (!(val & (1 << (msg_obj_no - 1)))) { @@ -809,6 +813,8 @@ static void c_can_do_tx(struct net_device *dev) if (((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) != 0) || ((priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) == 0)) netif_wake_queue(dev); + + spin_unlock_bh(&priv->xmit_lock); } /* @@ -1262,6 +1268,7 @@ struct net_device *alloc_c_can_dev(void) return NULL; priv = netdev_priv(dev); + spin_lock_init(&priv->xmit_lock); netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT); priv->dev = dev; diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h index d2e1c21b143f..5097c802a61e 100644 --- a/drivers/net/can/c_can/c_can.h +++ b/drivers/net/can/c_can/c_can.h @@ -156,6 +156,7 @@ struct c_can_priv { struct napi_struct napi; struct net_device *dev; struct device *device; + spinlock_t xmit_lock; int tx_object; int current_status; int last_status; From 520f570c4378d23b595e52213601d62332174462 Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 19:27:42 +0100 Subject: [PATCH 12/16] can: c_can: Make the code readable If every other line contains line breaks, that's a clear sign for indentation level madness. Split out the inner loop and move the code to a separate function. gcc creates slightly worse code for that, but we'll fix that in the next step. Signed-off-by: Thomas Gleixner [mkl: adjusted subject] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 107 ++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 51 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 1fe79ce50c5a..bd7234eb42f4 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -849,6 +849,52 @@ static u32 c_can_adjust_pending(u32 pend) return pend & ~((1 << lasts) - 1); } +static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv, + u32 pend, int quota) +{ + u32 pkts = 0, ctrl, obj; + + while ((obj = ffs(pend)) && quota > 0) { + pend &= ~BIT(obj - 1); + + c_can_object_get(dev, IF_RX, obj, IF_COMM_ALL & ~IF_COMM_TXRQST); + ctrl = priv->read_reg(priv, C_CAN_IFACE(MSGCTRL_REG, IF_RX)); + + if (ctrl & IF_MCONT_MSGLST) { + int n = c_can_handle_lost_msg_obj(dev, IF_RX, obj, ctrl); + + pkts += n; + quota -= n; + continue; + } + + /* + * This really should not happen, but this covers some + * odd HW behaviour. Do not remove that unless you + * want to brick your machine. + */ + if (!(ctrl & IF_MCONT_NEWDAT)) + continue; + + /* read the data from the message object */ + c_can_read_msg_object(dev, IF_RX, ctrl); + + if (obj < C_CAN_MSG_RX_LOW_LAST) + c_can_mark_rx_msg_obj(dev, IF_RX, ctrl, obj); + else if (obj > C_CAN_MSG_RX_LOW_LAST) + /* activate this msg obj */ + c_can_activate_rx_msg_obj(dev, IF_RX, ctrl, obj); + else if (obj == C_CAN_MSG_RX_LOW_LAST) + /* activate all lower message objects */ + c_can_activate_all_lower_rx_msg_obj(dev, IF_RX, ctrl); + + pkts++; + quota--; + } + + return pkts; +} + /* * theory of operation: * @@ -873,10 +919,8 @@ static u32 c_can_adjust_pending(u32 pend) */ static int c_can_do_rx_poll(struct net_device *dev, int quota) { - u32 num_rx_pkts = 0; - unsigned int msg_obj, msg_ctrl_save; struct c_can_priv *priv = netdev_priv(dev); - u32 val, pend = 0; + u32 pkts = 0, pend = 0, toread, n; /* * It is faster to read only one 16bit register. This is only possible @@ -886,65 +930,26 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) "Implementation does not support more message objects than 16"); while (quota > 0) { - if (!pend) { pend = priv->read_reg(priv, C_CAN_INTPND1_REG); if (!pend) - return num_rx_pkts; + break; /* * If the pending field has a gap, handle the * bits above the gap first. */ - val = c_can_adjust_pending(pend); + toread = c_can_adjust_pending(pend); } else { - val = pend; + toread = pend; } /* Remove the bits from pend */ - pend &= ~val; - - while ((msg_obj = ffs(val)) && quota > 0) { - val &= ~BIT(msg_obj - 1); - - c_can_object_get(dev, IF_RX, msg_obj, IF_COMM_ALL & - ~IF_COMM_TXRQST); - msg_ctrl_save = priv->read_reg(priv, - C_CAN_IFACE(MSGCTRL_REG, IF_RX)); - - if (msg_ctrl_save & IF_MCONT_MSGLST) { - int n; - - n = c_can_handle_lost_msg_obj(dev, IF_RX, - msg_obj, - msg_ctrl_save); - num_rx_pkts += n; - quota -=n; - continue; - } - - if (!(msg_ctrl_save & IF_MCONT_NEWDAT)) - continue; - - /* read the data from the message object */ - c_can_read_msg_object(dev, IF_RX, msg_ctrl_save); - - if (msg_obj < C_CAN_MSG_RX_LOW_LAST) - c_can_mark_rx_msg_obj(dev, IF_RX, - msg_ctrl_save, msg_obj); - else if (msg_obj > C_CAN_MSG_RX_LOW_LAST) - /* activate this msg obj */ - c_can_activate_rx_msg_obj(dev, IF_RX, - msg_ctrl_save, msg_obj); - else if (msg_obj == C_CAN_MSG_RX_LOW_LAST) - /* activate all lower message objects */ - c_can_activate_all_lower_rx_msg_obj(dev, - IF_RX, msg_ctrl_save); - - num_rx_pkts++; - quota--; - } + pend &= ~toread; + /* Read the objects */ + n = c_can_read_objects(dev, priv, toread, quota); + pkts += n; + quota -= n; } - - return num_rx_pkts; + return pkts; } static inline int c_can_has_and_handle_berr(struct c_can_priv *priv) From c0a9f4d396c9d3cf34d21e318a422f9461d93042 Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:13 +0000 Subject: [PATCH 13/16] can: c_can: Reduce register access commit 4ce78a838c (can: c_can: Speed up rx_poll function) hyped a performance improvement by reducing the access to the interrupt pending register from a dual 16 bit to a single 16 bit access. Wow! Thereby it crippled the driver to cast the 16 msg objects in stone, which is completly braindead as contemporary hardware has up to 128 message objects. Supporting larger object buffers is a major surgery, but it'd be definitely worth it especially as the driver does not support HW message filtering .... The logic of the "FIFO" implementation is to split the FIFO in half. For the lower half we read the buffers and clear the interrupt pending bit, but keep the newdat bit set, so the HW will queue above those buffers. When we read out the last low buffer then we reenable all the low half buffers by clearing the newdat bit. The upper half buffers clear the newdat and the interrupt pending bit right away as we know that the lower half bits are clear and give us a headstart against the hardware. Now the implementation is: transfer_message_object() read_object_and_put_into_skb(); if (obj < END_OF_LOW_BUF) clear_intpending(obj) else if (obj > END_OF_LOW_BUF) clear_intpending_and_newdat(obj) else if (obj == END_OF_LOW_BUF) clear_newdat_of_all_low_objects() The hardware allows to avoid most of the mess simply because we can tell the transfer_message_object() function to clear bits right away. So we can be clever and do: if (obj <= END_OF_LOW_BUF) ctrl = TRANSFER_MSG | CLEAR_INTPND; else ctrl = TRANSFER_MSG | CLEAR_INTPND | CLEAR_NEWDAT; transfer_message_object(ctrl) read_object_and_put_into_skb(); if (obj == END_OF_LOW_BUF) clear_newdat_of_all_low_objects() So we save a complete control operation on all message objects except the one which is the end of the low buffer. That's a few micro seconds per object. I'm not adding a boasting profile to that, simply because it's self explaining. Signed-off-by: Thomas Gleixner [mkl: adjusted subject and commit message] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 49 +++++++++++------------------------ 1 file changed, 15 insertions(+), 34 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index bd7234eb42f4..8ea1379a398b 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -114,6 +114,14 @@ IF_COMM_CONTROL | IF_COMM_TXRQST | \ IF_COMM_DATAA | IF_COMM_DATAB) +/* For the low buffers we clear the interrupt bit, but keep newdat */ +#define IF_COMM_RCV_LOW (IF_COMM_MASK | IF_COMM_ARB | \ + IF_COMM_CONTROL | IF_COMM_CLR_INT_PND | \ + IF_COMM_DATAA | IF_COMM_DATAB) + +/* For the high buffers we clear the interrupt bit and newdat */ +#define IF_COMM_RCV_HIGH (IF_COMM_RCV_LOW | IF_COMM_TXRQST) + /* IFx arbitration */ #define IF_ARB_MSGVAL BIT(15) #define IF_ARB_MSGXTD BIT(14) @@ -371,18 +379,6 @@ static void c_can_write_msg_object(struct net_device *dev, c_can_object_put(dev, iface, objno, IF_COMM_ALL); } -static inline void c_can_mark_rx_msg_obj(struct net_device *dev, - int iface, int ctrl_mask, - int obj) -{ - struct c_can_priv *priv = netdev_priv(dev); - - priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), - ctrl_mask & ~(IF_MCONT_MSGLST | IF_MCONT_INTPND)); - c_can_object_put(dev, iface, obj, IF_COMM_CONTROL); - -} - static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev, int iface, int ctrl_mask) @@ -392,24 +388,11 @@ static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev, for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_RX_LOW_LAST; i++) { priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), - ctrl_mask & ~(IF_MCONT_MSGLST | - IF_MCONT_INTPND | IF_MCONT_NEWDAT)); + ctrl_mask & ~IF_MCONT_NEWDAT); c_can_object_put(dev, iface, i, IF_COMM_CONTROL); } } -static inline void c_can_activate_rx_msg_obj(struct net_device *dev, - int iface, int ctrl_mask, - int obj) -{ - struct c_can_priv *priv = netdev_priv(dev); - - priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), - ctrl_mask & ~(IF_MCONT_MSGLST | - IF_MCONT_INTPND | IF_MCONT_NEWDAT)); - c_can_object_put(dev, iface, obj, IF_COMM_CONTROL); -} - static int c_can_handle_lost_msg_obj(struct net_device *dev, int iface, int objno, u32 ctrl) { @@ -852,12 +835,15 @@ static u32 c_can_adjust_pending(u32 pend) static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv, u32 pend, int quota) { - u32 pkts = 0, ctrl, obj; + u32 pkts = 0, ctrl, obj, mcmd; while ((obj = ffs(pend)) && quota > 0) { pend &= ~BIT(obj - 1); - c_can_object_get(dev, IF_RX, obj, IF_COMM_ALL & ~IF_COMM_TXRQST); + mcmd = obj < C_CAN_MSG_RX_LOW_LAST ? + IF_COMM_RCV_LOW : IF_COMM_RCV_HIGH; + + c_can_object_get(dev, IF_RX, obj, mcmd); ctrl = priv->read_reg(priv, C_CAN_IFACE(MSGCTRL_REG, IF_RX)); if (ctrl & IF_MCONT_MSGLST) { @@ -879,12 +865,7 @@ static int c_can_read_objects(struct net_device *dev, struct c_can_priv *priv, /* read the data from the message object */ c_can_read_msg_object(dev, IF_RX, ctrl); - if (obj < C_CAN_MSG_RX_LOW_LAST) - c_can_mark_rx_msg_obj(dev, IF_RX, ctrl, obj); - else if (obj > C_CAN_MSG_RX_LOW_LAST) - /* activate this msg obj */ - c_can_activate_rx_msg_obj(dev, IF_RX, ctrl, obj); - else if (obj == C_CAN_MSG_RX_LOW_LAST) + if (obj == C_CAN_MSG_RX_LOW_LAST) /* activate all lower message objects */ c_can_activate_all_lower_rx_msg_obj(dev, IF_RX, ctrl); From 902470085406934d3f20c4af02de7d79c01b6b93 Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:14 +0000 Subject: [PATCH 14/16] can: c_can: Store dlc private We can avoid the HW access in TX cleanup path for retrieving the DLC of the sent package if we store the DLC in a private array. Ideally this should be handled in the can_echo_skb functions, but I leave that exercise to the CAN folks. Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 28 +--------------------------- drivers/net/can/c_can/c_can.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 8ea1379a398b..1e75223d614f 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -145,33 +145,6 @@ #define IF_RX 0 #define IF_TX 1 -/* - * IFx register masks: - * allow easy operation on 16-bit registers when the - * argument is 32-bit instead - */ -#define IFX_WRITE_LOW_16BIT(x) ((x) & 0xFFFF) -#define IFX_WRITE_HIGH_16BIT(x) (((x) & 0xFFFF0000) >> 16) - -/* message object split */ -#define C_CAN_NO_OF_OBJECTS 32 -#define C_CAN_MSG_OBJ_RX_NUM 16 -#define C_CAN_MSG_OBJ_TX_NUM 16 - -#define C_CAN_MSG_OBJ_RX_FIRST 1 -#define C_CAN_MSG_OBJ_RX_LAST (C_CAN_MSG_OBJ_RX_FIRST + \ - C_CAN_MSG_OBJ_RX_NUM - 1) - -#define C_CAN_MSG_OBJ_TX_FIRST (C_CAN_MSG_OBJ_RX_LAST + 1) -#define C_CAN_MSG_OBJ_TX_LAST (C_CAN_MSG_OBJ_TX_FIRST + \ - C_CAN_MSG_OBJ_TX_NUM - 1) - -#define C_CAN_MSG_OBJ_RX_SPLIT 9 -#define C_CAN_MSG_RX_LOW_LAST (C_CAN_MSG_OBJ_RX_SPLIT - 1) - -#define C_CAN_NEXT_MSG_OBJ_MASK (C_CAN_MSG_OBJ_TX_NUM - 1) -#define RECEIVE_OBJECT_BITS 0x0000ffff - /* status interrupt */ #define STATUS_INTERRUPT 0x8000 @@ -537,6 +510,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb, /* prepare message object for transmission */ c_can_write_msg_object(dev, IF_TX, frame, msg_obj_no); + priv->dlc[msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST] = frame->can_dlc; can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST); /* diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h index 5097c802a61e..faa8404162b3 100644 --- a/drivers/net/can/c_can/c_can.h +++ b/drivers/net/can/c_can/c_can.h @@ -22,6 +22,33 @@ #ifndef C_CAN_H #define C_CAN_H +/* + * IFx register masks: + * allow easy operation on 16-bit registers when the + * argument is 32-bit instead + */ +#define IFX_WRITE_LOW_16BIT(x) ((x) & 0xFFFF) +#define IFX_WRITE_HIGH_16BIT(x) (((x) & 0xFFFF0000) >> 16) + +/* message object split */ +#define C_CAN_NO_OF_OBJECTS 32 +#define C_CAN_MSG_OBJ_RX_NUM 16 +#define C_CAN_MSG_OBJ_TX_NUM 16 + +#define C_CAN_MSG_OBJ_RX_FIRST 1 +#define C_CAN_MSG_OBJ_RX_LAST (C_CAN_MSG_OBJ_RX_FIRST + \ + C_CAN_MSG_OBJ_RX_NUM - 1) + +#define C_CAN_MSG_OBJ_TX_FIRST (C_CAN_MSG_OBJ_RX_LAST + 1) +#define C_CAN_MSG_OBJ_TX_LAST (C_CAN_MSG_OBJ_TX_FIRST + \ + C_CAN_MSG_OBJ_TX_NUM - 1) + +#define C_CAN_MSG_OBJ_RX_SPLIT 9 +#define C_CAN_MSG_RX_LOW_LAST (C_CAN_MSG_OBJ_RX_SPLIT - 1) + +#define C_CAN_NEXT_MSG_OBJ_MASK (C_CAN_MSG_OBJ_TX_NUM - 1) +#define RECEIVE_OBJECT_BITS 0x0000ffff + enum reg { C_CAN_CTRL_REG = 0, C_CAN_CTRL_EX_REG, @@ -173,6 +200,7 @@ struct c_can_priv { u32 __iomem *raminit_ctrlreg; unsigned int instance; void (*raminit) (const struct c_can_priv *priv, bool enable); + u32 dlc[C_CAN_MSG_OBJ_TX_NUM]; }; struct net_device *alloc_c_can_dev(void); From 5a7513adab521909e836fa5b9aaabbf22b48859f Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:14 +0000 Subject: [PATCH 15/16] can: c_can: Simplify TX interrupt cleanup The function loads the message object from the hardware to get the payload length. The previous patch stores that information in an array, so we can avoid the hardware access. Remove the hardware access and move the led toggle outside of the spinlocked region. Toggle the led only once when at least one packet has been received. Binary size shrinks along with the code Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 37 ++++++++++++++++------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 1e75223d614f..42c038d523c9 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -232,10 +232,9 @@ static inline int get_tx_next_msg_obj(const struct c_can_priv *priv) C_CAN_MSG_OBJ_TX_FIRST; } -static inline int get_tx_echo_msg_obj(const struct c_can_priv *priv) +static inline int get_tx_echo_msg_obj(int txecho) { - return (priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) + - C_CAN_MSG_OBJ_TX_FIRST; + return (txecho & C_CAN_NEXT_MSG_OBJ_MASK) + C_CAN_MSG_OBJ_TX_FIRST; } static u32 c_can_read_reg32(struct c_can_priv *priv, enum reg index) @@ -729,8 +728,6 @@ static int c_can_get_berr_counter(const struct net_device *dev, } /* - * theory of operation: - * * priv->tx_echo holds the number of the oldest can_frame put for * transmission into the hardware, but not yet ACKed by the CAN tx * complete IRQ. @@ -741,29 +738,23 @@ static int c_can_get_berr_counter(const struct net_device *dev, */ static void c_can_do_tx(struct net_device *dev) { - u32 val; - u32 msg_obj_no; struct c_can_priv *priv = netdev_priv(dev); struct net_device_stats *stats = &dev->stats; + u32 val, obj, pkts = 0, bytes = 0; spin_lock_bh(&priv->xmit_lock); for (; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) { - msg_obj_no = get_tx_echo_msg_obj(priv); + obj = get_tx_echo_msg_obj(priv->tx_echo); val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG); - if (!(val & (1 << (msg_obj_no - 1)))) { - can_get_echo_skb(dev, - msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST); - c_can_object_get(dev, IF_TX, msg_obj_no, IF_COMM_ALL); - stats->tx_bytes += priv->read_reg(priv, - C_CAN_IFACE(MSGCTRL_REG, IF_TX)) - & IF_MCONT_DLC_MASK; - stats->tx_packets++; - can_led_event(dev, CAN_LED_EVENT_TX); - c_can_inval_msg_object(dev, IF_TX, msg_obj_no); - } else { + + if (val & (1 << (obj - 1))) break; - } + + can_get_echo_skb(dev, obj - C_CAN_MSG_OBJ_TX_FIRST); + bytes += priv->dlc[obj - C_CAN_MSG_OBJ_TX_FIRST]; + pkts++; + c_can_inval_msg_object(dev, IF_TX, obj); } /* restart queue if wrap-up or if queue stalled on last pkt */ @@ -772,6 +763,12 @@ static void c_can_do_tx(struct net_device *dev) netif_wake_queue(dev); spin_unlock_bh(&priv->xmit_lock); + + if (pkts) { + stats->tx_bytes += bytes; + stats->tx_packets += pkts; + can_led_event(dev, CAN_LED_EVENT_TX); + } } /* From b1d8e431bd5639c03ff99d08fd2d5d621969bdc5 Mon Sep 17 00:00:00 2001 From: Thomas Gleixner Date: Tue, 18 Mar 2014 17:19:15 +0000 Subject: [PATCH 16/16] can: c_can: Avoid led toggling for every packet. There is no point to toggle the RX led for every packet. Especially if we have a full FIFO we want to avoid everything we can. Signed-off-by: Thomas Gleixner Signed-off-by: Marc Kleine-Budde --- drivers/net/can/c_can/c_can.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index 42c038d523c9..01dc4941e3d0 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -433,9 +433,6 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl) stats->rx_packets++; stats->rx_bytes += frame->can_dlc; - - can_led_event(dev, CAN_LED_EVENT_RX); - return 0; } @@ -901,6 +898,10 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota) pkts += n; quota -= n; } + + if (pkts) + can_led_event(dev, CAN_LED_EVENT_RX); + return pkts; }