[patch 00/12] can: c_can: Fix a series of serious bugs and improve the performance

[patch 00/12] can: c_can: Fix a series of serious bugs and improve the performance

[Thomas Gleixner]

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(-)

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

[Thomas Gleixner]

[-- Attachment #1: can-c_can-wait-for-raminit.patch --]
[-- Type: text/plain, Size: 2808 bytes --]

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 <tglx@linutronix.de>
---
 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
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can_platform.c
+++ 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[] = {

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

[Marc Kleine-Budde]

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

On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> 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 <tglx@linutronix.de>
> ---
>  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
> ===================================================================
> --- linux.orig/drivers/net/can/c_can/c_can_platform.c
> +++ 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);

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.

> +}
> +
>  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.
> +	 */

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.

> +	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[] = {

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

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

[Thomas Gleixner]

On Tue, 18 Mar 2014, Marc Kleine-Budde wrote:
> On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> > +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

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

[Oliver Hartkopp]

On 18.03.2014 23:15, Thomas Gleixner wrote:
> On Tue, 18 Mar 2014, Marc Kleine-Budde wrote:
>> On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
>>> +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.

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

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

[Thomas Gleixner]

On Wed, 19 Mar 2014, Oliver Hartkopp wrote:
> On 18.03.2014 23:15, Thomas Gleixner wrote:
> > On Tue, 18 Mar 2014, Marc Kleine-Budde wrote:
> >> On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> >>> +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.
> 
> 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

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

[Thomas Gleixner]

[-- Attachment #1: cac-c_can-wait-for-control-init.patch --]
[-- Type: text/plain, Size: 2037 bytes --]

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 <b.spranger@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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);
 }
 
 /*

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

[Marc Kleine-Budde]

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

On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> According to the documentation the CPU must wait for CONTROL_INIT to
> be cleared before writing to the baudrate registers.

Thanks for the catch.

> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> ---
>  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
> ===================================================================
> --- linux.orig/drivers/net/can/c_can/c_can.c
> +++ 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);

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

>  }
>  
>  /*

Thanks,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

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

[Thomas Gleixner]

On Tue, 18 Mar 2014, Marc Kleine-Budde wrote:
> On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> > According to the documentation the CPU must wait for CONTROL_INIT to
> > be cleared before writing to the baudrate registers.
> 
> Thanks for the catch.
> > +	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

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

[Thomas Gleixner]

[-- Attachment #1: can-c_can-use-if1-2.patch --]
[-- Type: text/plain, Size: 4418 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |   36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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--;

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

[Marc Kleine-Budde]

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

On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> 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.

Do both c_can and d_can have two control interfaces?

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Otherwise, looks good.

Tnx,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

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

[Thomas Gleixner]

On Tue, 18 Mar 2014, Marc Kleine-Budde wrote:

> On 03/18/2014 06:19 PM, Thomas Gleixner wrote:
> > 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.
> 
> Do both c_can and d_can have two control interfaces?

According to the manual it has.

Thanks,

	tglx

[patch 04/12] can: c_can: Fix buffer ordering for real

[Thomas Gleixner]

[-- Attachment #1: can-c-can-fix-buffer-handling.patch --]
[-- Type: text/plain, Size: 3070 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |   52 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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);
 

[patch 05/12] can: c_can: Fix the lost message handling

[Thomas Gleixner]

[-- Attachment #1: can-c-can-fix-msg-lost-handling.patch --]
[-- Type: text/plain, Size: 3343 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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;
 			}
 

[patch 06/12] can: c_can: Remove braindamaged EOB exit

[Thomas Gleixner]

[-- Attachment #1: can-c-can-fix-eob-handling.patch --]
[-- Type: text/plain, Size: 1921 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |    3 ---
 1 file changed, 3 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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;
 

[patch 07/12] can: c_can: Provide protection in the xmit path

[Thomas Gleixner]

[-- Attachment #1: can-c_can-fix-xmit.patch --]
[-- Type: text/plain, Size: 2600 bytes --]

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 <tglx@linutronix.de>
---
 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(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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;
Index: linux/drivers/net/can/c_can/c_can.h
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.h
+++ 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;

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

[Thomas Gleixner]

[-- Attachment #1: can-c-can-make-code-readable.patch --]
[-- Type: text/plain, Size: 4302 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |  107 +++++++++++++++++++++---------------------
 1 file changed, 56 insertions(+), 51 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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)

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

[Joe Perches]

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.

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

[Thomas Gleixner]

On Tue, 18 Mar 2014, Joe Perches wrote:

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

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

Thanks

	tglx

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

[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 <tglx@linutronix.de>
---
 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(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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)

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

[Joe Perches]

On Tue, 2014-03-18 at 19:27 +0100, Thomas Gleixner wrote:
>  V2: Remove the pointless multiwhile.

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

[patch 09/12] can: c_can: Reduce register access for real

[Thomas Gleixner]

[-- Attachment #1: can-c-can-reduce-ctrl-register-access.patch --]
[-- Type: text/plain, Size: 5655 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |   49 ++++++++++++------------------------------
 1 file changed, 15 insertions(+), 34 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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);
 

[patch 11/12] can: c_can: Simplify TX interrupt cleanup

[Thomas Gleixner]

[-- Attachment #1: can-c_can-simplify-xmit.patch --]
[-- Type: text/plain, Size: 2925 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |   37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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);
+	}
 }
 
 /*

[patch 10/12] can: c_can: Store dlc private

[Thomas Gleixner]

[-- Attachment #1: can-c_can-store-dlc-private.patch --]
[-- Type: text/plain, Size: 3316 bytes --]

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 <tglx@linutronix.de>
---
 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(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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);
 
 	/*
Index: linux/drivers/net/can/c_can/c_can.h
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.h
+++ 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);

[patch 12/12] can: c_can: Avoid led toggling for every packet.

[Thomas Gleixner]

[-- Attachment #1: can-c_can-move-rx-led-toggle.patch --]
[-- Type: text/plain, Size: 831 bytes --]

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 <tglx@linutronix.de>
---
 drivers/net/can/c_can/c_can.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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;
 }
 

can: c_can: Reduce interrupt load by 50%

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

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 <tglx@linutronix.de>
---

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(-)

Index: linux/drivers/net/can/c_can/c_can.c
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.c
+++ 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 */
Index: linux/drivers/net/can/c_can/c_can.h
===================================================================
--- linux.orig/drivers/net/can/c_can/c_can.h
+++ 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];
 };
 

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

[Joe Perches]

On Tue, 2014-03-18 at 21:18 +0100, Thomas Gleixner wrote:
> 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?

> performance improment

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

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

[Thomas Gleixner]

On Tue, 18 Mar 2014, Joe Perches wrote:

> On Tue, 2014-03-18 at 21:18 +0100, Thomas Gleixner wrote:
> > 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.
 
> > performance improment
> 
> Now that the code's not being written by amateurs?

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

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

[Joe Perches]

On Tue, 2014-03-18 at 21:43 +0100, Thomas Gleixner wrote:
> On Tue, 18 Mar 2014, Joe Perches wrote:
> > On Tue, 2014-03-18 at 21:18 +0100, Thomas Gleixner wrote:
> > > 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

Re: [patch 00/12] can: c_can: Fix a series of serious bugs and improve the performance

[Thomas Gleixner]

Dear Maintainers,

On Tue, 18 Mar 2014, Thomas Gleixner wrote:
> 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.

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

Re: [patch 00/12] can: c_can: Fix a series of serious bugs and improve the performance

[Marc Kleine-Budde]

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

On 04/01/2014 12:35 AM, Thomas Gleixner wrote:
> Dear Maintainers,
> 
> On Tue, 18 Mar 2014, Thomas Gleixner wrote:
>> 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.
> 
> What's the state of this series?

I'll apply your patches today.

regards,
Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [patch 00/12] can: c_can: Fix a series of serious bugs and improve the performance

[Thomas Gleixner]

On Tue, 1 Apr 2014, Marc Kleine-Budde wrote:
> On 04/01/2014 12:35 AM, Thomas Gleixner wrote:
> > Dear Maintainers,
> > 
> > On Tue, 18 Mar 2014, Thomas Gleixner wrote:
> >> 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.
> > 
> > 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

Re: [patch 00/12] can: c_can: Fix a series of serious bugs and improve the performance

[Marc Kleine-Budde]

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

On 04/01/2014 11:07 AM, Thomas Gleixner wrote:
>>> 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

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [patch 00/12] can: c_can: Fix a series of serious bugs and improve the performance

[Marc Kleine-Budde]

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

On 04/01/2014 10:09 AM, Marc Kleine-Budde wrote:
>> 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

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]