[PATCH 0/3] can: m_can: stabilise peripheral m_can RX and TX

[PATCH 0/3] can: m_can: stabilise peripheral m_can RX and TX

[Torin Cooper-Bennun]

In this patch series, rx-offload is utilised in the m_can driver,
instead of sending skbs directly from a threaded IRQ using
netif_receive_skb() (which is meant to be called only from a softirq
context). This should fix the major stability problems with M_CAN
peripherals (tcan4x5x being the only practical example at the moment).

I have tested this with a Raspberry Pi 3 Model B and a TCAN4550. The
driver no longer crashes outright, and TX works consistently and echoes
correctly. RX is also much more stable, but I think more testing is
required with different hardware to really ensure stability.

There is definitely at least one outstanding issue. In my tests, if the
TCAN4550 encounters bus activity immediately after init, such that RX
FIFO 0 is already full by the time the IRQ handler is first called, then
in the initial round of RX polling, the frames are apparently read out
empty (DLC = 0, ID = 0). After this point, RX works as expected. I
assume there's a data race hidden in the init process somewhere, but I
have failed to find it!

On my hardware setup, I still have occasional incorrectly-read-out
frames (e.g. DLC wrong, or the odd 4 data bytes reading 0), but I am yet
to rule out SPI signal problems, nor Raspberry Pi SPI hardware problems.
In any case, these problems existed in my setup before this patch
series.

I would be grateful if someone were able to test these changes with a
different hardware setup, and also with non-peripheral M_CAN.

Many thanks,
Torin

[PATCH 1/3] can: m_can: add infrastructure for internal timestamps

[Torin Cooper-Bennun]

Add infrastucture to allow internal timestamps from the M_CAN to be
configured and retrieved.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/m_can.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 0c8d36bc668c..ea79cf0640a8 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -148,6 +148,19 @@ enum m_can_reg {
 #define NBTP_NTSEG2_SHIFT	0
 #define NBTP_NTSEG2_MASK	(0x7f << NBTP_NTSEG2_SHIFT)
 
+/* Timestamp Counter Configuration Register (TSCC) */
+#define TSCC_TCP_SHIFT		16
+#define TSCC_TCP_MASK		(0xf << TSCC_TCP_SHIFT)
+#define TSCC_TSS_SHIFT		0
+#define TSCC_TSS_MASK		(0x3 << TSCC_TSS_SHIFT)
+#define TSCC_TSS_DISABLE	(0x0 << TSCC_TSS_SHIFT)
+#define TSCC_TSS_INTERNAL	(0x1 << TSCC_TSS_SHIFT)
+#define TSCC_TSS_EXTERNAL	(0x2 << TSCC_TSS_SHIFT)
+
+/* Timestamp Counter Value Register (TSCV) */
+#define TSCV_TSC_SHIFT		0
+#define TSCV_TSC_MASK		(0xffff << TSCV_TSC_SHIFT)
+
 /* Error Counter Register(ECR) */
 #define ECR_RP			BIT(15)
 #define ECR_REC_SHIFT		8
@@ -302,6 +315,8 @@ enum m_can_reg {
 #define RX_BUF_ANMF		BIT(31)
 #define RX_BUF_FDF		BIT(21)
 #define RX_BUF_BRS		BIT(20)
+#define RX_BUF_RXTS_SHIFT	0
+#define RX_BUF_RXTS_MASK	(0xffff << RX_BUF_RXTS_SHIFT)
 
 /* Tx Buffer Element */
 /* T0 */
@@ -319,6 +334,8 @@ enum m_can_reg {
 /* E1 */
 #define TX_EVENT_MM_SHIFT	TX_BUF_MM_SHIFT
 #define TX_EVENT_MM_MASK	(0xff << TX_EVENT_MM_SHIFT)
+#define TX_EVENT_TXTS_SHIFT	0
+#define TX_EVENT_TXTS_MASK	(0xffff << TX_EVENT_TXTS_SHIFT)
 
 static inline u32 m_can_read(struct m_can_classdev *cdev, enum m_can_reg reg)
 {
@@ -413,6 +430,20 @@ static inline void m_can_disable_all_interrupts(struct m_can_classdev *cdev)
 	m_can_write(cdev, M_CAN_ILE, 0x0);
 }
 
+/* Retrieve internal timestamp counter from TSCV.TSC, and shift it to 32-bit
+ * width.
+ */
+static u32 m_can_get_timestamp(struct m_can_classdev *cdev)
+{
+	u32 tscv;
+	u32 tsc;
+
+	tscv = m_can_read(cdev, M_CAN_TSCV);
+	tsc = (tscv & TSCV_TSC_MASK) >> TSCV_TSC_SHIFT;
+
+	return (tsc << 16);
+}
+
 static void m_can_clean(struct net_device *net)
 {
 	struct m_can_classdev *cdev = netdev_priv(net);
-- 
2.30.1

[PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure internal timestamps

[Torin Cooper-Bennun]

This is a prerequisite for transitioning the m_can driver to rx-offload,
which works best with TX and RX timestamps.

The timestamps provided by M_CAN are 16-bit, timed according to the
nominal bit timing, and may be prescaled by a multiplier up to 16. We
choose the highest prescalar so that the timestamp wraps every 2^20 bit
times, or 209 ms at a bus speed of 5 Mbit/s. Timestamps will have a
precision of 16 bit times.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/m_can.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index ea79cf0640a8..83a673417e7c 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1139,6 +1139,7 @@ static int m_can_set_bittiming(struct net_device *dev)
  *		- >= v3.1.x: TX FIFO is used
  * - configure mode
  * - setup bittiming
+ * - configure timestamp generation
  */
 static void m_can_chip_config(struct net_device *dev)
 {
@@ -1250,6 +1251,11 @@ static void m_can_chip_config(struct net_device *dev)
 	/* set bittiming params */
 	m_can_set_bittiming(dev);
 
+	/* enable internal timestamp generation, with a prescalar of 16. The
+	 * prescalar is applied to the nominal bit timing */
+	m_can_write(cdev, M_CAN_TSCC,
+		    (0xf << TSCC_TCP_SHIFT) | TSCC_TSS_INTERNAL);
+
 	m_can_config_endisable(cdev, false);
 
 	if (cdev->ops->init)
-- 
2.30.1

[PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context

[Torin Cooper-Bennun]

For peripheral devices, m_can sent skbs directly from a threaded irq
instead of from a softirq context. This patch transitions the driver to
use the rx-offload helper, ensuring the skbs are sent from the correct
context, with h/w timestamping to ensure correct ordering.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/m_can.c | 50 ++++++++++++++++++++++++++---------
 drivers/net/can/m_can/m_can.h |  2 ++
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 83a673417e7c..ebdec9c6c0b5 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -467,7 +467,7 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	struct canfd_frame *cf;
 	struct sk_buff *skb;
-	u32 id, fgi, dlc;
+	u32 id, fgi, dlc, timestamp;
 	int i;
 
 	/* calculate the fifo get index for where to read data */
@@ -516,7 +516,9 @@ static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
 	stats->rx_packets++;
 	stats->rx_bytes += cf->len;
 
-	netif_receive_skb(skb);
+	timestamp = ((dlc & RX_BUF_RXTS_MASK) >> RX_BUF_RXTS_SHIFT) << 16;
+
+	can_rx_offload_queue_sorted(&cdev->offload, skb, timestamp);
 }
 
 static int m_can_do_rx_poll(struct net_device *dev, int quota)
@@ -547,6 +549,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
 
 static int m_can_handle_lost_msg(struct net_device *dev)
 {
+	struct m_can_classdev *cdev = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
 	struct sk_buff *skb;
 	struct can_frame *frame;
@@ -563,7 +566,8 @@ static int m_can_handle_lost_msg(struct net_device *dev)
 	frame->can_id |= CAN_ERR_CRTL;
 	frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
 
-	netif_receive_skb(skb);
+	can_rx_offload_queue_sorted(&cdev->offload, skb,
+				    m_can_get_timestamp(cdev));
 
 	return 1;
 }
@@ -620,7 +624,8 @@ static int m_can_handle_lec_err(struct net_device *dev,
 
 	stats->rx_packets++;
 	stats->rx_bytes += cf->len;
-	netif_receive_skb(skb);
+	can_rx_offload_queue_sorted(&cdev->offload, skb,
+				    m_can_get_timestamp(cdev));
 
 	return 1;
 }
@@ -739,7 +744,8 @@ static int m_can_handle_state_change(struct net_device *dev,
 
 	stats->rx_packets++;
 	stats->rx_bytes += cf->len;
-	netif_receive_skb(skb);
+	can_rx_offload_queue_sorted(&cdev->offload, skb,
+				    m_can_get_timestamp(cdev));
 
 	return 1;
 }
@@ -825,7 +831,8 @@ static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
 		netdev_dbg(dev, "allocation of skb failed\n");
 		return 0;
 	}
-	netif_receive_skb(skb);
+	can_rx_offload_queue_sorted(&cdev->offload, skb,
+				    m_can_get_timestamp(cdev));
 
 	return 1;
 }
@@ -926,6 +933,19 @@ static int m_can_poll(struct napi_struct *napi, int quota)
 	return work_done;
 }
 
+static void m_can_tx_update_stats(struct m_can_classdev *cdev,
+				  unsigned int msg_mark)
+{
+	struct net_device_stats *stats = &cdev->net->stats;
+
+	stats->tx_bytes +=
+		can_rx_offload_get_echo_skb(&cdev->offload,
+					    msg_mark,
+					    m_can_get_timestamp(cdev),
+					    NULL);
+	stats->tx_packets++;
+}
+
 static void m_can_echo_tx_event(struct net_device *dev)
 {
 	u32 txe_count = 0;
@@ -935,7 +955,6 @@ static void m_can_echo_tx_event(struct net_device *dev)
 	unsigned int msg_mark;
 
 	struct m_can_classdev *cdev = netdev_priv(dev);
-	struct net_device_stats *stats = &dev->stats;
 
 	/* read tx event fifo status */
 	m_can_txefs = m_can_read(cdev, M_CAN_TXEFS);
@@ -958,8 +977,7 @@ static void m_can_echo_tx_event(struct net_device *dev)
 						(fgi << TXEFA_EFAI_SHIFT)));
 
 		/* update stats */
-		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, NULL);
-		stats->tx_packets++;
+		m_can_tx_update_stats(cdev, msg_mark);
 	}
 }
 
@@ -967,7 +985,6 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 {
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct m_can_classdev *cdev = netdev_priv(dev);
-	struct net_device_stats *stats = &dev->stats;
 	u32 ir;
 
 	if (pm_runtime_suspended(cdev->dev))
@@ -1000,8 +1017,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	if (cdev->version == 30) {
 		if (ir & IR_TC) {
 			/* Transmission Complete Interrupt*/
-			stats->tx_bytes += can_get_echo_skb(dev, 0, NULL);
-			stats->tx_packets++;
+			m_can_tx_update_stats(cdev, 0);
 			can_led_event(dev, CAN_LED_EVENT_TX);
 			netif_wake_queue(dev);
 		}
@@ -1463,6 +1479,7 @@ static int m_can_close(struct net_device *dev)
 		cdev->tx_wq = NULL;
 	}
 
+	can_rx_offload_disable(&cdev->offload);
 	close_candev(dev);
 	can_led_event(dev, CAN_LED_EVENT_STOP);
 
@@ -1661,6 +1678,8 @@ static int m_can_open(struct net_device *dev)
 		goto exit_disable_clks;
 	}
 
+	can_rx_offload_enable(&cdev->offload);
+
 	/* register interrupt handler */
 	if (cdev->is_peripheral) {
 		cdev->tx_skb = NULL;
@@ -1702,6 +1721,7 @@ static int m_can_open(struct net_device *dev)
 	if (cdev->is_peripheral)
 		destroy_workqueue(cdev->tx_wq);
 out_wq_fail:
+	can_rx_offload_disable(&cdev->offload);
 	close_candev(dev);
 exit_disable_clks:
 	m_can_clk_stop(cdev);
@@ -1855,6 +1875,11 @@ int m_can_class_register(struct m_can_classdev *cdev)
 			return ret;
 	}
 
+	ret = can_rx_offload_add_manual(cdev->net, &cdev->offload,
+					M_CAN_NAPI_WEIGHT);
+	if (ret)
+		goto clk_disable;
+
 	ret = m_can_dev_setup(cdev);
 	if (ret)
 		goto clk_disable;
@@ -1885,6 +1910,7 @@ EXPORT_SYMBOL_GPL(m_can_class_register);
 
 void m_can_class_unregister(struct m_can_classdev *cdev)
 {
+	can_rx_offload_del(&cdev->offload);
 	unregister_candev(cdev->net);
 }
 EXPORT_SYMBOL_GPL(m_can_class_unregister);
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 3fda84cef351..ace071c3e58c 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -8,6 +8,7 @@
 
 #include <linux/can/core.h>
 #include <linux/can/led.h>
+#include <linux/can/rx-offload.h>
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
@@ -71,6 +72,7 @@ struct m_can_ops {
 
 struct m_can_classdev {
 	struct can_priv can;
+	struct can_rx_offload offload;
 	struct napi_struct napi;
 	struct net_device *net;
 	struct device *dev;
-- 
2.30.1

Re: [PATCH 1/3] can: m_can: add infrastructure for internal timestamps

[Marc Kleine-Budde]

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

On 05.03.2021 17:20:13, Torin Cooper-Bennun wrote:
> Add infrastucture to allow internal timestamps from the M_CAN to be
> configured and retrieved.
>
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> ---
>  drivers/net/can/m_can/m_can.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 0c8d36bc668c..ea79cf0640a8 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -148,6 +148,19 @@ enum m_can_reg {
>  #define NBTP_NTSEG2_SHIFT	0
>  #define NBTP_NTSEG2_MASK	(0x7f << NBTP_NTSEG2_SHIFT)
>
> +/* Timestamp Counter Configuration Register (TSCC) */
> +#define TSCC_TCP_SHIFT		16
> +#define TSCC_TCP_MASK		(0xf << TSCC_TCP_SHIFT)
> +#define TSCC_TSS_SHIFT		0
> +#define TSCC_TSS_MASK		(0x3 << TSCC_TSS_SHIFT)
> +#define TSCC_TSS_DISABLE	(0x0 << TSCC_TSS_SHIFT)
> +#define TSCC_TSS_INTERNAL	(0x1 << TSCC_TSS_SHIFT)
> +#define TSCC_TSS_EXTERNAL	(0x2 << TSCC_TSS_SHIFT)

Even if the driver doesn't make use of GENMASK(), please add new masks
with GENMASK(), there's no need to define the _SHIFT values.

> +
> +/* Timestamp Counter Value Register (TSCV) */
> +#define TSCV_TSC_SHIFT		0
> +#define TSCV_TSC_MASK		(0xffff << TSCV_TSC_SHIFT)
> +
>  /* Error Counter Register(ECR) */
>  #define ECR_RP			BIT(15)
>  #define ECR_REC_SHIFT		8
> @@ -302,6 +315,8 @@ enum m_can_reg {
>  #define RX_BUF_ANMF		BIT(31)
>  #define RX_BUF_FDF		BIT(21)
>  #define RX_BUF_BRS		BIT(20)
> +#define RX_BUF_RXTS_SHIFT	0
> +#define RX_BUF_RXTS_MASK	(0xffff << RX_BUF_RXTS_SHIFT)
>
>  /* Tx Buffer Element */
>  /* T0 */
> @@ -319,6 +334,8 @@ enum m_can_reg {
>  /* E1 */
>  #define TX_EVENT_MM_SHIFT	TX_BUF_MM_SHIFT
>  #define TX_EVENT_MM_MASK	(0xff << TX_EVENT_MM_SHIFT)
> +#define TX_EVENT_TXTS_SHIFT	0
> +#define TX_EVENT_TXTS_MASK	(0xffff << TX_EVENT_TXTS_SHIFT)
>
>  static inline u32 m_can_read(struct m_can_classdev *cdev, enum m_can_reg reg)
>  {
> @@ -413,6 +430,20 @@ static inline void m_can_disable_all_interrupts(struct m_can_classdev *cdev)
>  	m_can_write(cdev, M_CAN_ILE, 0x0);
>  }
>
> +/* Retrieve internal timestamp counter from TSCV.TSC, and shift it to 32-bit
> + * width.
> + */
> +static u32 m_can_get_timestamp(struct m_can_classdev *cdev)
> +{
> +	u32 tscv;
> +	u32 tsc;
> +
> +	tscv = m_can_read(cdev, M_CAN_TSCV);
> +	tsc = (tscv & TSCV_TSC_MASK) >> TSCV_TSC_SHIFT;

Make use of FIELD_GET here:

     tsc = FIELD_GET(TSCV_TSC_MASK, tcsv);

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 2/3] can: m_can: m_can_chip_config(): enable and configure internal timestamps

[Marc Kleine-Budde]

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

On 05.03.2021 17:20:14, Torin Cooper-Bennun wrote:
> This is a prerequisite for transitioning the m_can driver to rx-offload,
> which works best with TX and RX timestamps.
> 
> The timestamps provided by M_CAN are 16-bit, timed according to the
> nominal bit timing, and may be prescaled by a multiplier up to 16. We
> choose the highest prescalar so that the timestamp wraps every 2^20 bit
> times, or 209 ms at a bus speed of 5 Mbit/s. Timestamps will have a
> precision of 16 bit times.
> 
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> ---
>  drivers/net/can/m_can/m_can.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index ea79cf0640a8..83a673417e7c 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1139,6 +1139,7 @@ static int m_can_set_bittiming(struct net_device *dev)
>   *		- >= v3.1.x: TX FIFO is used
>   * - configure mode
>   * - setup bittiming
> + * - configure timestamp generation
>   */
>  static void m_can_chip_config(struct net_device *dev)
>  {
> @@ -1250,6 +1251,11 @@ static void m_can_chip_config(struct net_device *dev)
>  	/* set bittiming params */
>  	m_can_set_bittiming(dev);
>  
> +	/* enable internal timestamp generation, with a prescalar of 16. The
> +	 * prescalar is applied to the nominal bit timing */
> +	m_can_write(cdev, M_CAN_TSCC,
> +		    (0xf << TSCC_TCP_SHIFT) | TSCC_TSS_INTERNAL);

FIELD_PREP(TSCC_TCP_MASK, 0xf)

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 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context

[Marc Kleine-Budde]

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

On 05.03.2021 17:20:15, Torin Cooper-Bennun wrote:
> For peripheral devices, m_can sent skbs directly from a threaded irq
> instead of from a softirq context. This patch transitions the driver to
> use the rx-offload helper, ensuring the skbs are sent from the correct
> context, with h/w timestamping to ensure correct ordering.

I think you beak the non-peripheral drivers here. They already have a
NAPI function m_can_poll(). It makes no sense and doesn't work, if you
do the RX in NAPI and then queue to rx-offload, which then needs to run
from NAPI again. But it cannot as m_can_poll is the NAPI function.

For peripherals it works, as you do the RX in the threaded IRQ, queue to
rx-offload, which then schedules a NAPI, to push the CAN frames into the
networking stack.

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 2/3] can: m_can: m_can_chip_config(): enable and configure internal timestamps

[Torin Cooper-Bennun]

On Fri, Mar 05, 2021 at 10:34:42PM +0100, Marc Kleine-Budde wrote:
> FIELD_PREP(TSCC_TCP_MASK, 0xf)

Noted GENMASK, FIELD_GET, FIELD_PREP improvements with thanks, will fix

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com

Re: [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context

[Torin Cooper-Bennun]

On Fri, Mar 05, 2021 at 11:29:57PM +0100, Marc Kleine-Budde wrote:
> On 05.03.2021 17:20:15, Torin Cooper-Bennun wrote:
> > For peripheral devices, m_can sent skbs directly from a threaded irq
> > instead of from a softirq context. This patch transitions the driver to
> > use the rx-offload helper, ensuring the skbs are sent from the correct
> > context, with h/w timestamping to ensure correct ordering.
> 
> I think you beak the non-peripheral drivers here. They already have a
> NAPI function m_can_poll(). It makes no sense and doesn't work, if you
> do the RX in NAPI and then queue to rx-offload, which then needs to run
> from NAPI again. But it cannot as m_can_poll is the NAPI function.
> 
> For peripherals it works, as you do the RX in the threaded IRQ, queue to
> rx-offload, which then schedules a NAPI, to push the CAN frames into the
> networking stack.

Understood, I will make the skb handling conditional on
cdev->is_peripheral and retain netif_receive_skb and can_get_echo_skb
usage for non-peripherals as before.

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com

Re: [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1465 bytes --]

On 3/8/21 10:11 AM, Torin Cooper-Bennun wrote:
> On Fri, Mar 05, 2021 at 11:29:57PM +0100, Marc Kleine-Budde wrote:
>> On 05.03.2021 17:20:15, Torin Cooper-Bennun wrote:
>>> For peripheral devices, m_can sent skbs directly from a threaded irq
>>> instead of from a softirq context. This patch transitions the driver to
>>> use the rx-offload helper, ensuring the skbs are sent from the correct
>>> context, with h/w timestamping to ensure correct ordering.
>>
>> I think you beak the non-peripheral drivers here. They already have a
>> NAPI function m_can_poll(). It makes no sense and doesn't work, if you
>> do the RX in NAPI and then queue to rx-offload, which then needs to run
>> from NAPI again. But it cannot as m_can_poll is the NAPI function.
>>
>> For peripherals it works, as you do the RX in the threaded IRQ, queue to
>> rx-offload, which then schedules a NAPI, to push the CAN frames into the
>> networking stack.
> 
> Understood, I will make the skb handling conditional on
> cdev->is_peripheral and retain netif_receive_skb and can_get_echo_skb
> usage for non-peripherals as before.

Not beautify, but should make the tcan driver work at least.

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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context

[Torin Cooper-Bennun]

On Mon, Mar 08, 2021 at 10:16:54AM +0100, Marc Kleine-Budde wrote:
> On 3/8/21 10:11 AM, Torin Cooper-Bennun wrote:
> > On Fri, Mar 05, 2021 at 11:29:57PM +0100, Marc Kleine-Budde wrote:
> >> On 05.03.2021 17:20:15, Torin Cooper-Bennun wrote:
> >>> For peripheral devices, m_can sent skbs directly from a threaded irq
> >>> instead of from a softirq context. This patch transitions the driver to
> >>> use the rx-offload helper, ensuring the skbs are sent from the correct
> >>> context, with h/w timestamping to ensure correct ordering.
> >>
> >> I think you beak the non-peripheral drivers here. They already have a
> >> NAPI function m_can_poll(). It makes no sense and doesn't work, if you
> >> do the RX in NAPI and then queue to rx-offload, which then needs to run
> >> from NAPI again. But it cannot as m_can_poll is the NAPI function.
> >>
> >> For peripherals it works, as you do the RX in the threaded IRQ, queue to
> >> rx-offload, which then schedules a NAPI, to push the CAN frames into the
> >> networking stack.
> > 
> > Understood, I will make the skb handling conditional on
> > cdev->is_peripheral and retain netif_receive_skb and can_get_echo_skb
> > usage for non-peripherals as before.
> 
> Not beautify, but should make the tcan driver work at least.

Submitted v2 of patches, hopefully hit a good compromise :)

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com