[PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
mailbox space capable of holding up to 63 messages.

This space was largely unused, limiting the permissible latency from interrupt
to NAPI to only 6 messages. This patch uses all available MBs for message
reception and frees the MBs in the IRQ handler to greatly decrease the
likelihood of receive overruns.

Signed-off-by: David Jander <david@protonic.nl>
---

Changed since v4:
 - Implemented RX copy-out as a ring buffer instead of a plain copy of the MB
   area.
 - Squashed in Marc's patch: make use of FLEXCAN_MB_CODE_
 - Moved CODE checking for OVERRUN from IRQ to NAPI call
 - Used a define (FLEXCAN_RX_AREA_START) to start for loops for RX MB looping.
 - Created and explained a define for FLEXCAN_MCR_MAXMB_USED

 drivers/net/can/flexcan.c | 324 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 243 insertions(+), 81 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 20f5c0e..62432dc 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2005-2006 Varma Electronics Oy
  * Copyright (c) 2009 Sascha Hauer, Pengutronix
  * Copyright (c) 2010 Marc Kleine-Budde, Pengutronix
+ * Copyright (c) 2014 David Jander, Protonic Holland
  *
  * Based on code originally by Andrey Volkov <avolkov@varma-el.com>
  *
@@ -24,6 +25,7 @@
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
 #include <linux/can/led.h>
+#include <linux/circ_buf.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/if_arp.h>
@@ -40,8 +42,8 @@
 
 #define DRV_NAME			"flexcan"
 
-/* 8 for RX fifo and 2 error handling */
-#define FLEXCAN_NAPI_WEIGHT		(8 + 2)
+/* 64 MB's */
+#define FLEXCAN_NAPI_WEIGHT		(64)
 
 /* FLEXCAN module configuration register (CANMCR) bits */
 #define FLEXCAN_MCR_MDIS		BIT(31)
@@ -63,6 +65,18 @@
 #define FLEXCAN_MCR_LPRIO_EN		BIT(13)
 #define FLEXCAN_MCR_AEN			BIT(12)
 #define FLEXCAN_MCR_MAXMB(x)		((x) & 0x7f)
+/*
+ * MCR_MAXMB:
+ * This field is 7 bits wide on V10 and newer flexcan cores, but the
+ * documentation of older versions states this field as being only 6 bits wide.
+ * This contradicts the fact that those cores still have 64 MB's, because with
+ * a 6-bit field the MAXMB value can never be higher than 63 (0x3f) which would
+ * implicate that the last MB could never be used. Since we cannot be sure
+ * whether just the documentation was wrong or older versions of the flexcan
+ * core can indeed not use the last MB, we chose the safest action and make
+ * this driver only use 63 MB's in every case for simplicity.
+ */
+#define FLEXCAN_MCR_MAXMB_USED		(0x3f)
 #define FLEXCAN_MCR_IDAM_A		(0 << 8)
 #define FLEXCAN_MCR_IDAM_B		(1 << 8)
 #define FLEXCAN_MCR_IDAM_C		(2 << 8)
@@ -113,6 +127,9 @@
 #define FLEXCAN_MECR_ECCDIS		BIT(8)
 #define FLEXCAN_MECR_NCEFAFRZ		BIT(7)
 
+/* FLEXCAN control register 2 (CTRL2) bits */
+#define FLEXCAN_CTRL2_EACEN		BIT(16)
+
 /* FLEXCAN error and status register (ESR) bits */
 #define FLEXCAN_ESR_TWRN_INT		BIT(17)
 #define FLEXCAN_ESR_RWRN_INT		BIT(16)
@@ -147,17 +164,16 @@
 
 /* FLEXCAN interrupt flag register (IFLAG) bits */
 /* Errata ERR005829 step7: Reserve first valid MB */
-#define FLEXCAN_TX_BUF_RESERVED		8
-#define FLEXCAN_TX_BUF_ID		9
+#define FLEXCAN_TX_BUF_RESERVED		0
+#define FLEXCAN_TX_BUF_ID		1
+#define FLEXCAN_RX_AREA_START		(FLEXCAN_TX_BUF_ID + 1)
 #define FLEXCAN_IFLAG_BUF(x)		BIT(x)
-#define FLEXCAN_IFLAG_RX_FIFO_OVERFLOW	BIT(7)
-#define FLEXCAN_IFLAG_RX_FIFO_WARN	BIT(6)
-#define FLEXCAN_IFLAG_RX_FIFO_AVAILABLE	BIT(5)
-#define FLEXCAN_IFLAG_DEFAULT \
-	(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW | FLEXCAN_IFLAG_RX_FIFO_AVAILABLE | \
-	 FLEXCAN_IFLAG_BUF(FLEXCAN_TX_BUF_ID))
+#define FLEXCAN_IFLAG1_DEFAULT 		(0xfffffffe)
+#define FLEXCAN_IFLAG2_DEFAULT 		(0xffffffff)
 
 /* FLEXCAN message buffers */
+#define FLEXCAN_MB_CODE_MASK		(0xf << 24)
+#define FLEXCAN_MB_CODE_RX_BUSY_BIT	(0x1 << 24)
 #define FLEXCAN_MB_CODE_RX_INACTIVE	(0x0 << 24)
 #define FLEXCAN_MB_CODE_RX_EMPTY	(0x4 << 24)
 #define FLEXCAN_MB_CODE_RX_FULL		(0x2 << 24)
@@ -175,8 +191,6 @@
 #define FLEXCAN_MB_CNT_LENGTH(x)	(((x) & 0xf) << 16)
 #define FLEXCAN_MB_CNT_TIMESTAMP(x)	((x) & 0xffff)
 
-#define FLEXCAN_MB_CODE_MASK		(0xf0ffffff)
-
 #define FLEXCAN_TIMEOUT_US             (50)
 
 /*
@@ -239,7 +253,9 @@ struct flexcan_regs {
 	 *			  	size conf'ed via ctrl2::RFFN
 	 *				(mx6, vf610)
 	 */
-	u32 _reserved4[408];
+	u32 _reserved4[256];	/* 0x480 */
+	u32 rximr[64];		/* 0x880 */
+	u32 _reserved5[88];	/* 0x980 */
 	u32 mecr;		/* 0xae0 */
 	u32 erriar;		/* 0xae4 */
 	u32 erridpr;		/* 0xae8 */
@@ -268,6 +284,10 @@ struct flexcan_priv {
 	struct flexcan_platform_data *pdata;
 	const struct flexcan_devtype_data *devtype_data;
 	struct regulator *reg_xceiver;
+	struct flexcan_mb cantxfg_copy[FLEXCAN_NAPI_WEIGHT];
+	bool second_first;
+	unsigned int rx_head;
+	unsigned int rx_tail;
 };
 
 static struct flexcan_devtype_data fsl_p1010_devtype_data = {
@@ -697,16 +717,38 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 	return 1;
 }
 
-static void flexcan_read_fifo(const struct net_device *dev,
-			      struct can_frame *cf)
+static int flexcan_read_frame(struct net_device *dev, int n)
 {
-	const struct flexcan_priv *priv = netdev_priv(dev);
-	struct flexcan_regs __iomem *regs = priv->base;
-	struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
+	struct net_device_stats *stats = &dev->stats;
+	struct flexcan_priv *priv = netdev_priv(dev);
+	struct flexcan_mb *mb = &priv->cantxfg_copy[n];
+	struct can_frame *cf;
+	struct sk_buff *skb;
 	u32 reg_ctrl, reg_id;
+	u32 code;
 
-	reg_ctrl = flexcan_read(&mb->can_ctrl);
-	reg_id = flexcan_read(&mb->can_id);
+	reg_ctrl = ACCESS_ONCE(mb->can_ctrl);
+	smp_rmb(); /* reg_ctrl is written last in IRQ */
+	code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+
+	/* is this MB empty? */
+	if ((code != FLEXCAN_MB_CODE_RX_FULL) &&
+	    (code != FLEXCAN_MB_CODE_RX_OVERRRUN))
+		return 0;
+
+	if (code == FLEXCAN_MB_CODE_RX_OVERRRUN) {
+		/* This MB was overrun, we lost data */
+		priv->dev->stats.rx_over_errors++;
+		priv->dev->stats.rx_errors++;
+	}
+
+	skb = alloc_can_skb(dev, &cf);
+	if (unlikely(!skb)) {
+		stats->rx_dropped++;
+		return 0;
+	}
+
+	reg_id = mb->can_id;
 	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
 		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
 	else
@@ -716,27 +758,9 @@ static void flexcan_read_fifo(const struct net_device *dev,
 		cf->can_id |= CAN_RTR_FLAG;
 	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
 
-	*(__be32 *)(cf->data + 0) = cpu_to_be32(flexcan_read(&mb->data[0]));
-	*(__be32 *)(cf->data + 4) = cpu_to_be32(flexcan_read(&mb->data[1]));
+	*(__be32 *)(cf->data + 0) = cpu_to_be32(mb->data[0]);
+	*(__be32 *)(cf->data + 4) = cpu_to_be32(mb->data[1]);
 
-	/* mark as read */
-	flexcan_write(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
-	flexcan_read(&regs->timer);
-}
-
-static int flexcan_read_frame(struct net_device *dev)
-{
-	struct net_device_stats *stats = &dev->stats;
-	struct can_frame *cf;
-	struct sk_buff *skb;
-
-	skb = alloc_can_skb(dev, &cf);
-	if (unlikely(!skb)) {
-		stats->rx_dropped++;
-		return 0;
-	}
-
-	flexcan_read_fifo(dev, cf);
 	netif_receive_skb(skb);
 
 	stats->rx_packets++;
@@ -750,10 +774,13 @@ static int flexcan_read_frame(struct net_device *dev)
 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;
+	int ret;
+	unsigned int head;
+	unsigned int tail;
 
 	/*
 	 * The error bits are cleared on read,
@@ -764,38 +791,174 @@ static int flexcan_poll(struct napi_struct *napi, int quota)
 	/* 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 &&
-	       work_done < quota) {
-		work_done += flexcan_read_frame(dev);
-		reg_iflag1 = flexcan_read(&regs->iflag1);
+	/* handle mailboxes */
+	head = smp_load_acquire(&priv->rx_head);
+	tail = priv->rx_tail;
+	while ((CIRC_CNT(head, tail, ARRAY_SIZE(priv->cantxfg_copy)) >= 1) &&
+			(work_done < quota)) {
+		ret = flexcan_read_frame(dev, tail);
+		work_done += ret;
+		tail = (tail + 1) & (ARRAY_SIZE(priv->cantxfg_copy) -1);
+		smp_store_release(&priv->rx_tail, tail);
 	}
 
 	/* report bus errors */
 	if (flexcan_has_and_handle_berr(priv, reg_esr) && work_done < quota)
 		work_done += flexcan_poll_bus_err(dev, reg_esr);
 
-	if (work_done < 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;
 }
 
+static u32 flexcan_copy_one_rxmb(struct flexcan_priv *priv, int i)
+{
+	struct flexcan_regs __iomem *regs = priv->base;
+	struct flexcan_mb __iomem *mb;
+	struct flexcan_mb *dst;
+	u32 reg_ctrl;
+	u32 code;
+	unsigned int head;
+	unsigned int tail;
+
+	mb = &regs->cantxfg[i];
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+
+	while (code & FLEXCAN_MB_CODE_RX_BUSY_BIT) {
+		/* MB busy, shouldn't take long */
+		reg_ctrl = flexcan_read(&mb->can_ctrl);
+		code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+	}
+
+	/* No need to process if neither FULL nor OVERRRUN */
+	if ((code != FLEXCAN_MB_CODE_RX_FULL) &&
+	    (code != FLEXCAN_MB_CODE_RX_OVERRRUN))
+		return code;
+
+	head = priv->rx_head;
+	tail = ACCESS_ONCE(priv->rx_tail);
+	if (CIRC_SPACE(head, tail, ARRAY_SIZE(priv->cantxfg_copy)) >= 1) {
+		/* Copy contents */
+		dst = &priv->cantxfg_copy[priv->rx_head];
+		dst->can_id = flexcan_read(&mb->can_id);
+		dst->data[0] = flexcan_read(&mb->data[0]);
+		dst->data[1] = flexcan_read(&mb->data[1]);
+		smp_wmb(); /* Write reg_ctrl last */
+		dst->can_ctrl = reg_ctrl;
+
+		smp_store_release(&priv->rx_head,
+			(head + 1) & (ARRAY_SIZE(priv->cantxfg_copy) - 1));
+	} else {
+		/* Buffer space overrun, we lost data */
+		priv->dev->stats.rx_over_errors++;
+		priv->dev->stats.rx_errors++;
+	}
+
+	/* Clear the interrupt flag and lock MB */
+	if (i < 32)
+		flexcan_write(BIT(i), &regs->iflag1);
+	else
+		flexcan_write(BIT(i - 32), &regs->iflag2);
+	flexcan_write(FLEXCAN_MB_CODE_RX_INACTIVE, &mb->can_ctrl);
+
+	return code;
+}
+
+static void flexcan_unlock_if_locked(struct flexcan_priv *priv, int i)
+{
+	struct flexcan_regs __iomem *regs = priv->base;
+	struct flexcan_mb __iomem *mb;
+	u32 reg_ctrl;
+	u32 code;
+
+	mb = &regs->cantxfg[i];
+	reg_ctrl = flexcan_read(&mb->can_ctrl);
+	code = reg_ctrl & FLEXCAN_MB_CODE_MASK;
+	if (code == FLEXCAN_MB_CODE_RX_INACTIVE)
+		flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY, &mb->can_ctrl);
+}
+
+/*
+ * flexcan_copy_rxmbs
+ *
+ * This function is called from interrupt and needs to make a safe copy
+ * of the MB area to offload the chip immediately to avoid loosing
+ * messages due to latency.
+ * To avoid loosing messages due to race-conditions while reading the MB's,
+ * we need to make use of the locking mechanism of FlexCAN. Unfortunately,
+ * in this mode, automatic locking applies _only_ to MBs that are _not_ EMPTY.
+ * This means we could identify a MB as EMPTY while it is about to get filled.
+ * To avoid this situation from disturbing our queue ordering, we do the
+ * following:
+ * We split the MB area into two halves:
+ * The lower 32 (-2 due to one TX-MB and errata ERR005829) and the upper 32.
+ * We start with all MBs in EMPTY state and all filters disabled (don't care).
+ * FlexCAN will start filling from the lowest MB. Once this function is called,
+ * we copy and disable in an atomic operation all FULL MBs. The first EMPTY one
+ * we encounter may be about to get filled so we stop there. Next time FlexCAN
+ * will have filled more MBs. Once there are no EMPTY MBs in the lower half, we
+ * EMPTY all FULL or locked MBs again.
+ * Next time we have the following situation:
+ * If there is a FULL MB in the upper half, it is because it was about to get
+ * filled when we scanned last time, or got filled just before emptying the
+ * lowest MB, so this will be the first MB we need to copy now. If there are
+ * no EMPTY MBs in the lower half at this time, it means we cannot guarantee
+ * ordering anymore. It also means latency exceeded 30 messages.
+ */
+static void flexcan_copy_rxmbs(struct flexcan_priv *priv, u32 iflag1, u32 iflag2)
+{
+	struct flexcan_regs __iomem *regs = priv->base;
+	int i;
+	unsigned int code;
+
+	if (priv->second_first) {
+		/* Begin with second half */
+		for(i = 32; i < 64; i++) {
+			flexcan_copy_one_rxmb(priv, i);
+			flexcan_unlock_if_locked(priv, i);
+		}
+	}
+
+	/* Copy and disable FULL MBs */
+	for(i = FLEXCAN_RX_AREA_START; i < 64; i++) {
+		code = flexcan_copy_one_rxmb(priv, i);
+		if (code == FLEXCAN_MB_CODE_RX_EMPTY)
+			break;
+	}
+
+	if ((i >= 32) && priv->second_first)
+		netdev_warn(priv->dev, "RX order cannot be guaranteed. count=%d\n", i);
+
+	priv->second_first = false;
+
+	/* No EMPTY MB in first half? */
+	if (i >= 32) {
+		/* Re-enable all disabled MBs */
+		for(i = FLEXCAN_RX_AREA_START; i < 64; i++)
+			flexcan_unlock_if_locked(priv, i);
+
+		/* Next time we need to check the second half first */
+		priv->second_first = true;
+	}
+
+	/* Unlock the last locked MB if any */
+	flexcan_read(&regs->timer);
+}
+
 static irqreturn_t flexcan_irq(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
 	struct net_device_stats *stats = &dev->stats;
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct flexcan_regs __iomem *regs = priv->base;
-	u32 reg_iflag1, reg_esr;
+	u32 reg_iflag1, reg_iflag2, reg_esr;
 
 	reg_iflag1 = flexcan_read(&regs->iflag1);
+	reg_iflag2 = flexcan_read(&regs->iflag2);
 	reg_esr = flexcan_read(&regs->esr);
+
 	/* ACK all bus error and state change IRQ sources */
 	if (reg_esr & FLEXCAN_ESR_ALL_INT)
 		flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT, &regs->esr);
@@ -806,7 +969,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	 * - state change IRQ
 	 * - bus error IRQ and bus error reporting is activated
 	 */
-	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
+	if ((reg_iflag1 & ~(1 << FLEXCAN_TX_BUF_ID)) || reg_iflag2 ||
 	    (reg_esr & FLEXCAN_ESR_ERR_STATE) ||
 	    flexcan_has_and_handle_berr(priv, reg_esr)) {
 		/*
@@ -814,20 +977,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		 * 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);
+		flexcan_copy_rxmbs(priv, reg_iflag1, reg_iflag2);
 		napi_schedule(&priv->napi);
 	}
 
-	/* FIFO overflow */
-	if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
-		flexcan_write(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
-		dev->stats.rx_over_errors++;
-		dev->stats.rx_errors++;
-	}
-
 	/* transmission complete interrupt */
 	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
 		stats->tx_bytes += can_get_echo_skb(dev, 0);
@@ -920,11 +1073,11 @@ static int flexcan_chip_start(struct net_device *dev)
 	 *
 	 */
 	reg_mcr = flexcan_read(&regs->mcr);
-	reg_mcr &= ~FLEXCAN_MCR_MAXMB(0xff);
-	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
+	reg_mcr &= ~(FLEXCAN_MCR_MAXMB(0xff) | FLEXCAN_MCR_FEN);
+	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
 		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS |
-		FLEXCAN_MCR_MAXMB(FLEXCAN_TX_BUF_ID);
+		FLEXCAN_MCR_BCC | FLEXCAN_MCR_MAXMB(FLEXCAN_MCR_MAXMB_USED);
 	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
@@ -960,28 +1113,36 @@ static int flexcan_chip_start(struct net_device *dev)
 	netdev_dbg(dev, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
 	flexcan_write(reg_ctrl, &regs->ctrl);
 
-	/* clear and invalidate all mailboxes first */
-	for (i = FLEXCAN_TX_BUF_ID; i < ARRAY_SIZE(regs->cantxfg); i++) {
-		flexcan_write(FLEXCAN_MB_CODE_RX_INACTIVE,
-			      &regs->cantxfg[i].can_ctrl);
+	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES) {
+		/* CTRL2: Enable EACEN */
+		reg_ctrl2 = flexcan_read(&regs->ctrl2);
+		reg_ctrl2 |= FLEXCAN_CTRL2_EACEN;
+		flexcan_write(reg_ctrl2, &regs->ctrl2);
 	}
 
+	/* Prepare RX mailboxes */
+	for (i = FLEXCAN_RX_AREA_START; i < ARRAY_SIZE(regs->cantxfg); i++) {
+		flexcan_write(FLEXCAN_MB_CODE_RX_EMPTY,
+		      &regs->cantxfg[i].can_ctrl);
+		flexcan_write(0, &regs->rximr[i]); /* Clear filter */
+	}
+
+	/* Prepare TX mailbox */
+	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
+		&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+
 	/* Errata ERR005829: mark first TX mailbox as INACTIVE */
 	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
 		      &regs->cantxfg[FLEXCAN_TX_BUF_RESERVED].can_ctrl);
 
-	/* mark TX mailbox as INACTIVE */
-	flexcan_write(FLEXCAN_MB_CODE_TX_INACTIVE,
-		      &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+	priv->second_first = false;
+	priv->rx_head = priv->rx_tail = 0;
 
 	/* acceptance mask/acceptance code (accept everything) */
 	flexcan_write(0x0, &regs->rxgmask);
 	flexcan_write(0x0, &regs->rx14mask);
 	flexcan_write(0x0, &regs->rx15mask);
 
-	if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
-		flexcan_write(0x0, &regs->rxfgmask);
-
 	/*
 	 * On Vybrid, disable memory error detection interrupts
 	 * and freeze mode.
@@ -1018,8 +1179,9 @@ static int flexcan_chip_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	/* enable FIFO interrupts */
-	flexcan_write(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
+	/* enable all MB interrupts */
+	flexcan_write(FLEXCAN_IFLAG1_DEFAULT, &regs->imask1);
+	flexcan_write(FLEXCAN_IFLAG2_DEFAULT, &regs->imask2);
 
 	/* print chip status */
 	netdev_dbg(dev, "%s: reading mcr=0x%08x ctrl=0x%08x\n", __func__,
-- 
1.9.1

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Alexander Stein]

On Monday 29 September 2014 14:52:55, David Jander wrote:
> The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
> mailbox space capable of holding up to 63 messages.
> 
> This space was largely unused, limiting the permissible latency from interrupt
> to NAPI to only 6 messages. This patch uses all available MBs for message
> reception and frees the MBs in the IRQ handler to greatly decrease the
> likelihood of receive overruns.
> 
> Signed-off-by: David Jander <david@protonic.nl>

AFAICT, If you disable Rx FIFO mode, you essentially break RTR reception on (at least) i.MX3. Please refere to the reference manual 24.4.8.1 Remote Frames.
Vybrid and i.MX6 (not sure about i.MX5) seem to have more features about RTR reception.

Best regards,
Alexander

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Dear Alexander,

On Mon, 29 Sep 2014 15:29:28 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Monday 29 September 2014 14:52:55, David Jander wrote:
> > The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
> > mailbox space capable of holding up to 63 messages.
> > 
> > This space was largely unused, limiting the permissible latency from
> > interrupt to NAPI to only 6 messages. This patch uses all available MBs
> > for message reception and frees the MBs in the IRQ handler to greatly
> > decrease the likelihood of receive overruns.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> 
> AFAICT, If you disable Rx FIFO mode, you essentially break RTR reception on
> (at least) i.MX3. Please refere to the reference manual 24.4.8.1 Remote
> Frames. Vybrid and i.MX6 (not sure about i.MX5) seem to have more features
> about RTR reception.

Argh! Looks like you are right!
RTR reception did not work for i.MX6 either, but that is because I forgot to
set RRS bit in CTRL2... which does not exist on i.MX53 nor i.MX35.
What's strange is the fact that the i.MX53 RM does not contain the chapter you
mention (it is contained in the i.MX35 RM though), and this is the only place
that clearly seems to indicate that this indeed will not work on the i.MX3:

"A received remote request frame is not stored in a receive buffer. It is only
used to trigger a transmission of a frame in response."

AFAICS, we have little choice but to use the Rx FIFO, at least for i.MX3/5 or
older IPs...

Maybe I can re-factor the code in such a way that the same construction is
used outside the IRQ context, but the IRQ routine will either empty the FIFO
(for revision 3 and older flexcan) or the while MB area (for revision 10 and
newer).

The BIG drawback of using the RX FIFO is that it is really tiny. Not using it
is really a big win for i.MX6 and newer... which I'd like to keep.

Nevertheless, emptying the FIFO in the IRQ handler will still be a big
improvement, since the only thing that could still kill the driver and cause
message loss is interrupt latency, which normally should not be so high. NAPI
scheduling latency is probably much worse, and this is the biggest issue with
the current driver.

Any suggestion on what to do?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Alexander Stein]

On Monday 29 September 2014 16:39:32, David Jander wrote:
> 
> Dear Alexander,
> 
> On Mon, 29 Sep 2014 15:29:28 +0200
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> 
> > On Monday 29 September 2014 14:52:55, David Jander wrote:
> > > The FlexCAN controller has a RX FIFO that is only 6 messages deep, and a
> > > mailbox space capable of holding up to 63 messages.
> > > 
> > > This space was largely unused, limiting the permissible latency from
> > > interrupt to NAPI to only 6 messages. This patch uses all available MBs
> > > for message reception and frees the MBs in the IRQ handler to greatly
> > > decrease the likelihood of receive overruns.
> > > 
> > > Signed-off-by: David Jander <david@protonic.nl>
> > 
> > AFAICT, If you disable Rx FIFO mode, you essentially break RTR reception on
> > (at least) i.MX3. Please refere to the reference manual 24.4.8.1 Remote
> > Frames. Vybrid and i.MX6 (not sure about i.MX5) seem to have more features
> > about RTR reception.
> 
> Argh! Looks like you are right!
> RTR reception did not work for i.MX6 either, but that is because I forgot to
> set RRS bit in CTRL2... which does not exist on i.MX53 nor i.MX35.
> What's strange is the fact that the i.MX53 RM does not contain the chapter you
> mention (it is contained in the i.MX35 RM though), and this is the only place
> that clearly seems to indicate that this indeed will not work on the i.MX3:
> 
> "A received remote request frame is not stored in a receive buffer. It is only
> used to trigger a transmission of a frame in response."

Yep, it seems that the FlexCAN part of the RM is even more shorter in i.MX5 than i.MX3 or i.MX6...

> AFAICS, we have little choice but to use the Rx FIFO, at least for i.MX3/5 or
> older IPs...
> 
> Maybe I can re-factor the code in such a way that the same construction is
> used outside the IRQ context, but the IRQ routine will either empty the FIFO
> (for revision 3 and older flexcan) or the while MB area (for revision 10 and
> newer).
> 
> The BIG drawback of using the RX FIFO is that it is really tiny. Not using it
> is really a big win for i.MX6 and newer... which I'd like to keep.

I don't know how the MB actually work, but I know about race conditions in C_CAN (actually pch can) with the pseudo FIFO implemented using message boxes. May this also happen here? That's a reason I'm really happy there is a real FIFO in hardware.

> Nevertheless, emptying the FIFO in the IRQ handler will still be a big
> improvement, since the only thing that could still kill the driver and cause
> message loss is interrupt latency, which normally should not be so high. NAPI
> scheduling latency is probably much worse, and this is the biggest issue with
> the current driver.
> 
> Any suggestion on what to do?

Get rid of NAPI and use RT-preempt with proper priorities :) But joke aside, which workload does increase the NAPI latency so much, an overrun occurs? I tested CAN bursts on i.MX35 without any loss.

Best regards
Alexander

-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Dear Alexander,

On Mon, 29 Sep 2014 17:02:39 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Monday 29 September 2014 16:39:32, David Jander wrote:
> > 
> > Dear Alexander,
> > 
> > On Mon, 29 Sep 2014 15:29:28 +0200
> > Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> > 
> > > On Monday 29 September 2014 14:52:55, David Jander wrote:
> > > > The FlexCAN controller has a RX FIFO that is only 6 messages deep, and
> > > > a mailbox space capable of holding up to 63 messages.
> > > > 
> > > > This space was largely unused, limiting the permissible latency from
> > > > interrupt to NAPI to only 6 messages. This patch uses all available MBs
> > > > for message reception and frees the MBs in the IRQ handler to greatly
> > > > decrease the likelihood of receive overruns.
> > > > 
> > > > Signed-off-by: David Jander <david@protonic.nl>
> > > 
> > > AFAICT, If you disable Rx FIFO mode, you essentially break RTR reception
> > > on (at least) i.MX3. Please refere to the reference manual 24.4.8.1
> > > Remote Frames. Vybrid and i.MX6 (not sure about i.MX5) seem to have more
> > > features about RTR reception.
> > 
> > Argh! Looks like you are right!
> > RTR reception did not work for i.MX6 either, but that is because I forgot
> > to set RRS bit in CTRL2... which does not exist on i.MX53 nor i.MX35.
> > What's strange is the fact that the i.MX53 RM does not contain the chapter
> > you mention (it is contained in the i.MX35 RM though), and this is the
> > only place that clearly seems to indicate that this indeed will not work
> > on the i.MX3:
> > 
> > "A received remote request frame is not stored in a receive buffer. It is
> > only used to trigger a transmission of a frame in response."
> 
> Yep, it seems that the FlexCAN part of the RM is even more shorter in i.MX5
> than i.MX3 or i.MX6...
> 
> > AFAICS, we have little choice but to use the Rx FIFO, at least for i.MX3/5
> > or older IPs...
> > 
> > Maybe I can re-factor the code in such a way that the same construction is
> > used outside the IRQ context, but the IRQ routine will either empty the
> > FIFO (for revision 3 and older flexcan) or the while MB area (for revision
> > 10 and newer).
> > 
> > The BIG drawback of using the RX FIFO is that it is really tiny. Not using
> > it is really a big win for i.MX6 and newer... which I'd like to keep.
> 
> I don't know how the MB actually work, but I know about race conditions in
> C_CAN (actually pch can) with the pseudo FIFO implemented using message
> boxes. May this also happen here? That's a reason I'm really happy there is
> a real FIFO in hardware.

I can think of a few possible race conditions that can happen when doing this,
but AFAIK, I have them all covered in this patch. I have done quite some
testing looking at message ordering and message loss, but it seems very
robust. If you read the comment for the function flexcan_copy_rxmbs(), you can
see that there is a condition that can produce out of order messages, but that
only happens if interrupt latency goes beyond 30 messages... and you get a
nice warning in the kernel message log.

When I first looked at the flexcan peripheral I also thought: "Cool, at last a
CAN controller with a real FIFO", but unfortunately that FIFO is only 6
messages deep... compared to a MB area of a whopping 64 MBs that will go
almost completely unused!

> > Nevertheless, emptying the FIFO in the IRQ handler will still be a big
> > improvement, since the only thing that could still kill the driver and
> > cause message loss is interrupt latency, which normally should not be so
> > high. NAPI scheduling latency is probably much worse, and this is the
> > biggest issue with the current driver.
> > 
> > Any suggestion on what to do?
> 
> Get rid of NAPI and use RT-preempt with proper priorities :) But joke aside,
> which workload does increase the NAPI latency so much, an overrun occurs? I
> tested CAN bursts on i.MX35 without any loss.

I have seen overruns on an i.MX6 at only 250kbaud receiving back-to-back
messages of 1 byte long. I usually test bursts of 10000 messages or more.

Things get a lot worse if you also happen to have kernel messages output to a
serial console and plug in an USB device (because there are printk's in the
EHCI driver inside spin locks with interrupts disabled!!), but that's a
different story.

6 messages at 250kbaud is little over 1 ms latency in a worst-case scenario,
and at 1Mbaud it is just a few 100 microseconds. I am not an expert in Linux
schedulers, but IMHO for a non-RT kernel these latency times are totally
off-limits. Maybe if the controller is off-loaded in the IRQ handler and
interrupt priorities are well adjusted 6 messages can be feasible at 250kbaud,
but I wouldn't dare to go beyond that.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Alexander Stein]

Hello David,

On Tuesday 30 September 2014 09:13:55, David Jander wrote:
> > > Nevertheless, emptying the FIFO in the IRQ handler will still be a big
> > > improvement, since the only thing that could still kill the driver and
> > > cause message loss is interrupt latency, which normally should not be so
> > > high. NAPI scheduling latency is probably much worse, and this is the
> > > biggest issue with the current driver.
> > > 
> > > Any suggestion on what to do?
> > 
> > Get rid of NAPI and use RT-preempt with proper priorities :) But joke aside,
> > which workload does increase the NAPI latency so much, an overrun occurs? I
> > tested CAN bursts on i.MX35 without any loss.
> 
> I have seen overruns on an i.MX6 at only 250kbaud receiving back-to-back
> messages of 1 byte long. I usually test bursts of 10000 messages or more.

Mh, that's odd. I have run several tests a 1MBaud on an i.MX35 with 2 CAN nodes attached each sending bursts of 250 message every 200ms with a total message count of 250000 each. No overruns, losses or message misordering.

> Things get a lot worse if you also happen to have kernel messages output to a
> serial console and plug in an USB device (because there are printk's in the
> EHCI driver inside spin locks with interrupts disabled!!), but that's a
> different story.

Eek. Well, adding quiet to the command line avoids that. IIRC there is even a Kconfig option to disable that announce to kernel log.

Best regards,
Alexander

-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Dear Alexander,

On Tue, 30 Sep 2014 09:43:33 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> Hello David,
> 
> On Tuesday 30 September 2014 09:13:55, David Jander wrote:
> > > > Nevertheless, emptying the FIFO in the IRQ handler will still be a big
> > > > improvement, since the only thing that could still kill the driver and
> > > > cause message loss is interrupt latency, which normally should not be
> > > > so high. NAPI scheduling latency is probably much worse, and this is
> > > > the biggest issue with the current driver.
> > > > 
> > > > Any suggestion on what to do?
> > > 
> > > Get rid of NAPI and use RT-preempt with proper priorities :) But joke
> > > aside, which workload does increase the NAPI latency so much, an overrun
> > > occurs? I tested CAN bursts on i.MX35 without any loss.
> > 
> > I have seen overruns on an i.MX6 at only 250kbaud receiving back-to-back
> > messages of 1 byte long. I usually test bursts of 10000 messages or more.
> 
> Mh, that's odd. I have run several tests a 1MBaud on an i.MX35 with 2 CAN
> nodes attached each sending bursts of 250 message every 200ms with a total
> message count of 250000 each. No overruns, losses or message misordering.

Do you have any other system-load? Have you tried something like flood-pinging
the ethernet port at the same time?
Your results sound very impressive. If messages are really sent back-to-back,
then there's about 300 microseconds of permissible latency from interrupt to
NAPI... how can you not get over that limit at least once?
You are not running PREEMPT_RT, do you?

> > Things get a lot worse if you also happen to have kernel messages output
> > to a serial console and plug in an USB device (because there are printk's
> > in the EHCI driver inside spin locks with interrupts disabled!!), but
> > that's a different story.
> 
> Eek. Well, adding quiet to the command line avoids that. IIRC there is even
> a Kconfig option to disable that announce to kernel log.

Yes, I know, but what should one expect on a non-RT system, with all sorts of
drivers (SPI, I2C, NAND, SDHCI, Ethernet,...) doing their work?

> Best regards,
> Alexander

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Alexander Stein]

On Wednesday 01 October 2014 08:29:32, David Jander wrote:
> On Tue, 30 Sep 2014 09:43:33 +0200
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> 
> > Hello David,
> > 
> > On Tuesday 30 September 2014 09:13:55, David Jander wrote:
> > > > > Nevertheless, emptying the FIFO in the IRQ handler will still be a big
> > > > > improvement, since the only thing that could still kill the driver and
> > > > > cause message loss is interrupt latency, which normally should not be
> > > > > so high. NAPI scheduling latency is probably much worse, and this is
> > > > > the biggest issue with the current driver.
> > > > > 
> > > > > Any suggestion on what to do?
> > > > 
> > > > Get rid of NAPI and use RT-preempt with proper priorities :) But joke
> > > > aside, which workload does increase the NAPI latency so much, an overrun
> > > > occurs? I tested CAN bursts on i.MX35 without any loss.
> > > 
> > > I have seen overruns on an i.MX6 at only 250kbaud receiving back-to-back
> > > messages of 1 byte long. I usually test bursts of 10000 messages or more.
> > 
> > Mh, that's odd. I have run several tests a 1MBaud on an i.MX35 with 2 CAN
> > nodes attached each sending bursts of 250 message every 200ms with a total
> > message count of 250000 each. No overruns, losses or message misordering.
> 
> Do you have any other system-load? Have you tried something like flood-pinging
> the ethernet port at the same time?
> Your results sound very impressive. If messages are really sent back-to-back,
> then there's about 300 microseconds of permissible latency from interrupt to
> NAPI... how can you not get over that limit at least once?
> You are not running PREEMPT_RT, do you?

It has been a long time ago, but IIRC there was no load despite CAN reception (!), no messages were sent. I'm not sure if I ever ran this test on that i.MX35 without PREEMPT_RT. Currently I don't have access to this hardware, so I cannot use it on a non-rt kernel.

> > > Things get a lot worse if you also happen to have kernel messages output
> > > to a serial console and plug in an USB device (because there are printk's
> > > in the EHCI driver inside spin locks with interrupts disabled!!), but
> > > that's a different story.
> > 
> > Eek. Well, adding quiet to the command line avoids that. IIRC there is even
> > a Kconfig option to disable that announce to kernel log.
> 
> Yes, I know, but what should one expect on a non-RT system, with all sorts of
> drivers (SPI, I2C, NAND, SDHCI, Ethernet,...) doing their work?

Sure, each subsystem will add latency. I don't say your intention is wrong.

BTW: You posted a patch for at91_can in June (Din't get opportunity to try it yet), where you use a kfifo to accomplish the same, why not here?

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

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

On 10/01/2014 09:11 AM, Alexander Stein wrote:
> BTW: You posted a patch for at91_can in June (Din't get opportunity
> to try it yet), where you use a kfifo to accomplish the same, why not
> here?

The cyclic buffer approach is okay, from my point of view.

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: 181 bytes --]

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Alexander Stein]

On Wednesday 01 October 2014 09:15:46, Marc Kleine-Budde wrote:
> On 10/01/2014 09:11 AM, Alexander Stein wrote:
> > BTW: You posted a patch for at91_can in June (Din't get opportunity
> > to try it yet), where you use a kfifo to accomplish the same, why not
> > here?
> 
> The cyclic buffer approach is okay, from my point of view.

I'M just wondering why 2 different approaches have been chosen.

Regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

On Wed, 01 Oct 2014 10:29:41 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Wednesday 01 October 2014 09:15:46, Marc Kleine-Budde wrote:
> > On 10/01/2014 09:11 AM, Alexander Stein wrote:
> > > BTW: You posted a patch for at91_can in June (Din't get opportunity
> > > to try it yet), where you use a kfifo to accomplish the same, why not
> > > here?
> > 
> > The cyclic buffer approach is okay, from my point of view.
> 
> I'M just wondering why 2 different approaches have been chosen.

Good question.... I did the at91_can modification a long time ago, and the
initial approach was more or less guided by the current architecture of the
at91_can driver. I also probably got better ideas when doing something very
similar for the second time (flexcan).
Also, I can imagine that there are more CAN drivers that have similar
problems, and maybe we should think about a solution in the SocketCAN
framework itself instead.

Talking about at91_can... I have posted that patch twice already and had
no reaction so far. Unfortunately now I don't have the hardware anymore, so I
doubt I can pick this up again, let alone re-write that patch, unless someone
is willing to help with testing...

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

On Wed, 01 Oct 2014 09:11:37 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Wednesday 01 October 2014 08:29:32, David Jander wrote:
> > On Tue, 30 Sep 2014 09:43:33 +0200
> > Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> > 
> > > Hello David,
> > > 
> > > On Tuesday 30 September 2014 09:13:55, David Jander wrote:
> > > > > > Nevertheless, emptying the FIFO in the IRQ handler will still be a
> > > > > > big improvement, since the only thing that could still kill the
> > > > > > driver and cause message loss is interrupt latency, which normally
> > > > > > should not be so high. NAPI scheduling latency is probably much
> > > > > > worse, and this is the biggest issue with the current driver.
> > > > > > 
> > > > > > Any suggestion on what to do?
> > > > > 
> > > > > Get rid of NAPI and use RT-preempt with proper priorities :) But joke
> > > > > aside, which workload does increase the NAPI latency so much, an
> > > > > overrun occurs? I tested CAN bursts on i.MX35 without any loss.
> > > > 
> > > > I have seen overruns on an i.MX6 at only 250kbaud receiving
> > > > back-to-back messages of 1 byte long. I usually test bursts of 10000
> > > > messages or more.
> > > 
> > > Mh, that's odd. I have run several tests a 1MBaud on an i.MX35 with 2 CAN
> > > nodes attached each sending bursts of 250 message every 200ms with a
> > > total message count of 250000 each. No overruns, losses or message
> > > misordering.
> > 
> > Do you have any other system-load? Have you tried something like
> > flood-pinging the ethernet port at the same time?
> > Your results sound very impressive. If messages are really sent
> > back-to-back, then there's about 300 microseconds of permissible latency
> > from interrupt to NAPI... how can you not get over that limit at least
> > once? You are not running PREEMPT_RT, do you?
> 
> It has been a long time ago, but IIRC there was no load despite CAN
> reception (!), no messages were sent. I'm not sure if I ever ran this test
> on that i.MX35 without PREEMPT_RT. Currently I don't have access to this
> hardware, so I cannot use it on a non-rt kernel.

It would be very interesting to see how it performs on a non-RT kernel under
load. In my experience flood-pinging, USB traffic and some CPU-bound load
(benchmark or similar) are interesting attacks to try each on its own or in
combination while the CAN test runs.
On the CAN side, I'd recommend long bursts (10000) of short back-to-back
messages (DLC=1) at a sensible baudrate (250kbaud, 500kbaud and 1Mbaud).
I used a Peak USB-CAN dongle on my PC to generate messages with can-utils
cangen. On the target candump to a log-file and a python script to analyze the
log-file for packet-ordering and -count, and a scope to make sure there is no
space between messages.

[...]

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Alexander Stein]

Hello David,

On Wednesday 01 October 2014 11:07:41, David Jander wrote:
> On Wed, 01 Oct 2014 10:29:41 +0200
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> 
> > On Wednesday 01 October 2014 09:15:46, Marc Kleine-Budde wrote:
> > > On 10/01/2014 09:11 AM, Alexander Stein wrote:
> > > > BTW: You posted a patch for at91_can in June (Din't get opportunity
> > > > to try it yet), where you use a kfifo to accomplish the same, why not
> > > > here?
> > > 
> > > The cyclic buffer approach is okay, from my point of view.
> > 
> > I'M just wondering why 2 different approaches have been chosen.
> 
> Good question.... I did the at91_can modification a long time ago, and the
> initial approach was more or less guided by the current architecture of the
> at91_can driver. I also probably got better ideas when doing something very
> similar for the second time (flexcan).
> Also, I can imagine that there are more CAN drivers that have similar
> problems, and maybe we should think about a solution in the SocketCAN
> framework itself instead.

Sounds reasonable too. If someone ever wants to support flexcan on coldfire (m68k) there is only a single message box (+ a shift register), there you will need this feature for sure :)
But I'm still curious which approach is better: Implement an own cyclic buffer or use kfifo (which might be a cyclic buffer itself, dunno)?

> Talking about at91_can... I have posted that patch twice already and had
> no reaction so far. Unfortunately now I don't have the hardware anymore, so I
> doubt I can pick this up again, let alone re-write that patch, unless someone
> is willing to help with testing...

It is on my TODO, but did not get chance to test it yet. There is the statement that a proprietary driver here is better that it doesn't drop any frames under heavy load while socketcan one does. So it might actually improve the situation.

Best regards,
Alexander

-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Dear Alexander,

On Wed, 01 Oct 2014 11:19:54 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> Hello David,
> 
> On Wednesday 01 October 2014 11:07:41, David Jander wrote:
> > On Wed, 01 Oct 2014 10:29:41 +0200
> > Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> > 
> > > On Wednesday 01 October 2014 09:15:46, Marc Kleine-Budde wrote:
> > > > On 10/01/2014 09:11 AM, Alexander Stein wrote:
> > > > > BTW: You posted a patch for at91_can in June (Din't get opportunity
> > > > > to try it yet), where you use a kfifo to accomplish the same, why not
> > > > > here?
> > > > 
> > > > The cyclic buffer approach is okay, from my point of view.
> > > 
> > > I'M just wondering why 2 different approaches have been chosen.
> > 
> > Good question.... I did the at91_can modification a long time ago, and the
> > initial approach was more or less guided by the current architecture of the
> > at91_can driver. I also probably got better ideas when doing something very
> > similar for the second time (flexcan).
> > Also, I can imagine that there are more CAN drivers that have similar
> > problems, and maybe we should think about a solution in the SocketCAN
> > framework itself instead.
> 
> Sounds reasonable too. If someone ever wants to support flexcan on coldfire
> (m68k) there is only a single message box (+ a shift register), there you
> will need this feature for sure :) But I'm still curious which approach is
> better: Implement an own cyclic buffer or use kfifo (which might be a cyclic
> buffer itself, dunno)?

I guess the cyclic-buffer with power-of-2 size might be more efficient...
also I think the kfifo uses a spin-lock, which is not always necessary if you
are careful with the cyclic-buffer.

> > Talking about at91_can... I have posted that patch twice already and had
> > no reaction so far. Unfortunately now I don't have the hardware anymore,
> > so I doubt I can pick this up again, let alone re-write that patch, unless
> > someone is willing to help with testing...
> 
> It is on my TODO, but did not get chance to test it yet. There is the
> statement that a proprietary driver here is better that it doesn't drop any
> frames under heavy load while socketcan one does. So it might actually
> improve the situation.

I think so.
I just wrote that patch in a two-day marathon to help a desperate customer
with a platform that is not even ours (hence I don't have the hardware
anymore). I also heard that story about the closed-source alternative and
thought it was unconceivable to have someone say that SocketCAN is in any way
inferior to a commercial product! That thought alone made me finish the patch
in two days and send it out to linux-can.... cooling down came when no one
reacted to it :-)

Back to topic: Can one of the maintainers (Wolfgang, Marc) give an opinion on
how to best solve the following two issues:

1.- Using MB area instead of FIFO for flexcan breaks RTR reception on older
SoC's. My proposal is to modify my approach as to have two different IRQ
handling paths: One that off-loads the RX-FIFO into the cyclic-buffer for
older chips and one that uses the whole MB area and off-loads it into the same
cyclic buffer for i.MX6, Vybrid and newer chips.

2.- Since the problem addressed by my patch to at91_can is very similar, what
about solving these problems in the SocketCAN framework (if that is possible)?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

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

On 10/01/2014 11:34 AM, David Jander wrote:
[...]

> Back to topic: Can one of the maintainers (Wolfgang, Marc) give an opinion on
> how to best solve the following two issues:
> 
> 1.- Using MB area instead of FIFO for flexcan breaks RTR reception on older
> SoC's. My proposal is to modify my approach as to have two different IRQ
> handling paths: One that off-loads the RX-FIFO into the cyclic-buffer for
> older chips and one that uses the whole MB area and off-loads it into the same
> cyclic buffer for i.MX6, Vybrid and newer chips.

Sounds good.

> 2.- Since the problem addressed by my patch to at91_can is very similar, what
> about solving these problems in the SocketCAN framework (if that is possible)?

Have you had a look at my rx-fifo branch in
https://gitorious.org/linux-can/linux-can-next? It already tries to
abstract the simulation of the FIFO with the linear mailboxes.

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: 181 bytes --]

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Dear Marc,

On Wed, 01 Oct 2014 11:58:16 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/01/2014 11:34 AM, David Jander wrote:
> [...]
> 
> > Back to topic: Can one of the maintainers (Wolfgang, Marc) give an opinion
> > on how to best solve the following two issues:
> > 
> > 1.- Using MB area instead of FIFO for flexcan breaks RTR reception on older
> > SoC's. My proposal is to modify my approach as to have two different IRQ
> > handling paths: One that off-loads the RX-FIFO into the cyclic-buffer for
> > older chips and one that uses the whole MB area and off-loads it into the
> > same cyclic buffer for i.MX6, Vybrid and newer chips.
> 
> Sounds good.

Ok thanks. I will try to do that.

> > 2.- Since the problem addressed by my patch to at91_can is very similar,
> > what about solving these problems in the SocketCAN framework (if that is
> > possible)?
> 
> Have you had a look at my rx-fifo branch in
> https://gitorious.org/linux-can/linux-can-next? It already tries to
> abstract the simulation of the FIFO with the linear mailboxes.

Looks interesting. I think it is a good idea to do this in dev.c, since there
are obviously more CAN drivers that can use this. Unfortunately it seems you
are still pretending the napi-poll handler to call can_rx_fifo_poll().
Wouldn't it be better to just empty all MBs into a circular buffer or kfifo
from the interrupt handler instead?

I still don't understand the results Alexander is getting, though....

What are you going to do with the rx-fifo work? Do you recommend to base my
patch on that? In that case, calling can_rx_fifo_poll() from the interrupt
handler will look a little awkward... but it should work. Or should I propose
an extension to rx-fifo?

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

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

On 10/06/2014 09:28 AM, David Jander wrote:
>>> 2.- Since the problem addressed by my patch to at91_can is very similar,
>>> what about solving these problems in the SocketCAN framework (if that is
>>> possible)?
>>
>> Have you had a look at my rx-fifo branch in
>> https://gitorious.org/linux-can/linux-can-next? It already tries to
>> abstract the simulation of the FIFO with the linear mailboxes.
> 
> Looks interesting. I think it is a good idea to do this in dev.c, since there
> are obviously more CAN drivers that can use this. Unfortunately it seems you
> are still pretending the napi-poll handler to call can_rx_fifo_poll().
> Wouldn't it be better to just empty all MBs into a circular buffer or kfifo
> from the interrupt handler instead?

Yes probably, I started the rx-fifo patch before you came up with that idea.

> I still don't understand the results Alexander is getting, though....
> 
> What are you going to do with the rx-fifo work? Do you recommend to base my
> patch on that? In that case, calling can_rx_fifo_poll() from the interrupt
> handler will look a little awkward... but it should work. Or should I propose
> an extension to rx-fifo?

My plans, or rather the points that need to be addressed for the rx-fifo
are:
- improve to work with more than 32 mailboxes. 64 are probably enough
  for everybody :)
- make it work with the flexcan linear buffers
- make it work with the ti_hecc driver
- add option or convert to run from interrupt handler and copy to
  kfifo/cyclic buffer/...

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: 181 bytes --]

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Dear Marc,

On Mon, 06 Oct 2014 12:00:03 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/06/2014 09:28 AM, David Jander wrote:
> >>> 2.- Since the problem addressed by my patch to at91_can is very similar,
> >>> what about solving these problems in the SocketCAN framework (if that is
> >>> possible)?
> >>
> >> Have you had a look at my rx-fifo branch in
> >> https://gitorious.org/linux-can/linux-can-next? It already tries to
> >> abstract the simulation of the FIFO with the linear mailboxes.
> > 
> > Looks interesting. I think it is a good idea to do this in dev.c, since
> > there are obviously more CAN drivers that can use this. Unfortunately it
> > seems you are still pretending the napi-poll handler to call
> > can_rx_fifo_poll(). Wouldn't it be better to just empty all MBs into a
> > circular buffer or kfifo from the interrupt handler instead?
> 
> Yes probably, I started the rx-fifo patch before you came up with that idea.
> 
> > I still don't understand the results Alexander is getting, though....
> > 
> > What are you going to do with the rx-fifo work? Do you recommend to base my
> > patch on that? In that case, calling can_rx_fifo_poll() from the interrupt
> > handler will look a little awkward... but it should work. Or should I
> > propose an extension to rx-fifo?
> 
> My plans, or rather the points that need to be addressed for the rx-fifo
> are:
> - improve to work with more than 32 mailboxes. 64 are probably enough
>   for everybody :)

Just did that. Basically a s/u32/u64/g and s/BIT/BIT_ULL/g sort of thing.

> - make it work with the flexcan linear buffers

I am busy with that one... one thing I am pondering whether the "disable first
and then read-out" -logic holds up here, since the flexcan has it's own locking
thing... I'll have to see.

> - make it work with the ti_hecc driver

Never seen that one...

> - add option or convert to run from interrupt handler and copy to
>   kfifo/cyclic buffer/...

Almost done. Basically I am introducing this:

--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -75,6 +75,8 @@ struct can_rx_fifo {
        void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64 mask);
        void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
        void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int mb);
+       void (*mailbox_move_to_buffer)(struct can_rx_fifo *rx_fifo,
+               struct can_frame *frame, unsigned int mb);
 
        u64 mask_low;
        u64 mask_high;
@@ -83,6 +85,10 @@ struct can_rx_fifo {
        unsigned int next;
 
        bool inc;
+
+       struct can_frame *ring;
+       unsigned int ring_head;
+       unsigned int ring_tail;
 };

If the user defines fifo->mailbox_move_to_buffer() it is called instead of
fifo->mailbox_receive() and he whole circular buffer magic is done in
can_rx_fifo_poll(). I also need to add something like a generic NAPI poll
handler then, that just reads out this ring-buffer. Should simplify drivers a
lot I guess.

What do you think?

Best regards,

-- 
David Jander
Protonic Holland.

[RFC PATCH 1/2] can: rx-fifo: Increase MB size limit from 32 to 64

[David Jander]

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/dev.c   | 24 ++++++++++++------------
 include/linux/can/dev.h | 12 ++++++------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c1e53e9..fe45516 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -289,20 +289,20 @@ static unsigned int can_rx_fifo_inc(struct can_rx_fifo *fifo, unsigned int *val)
 		return (*val)--;
 }
 
-static u32 can_rx_fifo_mask_low(struct can_rx_fifo *fifo)
+static u64 can_rx_fifo_mask_low(struct can_rx_fifo *fifo)
 {
 	if (fifo->inc)
-		return ~0U >> (32 + fifo->low_first - fifo->high_first) << fifo->low_first;
+		return ~0U >> (64 + fifo->low_first - fifo->high_first) << fifo->low_first;
 	else
-		return ~0U >> (32 - fifo->low_first + fifo->high_first) << (fifo->high_first + 1);
+		return ~0U >> (64 - fifo->low_first + fifo->high_first) << (fifo->high_first + 1);
 }
 
-static u32 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
+static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
 {
 	if (fifo->inc)
-		return ~0U >> (32 + fifo->high_first - fifo->high_last - 1) << fifo->high_first;
+		return ~0U >> (64 + fifo->high_first - fifo->high_last - 1) << fifo->high_first;
 	else
-		return ~0U >> (32 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
+		return ~0U >> (64 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
 }
 
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
@@ -331,7 +331,7 @@ int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 
 	netdev_dbg(dev, "%s: low_first=%d, high_first=%d, high_last=%d\n", __func__,
 		   fifo->low_first, fifo->high_first, fifo->high_last);
-	netdev_dbg(dev, "%s: mask_low=0x%08x mask_high=0x%08x\n", __func__,
+	netdev_dbg(dev, "%s: mask_low=0x%016llx mask_high=0x%016llx\n", __func__,
 		   fifo->mask_low, fifo->mask_high);
 
 	return 0;
@@ -341,14 +341,14 @@ EXPORT_SYMBOL_GPL(can_rx_fifo_add);
 int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 {
 	int received = 0;
-	u32 pending;
+	u64 pending;
 	unsigned int mb;
 
 	do {
 		pending = fifo->read_pending(fifo);
 		pending &= fifo->active;
 
-		if (!(pending & BIT(fifo->next))) {
+		if (!(pending & BIT_ULL(fifo->next))) {
 			/*
 			 * Wrap around only if:
 			 * - we are in the upper group and
@@ -356,7 +356,7 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 			 *   of the lower group.
 			 */
 			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
-			    (pending & BIT(fifo->low_first))) {
+			    (pending & BIT_ULL(fifo->low_first))) {
 				fifo->next = fifo->low_first;
 
 				fifo->active |= fifo->mask_high;
@@ -369,7 +369,7 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 		mb = can_rx_fifo_inc(fifo, &fifo->next);
 
 		/* disable mailbox */
-		fifo->active &= ~BIT(mb);
+		fifo->active &= ~BIT_ULL(mb);
 		fifo->mailbox_disable(fifo, mb);
 
 		fifo->mailbox_receive(fifo, mb);
@@ -387,7 +387,7 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_poll);
 
-u32 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
+u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
 {
 	return fifo->active;
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index e9468d0..ed46f7d 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -71,14 +71,14 @@ struct can_rx_fifo {
 	unsigned int high_first;
 	unsigned int high_last;		/* not needed during runtime */
 
-	u32 (*read_pending)(struct can_rx_fifo *rx_fifo);
-	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u32 mask);
+	u64 (*read_pending)(struct can_rx_fifo *rx_fifo);
+	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64 mask);
 	void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
 	void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int mb);
 
-	u32 mask_low;
-	u32 mask_high;
-	u32 active;
+	u64 mask_low;
+	u64 mask_high;
+	u64 active;
 
 	unsigned int next;
 
@@ -128,7 +128,7 @@ u8 can_len2dlc(u8 len);
 
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo);
 int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota);
-u32 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
+u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo);
 
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);
-- 
1.9.1

[RFC PATCH 2/2] can: rx-fifo: Add support for IRQ readout and NAPI poll

[David Jander]

WIP!
The idea is to use rx-fifo from interrupt context and take away the need
for NAPI polling from the driver. Currently no support for error-handling
is included.

Signed-off-by: David Jander <david@protonic.nl>
---
 drivers/net/can/dev.c   | 116 +++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/can/dev.h |   8 ++++
 2 files changed, 118 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index fe45516..6efceb7 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -26,6 +26,7 @@
 #include <linux/can/skb.h>
 #include <linux/can/netlink.h>
 #include <linux/can/led.h>
+#include <linux/circ_buf.h>
 #include <net/rtnetlink.h>
 
 #define MOD_DESC "CAN device driver interface"
@@ -305,23 +306,86 @@ static u64 can_rx_fifo_mask_high(struct can_rx_fifo *fifo)
 		return ~0U >> (64 - fifo->high_first + fifo->high_last - 1) << fifo->high_last;
 }
 
+static int can_rx_fifo_read_napi_frame(struct can_rx_fifo *fifo, int index)
+{
+	struct net_device *dev = fifo->dev;
+	struct net_device_stats *stats = &dev->stats;
+	struct sk_buff *skb;
+	struct can_frame *cf;
+
+	skb = alloc_can_skb(dev, &cf);
+	if (unlikely(!skb)) {
+		stats->rx_dropped++;
+		return 0;
+	}
+
+	memcpy(cf, &fifo->ring[index], sizeof(*cf));
+
+	netif_receive_skb(skb);
+
+	can_led_event(dev, CAN_LED_EVENT_RX);
+
+	return 1;
+}
+
+static int can_rx_fifo_napi_poll(struct napi_struct *napi, int quota)
+{
+	struct can_rx_fifo *fifo = container_of(napi, struct can_rx_fifo, napi);
+	int work_done = 0;
+	int ret;
+	unsigned int head;
+	unsigned int tail;
+
+	/* handle mailboxes */
+	head = smp_load_acquire(&fifo->ring_head);
+	tail = fifo->ring_tail;
+	while ((CIRC_CNT(head, tail, fifo->ring_size) >= 1) &&
+			(work_done < quota)) {
+		ret = can_rx_fifo_read_napi_frame(fifo, tail);
+		work_done += ret;
+		tail = (tail + 1) & (fifo->ring_size -1);
+		smp_store_release(&fifo->ring_tail, tail);
+	}
+
+	if (work_done < quota)
+		napi_complete(napi);
+
+	return work_done;
+}
+
 int can_rx_fifo_add(struct net_device *dev, struct can_rx_fifo *fifo)
 {
+	unsigned int weight;
 	fifo->dev = dev;
 
 	if ((fifo->low_first < fifo->high_first) &&
-	    (fifo->high_first < fifo->high_last))
+	    (fifo->high_first < fifo->high_last)) {
 		fifo->inc = true;
-	else if ((fifo->low_first > fifo->high_first) &&
-		 (fifo->high_first > fifo->high_last))
+		weight = fifo->high_last - fifo->low_first;
+	} else if ((fifo->low_first > fifo->high_first) &&
+		 (fifo->high_first > fifo->high_last)) {
 		fifo->inc = false;
-	else
+		weight = fifo->low_first - fifo->high_last;
+	} else
 		return -EINVAL;
 
 	if (!fifo->read_pending || !fifo->mailbox_enable_mask ||
-	    !fifo->mailbox_disable || !fifo->mailbox_receive)
+	    !fifo->mailbox_disable ||
+	    (!fifo->mailbox_receive && !fifo->mailbox_move_to_buffer))
 		return -EINVAL;
 
+	if (fifo->mailbox_move_to_buffer) {
+		/* Make ring-buffer a sensible size that is a power of 2 */
+		fifo->ring_size = (2 << fls(weight));
+		fifo->ring = kzalloc(sizeof(struct can_frame) * fifo->ring_size,
+				     GFP_KERNEL);
+		if (!fifo->ring)
+			return -ENOMEM;
+
+		/* Take care of NAPI handling */
+		netif_napi_add(dev, &fifo->napi, can_rx_fifo_napi_poll, weight);
+	}
+
 	/* init variables */
 	fifo->mask_low = can_rx_fifo_mask_low(fifo);
 	fifo->mask_high = can_rx_fifo_mask_high(fifo);
@@ -343,6 +407,7 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 	int received = 0;
 	u64 pending;
 	unsigned int mb;
+	unsigned int head, tail;
 
 	do {
 		pending = fifo->read_pending(fifo);
@@ -372,7 +437,17 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 		fifo->active &= ~BIT_ULL(mb);
 		fifo->mailbox_disable(fifo, mb);
 
-		fifo->mailbox_receive(fifo, mb);
+		if (fifo->mailbox_move_to_buffer) {
+			head = fifo->ring_head;
+			tail = ACCESS_ONCE(fifo->ring_tail);
+			if (CIRC_SPACE(head, tail, fifo->ring_size) < 1)
+				break;
+			fifo->mailbox_move_to_buffer(fifo, &fifo->ring[head], mb);
+			smp_store_release(&fifo->ring_head,
+				(head + 1) & (fifo->ring_size - 1));
+		} else {
+			fifo->mailbox_receive(fifo, mb);
+		}
 
 		if (fifo->next == fifo->high_first) {
 			fifo->active |= fifo->mask_low;
@@ -383,6 +458,9 @@ int can_rx_fifo_poll(struct can_rx_fifo *fifo, int quota)
 		quota--;
 	} while (quota);
 
+	if (received && fifo->mailbox_move_to_buffer)
+		napi_schedule(&fifo->napi);
+
 	return received;
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_poll);
@@ -393,6 +471,32 @@ u64 can_rx_fifo_get_active_mb_mask(const struct can_rx_fifo *fifo)
 }
 EXPORT_SYMBOL_GPL(can_rx_fifo_get_active_mb_mask);
 
+int can_rx_fifo_irq_offload(struct can_rx_fifo *fifo)
+{
+	return can_rx_fifo_poll(fifo, 64);
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_irq_offload);
+
+void can_rx_fifo_napi_enable(struct can_rx_fifo *fifo)
+{
+	napi_enable(&fifo->napi);
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_napi_enable);
+
+void can_rx_fifo_napi_disable(struct can_rx_fifo *fifo)
+{
+	napi_disable(&fifo->napi);
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_napi_disable);
+
+void can_rx_fifo_del(struct can_rx_fifo *fifo)
+{
+	if (fifo->ring)
+		kfree(fifo->ring);
+	netif_napi_del(&fifo->napi);
+}
+EXPORT_SYMBOL_GPL(can_rx_fifo_del);
+
 /*
  * Local echo of CAN messages
  *
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index ed46f7d..b71f30f 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -75,6 +75,8 @@ struct can_rx_fifo {
 	void (*mailbox_enable_mask)(struct can_rx_fifo *rx_fifo, u64 mask);
 	void (*mailbox_disable)(struct can_rx_fifo *rx_fifo, unsigned int mb);
 	void (*mailbox_receive)(struct can_rx_fifo *rx_fifo, unsigned int mb);
+	void (*mailbox_move_to_buffer)(struct can_rx_fifo *rx_fifo,
+		struct can_frame *frame, unsigned int mb);
 
 	u64 mask_low;
 	u64 mask_high;
@@ -83,6 +85,12 @@ struct can_rx_fifo {
 	unsigned int next;
 
 	bool inc;
+
+	struct can_frame *ring;		/* Used by can_rx_fifo_irq_offload */
+	size_t ring_size;
+	unsigned int ring_head;
+	unsigned int ring_tail;
+	struct napi_struct napi;
 };
 
 /*
-- 
1.9.1

Re: [RFC PATCH 1/2] can: rx-fifo: Increase MB size limit from 32 to 64

[Marc Kleine-Budde]

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

On 10/07/2014 11:30 AM, David Jander wrote:
> Signed-off-by: David Jander <david@protonic.nl>

Applied to the rx-fifo branch. I can rebase the branch if you need e.g.
v3.17....

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: 181 bytes --]

Re: [RFC PATCH 1/2] can: rx-fifo: Increase MB size limit from 32 to 64

[David Jander]

Dear Marc,

On Tue, 07 Oct 2014 15:17:20 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/07/2014 11:30 AM, David Jander wrote:
> > Signed-off-by: David Jander <david@protonic.nl>
> 
> Applied to the rx-fifo branch. I can rebase the branch if you need e.g.
> v3.17....

Uh-oh! I meant to have comments on the idea more than apply immediately... I
thought a cautious "RFC" in the subject would do that ;-)

Never mind, there are still some bugs in that patch.... the
can_rx_fifo_mask_*() functions contain some 0U constants that need to change
to 0LLU. I think it is better you drop the current version for now...
I am currently debugging a new version of flexcan that uses these two patches,
to see if it works, but I'd like to have your (and other people's) opinion on
this whole idea before going too far into that direction...

Best regards,

-- 
David Jander
Protonic Holland.

Re: [RFC PATCH 1/2] can: rx-fifo: Increase MB size limit from 32 to 64

[Marc Kleine-Budde]

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

On 10/07/2014 03:27 PM, David Jander wrote:
>> Applied to the rx-fifo branch. I can rebase the branch if you need e.g.
>> v3.17....
> 
> Uh-oh! I meant to have comments on the idea more than apply immediately... I
> thought a cautious "RFC" in the subject would do that ;-)

No problem. Pushed the old code.

> Never mind, there are still some bugs in that patch.... the
> can_rx_fifo_mask_*() functions contain some 0U constants that need to change
> to 0LLU. I think it is better you drop the current version for now...
> I am currently debugging a new version of flexcan that uses these two patches,
> to see if it works, but I'd like to have your (and other people's) opinion on
> this whole idea before going too far into that direction...

The general code looks good. I'm not sure if you/we should take care of
CAN-fd support for the rx-fifo yet. I think we should wait until we have
a CAN-fd device without a proper hardware fifo.

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: 181 bytes --]

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Dear Marc,

On Mon, 06 Oct 2014 12:00:03 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/06/2014 09:28 AM, David Jander wrote:
> >>> 2.- Since the problem addressed by my patch to at91_can is very similar,
> >>> what about solving these problems in the SocketCAN framework (if that is
> >>> possible)?
> >>
> >> Have you had a look at my rx-fifo branch in
> >> https://gitorious.org/linux-can/linux-can-next? It already tries to
> >> abstract the simulation of the FIFO with the linear mailboxes.
> > 
> > Looks interesting. I think it is a good idea to do this in dev.c, since
> > there are obviously more CAN drivers that can use this. Unfortunately it
> > seems you are still pretending the napi-poll handler to call
> > can_rx_fifo_poll(). Wouldn't it be better to just empty all MBs into a
> > circular buffer or kfifo from the interrupt handler instead?
> 
> Yes probably, I started the rx-fifo patch before you came up with that idea.
> 
> > I still don't understand the results Alexander is getting, though....
> > 
> > What are you going to do with the rx-fifo work? Do you recommend to base my
> > patch on that? In that case, calling can_rx_fifo_poll() from the interrupt
> > handler will look a little awkward... but it should work. Or should I
> > propose an extension to rx-fifo?
> 
> My plans, or rather the points that need to be addressed for the rx-fifo
> are:
> - improve to work with more than 32 mailboxes. 64 are probably enough
>   for everybody :)
> - make it work with the flexcan linear buffers
> - make it work with the ti_hecc driver
> - add option or convert to run from interrupt handler and copy to
>   kfifo/cyclic buffer/...

Can you lend you brain for a sec on this problem... I think I discovered a
race condition in your rx-fifo code, but it might just be me not understanding
it correctly....

	do {
		pending = fifo->read_pending(fifo);
		pending &= fifo->active;

		if (!(pending & BIT_ULL(fifo->next))) {
			/*
			 * Wrap around only if:
			 * - we are in the upper group and
			 * - there is a CAN frame in the first mailbox
			 *   of the lower group.
			 */
			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
			    (pending & BIT_ULL(fifo->low_first))) {
				fifo->next = fifo->low_first;
				fifo->active |= fifo->mask_high;
				fifo->mailbox_enable_mask(fifo, fifo->mask_high);
			} else {
				break;
			}
		}

In this piece of code, suppose that fifo->next is in the upper half and a
message is being written to the MB it is pointing at, but it is still not
pending. The lower half has already been enabled and filled up completely (due
to latency). In that case, fifo-next will jump back to fifo->low_first and
leave a lonely filled MB in the middle of the upper half, that will trigger
and infinite loop between IRQ and can_rx_fifo_poll(). The interrupt will never
get cleared again.
I know this is an extreme case of latency being so high as to fill more than
the complete lower half, but if it strikes, it results in a lock-up. Am I
right, or did I screw up somewhere?

Thanks.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

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

On 10/08/2014 11:08 AM, David Jander wrote:
> 
> Dear Marc,
> 
> On Mon, 06 Oct 2014 12:00:03 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 10/06/2014 09:28 AM, David Jander wrote:
>>>>> 2.- Since the problem addressed by my patch to at91_can is very similar,
>>>>> what about solving these problems in the SocketCAN framework (if that is
>>>>> possible)?
>>>>
>>>> Have you had a look at my rx-fifo branch in
>>>> https://gitorious.org/linux-can/linux-can-next? It already tries to
>>>> abstract the simulation of the FIFO with the linear mailboxes.
>>>
>>> Looks interesting. I think it is a good idea to do this in dev.c, since
>>> there are obviously more CAN drivers that can use this. Unfortunately it
>>> seems you are still pretending the napi-poll handler to call
>>> can_rx_fifo_poll(). Wouldn't it be better to just empty all MBs into a
>>> circular buffer or kfifo from the interrupt handler instead?
>>
>> Yes probably, I started the rx-fifo patch before you came up with that idea.
>>
>>> I still don't understand the results Alexander is getting, though....
>>>
>>> What are you going to do with the rx-fifo work? Do you recommend to base my
>>> patch on that? In that case, calling can_rx_fifo_poll() from the interrupt
>>> handler will look a little awkward... but it should work. Or should I
>>> propose an extension to rx-fifo?
>>
>> My plans, or rather the points that need to be addressed for the rx-fifo
>> are:
>> - improve to work with more than 32 mailboxes. 64 are probably enough
>>   for everybody :)
>> - make it work with the flexcan linear buffers
>> - make it work with the ti_hecc driver
>> - add option or convert to run from interrupt handler and copy to
>>   kfifo/cyclic buffer/...
> 
> Can you lend you brain for a sec on this problem... I think I discovered a
> race condition in your rx-fifo code, but it might just be me not understanding
> it correctly....
> 
> 	do {
> 		pending = fifo->read_pending(fifo);
> 		pending &= fifo->active;
> 
> 		if (!(pending & BIT_ULL(fifo->next))) {
> 			/*
> 			 * Wrap around only if:
> 			 * - we are in the upper group and
> 			 * - there is a CAN frame in the first mailbox
> 			 *   of the lower group.
> 			 */
> 			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
> 			    (pending & BIT_ULL(fifo->low_first))) {
> 				fifo->next = fifo->low_first;
> 				fifo->active |= fifo->mask_high;
> 				fifo->mailbox_enable_mask(fifo, fifo->mask_high);
> 			} else {
> 				break;
> 			}
> 		}
> 
> In this piece of code, suppose that fifo->next is in the upper half and a
> message is being written to the MB it is pointing at, but it is still not
> pending. The lower half has already been enabled and filled up completely (due
> to latency). In that case, fifo-next will jump back to fifo->low_first and
> leave a lonely filled MB in the middle of the upper half, that will trigger
> and infinite loop between IRQ and can_rx_fifo_poll(). The interrupt will never
> get cleared again.
> I know this is an extreme case of latency being so high as to fill more than
> the complete lower half, but if it strikes, it results in a lock-up. Am I
> right, or did I screw up somewhere?

Correct analysis :( At least it shows it makes sense to have this code
in a central place.....

If we handle the low_first mailbox, we might have to check if the "old"
next, or better, if any of the mailboxes >= old_next are pending *and*
there is a non pending mailbox < "old" next. This should be possible
with one or two clever bitmasks.

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: 181 bytes --]

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Alexander Stein]

On Wednesday 08 October 2014 11:56:04, Marc Kleine-Budde wrote:
> On 10/08/2014 11:08 AM, David Jander wrote:
> > 
> > Dear Marc,
> > 
> > On Mon, 06 Oct 2014 12:00:03 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 10/06/2014 09:28 AM, David Jander wrote:
> >>>>> 2.- Since the problem addressed by my patch to at91_can is very similar,
> >>>>> what about solving these problems in the SocketCAN framework (if that is
> >>>>> possible)?
> >>>>
> >>>> Have you had a look at my rx-fifo branch in
> >>>> https://gitorious.org/linux-can/linux-can-next? It already tries to
> >>>> abstract the simulation of the FIFO with the linear mailboxes.
> >>>
> >>> Looks interesting. I think it is a good idea to do this in dev.c, since
> >>> there are obviously more CAN drivers that can use this. Unfortunately it
> >>> seems you are still pretending the napi-poll handler to call
> >>> can_rx_fifo_poll(). Wouldn't it be better to just empty all MBs into a
> >>> circular buffer or kfifo from the interrupt handler instead?
> >>
> >> Yes probably, I started the rx-fifo patch before you came up with that idea.
> >>
> >>> I still don't understand the results Alexander is getting, though....
> >>>
> >>> What are you going to do with the rx-fifo work? Do you recommend to base my
> >>> patch on that? In that case, calling can_rx_fifo_poll() from the interrupt
> >>> handler will look a little awkward... but it should work. Or should I
> >>> propose an extension to rx-fifo?
> >>
> >> My plans, or rather the points that need to be addressed for the rx-fifo
> >> are:
> >> - improve to work with more than 32 mailboxes. 64 are probably enough
> >>   for everybody :)
> >> - make it work with the flexcan linear buffers
> >> - make it work with the ti_hecc driver
> >> - add option or convert to run from interrupt handler and copy to
> >>   kfifo/cyclic buffer/...
> > 
> > Can you lend you brain for a sec on this problem... I think I discovered a
> > race condition in your rx-fifo code, but it might just be me not understanding
> > it correctly....
> > 
> > 	do {
> > 		pending = fifo->read_pending(fifo);
> > 		pending &= fifo->active;
> > 
> > 		if (!(pending & BIT_ULL(fifo->next))) {
> > 			/*
> > 			 * Wrap around only if:
> > 			 * - we are in the upper group and
> > 			 * - there is a CAN frame in the first mailbox
> > 			 *   of the lower group.
> > 			 */
> > 			if (can_rx_fifo_ge(fifo, fifo->next, fifo->high_first) &&
> > 			    (pending & BIT_ULL(fifo->low_first))) {
> > 				fifo->next = fifo->low_first;
> > 				fifo->active |= fifo->mask_high;
> > 				fifo->mailbox_enable_mask(fifo, fifo->mask_high);
> > 			} else {
> > 				break;
> > 			}
> > 		}
> > 
> > In this piece of code, suppose that fifo->next is in the upper half and a
> > message is being written to the MB it is pointing at, but it is still not
> > pending. The lower half has already been enabled and filled up completely (due
> > to latency). In that case, fifo-next will jump back to fifo->low_first and
> > leave a lonely filled MB in the middle of the upper half, that will trigger
> > and infinite loop between IRQ and can_rx_fifo_poll(). The interrupt will never
> > get cleared again.
> > I know this is an extreme case of latency being so high as to fill more than
> > the complete lower half, but if it strikes, it results in a lock-up. Am I
> > right, or did I screw up somewhere?
> 
> Correct analysis :( At least it shows it makes sense to have this code
> in a central place.....

I didn't reviewed that piece of code, I just read David's description. Well, I actually saw this scenario on pch_can where the rx mailboxes are split in lower and upper half. The current upper MB was empty and rx_poll left handling MBs and freed the lower MB. Meanwhile a frame was about beeing inserted in the current upper MB. Upon next interrupt reception started on lower MBs until eventually the remained frame in upper MB was read. But at this time the order is messed up. There was no lockup, because the interrupt signaling worked a bit different.

> If we handle the low_first mailbox, we might have to check if the "old"
> next, or better, if any of the mailboxes >= old_next are pending *and*
> there is a non pending mailbox < "old" next. This should be possible
> with one or two clever bitmasks.

If we detect that (all) MBs before the old one are pending again, we even can't ensure proper CAN frame order. All MBs below and even after old_next coul have been written meanwhile. That's why I hate those MB interfaces and prefer a real FIFO.
Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[Marc Kleine-Budde]

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

On 10/08/2014 12:36 PM, Alexander Stein wrote:
[...]
>>> In this piece of code, suppose that fifo->next is in the upper half and a
>>> message is being written to the MB it is pointing at, but it is still not
>>> pending. The lower half has already been enabled and filled up completely (due
>>> to latency). In that case, fifo-next will jump back to fifo->low_first and
>>> leave a lonely filled MB in the middle of the upper half, that will trigger
>>> and infinite loop between IRQ and can_rx_fifo_poll(). The interrupt will never
>>> get cleared again.
>>> I know this is an extreme case of latency being so high as to fill more than
>>> the complete lower half, but if it strikes, it results in a lock-up. Am I
>>> right, or did I screw up somewhere?
>>
>> Correct analysis :( At least it shows it makes sense to have this code
>> in a central place.....
> 
> I didn't reviewed that piece of code, I just read David's
> description. Well, I actually saw this scenario on pch_can where the
> rx mailboxes are split in lower and upper half. The current upper MB
> was empty and rx_poll left handling MBs and freed the lower MB.
> Meanwhile a frame was about beeing inserted in the current upper MB.
> Upon next interrupt reception started on lower MBs until eventually
> the remained frame in upper MB was read. But at this time the order
> is messed up. There was no lockup, because the interrupt signaling
> worked a bit different.
> 
>> If we handle the low_first mailbox, we might have to check if the "old"
>> next, or better, if any of the mailboxes >= old_next are pending *and*
>> there is a non pending mailbox < "old" next. This should be possible
>> with one or two clever bitmasks.
> 
> If we detect that (all) MBs before the old one are pending again, we
> even can't ensure proper CAN frame order. All MBs below and even

ACK, this is why I suggested the "non pending MB in front of old next"
check.

> after old_next coul have been written meanwhile. That's why I hate
> those MB interfaces and prefer a real FIFO.

If you have a FIFO, which is big enough....(for no various definitions
of big enough) of course a FIFO is preferred.

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: 181 bytes --]

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

On Wed, 08 Oct 2014 11:56:04 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 10/08/2014 11:08 AM, David Jander wrote:
> > 
> > Dear Marc,
> > 
> > On Mon, 06 Oct 2014 12:00:03 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 10/06/2014 09:28 AM, David Jander wrote:
> >>>>> 2.- Since the problem addressed by my patch to at91_can is very
> >>>>> similar, what about solving these problems in the SocketCAN framework
> >>>>> (if that is possible)?
> >>>>
> >>>> Have you had a look at my rx-fifo branch in
> >>>> https://gitorious.org/linux-can/linux-can-next? It already tries to
> >>>> abstract the simulation of the FIFO with the linear mailboxes.
> >>>
> >>> Looks interesting. I think it is a good idea to do this in dev.c, since
> >>> there are obviously more CAN drivers that can use this. Unfortunately it
> >>> seems you are still pretending the napi-poll handler to call
> >>> can_rx_fifo_poll(). Wouldn't it be better to just empty all MBs into a
> >>> circular buffer or kfifo from the interrupt handler instead?
> >>
> >> Yes probably, I started the rx-fifo patch before you came up with that
> >> idea.
> >>
> >>> I still don't understand the results Alexander is getting, though....
> >>>
> >>> What are you going to do with the rx-fifo work? Do you recommend to base
> >>> my patch on that? In that case, calling can_rx_fifo_poll() from the
> >>> interrupt handler will look a little awkward... but it should work. Or
> >>> should I propose an extension to rx-fifo?
> >>
> >> My plans, or rather the points that need to be addressed for the rx-fifo
> >> are:
> >> - improve to work with more than 32 mailboxes. 64 are probably enough
> >>   for everybody :)
> >> - make it work with the flexcan linear buffers
> >> - make it work with the ti_hecc driver
> >> - add option or convert to run from interrupt handler and copy to
> >>   kfifo/cyclic buffer/...
> > 
> > Can you lend you brain for a sec on this problem... I think I discovered a
> > race condition in your rx-fifo code, but it might just be me not
> > understanding it correctly....
> > 
> > 	do {
> > 		pending = fifo->read_pending(fifo);
> > 		pending &= fifo->active;
> > 
> > 		if (!(pending & BIT_ULL(fifo->next))) {
> > 			/*
> > 			 * Wrap around only if:
> > 			 * - we are in the upper group and
> > 			 * - there is a CAN frame in the first mailbox
> > 			 *   of the lower group.
> > 			 */
> > 			if (can_rx_fifo_ge(fifo, fifo->next,
> > fifo->high_first) && (pending & BIT_ULL(fifo->low_first))) {
> > 				fifo->next = fifo->low_first;
> > 				fifo->active |= fifo->mask_high;
> > 				fifo->mailbox_enable_mask(fifo,
> > fifo->mask_high); } else {
> > 				break;
> > 			}
> > 		}
> > 
> > In this piece of code, suppose that fifo->next is in the upper half and a
> > message is being written to the MB it is pointing at, but it is still not
> > pending. The lower half has already been enabled and filled up completely
> > (due to latency). In that case, fifo-next will jump back to
> > fifo->low_first and leave a lonely filled MB in the middle of the upper
> > half, that will trigger and infinite loop between IRQ and
> > can_rx_fifo_poll(). The interrupt will never get cleared again.
> > I know this is an extreme case of latency being so high as to fill more
> > than the complete lower half, but if it strikes, it results in a lock-up.
> > Am I right, or did I screw up somewhere?
> 
> Correct analysis :( At least it shows it makes sense to have this code
> in a central place.....

I placed a printk() in the interrupt handler for debugging, and that made this
issue quite likely to hit.

> If we handle the low_first mailbox, we might have to check if the "old"
> next, or better, if any of the mailboxes >= old_next are pending *and*
> there is a non pending mailbox < "old" next. This should be possible
> with one or two clever bitmasks.

I've tried to do that, but if this situation happens, then one should first
read the lower MBs anyway and then continue with the "old" next MB and
re-enable them immediately....
Problem is, I can't seem to fit this logic nicely into the current do-while
loop anymore.
Part of the problem could be solved if we could dismiss the requirement of the
"quota" parameter, i.e. support only the "empty from interrupt into circular
buffer"-scenario. Then the whole loop could get a lot simpler. Any reason not
to do this?

Besides, what about simplifying the logic just a little bit like what I did in
the original flexcan patch (mildly reworded):

" We split the MB area into two halves:
The lower half and the upper half.
We start with all MBs in EMPTY state.
The CAN controller will start filling from the lowest MB. Once this function
is called, we copy and disable in an atomic operation all FULL MBs.
The first EMPTY one we encounter may be about to get filled so we stop there.
Next time the CAN controller will have filled more MBs. Once there are no
EMPTY MBs in the lower half, we EMPTY all FULL or locked MBs again.
Next time we have the following situation:
If there is a FULL MB in the upper half, it is because it was about to get
filled when we scanned last time, or got filled just before emptying the
lowest MB, so this will be the first MB we need to copy now. If there are
no EMPTY MBs in the lower half at this time, it means we cannot guarantee
ordering anymore. It also means latency exceeded the maximum amount of
messages this controller can safely handle."

This small change avoids the pitfall we have now, because we always enable all
MBs once we crossed the split limit. To ensure ordering we only need to check
whether there are pending messages in the upper half before reading the lower
half, if previously we had crossed the split.

This means also, we can tweak performance by making an un-even split. The MBs
below the split are our "latency-buffer", while the MBs above the split are
only needed to handle incoming messages while we are busy in the read-out
loop. This part can theoretically be much smaller. The old at91_can driver
intended to work more or less like this also, so I think this will work at
least for both drivers... not sure about ti_hecc though.

Best regards,

-- 
David Jander
Protonic Holland.

Re: [PATCH v5] can: flexcan: Re-write receive path to use MB queue instead of FIFO

[David Jander]

Hi Marc,

On Wed, 8 Oct 2014 16:01:13 +0200
David Jander <david@protonic.nl> wrote:

> On Wed, 08 Oct 2014 11:56:04 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
> > On 10/08/2014 11:08 AM, David Jander wrote:
> > > 
> > > Dear Marc,
> > > 
> > > On Mon, 06 Oct 2014 12:00:03 +0200
> > > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > 
> > >> On 10/06/2014 09:28 AM, David Jander wrote:
> > >>>>> 2.- Since the problem addressed by my patch to at91_can is very
> > >>>>> similar, what about solving these problems in the SocketCAN framework
> > >>>>> (if that is possible)?
> > >>>>
> > >>>> Have you had a look at my rx-fifo branch in
> > >>>> https://gitorious.org/linux-can/linux-can-next? It already tries to
> > >>>> abstract the simulation of the FIFO with the linear mailboxes.
> > >>>
> > >>> Looks interesting. I think it is a good idea to do this in dev.c, since
> > >>> there are obviously more CAN drivers that can use this. Unfortunately
> > >>> it seems you are still pretending the napi-poll handler to call
> > >>> can_rx_fifo_poll(). Wouldn't it be better to just empty all MBs into a
> > >>> circular buffer or kfifo from the interrupt handler instead?
> > >>
> > >> Yes probably, I started the rx-fifo patch before you came up with that
> > >> idea.
> > >>
> > >>> I still don't understand the results Alexander is getting, though....
> > >>>
> > >>> What are you going to do with the rx-fifo work? Do you recommend to
> > >>> base my patch on that? In that case, calling can_rx_fifo_poll() from
> > >>> the interrupt handler will look a little awkward... but it should
> > >>> work. Or should I propose an extension to rx-fifo?
> > >>
> > >> My plans, or rather the points that need to be addressed for the rx-fifo
> > >> are:
> > >> - improve to work with more than 32 mailboxes. 64 are probably enough
> > >>   for everybody :)
> > >> - make it work with the flexcan linear buffers
> > >> - make it work with the ti_hecc driver
> > >> - add option or convert to run from interrupt handler and copy to
> > >>   kfifo/cyclic buffer/...
> > > 
> > > Can you lend you brain for a sec on this problem... I think I discovered
> > > a race condition in your rx-fifo code, but it might just be me not
> > > understanding it correctly....
> > > 
> > > 	do {
> > > 		pending = fifo->read_pending(fifo);
> > > 		pending &= fifo->active;
> > > 
> > > 		if (!(pending & BIT_ULL(fifo->next))) {
> > > 			/*
> > > 			 * Wrap around only if:
> > > 			 * - we are in the upper group and
> > > 			 * - there is a CAN frame in the first mailbox
> > > 			 *   of the lower group.
> > > 			 */
> > > 			if (can_rx_fifo_ge(fifo, fifo->next,
> > > fifo->high_first) && (pending & BIT_ULL(fifo->low_first))) {
> > > 				fifo->next = fifo->low_first;
> > > 				fifo->active |= fifo->mask_high;
> > > 				fifo->mailbox_enable_mask(fifo,
> > > fifo->mask_high); } else {
> > > 				break;
> > > 			}
> > > 		}
> > > 
> > > In this piece of code, suppose that fifo->next is in the upper half and a
> > > message is being written to the MB it is pointing at, but it is still not
> > > pending. The lower half has already been enabled and filled up completely
> > > (due to latency). In that case, fifo-next will jump back to
> > > fifo->low_first and leave a lonely filled MB in the middle of the upper
> > > half, that will trigger and infinite loop between IRQ and
> > > can_rx_fifo_poll(). The interrupt will never get cleared again.
> > > I know this is an extreme case of latency being so high as to fill more
> > > than the complete lower half, but if it strikes, it results in a lock-up.
> > > Am I right, or did I screw up somewhere?
> > 
> > Correct analysis :( At least it shows it makes sense to have this code
> > in a central place.....
> 
> I placed a printk() in the interrupt handler for debugging, and that made
> this issue quite likely to hit.
> 
> > If we handle the low_first mailbox, we might have to check if the "old"
> > next, or better, if any of the mailboxes >= old_next are pending *and*
> > there is a non pending mailbox < "old" next. This should be possible
> > with one or two clever bitmasks.
> 
> I've tried to do that, but if this situation happens, then one should first
> read the lower MBs anyway and then continue with the "old" next MB and
> re-enable them immediately....
> Problem is, I can't seem to fit this logic nicely into the current do-while
> loop anymore.
> Part of the problem could be solved if we could dismiss the requirement of
> the "quota" parameter, i.e. support only the "empty from interrupt into
> circular buffer"-scenario. Then the whole loop could get a lot simpler. Any
> reason not to do this?
> 
> Besides, what about simplifying the logic just a little bit like what I did
> in the original flexcan patch (mildly reworded):
> 
> " We split the MB area into two halves:
> The lower half and the upper half.
> We start with all MBs in EMPTY state.
> The CAN controller will start filling from the lowest MB. Once this function
> is called, we copy and disable in an atomic operation all FULL MBs.
> The first EMPTY one we encounter may be about to get filled so we stop there.
> Next time the CAN controller will have filled more MBs. Once there are no
> EMPTY MBs in the lower half, we EMPTY all FULL or locked MBs again.
> Next time we have the following situation:
> If there is a FULL MB in the upper half, it is because it was about to get
> filled when we scanned last time, or got filled just before emptying the
> lowest MB, so this will be the first MB we need to copy now. If there are
> no EMPTY MBs in the lower half at this time, it means we cannot guarantee
> ordering anymore. It also means latency exceeded the maximum amount of
> messages this controller can safely handle."
> 
> This small change avoids the pitfall we have now, because we always enable
> all MBs once we crossed the split limit. To ensure ordering we only need to
> check whether there are pending messages in the upper half before reading
> the lower half, if previously we had crossed the split.

Ok, I have just re-implemented rx-fifo this way, and after some testing with
a modified flexcan driver I'm reasonably confident that it works correctly at
least for the flexcan case. See the V2 patches that just hit the list.

Unfortunately there are a few assumptions made by the API that I haven't found
a nice way to make otherwise fool proof or self-explanatory:

1.- fifo->mailbox_move_to_buffer() must atomically disable a mailbox before or
after reading it. I guess for at91_can it needs to be disabled before reading,
while for flexcan it uses its hardware MB locking feature.

2.- fifo->mailbox_move_to_buffer() should only read out valid full MBs and
ignore empty ones, and should reflect that in the return value. This is
necessary for the algorithm to work correcly with flexcan (due to the MB
locking thing), but should also be implementable in other CAN controllers

3.- fifo->mailbox_enable_mask() and fifo->mailbox_enable() should only enable
mailboxes that are actually disabled to avoid race conditions.

> This means also, we can tweak performance by making an un-even split. The MBs
> below the split are our "latency-buffer", while the MBs above the split are
> only needed to handle incoming messages while we are busy in the read-out
> loop. This part can theoretically be much smaller.

This is BS, please ignore.

> The old at91_can driver intended to work more or less like this also, so I
> think this will work at least for both drivers... not sure about ti_hecc
> though.

Best regards,

-- 
David Jander
Protonic Holland.