Re: Flexcan (was: Re: Fwd: Querying current tx_queue usage of a SocketCAN interface)

[Tom Evans <tom_usenet@optusnet.com.au>]

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

On 2/04/2015 5:28 PM, Marc Kleine-Budde wrote:
> On 04/02/2015 04:20 AM, Tom Evans wrote:
>> I had to seriously rewrite the FlexCAN drivers for our i.MX53 because the
>> Ethernet driver blocked the CAN driver for so long the CAN hardware receive
>> FIFO overflowed. Linux doesn't even support setting different interrupt
>> priorities on ARM (but my problem was with NAPI).
>
> Can you share your changes? I'll have some days to hack on a better
> generic RX-FIFO implementation for the at91 and flexcan.

Sure. "git diff -p" based of the 3.4 kernel sources.

Working from home over Easter using git under cygwin under Windows via 
Thunderbird, so the CRLFs and line breaks could be messed up, sorry. 
Patches inline and attached. If this lot is useless, tell me what to 
type to do it properly and I'll resend on Tuesday.

flexcan.c.patch.buffer

The problem with the FlexCan driver is that it takes the interrupt, then 
throws to NAPI to actually read the hardware buffers. How messed up is 
that? It isn't as if reading 16 bytes or so is going to take THAT long, 
and as there's only six message buffers it overflows in 600us at 1MHz. 
In the same kernel, the Ethernet driver DOESN'T throw to NAPI but 
handles as many 1500-byte Ethernet frames as there are coming in in the 
one ISR. Other things can delay NAPI as well. So I fixed it with a 
sledgehammer. It reads the messages during the ISR to an internal ring 
buffer (sized to 100, later to 128)

flexcan.c.patch.flood

A disconnected CAN bus generate an interrupt storm. This patch fixes it 
the same way it is apparently done in some 3.18 drivers. This may 
already be in there.

flexcan.c.patch.buffer

---------------------------------------------------

diff --git a/arm-linux/drivers/net/can/flexcan.c 
b/arm-linux/drivers/net/can/flexcan.c
old mode 100644
new mode 100755
index 14c8a86..96e0b55
--- a/arm-linux/drivers/net/can/flexcan.c
+++ b/arm-linux/drivers/net/can/flexcan.c
@@ -43,6 +43,9 @@
  /* 8 for RX fifo and 2 error handling */
  #define FLEXCAN_NAPI_WEIGHT		(8 + 2)

+/* Buffers in the interrupt-read ring, 10ms@100us per. */
+#define FLEXCAN_READ_RING_SIZE		(100)
+
  /* FLEXCAN module configuration register (CANMCR) bits */
  #define FLEXCAN_MCR_MDIS		BIT(31)
  #define FLEXCAN_MCR_FRZ			BIT(30)
@@ -170,6 +173,14 @@ struct flexcan_regs {
  	struct flexcan_mb cantxfg[64];
  };

+struct flexcan_ring {
+	u32 input;		/* Ring input index */
+	u32 output;		/* Ring output index */
+	u32 full;		/* ring full/wrapped flag */
+	spinlock_t ring_lock;
+	struct can_frame frames[FLEXCAN_READ_RING_SIZE];
+};
+
  struct flexcan_priv {
  	struct can_priv can;
  	struct net_device *dev;
@@ -182,6 +193,7 @@ struct flexcan_priv {
  	struct clk *clk;
  	struct flexcan_platform_data *pdata;
  	struct gpio_sw *xcvr_switch;
+	struct flexcan_ring read_ring;
  };

  static struct can_bittiming_const flexcan_bittiming_const = {
@@ -532,12 +544,70 @@ static int flexcan_read_frame(struct net_device *dev)
  	return 1;
  }

+static int flexcan_read_frame_to_ring(struct net_device *dev)
+{
+	struct net_device_stats *stats = &dev->stats;
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct can_frame *cf;
+
+	cf = &priv->read_ring.frames[priv->read_ring.input];
+
+	/* Read even if ring full to clear the interrupt request */
+	flexcan_read_fifo(dev, cf);
+
+	if (priv->read_ring.full) {
+		stats->rx_dropped++;
+		return 0;
+	}
+	priv->read_ring.input += 1;
+	if (priv->read_ring.input >= FLEXCAN_READ_RING_SIZE) {
+		priv->read_ring.input  = 0;
+	}
+	if (priv->read_ring.input == priv->read_ring.output) {
+		priv->read_ring.full = 1;
+	}
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+
+	return 1;
+}
+
+static int flexcan_read_frame_from_ring(struct net_device *dev)
+{
+	struct net_device_stats *stats = &dev->stats;
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	unsigned long flags;
+
+	skb = alloc_can_skb(dev, &cf);
+	if (unlikely(!skb)) {
+		stats->rx_dropped++;
+		return 1;	/* was 0, but that's an infinite loop bug */
+	}
+	/* Assumes that the ring-empty test already done */
+	memcpy(cf, &priv->read_ring.frames[priv->read_ring.output], sizeof(*cf));
+
+	spin_lock_irqsave(&priv->read_ring.ring_lock, flags);
+	priv->read_ring.full = 0;
+	priv->read_ring.output += 1;
+	if (priv->read_ring.output >= FLEXCAN_READ_RING_SIZE) {
+		priv->read_ring.output = 0;
+	}
+	spin_unlock_irqrestore(&priv->read_ring.ring_lock, flags);
+
+	netif_receive_skb(skb);
+
+	return 1;
+}
+
  static int flexcan_poll(struct napi_struct *napi, int quota)
  {
  	struct net_device *dev = napi->dev;
-	const struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_priv *priv = netdev_priv(dev);
  	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg_iflag1, reg_esr;
+	u32 reg_esr;
  	int work_done = 0;

  	/*
@@ -545,16 +615,16 @@ static int flexcan_poll(struct napi_struct *napi, 
int quota)
  	 * use saved value from irq handler.
  	 */
  	reg_esr = flexcan_read(&regs->esr) | priv->reg_esr;
+	priv->reg_esr = 0;

  	/* handle state changes */
  	work_done += flexcan_poll_state(dev, reg_esr);

-	/* handle RX-FIFO */
-	reg_iflag1 = flexcan_read(&regs->iflag1);
-	while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
+	/* handle RX-RING */
+	while (((priv->read_ring.full) ||
+		(priv->read_ring.input != priv->read_ring.output)) &&
  	       work_done < quota) {
-		work_done += flexcan_read_frame(dev);
-		reg_iflag1 = flexcan_read(&regs->iflag1);
+		work_done += flexcan_read_frame_from_ring(dev);
  	}

  	/* report bus errors */
@@ -563,9 +633,6 @@ static int flexcan_poll(struct napi_struct *napi, 
int quota)

  	if (work_done < quota) {
  		napi_complete(napi);
-		/* enable IRQs */
-		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
-		flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
  	}

  	return work_done;
@@ -598,11 +665,14 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
  		 * The error bits are cleared on read,
  		 * save them for later use.
  		 */
-		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
-		flexcan_write(FLEXCAN_IFLAG_DEFAULT &
-			~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->imask1);
-		flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
-		       &regs->ctrl);
+		priv->reg_esr |= reg_esr & FLEXCAN_ESR_ERR_BUS;
+		/*
+		 * Read the FIFO into the rings.
+		 */
+		while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) {
+			flexcan_read_frame_to_ring(dev);
+			reg_iflag1 = flexcan_read(&regs->iflag1);
+		}
  		napi_schedule(&priv->napi);
  	}

@@ -1012,6 +1082,7 @@ static int __devinit flexcan_probe(struct 
platform_device *pdev)
  	priv->dev = dev;
  	priv->clk = clk;
  	priv->pdata = pdev->dev.platform_data;
+	spin_lock_init(&priv->read_ring.ring_lock);

  	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);

---------------------------------------------------

flexcan.c.patch.flood

---------------------------------------------------

diff --git a/arm-linux/drivers/net/can/flexcan.c 
b/arm-linux/drivers/net/can/flexcan.c
index 2b25548..f773cb6 100644
--- a/arm-linux/drivers/net/can/flexcan.c
+++ b/arm-linux/drivers/net/can/flexcan.c
@@ -857,11 +857,26 @@ static int flexcan_chip_start(struct net_device *dev)
  	 * _note_: we enable the "error interrupt"
  	 * (FLEXCAN_CTRL_ERR_MSK), too. Otherwise we don't get any
  	 * warning or bus passive interrupts.
+	 *
+	 * __note__: The above is rubbish and causes interrupt
+	 * flooding. Change it to the 3.18 way without of obeying
+	 * CAN_CTRL_MODE_BERR_REPORTING and without enabling the
+	 * Error interrupts.
  	 */
  	reg_ctrl = flexcan_read(&regs->ctrl);
  	reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
  	reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
-		FLEXCAN_CTRL_ERR_STATE | FLEXCAN_CTRL_ERR_MSK;
+		FLEXCAN_CTRL_ERR_STATE /* | FLEXCAN_CTRL_ERR_MSK */;
+
+	/*
+	* enable the "error interrupt" (FLEXCAN_CTRL_ERR_MSK),
+	* on most Flexcan cores, too. Otherwise we don't get
+	* any error warning or passive interrupts.
+	*/
+	if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+		reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
+	else
+		reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;

  	/* save for later use */
  	priv->reg_ctrl_default = reg_ctrl;

---------------------------------------------------

Tom


[-- Attachment #2: flexcan.c.patch.buffer --]
[-- Type: text/plain, Size: 5085 bytes --]

diff --git a/arm-linux/drivers/net/can/flexcan.c b/arm-linux/drivers/net/can/flexcan.c
old mode 100644
new mode 100755
index 14c8a86..96e0b55
--- a/arm-linux/drivers/net/can/flexcan.c
+++ b/arm-linux/drivers/net/can/flexcan.c
@@ -43,6 +43,9 @@
 /* 8 for RX fifo and 2 error handling */
 #define FLEXCAN_NAPI_WEIGHT		(8 + 2)
 
+/* Buffers in the interrupt-read ring, 10ms@100us per. */
+#define FLEXCAN_READ_RING_SIZE		(100)
+
 /* FLEXCAN module configuration register (CANMCR) bits */
 #define FLEXCAN_MCR_MDIS		BIT(31)
 #define FLEXCAN_MCR_FRZ			BIT(30)
@@ -170,6 +173,14 @@ struct flexcan_regs {
 	struct flexcan_mb cantxfg[64];
 };
 
+struct flexcan_ring {
+	u32 input;		/* Ring input index */
+	u32 output;		/* Ring output index */
+	u32 full;		/* ring full/wrapped flag */
+	spinlock_t ring_lock;
+	struct can_frame frames[FLEXCAN_READ_RING_SIZE];
+};
+
 struct flexcan_priv {
 	struct can_priv can;
 	struct net_device *dev;
@@ -182,6 +193,7 @@ struct flexcan_priv {
 	struct clk *clk;
 	struct flexcan_platform_data *pdata;
 	struct gpio_sw *xcvr_switch;
+	struct flexcan_ring read_ring;
 };
 
 static struct can_bittiming_const flexcan_bittiming_const = {
@@ -532,12 +544,70 @@ static int flexcan_read_frame(struct net_device *dev)
 	return 1;
 }
 
+static int flexcan_read_frame_to_ring(struct net_device *dev)
+{
+	struct net_device_stats *stats = &dev->stats;
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct can_frame *cf;
+
+	cf = &priv->read_ring.frames[priv->read_ring.input];
+
+	/* Read even if ring full to clear the interrupt request */
+	flexcan_read_fifo(dev, cf);
+
+	if (priv->read_ring.full) {
+		stats->rx_dropped++;
+		return 0;
+	}
+	priv->read_ring.input += 1;
+	if (priv->read_ring.input >= FLEXCAN_READ_RING_SIZE) {
+		priv->read_ring.input  = 0;
+	}
+	if (priv->read_ring.input == priv->read_ring.output) {
+		priv->read_ring.full = 1;
+	}
+
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+
+	return 1;
+}
+
+static int flexcan_read_frame_from_ring(struct net_device *dev)
+{
+	struct net_device_stats *stats = &dev->stats;
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	unsigned long flags;
+
+	skb = alloc_can_skb(dev, &cf);
+	if (unlikely(!skb)) {
+		stats->rx_dropped++;
+		return 1;	/* was 0, but that's an infinite loop bug */
+	}
+	/* Assumes that the ring-empty test already done */
+	memcpy(cf, &priv->read_ring.frames[priv->read_ring.output], sizeof(*cf));
+
+	spin_lock_irqsave(&priv->read_ring.ring_lock, flags);
+	priv->read_ring.full = 0;
+	priv->read_ring.output += 1;
+	if (priv->read_ring.output >= FLEXCAN_READ_RING_SIZE) {
+		priv->read_ring.output = 0;
+	}
+	spin_unlock_irqrestore(&priv->read_ring.ring_lock, flags);
+
+	netif_receive_skb(skb);
+
+	return 1;
+}
+
 static int flexcan_poll(struct napi_struct *napi, int quota)
 {
 	struct net_device *dev = napi->dev;
-	const struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg_iflag1, reg_esr;
+	u32 reg_esr;
 	int work_done = 0;
 
 	/*
@@ -545,16 +615,16 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 	 * use saved value from irq handler.
 	 */
 	reg_esr = flexcan_read(&regs->esr) | priv->reg_esr;
+	priv->reg_esr = 0;
 
 	/* handle state changes */
 	work_done += flexcan_poll_state(dev, reg_esr);
 
-	/* handle RX-FIFO */
-	reg_iflag1 = flexcan_read(&regs->iflag1);
-	while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
+	/* handle RX-RING */
+	while (((priv->read_ring.full) ||
+		(priv->read_ring.input != priv->read_ring.output)) &&
 	       work_done < quota) {
-		work_done += flexcan_read_frame(dev);
-		reg_iflag1 = flexcan_read(&regs->iflag1);
+		work_done += flexcan_read_frame_from_ring(dev);
 	}
 
 	/* report bus errors */
@@ -563,9 +633,6 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 
 	if (work_done < quota) {
 		napi_complete(napi);
-		/* enable IRQs */
-		flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
-		flexcan_write(priv->reg_ctrl_default, &regs->ctrl);
 	}
 
 	return work_done;
@@ -598,11 +665,14 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		 * The error bits are cleared on read,
 		 * save them for later use.
 		 */
-		priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS;
-		flexcan_write(FLEXCAN_IFLAG_DEFAULT &
-			~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->imask1);
-		flexcan_write(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL,
-		       &regs->ctrl);
+		priv->reg_esr |= reg_esr & FLEXCAN_ESR_ERR_BUS;
+		/*
+		 * Read the FIFO into the rings.
+		 */
+		while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) {
+			flexcan_read_frame_to_ring(dev);
+			reg_iflag1 = flexcan_read(&regs->iflag1);
+		}
 		napi_schedule(&priv->napi);
 	}
 
@@ -1012,6 +1082,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 	priv->dev = dev;
 	priv->clk = clk;
 	priv->pdata = pdev->dev.platform_data;
+	spin_lock_init(&priv->read_ring.ring_lock);
 
 	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);
 

[-- Attachment #3: flexcan.c.patch.flood --]
[-- Type: text/plain, Size: 1244 bytes --]

diff --git a/arm-linux/drivers/net/can/flexcan.c b/arm-linux/drivers/net/can/flexcan.c
index 2b25548..f773cb6 100644
--- a/arm-linux/drivers/net/can/flexcan.c
+++ b/arm-linux/drivers/net/can/flexcan.c
@@ -857,11 +857,26 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * _note_: we enable the "error interrupt"
 	 * (FLEXCAN_CTRL_ERR_MSK), too. Otherwise we don't get any
 	 * warning or bus passive interrupts.
+	 *
+	 * __note__: The above is rubbish and causes interrupt
+	 * flooding. Change it to the 3.18 way without of obeying
+	 * CAN_CTRL_MODE_BERR_REPORTING and without enabling the
+	 * Error interrupts.
 	 */
 	reg_ctrl = flexcan_read(&regs->ctrl);
 	reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
 	reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
-		FLEXCAN_CTRL_ERR_STATE | FLEXCAN_CTRL_ERR_MSK;
+		FLEXCAN_CTRL_ERR_STATE /* | FLEXCAN_CTRL_ERR_MSK */;
+
+	/*
+	* enable the "error interrupt" (FLEXCAN_CTRL_ERR_MSK),
+	* on most Flexcan cores, too. Otherwise we don't get
+	* any error warning or passive interrupts.
+	*/
+	if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+		reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
+	else
+		reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;
 
 	/* save for later use */
 	priv->reg_ctrl_default = reg_ctrl;