[PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

Hello,

this is a proof of concept implementation to work around the
"double-RX" erratum found by Stefan Althöfer.

With the help of Thomas we found out that the chip has a time window
after receiving a CAN frame where the RX FIFO STA register content is
not read correctly.

From the driver's point of view, everything looks consistent at first,
but the head index of the chip is too large. This causes the driver to
rehandle old CAN frames that have already been processed.

The workaround uses the RX timestamp to distinguish between new and
old data. As soon as old data is found, processing is stopped.

The series applies against current net/main. The patches lack proper
descriptions, I'll add them in the next round.

Happy testing,
Marc

Link: https://lore.kernel.org/all/FR0P281MB1966273C216630B120ABB6E197E89@FR0P281MB1966.DEUP281.PROD.OUTLOOK.COM

[PATCH 1/5] can: mcp251xfd: setup cycle counter before mcp251xfd_ring_init()

[Marc Kleine-Budde]

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 20 ++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 68df6d4641b5..4c0e3580efc1 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -736,6 +736,7 @@ static void mcp251xfd_chip_stop(struct mcp251xfd_priv *priv,
 {
 	priv->can.state = state;
 
+	mcp251xfd_timestamp_stop(priv);
 	mcp251xfd_chip_interrupts_disable(priv);
 	mcp251xfd_chip_rx_int_disable(priv);
 	mcp251xfd_chip_sleep(priv);
@@ -757,34 +758,38 @@ static int mcp251xfd_chip_start(struct mcp251xfd_priv *priv)
 	if (err)
 		goto out_chip_stop;
 
+	mcp251xfd_timestamp_init(priv);
+	
 	err = mcp251xfd_set_bittiming(priv);
 	if (err)
-		goto out_chip_stop;
+		goto out_timestamp_stop;
 
 	err = mcp251xfd_chip_rx_int_enable(priv);
 	if (err)
-		goto out_chip_stop;
+		goto out_timestamp_stop;
 
 	err = mcp251xfd_chip_ecc_init(priv);
 	if (err)
-		goto out_chip_stop;
+		goto out_timestamp_stop;
 
 	err = mcp251xfd_ring_init(priv);
 	if (err)
-		goto out_chip_stop;
+		goto out_timestamp_stop;
 
 	err = mcp251xfd_chip_fifo_init(priv);
 	if (err)
-		goto out_chip_stop;
+		goto out_timestamp_stop;
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	err = mcp251xfd_chip_set_normal_mode(priv);
 	if (err)
-		goto out_chip_stop;
+		goto out_timestamp_stop;
 
 	return 0;
 
+ out_timestamp_stop:
+	mcp251xfd_timestamp_stop(priv);
  out_chip_stop:
 	mcp251xfd_dump(priv);
 	mcp251xfd_chip_stop(priv, CAN_STATE_STOPPED);
@@ -1608,7 +1613,6 @@ static int mcp251xfd_open(struct net_device *ndev)
 	if (err)
 		goto out_transceiver_disable;
 
-	mcp251xfd_timestamp_init(priv);
 	clear_bit(MCP251XFD_FLAGS_DOWN, priv->flags);
 	can_rx_offload_enable(&priv->offload);
 
@@ -1631,7 +1635,6 @@ static int mcp251xfd_open(struct net_device *ndev)
  out_can_rx_offload_disable:
 	can_rx_offload_disable(&priv->offload);
 	set_bit(MCP251XFD_FLAGS_DOWN, priv->flags);
-	mcp251xfd_timestamp_stop(priv);
  out_transceiver_disable:
 	mcp251xfd_transceiver_disable(priv);
  out_mcp251xfd_ring_free:
@@ -1656,7 +1659,6 @@ static int mcp251xfd_stop(struct net_device *ndev)
 	mcp251xfd_chip_interrupts_disable(priv);
 	free_irq(ndev->irq, priv);
 	can_rx_offload_disable(&priv->offload);
-	mcp251xfd_timestamp_stop(priv);
 	mcp251xfd_chip_stop(priv, CAN_STATE_STOPPED);
 	mcp251xfd_transceiver_disable(priv);
 	mcp251xfd_ring_free(priv);
-- 
2.39.0

[PATCH 2/5] can: mcp251xfd: introduce mcp251xfd_skb_set_timestamp_from_tbc() and make use of it

[Marc Kleine-Budde]

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 14 +++++------
 drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c  |  2 +-
 drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c |  2 +-
 .../can/spi/mcp251xfd/mcp251xfd-timestamp.c   | 18 +++----------
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     | 25 +++++++++++++++----
 5 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 4c0e3580efc1..a4bd5a75f361 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -866,18 +866,18 @@ static int mcp251xfd_get_berr_counter(const struct net_device *ndev,
 
 static struct sk_buff *
 mcp251xfd_alloc_can_err_skb(struct mcp251xfd_priv *priv,
-			    struct can_frame **cf, u32 *timestamp)
+			    struct can_frame **cf, u32 *tbc)
 {
 	struct sk_buff *skb;
 	int err;
 
-	err = mcp251xfd_get_timestamp(priv, timestamp);
+	err = mcp251xfd_get_tbc(priv, tbc);
 	if (err)
 		return NULL;
 
 	skb = alloc_can_err_skb(priv->ndev, cf);
 	if (skb)
-		mcp251xfd_skb_set_timestamp(priv, skb, *timestamp);
+		mcp251xfd_skb_set_timestamp_from_tbc(priv, skb, *tbc);
 
 	return skb;
 }
@@ -947,12 +947,12 @@ static int mcp251xfd_handle_txatif(struct mcp251xfd_priv *priv)
 static int mcp251xfd_handle_ivmif(struct mcp251xfd_priv *priv)
 {
 	struct net_device_stats *stats = &priv->ndev->stats;
-	u32 bdiag1, timestamp;
+	u32 bdiag1, tbc;
 	struct sk_buff *skb;
 	struct can_frame *cf = NULL;
 	int err;
 
-	err = mcp251xfd_get_timestamp(priv, &timestamp);
+	err = mcp251xfd_get_tbc(priv, &tbc);
 	if (err)
 		return err;
 
@@ -1034,8 +1034,8 @@ static int mcp251xfd_handle_ivmif(struct mcp251xfd_priv *priv)
 	if (!cf)
 		return 0;
 
-	mcp251xfd_skb_set_timestamp(priv, skb, timestamp);
-	err = can_rx_offload_queue_timestamp(&priv->offload, skb, timestamp);
+	mcp251xfd_skb_set_timestamp_from_tbc(priv, skb, tbc);
+	err = can_rx_offload_queue_timestamp(&priv->offload, skb, tbc);
 	if (err)
 		stats->rx_fifo_errors++;
 
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
index ced8d9c81f8c..d946d93b22e0 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
@@ -149,7 +149,7 @@ mcp251xfd_hw_rx_obj_to_skb(const struct mcp251xfd_priv *priv,
 	if (!(hw_rx_obj->flags & MCP251XFD_OBJ_FLAGS_RTR))
 		memcpy(cfd->data, hw_rx_obj->data, cfd->len);
 
-	mcp251xfd_skb_set_timestamp(priv, skb, hw_rx_obj->ts);
+	mcp251xfd_skb_set_timestamp_from_tbc(priv, skb, hw_rx_obj->ts);
 }
 
 static int
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c
index 237617b0c125..651eac105fc8 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c
@@ -109,7 +109,7 @@ mcp251xfd_handle_tefif_one(struct mcp251xfd_priv *priv,
 	tef_tail = mcp251xfd_get_tef_tail(priv);
 	skb = priv->can.echo_skb[tef_tail];
 	if (skb)
-		mcp251xfd_skb_set_timestamp(priv, skb, hw_tef_obj->ts);
+		mcp251xfd_skb_set_timestamp_from_tbc(priv, skb, hw_tef_obj->ts);
 	stats->tx_bytes +=
 		can_rx_offload_get_echo_skb(&priv->offload,
 					    tef_tail, hw_tef_obj->ts,
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
index 712e09186987..6881842310cd 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
@@ -2,7 +2,7 @@
 //
 // mcp251xfd - Microchip MCP251xFD Family CAN controller driver
 //
-// Copyright (c) 2021 Pengutronix,
+// Copyright (c) 2021, 2023 Pengutronix,
 //               Marc Kleine-Budde <kernel@pengutronix.de>
 //
 
@@ -14,17 +14,17 @@
 static u64 mcp251xfd_timestamp_read(const struct cyclecounter *cc)
 {
 	const struct mcp251xfd_priv *priv;
-	u32 timestamp = 0;
+	u32 tbc = 0;
 	int err;
 
 	priv = container_of(cc, struct mcp251xfd_priv, cc);
-	err = mcp251xfd_get_timestamp(priv, &timestamp);
+	err = mcp251xfd_get_tbc(priv, &tbc);
 	if (err)
 		netdev_err(priv->ndev,
 			   "Error %d while reading timestamp. HW timestamps may be inaccurate.",
 			   err);
 
-	return timestamp;
+	return tbc;
 }
 
 static void mcp251xfd_timestamp_work(struct work_struct *work)
@@ -39,16 +39,6 @@ static void mcp251xfd_timestamp_work(struct work_struct *work)
 			      MCP251XFD_TIMESTAMP_WORK_DELAY_SEC * HZ);
 }
 
-void mcp251xfd_skb_set_timestamp(const struct mcp251xfd_priv *priv,
-				 struct sk_buff *skb, u32 timestamp)
-{
-	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
-	u64 ns;
-
-	ns = timecounter_cyc2time(&priv->tc, timestamp);
-	hwtstamps->hwtstamp = ns_to_ktime(ns);
-}
-
 void mcp251xfd_timestamp_init(struct mcp251xfd_priv *priv)
 {
 	struct cyclecounter *cc = &priv->cc;
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index 2b0309fedfac..5d396f1311f5 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -2,7 +2,7 @@
  *
  * mcp251xfd - Microchip MCP251xFD Family CAN controller driver
  *
- * Copyright (c) 2019, 2020, 2021 Pengutronix,
+ * Copyright (c) 2019, 2020, 2021, 2023 Pengutronix,
  *               Marc Kleine-Budde <kernel@pengutronix.de>
  * Copyright (c) 2019 Martin Sperl <kernel@martin.sperl.org>
  */
@@ -786,12 +786,29 @@ mcp251xfd_spi_cmd_write(const struct mcp251xfd_priv *priv,
 	return data;
 }
 
-static inline int mcp251xfd_get_timestamp(const struct mcp251xfd_priv *priv,
-					  u32 *timestamp)
+static inline int mcp251xfd_get_tbc(const struct mcp251xfd_priv *priv,
+				    u32 *timestamp)
 {
 	return regmap_read(priv->map_reg, MCP251XFD_REG_TBC, timestamp);
 }
 
+static inline void mcp251xfd_skb_set_timestamp(struct sk_buff *skb, u64 ns)
+{
+	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+
+	hwtstamps->hwtstamp = ns_to_ktime(ns);
+}
+
+static inline
+void mcp251xfd_skb_set_timestamp_from_tbc(const struct mcp251xfd_priv *priv,
+					  struct sk_buff *skb, u32 tbc)
+{
+	u64 ns;
+
+	ns = timecounter_cyc2time(&priv->tc, tbc);
+	mcp251xfd_skb_set_timestamp(skb, tbc);
+}
+
 static inline u16 mcp251xfd_get_tef_obj_addr(u8 n)
 {
 	return MCP251XFD_RAM_START +
@@ -928,8 +945,6 @@ void mcp251xfd_ring_free(struct mcp251xfd_priv *priv);
 int mcp251xfd_ring_alloc(struct mcp251xfd_priv *priv);
 int mcp251xfd_handle_rxif(struct mcp251xfd_priv *priv);
 int mcp251xfd_handle_tefif(struct mcp251xfd_priv *priv);
-void mcp251xfd_skb_set_timestamp(const struct mcp251xfd_priv *priv,
-				 struct sk_buff *skb, u32 timestamp);
 void mcp251xfd_timestamp_init(struct mcp251xfd_priv *priv);
 void mcp251xfd_timestamp_stop(struct mcp251xfd_priv *priv);
 
-- 
2.39.0

[PATCH 3/5] can: mcp251xfd: mcp251xfd_handle_rxif_ring_uinc(): factor out in separate function

[Marc Kleine-Budde]

This is a preparation patch.

The sending of the UINC messages followed by the incrementing of the
tail pointer will be called from more 1 place in the upcoming patches,
so factor this out into a separate function.

Also make mcp251xfd_handle_rxif_ring_uinc() safe to be called with a
"len" of 0.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c | 50 +++++++++++++-------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
index d946d93b22e0..1b18867a9cd5 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
@@ -2,7 +2,7 @@
 //
 // mcp251xfd - Microchip MCP251xFD Family CAN controller driver
 //
-// Copyright (c) 2019, 2020, 2021 Pengutronix,
+// Copyright (c) 2019, 2020, 2021, 2023 Pengutronix,
 //               Marc Kleine-Budde <kernel@pengutronix.de>
 //
 // Based on:
@@ -197,6 +197,37 @@ mcp251xfd_rx_obj_read(const struct mcp251xfd_priv *priv,
 	return err;
 }
 
+static int
+mcp251xfd_handle_rxif_ring_uinc(const struct mcp251xfd_priv *priv,
+				struct mcp251xfd_rx_ring *ring,
+				u8 len)
+{
+	int offset;
+	int err;
+
+	if (!len)
+		return 0;
+
+	/* Increment the RX FIFO tail pointer 'len' times in a
+	 * single SPI message.
+	 *
+	 * Note:
+	 * Calculate offset, so that the SPI transfer ends on
+	 * the last message of the uinc_xfer array, which has
+	 * "cs_change == 0", to properly deactivate the chip
+	 * select.
+	 */
+	offset = ARRAY_SIZE(ring->uinc_xfer) - len;
+	err = spi_sync_transfer(priv->spi,
+				ring->uinc_xfer + offset, len);
+	if (err)
+		return err;
+
+	ring->tail += len;
+
+	return 0;
+}
+
 static int
 mcp251xfd_handle_rxif_ring(struct mcp251xfd_priv *priv,
 			   struct mcp251xfd_rx_ring *ring)
@@ -210,8 +241,6 @@ mcp251xfd_handle_rxif_ring(struct mcp251xfd_priv *priv,
 		return err;
 
 	while ((len = mcp251xfd_get_rx_linear_len(ring))) {
-		int offset;
-
 		rx_tail = mcp251xfd_get_rx_tail(ring);
 
 		err = mcp251xfd_rx_obj_read(priv, ring, hw_rx_obj,
@@ -227,22 +256,9 @@ mcp251xfd_handle_rxif_ring(struct mcp251xfd_priv *priv,
 				return err;
 		}
 
-		/* Increment the RX FIFO tail pointer 'len' times in a
-		 * single SPI message.
-		 *
-		 * Note:
-		 * Calculate offset, so that the SPI transfer ends on
-		 * the last message of the uinc_xfer array, which has
-		 * "cs_change == 0", to properly deactivate the chip
-		 * select.
-		 */
-		offset = ARRAY_SIZE(ring->uinc_xfer) - len;
-		err = spi_sync_transfer(priv->spi,
-					ring->uinc_xfer + offset, len);
+		err = mcp251xfd_handle_rxif_ring_uinc(priv, ring, len);
 		if (err)
 			return err;
-
-		ring->tail += len;
 	}
 
 	return 0;
-- 
2.39.0

[PATCH 4/5] can: mcp251xfd: rx: mcp251xfd_handle_rxif_ring()

[Marc Kleine-Budde]

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-ring.c    |  3 +-
 drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c  | 75 +++++++++++++------
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     | 12 +--
 3 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c
index bf3f0f150199..cbfba958e3b5 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c
@@ -2,7 +2,7 @@
 //
 // mcp251xfd - Microchip MCP251xFD Family CAN controller driver
 //
-// Copyright (c) 2019, 2020, 2021 Pengutronix,
+// Copyright (c) 2019, 2020, 2021, 2022 Pengutronix,
 //               Marc Kleine-Budde <kernel@pengutronix.de>
 //
 // Based on:
@@ -497,6 +497,7 @@ int mcp251xfd_ring_alloc(struct mcp251xfd_priv *priv)
 		}
 
 		rx_ring->obj_num = rx_obj_num;
+		rx_ring->obj_num_shift = ilog2(rx_obj_num);
 		rx_ring->obj_size = rx_obj_size;
 		priv->rx[i] = rx_ring;
 	}
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
index 1b18867a9cd5..811c4487c6fe 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
@@ -79,32 +79,59 @@ mcp251xfd_check_rx_tail(const struct mcp251xfd_priv *priv,
 	return 0;
 }
 
+static inline bool mcp251xfd_rx_fifo_sta_empty(u32 fifo_sta)
+{
+	return !(fifo_sta & MCP251XFD_REG_FIFOSTA_TFNRFNIF);
+}
+
+static inline bool mcp251xfd_rx_fifo_sta_full(u32 fifo_sta)
+{
+	return fifo_sta & MCP251XFD_REG_FIFOSTA_TFERFFIF;
+}
+
 static int
-mcp251xfd_rx_ring_update(const struct mcp251xfd_priv *priv,
-			 struct mcp251xfd_rx_ring *ring)
+mcp251xfd_get_rx_len(struct mcp251xfd_priv *priv,
+		     struct mcp251xfd_rx_ring *ring,
+		     u8 *len_p)
 {
-	u32 new_head;
-	u8 chip_rx_head;
-	bool fifo_empty;
+	u8 head, tail, shift, len;
+	u32 fifo_sta;
 	int err;
 
-	err = mcp251xfd_rx_head_get_from_chip(priv, ring, &chip_rx_head,
-					      &fifo_empty);
-	if (err || fifo_empty)
+	err = regmap_read(priv->map_reg, MCP251XFD_REG_FIFOSTA(ring->fifo_nr),
+			  &fifo_sta);
+	if (err)
 		return err;
 
-	/* chip_rx_head, is the next RX-Object filled by the HW.
-	 * The new RX head must be >= the old head.
-	 */
-	new_head = round_down(ring->head, ring->obj_num) + chip_rx_head;
-	if (new_head <= ring->head)
-		new_head += ring->obj_num;
+	if (mcp251xfd_rx_fifo_sta_empty(fifo_sta)) {
+		*len_p = 0;
+		return 0;
+	}
 
-	ring->head = new_head;
+	if (mcp251xfd_rx_fifo_sta_full(fifo_sta)) {
+		*len_p = ring->obj_num;
+		return 0;
+	}
+
+	head = FIELD_GET(MCP251XFD_REG_FIFOSTA_FIFOCI_MASK, fifo_sta);
+
+	err =  mcp251xfd_check_rx_tail(priv, ring);
+	if (err)
+		return err;
+	tail = mcp251xfd_get_rx_tail(ring);
 
-	return mcp251xfd_check_rx_tail(priv, ring);
+	/* First shift to full u8. The subtraction then works on
+	 * singed values, that keeps difference steady around the u8
+	 * overflow. The right shift acts on len, which is an u8.
+	 */
+	shift = BITS_PER_BYTE - ring->obj_num_shift;
+	len = (head << shift) - (tail << shift);
+	*len_p = len >> shift;
+
+	return 0;
 }
 
+
 static void
 mcp251xfd_hw_rx_obj_to_skb(const struct mcp251xfd_priv *priv,
 			   const struct mcp251xfd_hw_rx_obj_canfd *hw_rx_obj,
@@ -208,6 +235,8 @@ mcp251xfd_handle_rxif_ring_uinc(const struct mcp251xfd_priv *priv,
 	if (!len)
 		return 0;
 
+	ring->head += len;
+
 	/* Increment the RX FIFO tail pointer 'len' times in a
 	 * single SPI message.
 	 *
@@ -233,22 +262,22 @@ mcp251xfd_handle_rxif_ring(struct mcp251xfd_priv *priv,
 			   struct mcp251xfd_rx_ring *ring)
 {
 	struct mcp251xfd_hw_rx_obj_canfd *hw_rx_obj = ring->obj;
-	u8 rx_tail, len;
+	u8 rx_tail, len, l;
 	int err, i;
 
-	err = mcp251xfd_rx_ring_update(priv, ring);
+	err = mcp251xfd_get_rx_len(priv, ring, &len);
 	if (err)
 		return err;
 
-	while ((len = mcp251xfd_get_rx_linear_len(ring))) {
+	while ((l = mcp251xfd_get_rx_linear_len(ring, len))) {
 		rx_tail = mcp251xfd_get_rx_tail(ring);
 
 		err = mcp251xfd_rx_obj_read(priv, ring, hw_rx_obj,
-					    rx_tail, len);
+					    rx_tail, l);
 		if (err)
 			return err;
 
-		for (i = 0; i < len; i++) {
+		for (i = 0; i < l; i++) {
 			err = mcp251xfd_handle_rxif_one(priv, ring,
 							(void *)hw_rx_obj +
 							i * ring->obj_size);
@@ -256,9 +285,11 @@ mcp251xfd_handle_rxif_ring(struct mcp251xfd_priv *priv,
 				return err;
 		}
 
-		err = mcp251xfd_handle_rxif_ring_uinc(priv, ring, len);
+		err = mcp251xfd_handle_rxif_ring_uinc(priv, ring, l);
 		if (err)
 			return err;
+
+		len -= l;
 	}
 
 	return 0;
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index 5d396f1311f5..11f6e1c6fc60 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -550,6 +550,7 @@ struct mcp251xfd_rx_ring {
 	u8 nr;
 	u8 fifo_nr;
 	u8 obj_num;
+	u8 obj_num_shift;
 	u8 obj_size;
 
 	union mcp251xfd_write_reg_buf irq_enable_buf;
@@ -908,18 +909,9 @@ static inline u8 mcp251xfd_get_rx_tail(const struct mcp251xfd_rx_ring *ring)
 	return ring->tail & (ring->obj_num - 1);
 }
 
-static inline u8 mcp251xfd_get_rx_len(const struct mcp251xfd_rx_ring *ring)
-{
-	return ring->head - ring->tail;
-}
-
 static inline u8
-mcp251xfd_get_rx_linear_len(const struct mcp251xfd_rx_ring *ring)
+mcp251xfd_get_rx_linear_len(const struct mcp251xfd_rx_ring *ring, u8 len)
 {
-	u8 len;
-
-	len = mcp251xfd_get_rx_len(ring);
-
 	return min_t(u8, len, ring->obj_num - mcp251xfd_get_rx_tail(ring));
 }
 
-- 
2.39.0

[PATCH 5/5] can: mcp251xfd: implement workaround for double-RX erratum

[Marc Kleine-Budde]

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-ring.c    |  1 +
 drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c  | 26 ++++++++++++++++---
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     |  3 +++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c
index cbfba958e3b5..f90a83390db2 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-ring.c
@@ -196,6 +196,7 @@ mcp251xfd_ring_init_rx(struct mcp251xfd_priv *priv, u16 *base, u8 *fifo_nr)
 	int i, j;
 
 	mcp251xfd_for_each_rx_ring(priv, rx_ring, i) {
+		rx_ring->last_valid = timecounter_read(&priv->tc);
 		rx_ring->head = 0;
 		rx_ring->tail = 0;
 		rx_ring->base = *base;
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
index 811c4487c6fe..b9c4f7148b53 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-rx.c
@@ -175,8 +175,6 @@ mcp251xfd_hw_rx_obj_to_skb(const struct mcp251xfd_priv *priv,
 
 	if (!(hw_rx_obj->flags & MCP251XFD_OBJ_FLAGS_RTR))
 		memcpy(cfd->data, hw_rx_obj->data, cfd->len);
-
-	mcp251xfd_skb_set_timestamp_from_tbc(priv, skb, hw_rx_obj->ts);
 }
 
 static int
@@ -187,8 +185,21 @@ mcp251xfd_handle_rxif_one(struct mcp251xfd_priv *priv,
 	struct net_device_stats *stats = &priv->ndev->stats;
 	struct sk_buff *skb;
 	struct canfd_frame *cfd;
+	u64 timestamp;
 	int err;
 
+	timestamp = timecounter_cyc2time(&priv->tc, hw_rx_obj->ts);
+	if (timestamp <= ring->last_valid ) {
+		netdev_dbg(priv->ndev, "%s: last_valid=0x%016llx ts=0x%016llx d=0x%016llx data=%*ph - Dropping\n", __func__,
+			   ring->last_valid, timestamp,
+			   timestamp - ring->last_valid,
+			   8, hw_rx_obj->data);
+		stats->rx_fifo_errors++;
+
+		return -EBADMSG;
+	}
+	ring->last_valid = timestamp;
+
 	if (hw_rx_obj->flags & MCP251XFD_OBJ_FLAGS_FDF)
 		skb = alloc_canfd_skb(priv->ndev, &cfd);
 	else
@@ -199,6 +210,7 @@ mcp251xfd_handle_rxif_one(struct mcp251xfd_priv *priv,
 		return 0;
 	}
 
+	mcp251xfd_skb_set_timestamp(skb, timestamp);
 	mcp251xfd_hw_rx_obj_to_skb(priv, hw_rx_obj, skb);
 	err = can_rx_offload_queue_timestamp(&priv->offload, skb, hw_rx_obj->ts);
 	if (err)
@@ -281,7 +293,15 @@ mcp251xfd_handle_rxif_ring(struct mcp251xfd_priv *priv,
 			err = mcp251xfd_handle_rxif_one(priv, ring,
 							(void *)hw_rx_obj +
 							i * ring->obj_size);
-			if (err)
+			/* -EBADMSG means we're affected by an
+			 * erratum, i.e. that timestamp in the RX
+			 * object is older that the last valid
+			 * received CAN frame. Don't process any
+			 * further.
+			 */
+			if (err == -EBADMSG)
+				return mcp251xfd_handle_rxif_ring_uinc(priv, ring, i);
+			else if (err)
 				return err;
 		}
 
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index 11f6e1c6fc60..d973a89cf7f8 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -546,6 +546,9 @@ struct mcp251xfd_rx_ring {
 	unsigned int head;
 	unsigned int tail;
 
+	/* timestamp of the last valid received CAN frame */
+	u64 last_valid;
+
 	u16 base;
 	u8 nr;
 	u8 fifo_nr;
-- 
2.39.0

Re: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

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

On 11.01.2023 23:20:37, Marc Kleine-Budde wrote:
> this is a proof of concept implementation to work around the
> "double-RX" erratum found by Stefan Althöfer.
> 
> With the help of Thomas we found out that the chip has a time window
> after receiving a CAN frame where the RX FIFO STA register content is
> not read correctly.
> 
> From the driver's point of view, everything looks consistent at first,
> but the head index of the chip is too large. This causes the driver to
> rehandle old CAN frames that have already been processed.
> 
> The workaround uses the RX timestamp to distinguish between new and
> old data. As soon as old data is found, processing is stopped.
> 
> The series applies against current net/main. The patches lack proper
> descriptions, I'll add them in the next round.
> 
> Happy testing,
> Marc
> 
> Link: https://lore.kernel.org/all/FR0P281MB1966273C216630B120ABB6E197E89@FR0P281MB1966.DEUP281.PROD.OUTLOOK.COM

If you add a "#define DEBUG" before any #includes in mcp251xfd-rc.c
you'll get a debug message like this if the workaround triggers:

| Jan 11 22:53:10 riot kernel: mcp251xfd spi1.0 mcp251xfd0: mcp251xfd_handle_rxif_one: last_valid=0x17395fba3d56bb9c ts=0x17395fba3d08ee9e d=0xffffffffffb23302 data=00 02 80 fa 24 cc 43 41 - Dropping

You can also have a look at the interface statistics. The "RX errors:
fifo" is incremented if the workaround triggers.

| $ ip --details -s -s link show mcp251xfd0
| 5: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
|     link/can  promiscuity 0 minmtu 0 maxmtu 0 
|     can <LOOPBACK,BERR-REPORTING,FD> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 1000 
|           bitrate 1000000 sample-point 0.750
|           tq 25 prop-seg 14 phase-seg1 15 phase-seg2 10 sjw 1 brp 1
|           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
|           dbitrate 4000000 dsample-point 0.700
|           dtq 25 dprop-seg 3 dphase-seg1 3 dphase-seg2 3 dsjw 1 dbrp 1
|           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
|           clock 40000000 
|           re-started bus-errors arbit-lost error-warn error-pass bus-off
|           0          0          0          0          0          0         numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi1.0 
|     RX:  bytes packets errors dropped  missed   mcast           
|      190713920 9871978      0       0       0       0 
|     RX errors:  length    crc   frame    fifo overrun           
|                      0      0       0       1       0
                                           ^^^^
|     TX:  bytes packets errors dropped carrier collsns           
|       95356960 4935989      0       0       0       0 
|     TX errors: aborted   fifo  window heartbt transns
|                      0      0       0       0       1 

Note: Certain Out-of-Memory situations also increase the fifo error
counter, but I think you'll get a nice OOM error message from the
kernel, too.

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

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

On 11.01.2023 23:30:04, Marc Kleine-Budde wrote:
> On 11.01.2023 23:20:37, Marc Kleine-Budde wrote:
> > this is a proof of concept implementation to work around the
> > "double-RX" erratum found by Stefan Althöfer.
> > 
> > With the help of Thomas we found out that the chip has a time window
> > after receiving a CAN frame where the RX FIFO STA register content is
> > not read correctly.
> > 
> > From the driver's point of view, everything looks consistent at first,
> > but the head index of the chip is too large. This causes the driver to
> > rehandle old CAN frames that have already been processed.
> > 
> > The workaround uses the RX timestamp to distinguish between new and
> > old data. As soon as old data is found, processing is stopped.
> > 
> > The series applies against current net/main. The patches lack proper
> > descriptions, I'll add them in the next round.
> > 
> > Happy testing,
> > Marc
> > 
> > Link: https://lore.kernel.org/all/FR0P281MB1966273C216630B120ABB6E197E89@FR0P281MB1966.DEUP281.PROD.OUTLOOK.COM
> 
> If you add a "#define DEBUG" before any #includes in mcp251xfd-rc.c
> you'll get a debug message like this if the workaround triggers:
> 
> | Jan 11 22:53:10 riot kernel: mcp251xfd spi1.0 mcp251xfd0: mcp251xfd_handle_rxif_one: last_valid=0x17395fba3d56bb9c ts=0x17395fba3d08ee9e d=0xffffffffffb23302 data=00 02 80 fa 24 cc 43 41 - Dropping

In my over night test the workaround triggered 31 times. The test has
found no problems.

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Stefan Althöfer]

> In my over night test the workaround triggered 31 times. The test has found no problems.

This is frustrating: how did you manage to get that much triggers ;-)  I ran 4 targets in parallel and got 3 fifo errors in
total after 20 h and no test failures. I will now switch my setup from doing the selft-receiption test to a two-node-same-host transmission test
which is where I first detected the error.

BTW: "... (applies) against current net/main" - is this kh-lingo for using this: git clone https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/ ?

Oh and I "backported" the patched driver source into my 6.0.12 raspbian kernel.

mfg
Stefan

Re: AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

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

On 13.01.2023 12:39:58, Stefan Althöfer wrote:
> > In my over night test the workaround triggered 31 times. The test
> > has found no problems.
> 
> This is frustrating: how did you manage to get that much triggers ;-)

I don't know. In an older version of the workaround it triggered about
every 12 Minutes.

> I ran 4 targets in parallel and got 3 fifo errors in total after 20 h
> and no test failures.

At least the workaround triggered and the test didn't fail! \o/

> I will now switch my setup from doing the selft-receiption test to a
> two-node-same-host transmission test which is where I first detected
> the error.

Makes sense. If you're happy please reply to the patches with a
"Tested-by: Stefan Althöfer <Stefan.Althoefer@janztec.com>" tag.

> BTW: "... (applies) against current net/main" - is this kh-lingo for
> using this: git clone
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/ ?

Yes - it means that I applied the patches to the main branch of that
tree. On older kernels it might not apply without problems.

But you don't have to clone the tree, you better add it as a remote to
your existing tree, e.g.:

| git remote add -f net git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

...and then check it out:

| git checkout -b my-net-main net/main

> Oh and I "backported" the patched driver source into my 6.0.12
> raspbian kernel.

I think the patches apply without problems, there we not many changes
since v6.0.

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Thomas.Kopp]

> > > In my over night test the workaround triggered 31 times. The test
> > > has found no problems.
> >
> > This is frustrating: how did you manage to get that much triggers ;-)
> 
> I don't know. In an older version of the workaround it triggered about
> every 12 Minutes.

Don't have numbers but in my tests the Workaround triggered way more often than the original issue (in this kernel release). Then again this issue is all about the timing, so slight changes in the timing could have that effect.
I had the test running for the entire weekend on a pi4 with the original SEED CANFD hat and the latest 6.2 RC kernel from the RPI github repo and didn't find any issues related to this bug, so, 
Tested-by: Thomas Kopp <thomas.kopp@microchip.com>

Best Regards,
Thomas

AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Stefan Althöfer]

Tested this on two raspberry CM4 with two MCP1518 controllers each. CAN frames are sent (and received) by both sides "simultaneously".
In 67 h of testing an average of about 100 fifo errors occurred for each CAN controller. No wrong messages we received by the application.
Tested-by: Stefan Althöfer <Stefan.Althoefer@janztec.com>

Just to make sure I'm correctly understanding the issues impact: The driver reads one complete timed-back message (76 bytes) and then 
re-syncs to the fifo to read the now hopefully correct pointers: Hence there is a spurious extra latency of about < 100 us (assuming *'
10 MHz SPI clock)?

--
Stefan

AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Stefan Althöfer]

Hi,

> With the help of Thomas we found out that the chip has a time window after receiving a CAN frame where the RX FIFO STA register content is not read correctly.

Does the workaround assume that most of the messages in the fifo are old (already read) content? What happens if there is mostly new (not yet
read) content in the fifo? Suppose a slow host or coalescing. Can the temporarily incorrect RX FIFO STA register point to one of those (ahead of next-to-read)? 
Wouldn't we drop messages or cause a deadlock then?

--
Stefan

Re: AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

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

On 16.01.2023 19:49:18, Stefan Althöfer wrote:
> > With the help of Thomas we found out that the chip has a time window
> > after receiving a CAN frame where the RX FIFO STA register content
> > is not read correctly.
> 
> Does the workaround assume that most of the messages in the fifo are
> old (already read) content?

Before this change the driver used to read the RX FIFO head from the
chip (it points to the index that is written to next by the driver).
Then all CAN frames between the FIFO tail and head index are read from
the chip's RAM, pushed into the networking stack and finally marked as
read in the chip.

The driver 100% trusts the FIFO head read from the chip, in the bad case
it 1) pushed old messages into the networking stack and 2) confuses the
chip with marking "too many" messages as read. The rest are follow up
errors.


With the patch the driver still reads the RX FIFO head, but calculates
the number of RX'ed CAN frames waiting in the FIFO. (That is the
difference between the FIFO head and tail, plus taking wraparounds and
the FIFO full and empty bits into account.) The driver reads the waiting
CAN frames from the chip's RAM, and iterates over them.

In the good case the timestamps of the messages are always increasing.
(Again taking wraparounds and long pauses between messages into
account.)

In the bad case the driver encounters a timestamp that is older than the
last successfully received one, it assumes due to the erratum. The RX
FIFO head was read as too big, resulting in too many read messages. The
processing is stopped.

The driver still trusts that the RX messages are filled in FIFO manner
into the ring-buffer in the RAM. As soon as an old messages is
encountered, the driver assumes it has read too far and only "old"
messages will follow.

Thomas did some test on a µC, where the RX FIFO head index in read 2x in
a row. With the right timing the 1st head index is broken, while the 2nd
read shows correct data again.

In both the good and bad case now the driver's FIFO head and tail are
updated with the successful processed messages. These number of messages
are marked as read in the chip.

> What happens if there is mostly new (not yet read) content in the
> fifo? Suppose a slow host or coalescing. Can the temporarily incorrect
> RX FIFO STA register point to one of those (ahead of next-to-read)?

Let me explain how I understood you scenario:
- there are 4 messages pending
- the driver reads the FIFO head, read hit by erratum
- driver thinks it has to read 8 messages
- the host is slow and on the CAN bus 2 new messages arrive
- on the chip a 4+2=6 messages pending
- the driver reads 8 messages from the chip
- the driver iterates over the 8 messages
- the timestamp of message 6 is newer than message 7
- the driver stops processing after message 6
- 6 messages are marked are read in the chip
- with the next RX-IRQ the next loop begins...
  note: the next message that is processed is message 7
  because we stopped processing after 6 in the previous loop

> Wouldn't we drop messages or cause a deadlock then?

The driver doesn't "step over" old messages in the FIFO, it stops
processing, finishes, and waits for the next RX-IRQ to come.

Hope that makes it a bit clearer,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

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

On 16.01.2023 08:43:29, Stefan Althöfer wrote:
> Tested this on two raspberry CM4 with two MCP1518 controllers each.
> CAN frames are sent (and received) by both sides "simultaneously". In
> 67 h of testing an average of about 100 fifo errors occurred for each
> CAN controller. No wrong messages we received by the application.
>
> Tested-by: Stefan Althöfer <Stefan.Althoefer@janztec.com>

Added, thanks.

> Just to make sure I'm correctly understanding the issues impact: The
> driver reads one complete timed-back message (76 bytes)

The driver reads the RX FIFO head index and has a cached tail index. It
calculates the number of pending messages (which are too many in the bad
case) and reads them from the chip. It reads n * 76 bytes. This might be
some CAN frames too much.

> and then re-syncs to the fifo to read the now hopefully correct
> pointers: Hence there is a spurious extra latency of about < 100 us
> (assuming *' 10 MHz SPI clock)?

The latency introduced by the erratum is "76 bytes * number of old
messages", if the controller is configured for CAN-FD, it's only 20
bytes in the CAN-2.0 mode.

Note: I haven't invested much time in the optimization of CAN-FD, as it
was not important for our customers. There are several options:
- In CRC SPI transfer mode at max 252 bytes are read from the RAM,
  however the chip should support up to 1020 bytes in one transfer.
- The driver always reads full 76 bytes in CAN-FD mode. Splitting the
  transfer in to 2 parts might be more performant for small CAN-FD
  frames. In the first transfer the driver would read the timestamp, ID
  and DLC and some amount of data, after decoding the DLC the remaining
  data can be read in a 2nd transfer.

Contact me if you are interested. :)

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

AW: AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Stefan Althöfer]

> Before this change the driver used to read the RX FIFO head from the chip 
> (it points to the index that is **written** to next by the driver).

Read? 


My concern was about such a situation

      3     1   2
--------------------
N|N|O|O|O|0|N|N|N|N|
--------------------

Correct fifo head is [1]. What are the potential false reads?
Is it possible that a false read of fifo head will point to  [2].
In cases that I have seen it had pointed to older messages always
e.g. [3].

--
Stefan

Re: AW: AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

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

On 19.01.2023 07:47:44, Stefan Althöfer wrote:
> > Before this change the driver used to read the RX FIFO head from the chip
> > (it points to the index that is **written** to next by the driver).
>
> Read?

Doh, it should read:

it points to the index that is written to next by the *chip*.

> My concern was about such a situation
>
>       3     1   2
> --------------------
> N|N|O|O|O|0|N|N|N|N|
> --------------------

What's the meaning of 'N', 'O' and '0' (zero)?

> Correct fifo head is [1]. What are the potential false reads?
> Is it possible that a false read of fifo head will point to [2].
>
> In cases that I have seen it had pointed to older messages always
> e.g. [3].

|     4 3     1   2
| --------------------
| N|N|O|O|O|O|N|N|N|N|
| --------------------

If N means new, and O means old (I've replaced the 0 by O), then the
correct head would be [4]. That's the index the chip will write the next
RX'ed CAN frame. The chip's tail index would point to [1]. That means
all CAN frames between [1] (including) and [4] (excluding) are new and
must be read.

If the driver reads a wrong FIFO head, there are 2 possibilities:
a) driver reads FIFO head [2]
   - all frames between [1] (including) and [2] excluding are read and
     pushed into the networking stack (denoted by [R]):

|     4 3     1   2
| --------------------
| N|N|O|O|O|O|N|N|N|N|
| --------------------
|             R R

   - 2 frames are marked as read, the chip's tail pointer will advance
     from [1] to [2].

|     4 3     1   2
| --------------------
| N|N|O|O|O|O|O|O|N|N|
| --------------------
|

   - the RX interrupt stays active, as the RX FIFO is not empty
   - the usual RX IRQ handling takes place and the rest of the CAN
     frames are read.

|     4 3     1   2
| --------------------
| O|O|O|O|O|O|O|O|O|O|
| --------------------
|

   - the chip's tail index is correct [4]
   - the erratum results in a higher latency, as the driver could
     have handled 4 RX CAN frames in one burst instead of 2.

b) driver reads FIFO head [3]

   - all frames between [1] (including) end of FIFO are read and
     pushed into the networking stack (denoted by [R]):

|     4 3     1   2
| --------------------
| N|N|O|O|O|O|N|N|N|N|
| --------------------
|             R R R R

   - 4 frames are marked as read, the chip's tail pointer will advance
     from [1] to [5].

| 5   4 3     1   2
| --------------------
| N|N|O|O|O|O|O|O|O|O|
| --------------------
|

   - all frames between [5] (including) end [3] (excluding) are read

| 5   4 3     1   2
| --------------------
| N|N|O|O|O|O|O|O|O|O|
| --------------------
| R R R

   - during the iteration the driver sees an old timestamp for the 3rd
     CAN frame and stops processing there
   - although 3 CAN frames have been read from the FIFO, only 2 have
     been processes and pushed to the networking stack
   - 2 frames are marked as read, the chip's tail index will advance
     from [5] to [4].

| 5   4 3     1   2
| --------------------
| O|O|O|O|O|O|O|O|O|O|
| --------------------
|

   - the chip's tail index is correct [4]
   - the overhead due to the erratum was the longer SPI read of the last
     CAN RX frame and the additional processing time (which is probably
     neglectable compared to the SPI overhead).

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Tom Evans]

On 12/1/23 09:30, Marc Kleine-Budde wrote:
> On 11.01.2023 23:20:37, Marc Kleine-Budde wrote:
>> this is a proof of concept implementation to work around the
>> "double-RX" erratum found by Stefan Althöfer.
>>
>> With the help of Thomas we found out that the chip has a time window
>> after receiving a CAN frame where the RX FIFO STA register content is
>> not read correctly.

This is being called an "erratum". I take that to mean an admitted bug published by the chip 
manufacturer. Has there been any response from Microchip on this yet? If they could properly 
describe what's wrong, it might lead to more robust work arounds.

I've noticed people know about the "maximum SPI clock rate", and are getting close to it in testing. 
The chip might have more (and more frequent) problems near that limit.

The MCP2517FD has more errata items than the MCP2518FD. Anyone using the earlier chip might be 
seeing more problems than people using the MCP2518FD are.

The MCP2517FD (published Errata item #1) is sensitive to delays between SPI Write and delays between 
writes and Chip Select Deassertion. Some SPI drivers and setups don't use the SPI controller's 
native chip-select, but use GPIO pins for flexibility. On Linux that can result in long delays until 
the GPIO Chip Select is deasserted, and long delays between bytes. There are DMA-based SPI 
controllers without these problems, but there may not be full driver support for them. YMMV.

Anyone seeing a difference in errors between two different SPI controllers might be seeing the 
results of different timing (chip select and byte to byte) between them.

Tom

RE: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Thomas.Kopp]

Hi Tom,

> >> With the help of Thomas we found out that the chip has a time window
> >> after receiving a CAN frame where the RX FIFO STA register content is
> >> not read correctly.
> 
> This is being called an "erratum". I take that to mean an admitted bug
> published by the chip
> manufacturer. Has there been any response from Microchip on this yet? If
> they could properly
> describe what's wrong, it might lead to more robust work arounds.
That's the plan, yes. We don't have simulations yet to fully explain what's going on and more important to make sure that nothing is missed. Once that is fully understood I'll update the erratasheet. In the meantime I think it makes sense to work on and test the workaround. This was the approach for the max spi frequency bug as well. The initial workaround based on testing only was limiting the clock to 92.5% of clk/2(which proved pretty robust in testing) and was then changed to the now in place 85%.
If your suggestion was to call it bug until being an published erratum, yes - I agree that it might things easier to follow and see at which state we are.

> I've noticed people know about the "maximum SPI clock rate", and are
> getting close to it in testing.
> The chip might have more (and more frequent) problems near that limit.
That shouldn't be the case. The max frequency given in the erratum was chosen so that nothing can happen at that frequency or below that - at least not for this particular failure mode.

> The MCP2517FD has more errata items than the MCP2518FD. Anyone using
> the earlier chip might be
> seeing more problems than people using the MCP2518FD are.
Yes, at this point especially with the long delays erratum on the MCP2517FD and the way that most SPI Controllers under Linux behave the MCP2518FD is the much better choice. They're pin compatible anyway and mostly SW compatible (that is true unless you were ignoring things like leaving reserved bits untouched etc.) The MCP2518FD has had the sequence field extended and the LPMEN bit added but that's it. So for 99% of the people there's really no reason to still use the 17FD.

Thomas

Re: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Marc Kleine-Budde]

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

On 25.01.2023 16:41:50, Tom Evans wrote:
> On 12/1/23 09:30, Marc Kleine-Budde wrote:
> > On 11.01.2023 23:20:37, Marc Kleine-Budde wrote:
> > > this is a proof of concept implementation to work around the
> > > "double-RX" erratum found by Stefan Althöfer.
> > > 
> > > With the help of Thomas we found out that the chip has a time window
> > > after receiving a CAN frame where the RX FIFO STA register content is
> > > not read correctly.
> 
> This is being called an "erratum". I take that to mean an admitted bug
> published by the chip manufacturer. Has there been any response from
> Microchip on this yet? If they could properly describe what's wrong, it
> might lead to more robust work arounds.

I'm working with Thomas from Microchip on this actively on this. We're
waiting from simulation results from the hardware team...

> I've noticed people know about the "maximum SPI clock rate", and are getting
> close to it in testing. The chip might have more (and more frequent)
> problems near that limit.

In Stefan's test setup one SPI bus uses 16.67 MHz and the other 10 MHz.
Stefan, Thomas, which chip shows the problem?

I have reproduced the problem at 15 MHz on a different SoC (i.MX6).

> The MCP2517FD has more errata items than the MCP2518FD. Anyone using the
> earlier chip might be seeing more problems than people using the MCP2518FD
> are.

Yes, Thomas found problems with the loopback mode of the MCP2517FD, but
we haven't looked deeper into that.

> The MCP2517FD (published Errata item #1) is sensitive to delays between SPI
> Write and delays between writes and Chip Select Deassertion. Some SPI
> drivers and setups don't use the SPI controller's native chip-select, but
> use GPIO pins for flexibility. On Linux that can result in long delays until
> the GPIO Chip Select is deasserted, and long delays between bytes. There are
> DMA-based SPI controllers without these problems, but there may not be full
> driver support for them. YMMV.
> 
> Anyone seeing a difference in errors between two different SPI controllers
> might be seeing the results of different timing (chip select and byte to
> byte) between them.

Both Linux based tests system (rpi4, imx6) are using GPIO chip selects.
On the other hand Thomas has reproduced the broken RX FIFO STA register
read bug on a µC based setup (with a mcp2518fd). As far as we understand
it, the problem occurs in a critical timing window between the reception
of a CAN frame and the reading of the RX FIFO STA register.

Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

AW: [PATCH 0/5] can: mcp251xfd: workaround double-RX erratum

[Stefan Althöfer]

> In Stefan's test setup one SPI bus uses 16.67 MHz and the other 10 MHz.
> Stefan, Thomas, which chip shows the problem?

It happens on both. The problem does not scale with frequency. The probability
of hitting the critical case varies.

--
Stefan