Skip to contentSkip to conversation

can: c_can: Fix a series of serious bugs and improve the performance

PATCH In review Thomas Gleixner 4 participants 34 messages Mar 18, 2014 – Apr 1, 2014

Subsystems (3)
Networking Network drivers CAN network drivers
lore
Share & tools
lore

Thomas Gleixner

author
#0 lore raw

cover can: c_can: Fix a series of serious bugs and improve the performance

First of all, I'm really grumpy as hell.

This is the worst driver I looked at in the last 10 years. And that's an achievement.

The driver is full of serious bugs:

- Two HW init routines are not spec compliant.

- Completely defective message buffer handling in several ways That leads to interrupt storms and complete lockups.

- Complete lack of SMP awareness

What's amazing is that people "optimize" and "fix" the driver over and over, but nobody bothered to understand the manual and repair the code for real.

The series fixes _ALL_ bugs which I found so far, but I'm sure there are more issues burried in that unreadable mess. I'm just not able to trigger them.

The last few patches add real performance improvements by removing a gazillion of pointless hardware operations.

Thanks,

tglx

--------------->

 c_can.c          |  311 ++++++++++++++++++++++++++++++-------------------------
 c_can.h          |   29 +++++
 c_can_platform.c |   48 ++++++--
 3 files changed, 241 insertions(+), 147 deletions(-)
author
#1 lore raw

PATCH 1/12 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 <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)
drivers/net/can/c_can/c_can.c +23 -3
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -566,6 +566,21 @@ static netdev_tx_t c_can_start_xmit(stru
 	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 ne
 	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 ne
 		"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);
 }
 
 /*
maintainer
#2 lore raw

Re: [patch 01/12] can: c_can: Wait for CONTROL_INIT to be cleared

Quoted 2 lines · Thomas Gleixner

According to the documentation the CPU must wait for CONTROL_INIT to be cleared before writing to the baudrate registers.

Thanks for the catch.

Quoted 49 lines
Signed-off-by Benedikt Spranger <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================

drivers/net/can/c_can/c_can.c +18 -2
@@ -566,6 +566,21 @@ static netdev_tx_t c_can_start_xmit(stru
 	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 ne
 	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 ne
 		"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);

drivers/net/can/c_can/c_can.c static int c_can_set_bittiming(struct ne · line 619

Quoted 10 lines
drivers/net/can/c_can/c_can.c +5 -1
+	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);

I'll post a patch that adds return value checking of all direct and indirect users of c_can_set_bittiming().

drivers/net/can/c_can/c_can.c static int c_can_set_bittiming(struct ne · line 622

Quoted 3 lines
drivers/net/can/c_can/c_can.c +0 -0
 }
 
 /*

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   |
author
#3 lore raw

Re: [patch 01/12] can: c_can: Wait for CONTROL_INIT to be cleared

Quoted 9 lines · Marc Kleine-Budde
Quoted 2 lines · Thomas Gleixner

According to the documentation the CPU must wait for CONTROL_INIT to be cleared before writing to the baudrate registers.

Thanks for the catch.

drivers/net/can/c_can/c_can.c static int c_can_set_bittiming(struct ne · line 619

Quoted 1 line
drivers/net/can/c_can/c_can.c +1 -0
+	return c_can_wait_for_ctrl_init(dev, priv, 0);

I'll post a patch that adds return value checking of all direct and indirect users of c_can_set_bittiming().

Right, I forgot to mention, that an error return gets lost in lala land.

Thanks,

tglx

author
#4 lore raw

PATCH 2/12 can: c_can: Fix hardware raminit function

In reply to Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can_platform.c |   48 ++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 9 deletions(-)
drivers/net/can/c_can/c_can_platform.c +39 -9
Index: linux/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,44 @@ static void c_can_plat_write_reg_aligned
 	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;
+
+	spin_lock(&raminit_lock);
 
-	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);
+	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[] = {
maintainer
#5 lore raw

Re: [patch 02/12] can: c_can: Fix hardware raminit function

Quoted 39 lines · Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can_platform.c |   48 ++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 9 deletions(-)

Index: linux/drivers/net/can/c_can/c_can_platform.c
===================================================================

drivers/net/can/c_can/c_can_platform.c +4 -2
@@ -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

drivers/net/can/c_can/c_can_platform.c static void c_can_plat_write_reg_aligned · line 80

Quoted 10 lines
drivers/net/can/c_can/c_can_platform.c +7 -0
@@ -69,16 +71,44 @@ static void c_can_plat_write_reg_aligned
 	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);

Do we have to add a timeout here, or is it "safe" to have a potential endless loop here? As you have probably tortured the hardware and driver a lot (or have been tortured by them), I assume you would have added a timeout check if you had seen a lockup here.

Quoted 12 lines
drivers/net/can/c_can/c_can_platform.c +6 -3
+}
+
 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;
+
+	spin_lock(&raminit_lock);
 
-	val = readl(priv->raminit_ctrlreg);
-	if (enable)

drivers/net/can/c_can/c_can_platform.c static void c_can_plat_write_reg_aligned · line 95

Quoted 10 lines
drivers/net/can/c_can/c_can_platform.c +6 -4
-		val |= CAN_RAMINIT_START_MASK(priv->instance);
-	else
-		val &= ~CAN_RAMINIT_START_MASK(priv->instance);
-	writel(val, priv->raminit_ctrlreg);
+	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.
+	 */

nitpick: In the driver/net tree multi line comments look different, the text starts right after the /*. No need to resend, I'll adjust this while applying the patch.

Quoted 9 lines
drivers/net/can/c_can/c_can_platform.c +9 -0
+	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.

drivers/net/can/c_can/c_can_platform.c static void c_can_plat_write_reg_aligned · line 114

Quoted 10 lines
drivers/net/can/c_can/c_can_platform.c +7 -0
+		 */
+		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[] = {

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   |
author
#6 lore raw

Re: [patch 02/12] can: c_can: Fix hardware raminit function

Quoted 13 lines · Marc Kleine-Budde

drivers/net/can/c_can/c_can_platform.c static void c_can_plat_write_reg_aligned · line 80

Quoted 7 lines · Thomas Gleixner
drivers/net/can/c_can/c_can_platform.c +7 -0
+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);

Do we have to add a timeout here, or is it "safe" to have a potential endless loop here? As you have probably tortured the hardware and driver a lot (or have been tortured by them), I assume you would have added a timeout check if you had seen a lockup here.

I haven't seen any failure on that. We could add a timeout for paranoia reasons. I'm quite sure that the raminit works as advertised when we do it the right way. The only way to wreckage it so far is by not waiting for it to complete.

Thanks,

tglx

#7 lore raw

Re: [patch 02/12] can: c_can: Fix hardware raminit function

Quoted 15 lines · Thomas Gleixner
Quoted 13 lines · Marc Kleine-Budde

drivers/net/can/c_can/c_can_platform.c static void c_can_plat_write_reg_aligned · line 80

Quoted 7 lines · Thomas Gleixner
drivers/net/can/c_can/c_can_platform.c +7 -0
+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);

Do we have to add a timeout here, or is it "safe" to have a potential endless loop here? As you have probably tortured the hardware and driver a lot (or have been tortured by them), I assume you would have added a timeout check if you had seen a lockup here.

Quoted 4 lines

I haven't seen any failure on that. We could add a timeout for paranoia reasons. I'm quite sure that the raminit works as advertised when we do it the right way. The only way to wreckage it so far is by not waiting for it to complete.

As long as it is 100% guaranteed that we

1. really work on a valid C_CAN core
2. this CAN controller can not be unplugged

not adding a timeout would be ok.

I remember a system hang with a SJA1000 based PCMCIA card when unplugged under heavy load:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a7762b10c12a70c5dbf2253142764b728ac88c3a

Regards,
Oliver

author
#8 lore raw

Re: [patch 02/12] can: c_can: Fix hardware raminit function

Quoted 21 lines · Oliver Hartkopp
Quoted 15 lines · Thomas Gleixner
Quoted 13 lines · Marc Kleine-Budde

drivers/net/can/c_can/c_can_platform.c static void c_can_plat_write_reg_aligned · line 80

Quoted 7 lines · Thomas Gleixner
drivers/net/can/c_can/c_can_platform.c +7 -0
+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);

Do we have to add a timeout here, or is it "safe" to have a potential endless loop here? As you have probably tortured the hardware and driver a lot (or have been tortured by them), I assume you would have added a timeout check if you had seen a lockup here.

Quoted 4 lines

I haven't seen any failure on that. We could add a timeout for paranoia reasons. I'm quite sure that the raminit works as advertised when we do it the right way. The only way to wreckage it so far is by not waiting for it to complete.

Quoted 6 lines

As long as it is 100% guaranteed that we

1. really work on a valid C_CAN core
2. this CAN controller can not be unplugged

not adding a timeout would be ok.

The raminit port is a D_CAN specific thing. But yeah, adding a timeout wouldn't hurt at all.

Though the driver is incomplete anyway as not all D_CAN incarnations use the raminit port. There is another way to do that via the CFR register, which is not implemented in the driver.

It seems the chip integrator can chose between raminit port and CFR.

Thanks,

tglx

author
#9 lore raw

PATCH 3/12 can: c_can: Make it SMP safe

In reply to Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)
drivers/net/can/c_can/c_can.c +21 -15
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -133,6 +133,12 @@
 #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
  * argument is 32-bit instead

@@ -420,7 +426,7 @@ static void c_can_handle_lost_msg_obj(st
 	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(stru
 	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(
 
 	/* 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);
 }
 

@@ -783,13 +789,13 @@ static void c_can_do_tx(struct net_devic
 		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;
 		}

@@ -841,13 +847,13 @@ static int c_can_do_rx_poll(struct net_d
 		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;

@@ -860,19 +866,19 @@ static int c_can_do_rx_poll(struct net_d
 				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--;
maintainer
#10 lore raw

Re: [patch 03/12] can: c_can: Make it SMP safe

Quoted 9 lines · Thomas Gleixner

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

Quoted 4 lines

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.

Do both c_can and d_can have two control interfaces?

Quoted 1 line
Signed-off-by Thomas Gleixner <[email protected]>

Otherwise, looks good.

Tnx,
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   |
author
#11 lore raw

Re: [patch 03/12] can: c_can: Make it SMP safe

Quoted 10 lines · Marc Kleine-Budde
Quoted 9 lines · Thomas Gleixner

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

Quoted 6 lines
Quoted 4 lines

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.

Do both c_can and d_can have two control interfaces?

According to the manual it has.

Thanks,

tglx

author
#12 lore raw

PATCH 4/12 can: c_can: Fix buffer ordering for real

In reply to Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   52 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)
drivers/net/can/c_can/c_can.c +50 -2
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -808,6 +808,38 @@ static void c_can_do_tx(struct net_devic
 }
 
 /*
+ * 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:
  *
  * c_can core saves a received CAN message into the first free message

@@ -834,7 +866,7 @@ static int c_can_do_rx_poll(struct net_d
 	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

@@ -843,7 +875,23 @@ static int c_can_do_rx_poll(struct net_d
 	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);
author
#13 lore raw

PATCH 5/12 can: c_can: Fix the lost message handling

In reply to Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
drivers/net/can/c_can/c_can.c +15 -16
Index: linux/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
 	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(st
 	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)

@@ -901,9 +896,13 @@ static int c_can_do_rx_poll(struct net_d
 					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;
 			}
author
#14 lore raw

PATCH 6/12 can: c_can: Remove braindamaged EOB exit

In reply to Thomas Gleixner

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 the brainfart.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |    3 ---
 1 file changed, 3 deletions(-)
drivers/net/can/c_can/c_can.c +0 -3
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -906,9 +906,6 @@ static int c_can_do_rx_poll(struct net_d
 				continue;
 			}
 
-			if (msg_ctrl_save & IF_MCONT_EOB)
-				return num_rx_pkts;
-
 			if (!(msg_ctrl_save & IF_MCONT_NEWDAT))
 				continue;
author
#15 lore raw

PATCH 7/12 can: c_can: Provide protection in the xmit path

In reply to Thomas Gleixner

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 <[email protected]>
---
 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(-)
drivers/net/can/c_can/c_can.c +8 -1
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -549,6 +549,7 @@ static netdev_tx_t c_can_start_xmit(stru
 	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(stru
 	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;
 }

@@ -778,7 +780,9 @@ static void c_can_do_tx(struct net_devic
 	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)))) {

@@ -800,6 +804,8 @@ static void c_can_do_tx(struct net_devic
 	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);
 }
 
 /*

@@ -1250,6 +1256,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;
drivers/net/can/c_can/c_can.h +1 -0
Index: linux/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;
author
#16 lore raw

PATCH 8/12 can: c_can: Makethe code readable

In reply to Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can.c |  107 +++++++++++++++++++++---------------------
 1 file changed, 56 insertions(+), 51 deletions(-)
drivers/net/can/c_can/c_can.c +56 -51
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -840,6 +840,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--;
+	} while ((obj = ffs(pend)) && quota > 0);
+
+	return pkts;
+}
+
 /*
  * theory of operation:
  *

@@ -864,10 +910,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

@@ -877,65 +921,26 @@ static int c_can_do_rx_poll(struct net_d
 			"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)
#17 lore raw

Re: [patch 08/12] can: c_can: Makethe code readable

On Tue, 2014-03-18 at 17:19 +0000, Thomas Gleixner wrote:

[patch]

Hi

The multi-while loop

	while ((obj = ffs(pend)) && quota > 0) {
		...
	} while ((obj = ffs(pend)) && quota > 0);

looks, umm, unusual.

author
#18 lore raw

Re: [patch 08/12] can: c_can: Makethe code readable

Quoted 6 lines · Joe Perches

On Tue, 2014-03-18 at 17:19 +0000, Thomas Gleixner wrote:

[patch]

Hi

Quoted 7 lines

The multi-while loop

	while ((obj = ffs(pend)) && quota > 0) {
		...
	} while ((obj = ffs(pend)) && quota > 0);

looks, umm, unusual.

Indeed. That's a leftover of a previous variant. I'll sent out a corrected one.

Thanks

tglx

author
#19 lore raw

PATCH v2 8/12 can: c_can: Makethe code readable

In reply to Joe Perches

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 <[email protected]>

---
V2: Remove the pointless multiwhile. Noted by Joe Perches

Sorry for the noise.

 drivers/net/can/c_can/c_can.c |  107 +++++++++++++++++++++---------------------
 1 file changed, 56 insertions(+), 51 deletions(-)
drivers/net/can/c_can/c_can.c +56 -51
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -840,6 +840,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:
  *

@@ -864,10 +910,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

@@ -877,65 +921,26 @@ static int c_can_do_rx_poll(struct net_d
 			"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)
#20 lore raw

Re: [patch 08/12 V2] can: c_can: Makethe code readable

Quoted 1 line · Thomas Gleixner

V2: Remove the pointless multiwhile.

fyi, I didn't find another multiwhile in the kernel tree.

author
#21 lore raw

PATCH 9/12 can: c_can: Reduce register access for real

In reply to Thomas Gleixner

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 ....

Now Mr. Optimize totally missed that there is a way worse issue due to the implementation of msg objects handling.

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. I leave that analysis to Mr. Optimize.

Signed-off-by Thomas Gleixner <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   49 ++++++++++++------------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)
drivers/net/can/c_can/c_can.c +15 -34
Index: linux/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(struc
 	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_lo
 
 	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)
 {

@@ -843,12 +826,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) {

@@ -870,12 +856,7 @@ static int c_can_read_objects(struct net
 		/* 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);
author
#22 lore raw

PATCH 10/12 can: c_can: Store dlc private

In reply to Thomas Gleixner

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 <[email protected]>
---
 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(-)
drivers/net/can/c_can/c_can.c +1 -27
Index: linux/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(stru
 
 	/* 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);
 
 	/*
drivers/net/can/c_can/c_can.h +28 -0
Index: linux/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);
author
#23 lore raw

PATCH 11/12 can: c_can: Simplify TX interrupt cleanup

In reply to Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can.c |   37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)
drivers/net/can/c_can/c_can.c +17 -20
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -232,10 +232,9 @@ static inline int get_tx_next_msg_obj(co
 			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)

@@ -720,8 +719,6 @@ static int c_can_get_berr_counter(const
 }
 
 /*
- * 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.

@@ -732,29 +729,23 @@ static int c_can_get_berr_counter(const
  */
 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 */

@@ -763,6 +754,12 @@ static void c_can_do_tx(struct net_devic
 		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);
+	}
 }
 
 /*
author
#24 lore raw

PATCH 12/12 can: c_can: Avoid led toggling for every packet.

In reply to Thomas Gleixner

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 <[email protected]>
---
 drivers/net/can/c_can/c_can.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/net/can/c_can/c_can.c +4 -3
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -433,9 +433,6 @@ static int c_can_read_msg_object(struct
 
 	stats->rx_packets++;
 	stats->rx_bytes += frame->can_dlc;
-
-	can_led_event(dev, CAN_LED_EVENT_RX);
-
 	return 0;
 }
 

@@ -892,6 +889,10 @@ static int c_can_do_rx_poll(struct net_d
 		pkts += n;
 		quota -= n;
 	}
+
+	if (pkts)
+		can_led_event(dev, CAN_LED_EVENT_RX);
+
 	return pkts;
 }
author
#25 lore raw

can: c_can: Reduce interrupt load by 50%

The driver handles pointlessly TWO interrupts per packet. The reason is that it enables the status interrupt which fires for each rx and tx packet and it enables the per message object interrupts as well.

The status interrupt merily acks the TX/RX state and then the message object interrupt fires.

The message objects interrupts are only useful if all message objects have hardware filters activated.

But we don't have that and its not simple to implement in that driver without rewriting it completely.

So we can ditch the message object interrupts and handle the RX/TX right away from the status interrupt. Instead of TWO we handle ONE. That's a whopping performance improment, right?

If we ever have the need for HW filtering, then this code needs a complete overhaul and we can think about it then. For now we prefer a lower interrupt load.

Signed-off-by Thomas Gleixner <[email protected]>

---

Applies on top of the previous series.

 drivers/net/can/c_can/c_can.c |  129 ++++++++++++++++++------------------------
 drivers/net/can/c_can/c_can.h |    2 
 2 files changed, 57 insertions(+), 74 deletions(-)
drivers/net/can/c_can/c_can.c +56 -73
Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
@@ -346,8 +346,7 @@ static void c_can_write_msg_object(struc
 
 	/* enable interrupt for this message object */
 	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface),
-			IF_MCONT_TXIE | IF_MCONT_TXRQST | IF_MCONT_EOB |
-			frame->can_dlc);
+			IF_MCONT_TXRQST | IF_MCONT_EOB | frame->can_dlc);
 	c_can_object_put(dev, iface, objno, IF_COMM_ALL);
 }
 

@@ -594,11 +593,10 @@ static void c_can_configure_msg_objects(
 
 	/* 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, IF_RX, i, 0, 0,
-			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
+		c_can_setup_receive_object(dev, IF_RX, i, 0, 0, IF_MCONT_UMASK);
 
 	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);
+				   IF_MCONT_EOB | IF_MCONT_UMASK);
 }
 
 /*

@@ -656,8 +654,9 @@ static void c_can_start(struct net_devic
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	/* reset tx helper pointers */
+	/* reset tx helper pointers and the rx mask */
 	priv->tx_next = priv->tx_echo = 0;
+	priv->rxmasked = 0;
 
 	/* enable status change, error and module interrupts */
 	c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);

@@ -824,9 +823,13 @@ static int c_can_read_objects(struct net
 		/* read the data from the message object */
 		c_can_read_msg_object(dev, IF_RX, ctrl);
 
-		if (obj == C_CAN_MSG_RX_LOW_LAST)
+		if (obj < C_CAN_MSG_RX_LOW_LAST)
+			priv->rxmasked |= BIT(obj - 1);
+		else if (obj == C_CAN_MSG_RX_LOW_LAST) {
+			priv->rxmasked = 0;
 			/* activate all lower message objects */
 			c_can_activate_all_lower_rx_msg_obj(dev, IF_RX, ctrl);
+		}
 
 		pkts++;
 		quota--;

@@ -871,7 +874,8 @@ static int c_can_do_rx_poll(struct net_d
 
 	while (quota > 0) {
 		if (!pend) {
-			pend = priv->read_reg(priv, C_CAN_INTPND1_REG);
+			pend = priv->read_reg(priv, C_CAN_NEWDAT1_REG);
+			pend &= ~priv->rxmasked;
 			if (!pend)
 				break;
 			/*

@@ -1046,79 +1050,58 @@ static int c_can_handle_bus_err(struct n
 
 static int c_can_poll(struct napi_struct *napi, int quota)
 {
-	u16 irqstatus;
-	int lec_type = 0;
-	int work_done = 0;
 	struct net_device *dev = napi->dev;
 	struct c_can_priv *priv = netdev_priv(dev);
+	u16 curr = priv->current_status, last = priv->last_status;
+	int lec_type = 0, work_done = 0;
 
-	irqstatus = priv->irqstatus;
-	if (!irqstatus)
-		goto end;
-
-	/* status events have the highest priority */
-	if (irqstatus == STATUS_INTERRUPT) {
-		priv->current_status = priv->read_reg(priv,
-					C_CAN_STS_REG);
-
-		/* handle Tx/Rx events */
-		if (priv->current_status & STATUS_TXOK)
-			priv->write_reg(priv, C_CAN_STS_REG,
-					priv->current_status & ~STATUS_TXOK);
-
-		if (priv->current_status & STATUS_RXOK)
-			priv->write_reg(priv, C_CAN_STS_REG,
-					priv->current_status & ~STATUS_RXOK);
-
-		/* handle state changes */
-		if ((priv->current_status & STATUS_EWARN) &&
-				(!(priv->last_status & STATUS_EWARN))) {
-			netdev_dbg(dev, "entered error warning state\n");
-			work_done += c_can_handle_state_change(dev,
-						C_CAN_ERROR_WARNING);
-		}
-		if ((priv->current_status & STATUS_EPASS) &&
-				(!(priv->last_status & STATUS_EPASS))) {
-			netdev_dbg(dev, "entered error passive state\n");
-			work_done += c_can_handle_state_change(dev,
-						C_CAN_ERROR_PASSIVE);
-		}
-		if ((priv->current_status & STATUS_BOFF) &&
-				(!(priv->last_status & STATUS_BOFF))) {
-			netdev_dbg(dev, "entered bus off state\n");
-			work_done += c_can_handle_state_change(dev,
-						C_CAN_BUS_OFF);
-		}
+	curr = priv->current_status = priv->read_reg(priv, C_CAN_STS_REG);
 
-		/* handle bus recovery events */
-		if ((!(priv->current_status & STATUS_BOFF)) &&
-				(priv->last_status & STATUS_BOFF)) {
-			netdev_dbg(dev, "left bus off state\n");
-			priv->can.state = CAN_STATE_ERROR_ACTIVE;
-		}
-		if ((!(priv->current_status & STATUS_EPASS)) &&
-				(priv->last_status & STATUS_EPASS)) {
-			netdev_dbg(dev, "left error passive state\n");
-			priv->can.state = CAN_STATE_ERROR_ACTIVE;
-		}
+	/* handle state changes */
+	if ((curr & STATUS_EWARN) && (!(last & STATUS_EWARN))) {
+		netdev_dbg(dev, "entered error warning state\n");
+		work_done += c_can_handle_state_change(dev, C_CAN_ERROR_WARNING);
+	}
+
+	if ((curr & STATUS_EPASS) && (!(last & STATUS_EPASS))) {
+		netdev_dbg(dev, "entered error passive state\n");
+		work_done += c_can_handle_state_change(dev, C_CAN_ERROR_PASSIVE);
+	}
 
-		priv->last_status = priv->current_status;
+	if ((curr & STATUS_BOFF) && (!(last & STATUS_BOFF))) {
+		netdev_dbg(dev, "entered bus off state\n");
+		work_done += c_can_handle_state_change(dev, C_CAN_BUS_OFF);
+	}
+
+	/* handle bus recovery events */
+	if ((!(curr & STATUS_BOFF)) && (last & STATUS_BOFF)) {
+		netdev_dbg(dev, "left bus off state\n");
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	}
+	if ((!(curr & STATUS_EPASS)) && (last & STATUS_EPASS)) {
+		netdev_dbg(dev, "left error passive state\n");
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+	}
+
+	priv->last_status = curr;
 
-		/* handle lec errors on the bus */
-		lec_type = c_can_has_and_handle_berr(priv);
-		if (lec_type)
-			work_done += c_can_handle_bus_err(dev, lec_type);
-	} else if ((irqstatus >= C_CAN_MSG_OBJ_RX_FIRST) &&
-			(irqstatus <= C_CAN_MSG_OBJ_RX_LAST)) {
-		/* handle events corresponding to receive message objects */
+	/* handle lec errors on the bus */
+	lec_type = c_can_has_and_handle_berr(priv);
+	if (lec_type)
+		work_done += c_can_handle_bus_err(dev, lec_type);
+
+
+	/* handle Tx/Rx events */
+	if (curr & STATUS_RXOK) {
+		priv->write_reg(priv, C_CAN_STS_REG, curr & ~STATUS_RXOK);
 		work_done += c_can_do_rx_poll(dev, (quota - work_done));
-	} else if ((irqstatus >= C_CAN_MSG_OBJ_TX_FIRST) &&
-			(irqstatus <= C_CAN_MSG_OBJ_TX_LAST)) {
-		/* handle events corresponding to transmit message objects */
+	}
+
+	if (curr & STATUS_TXOK) {
+		priv->write_reg(priv, C_CAN_STS_REG, curr & ~STATUS_TXOK);
 		c_can_do_tx(dev);
 	}
 
-end:
 	if (work_done < quota) {
 		napi_complete(napi);
 		/* enable all IRQs */

@@ -1132,9 +1115,9 @@ static irqreturn_t c_can_isr(int irq, vo
 {
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct c_can_priv *priv = netdev_priv(dev);
+	u32 stat = priv->read_reg(priv, C_CAN_INT_REG);
 
-	priv->irqstatus = priv->read_reg(priv, C_CAN_INT_REG);
-	if (!priv->irqstatus)
+	if (!stat)
 		return IRQ_NONE;
 
 	/* disable all interrupts and schedule the NAPI */
drivers/net/can/c_can/c_can.h +1 -1
Index: linux/drivers/net/can/c_can/c_can.h
===================================================================
@@ -195,11 +195,11 @@ struct c_can_priv {
 	unsigned int tx_next;
 	unsigned int tx_echo;
 	void *priv;		/* for board-specific data */
-	u16 irqstatus;
 	enum c_can_dev_id type;
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
+	u32 rxmasked;
 	u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
 };
#26 lore raw

Re: can: c_can: Reduce interrupt load by 50%

Quoted 3 lines · Thomas Gleixner

The driver handles pointlessly TWO interrupts per packet. The reason is that it enables the status interrupt which fires for each rx and tx packet and it enables the per message object interrupts as well.

Are the message object interrupts a possible workaround for any defective hardware?

Quoted 1 line

performance improment

Now that the code's not being written by amateurs?

author
#27 lore raw

Re: can: c_can: Reduce interrupt load by 50%

Quoted 7 lines · Joe Perches
Quoted 3 lines · Thomas Gleixner

The driver handles pointlessly TWO interrupts per packet. The reason is that it enables the status interrupt which fires for each rx and tx packet and it enables the per message object interrupts as well.

Are the message object interrupts a possible workaround for any defective hardware?

Nope. I already explained what they are for, but you snipped that part.

Quoted 3 lines
Quoted 1 line

performance improment

Now that the code's not being written by amateurs?

You found a typo. Great, you can keep it!

#28 lore raw

Re: can: c_can: Reduce interrupt load by 50%

Quoted 5 lines · Thomas Gleixner
Quoted 3 lines · Joe Perches
Quoted 1 line · Thomas Gleixner

performance improment

Now that the code's not being written by amateurs?

You found a typo. Great, you can keep it!

Given it's in a commit log, we all get to keep it.

Please do try to ratchet down the snark level of your commit messages.

cheers, Joe

author
#29 lore raw
In reply to Thomas Gleixner

Dear Maintainers,

Quoted 9 lines · Thomas Gleixner

The driver is full of serious bugs:

- Two HW init routines are not spec compliant.

- Completely defective message buffer handling in several ways That leads to interrupt storms and complete lockups.

- Complete lack of SMP awareness

Quoted 7 lines

What's amazing is that people "optimize" and "fix" the driver over and over, but nobody bothered to understand the manual and repair the code for real.

The series fixes _ALL_ bugs which I found so far, but I'm sure there are more issues burried in that unreadable mess. I'm just not able to trigger them.

What's the state of this series?

The only reaction so far was a hasty commit of cosmetic add ons to my findings at [1] accompanied with the following mail which was not a direct reply to my patches but merily a new mail to the can mailing list:

" Subject: [PATCH 1/2] can: c_can: improve error checking

as mentioned in the other mail, a patch that adds return value checking to all users of c_can_set_bittiming(). Another patch adds the missing netif_napi_del(). Feel free to include the patches in your series or use the testing-c_can branch [1] as your git base.
"

I'm really grateful, that you allowed me to include these patches to my series. But that does not help anything at all:

- The driver is still broken as it has been for years

- You as the maintainer reacted by submitting a pointless patch to base my series on instead of applying the obvious and well documented bug fixes right away.

- Just for the record, the series fixes the fatal issues of that driver with and without the extra cosmetic fixes which are just a supplement for the real issues.

What's going on here? Are we going to have another few kernel releases with a completely defunct driver in place?

Thanks,

tglx
---
[1] git://gitorious.org/linux-can/linux-can-next.git testing-c_can

maintainer
#30 lore raw
Quoted 16 lines · Thomas Gleixner

Dear Maintainers,

Quoted 13 lines · Thomas Gleixner

The driver is full of serious bugs:

- Two HW init routines are not spec compliant.

- Completely defective message buffer handling in several ways That leads to interrupt storms and complete lockups.

- Complete lack of SMP awareness

What's amazing is that people "optimize" and "fix" the driver over and over, but nobody bothered to understand the manual and repair the code for real.

Quoted 5 lines
Quoted 3 lines

The series fixes _ALL_ bugs which I found so far, but I'm sure there are more issues burried in that unreadable mess. I'm just not able to trigger them.

What's the state of this series?

I'll apply your patches today.

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   |
author
#31 lore raw
Quoted 17 lines · Marc Kleine-Budde
Quoted 16 lines · Thomas Gleixner

Dear Maintainers,

Quoted 13 lines · Thomas Gleixner

The driver is full of serious bugs:

- Two HW init routines are not spec compliant.

- Completely defective message buffer handling in several ways That leads to interrupt storms and complete lockups.

- Complete lack of SMP awareness

What's amazing is that people "optimize" and "fix" the driver over and over, but nobody bothered to understand the manual and repair the code for real.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

The series fixes _ALL_ bugs which I found so far, but I'm sure there are more issues burried in that unreadable mess. I'm just not able to trigger them.

What's the state of this series?

I'll apply your patches today.

Thanks! Please drop the last one which was not part of the series:

Subject: can: c_can: Reduce interrupt load by 50%

While it works like a charm, we've seen an odd case were TX started to stall. Had not yet time to dig into that.

Thanks,

tglx

maintainer
#32 lore raw
Quoted 10 lines · Thomas Gleixner
Quoted 3 lines
Quoted 1 line

What's the state of this series?

I'll apply your patches today.

Thanks! Please drop the last one which was not part of the series:

Subject: can: c_can: Reduce interrupt load by 50%

While it works like a charm, we've seen an odd case were TX started to stall. Had not yet time to dig into that.

Okay, thanks for the update.

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   |
maintainer
#33 lore raw
In reply to Marc Kleine-Budde
Quoted 3 lines · Marc Kleine-Budde
Quoted 1 line

What's the state of this series?

I'll apply your patches today.

I've applied all patches of this series (with some with minor tweaks of the subject and/or commit message) to the can tree and skipped the non-series "can: c_can: Reduce interrupt load by 50%".

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   |