* [PATCH 0/3] can: m_can: stabilise peripheral m_can RX and TX
@ 2021-03-05 17:20 Torin Cooper-Bennun
2021-03-05 17:20 ` [PATCH 1/3] can: m_can: add infrastructure for internal timestamps Torin Cooper-Bennun
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Torin Cooper-Bennun @ 2021-03-05 17:20 UTC (permalink / raw)
To: linux-can; +Cc: Marc Kleine-Budde, Pankaj Sharma
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] can: m_can: add infrastructure for internal timestamps 2021-03-05 17:20 [PATCH 0/3] can: m_can: stabilise peripheral m_can RX and TX Torin Cooper-Bennun @ 2021-03-05 17:20 ` Torin Cooper-Bennun 2021-03-05 21:28 ` Marc Kleine-Budde 2021-03-05 17:20 ` [PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure " Torin Cooper-Bennun 2021-03-05 17:20 ` [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context Torin Cooper-Bennun 2 siblings, 1 reply; 11+ messages in thread From: Torin Cooper-Bennun @ 2021-03-05 17:20 UTC (permalink / raw) To: linux-can; +Cc: Marc Kleine-Budde, Pankaj Sharma, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] can: m_can: add infrastructure for internal timestamps 2021-03-05 17:20 ` [PATCH 1/3] can: m_can: add infrastructure for internal timestamps Torin Cooper-Bennun @ 2021-03-05 21:28 ` Marc Kleine-Budde 0 siblings, 0 replies; 11+ messages in thread From: Marc Kleine-Budde @ 2021-03-05 21:28 UTC (permalink / raw) To: Torin Cooper-Bennun; +Cc: linux-can, Pankaj Sharma [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure internal timestamps 2021-03-05 17:20 [PATCH 0/3] can: m_can: stabilise peripheral m_can RX and TX Torin Cooper-Bennun 2021-03-05 17:20 ` [PATCH 1/3] can: m_can: add infrastructure for internal timestamps Torin Cooper-Bennun @ 2021-03-05 17:20 ` Torin Cooper-Bennun 2021-03-05 21:34 ` Marc Kleine-Budde 2021-03-05 17:20 ` [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context Torin Cooper-Bennun 2 siblings, 1 reply; 11+ messages in thread From: Torin Cooper-Bennun @ 2021-03-05 17:20 UTC (permalink / raw) To: linux-can; +Cc: Marc Kleine-Budde, Pankaj Sharma, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure internal timestamps 2021-03-05 17:20 ` [PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure " Torin Cooper-Bennun @ 2021-03-05 21:34 ` Marc Kleine-Budde 2021-03-08 9:09 ` Torin Cooper-Bennun 0 siblings, 1 reply; 11+ messages in thread From: Marc Kleine-Budde @ 2021-03-05 21:34 UTC (permalink / raw) To: Torin Cooper-Bennun; +Cc: linux-can, Pankaj Sharma [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure internal timestamps 2021-03-05 21:34 ` Marc Kleine-Budde @ 2021-03-08 9:09 ` Torin Cooper-Bennun 0 siblings, 0 replies; 11+ messages in thread From: Torin Cooper-Bennun @ 2021-03-08 9:09 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: linux-can, Pankaj Sharma 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context 2021-03-05 17:20 [PATCH 0/3] can: m_can: stabilise peripheral m_can RX and TX Torin Cooper-Bennun 2021-03-05 17:20 ` [PATCH 1/3] can: m_can: add infrastructure for internal timestamps Torin Cooper-Bennun 2021-03-05 17:20 ` [PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure " Torin Cooper-Bennun @ 2021-03-05 17:20 ` Torin Cooper-Bennun 2021-03-05 22:29 ` Marc Kleine-Budde 2 siblings, 1 reply; 11+ messages in thread From: Torin Cooper-Bennun @ 2021-03-05 17:20 UTC (permalink / raw) To: linux-can; +Cc: Marc Kleine-Budde, Pankaj Sharma, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context 2021-03-05 17:20 ` [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context Torin Cooper-Bennun @ 2021-03-05 22:29 ` Marc Kleine-Budde 2021-03-08 9:11 ` Torin Cooper-Bennun 0 siblings, 1 reply; 11+ messages in thread From: Marc Kleine-Budde @ 2021-03-05 22:29 UTC (permalink / raw) To: Torin Cooper-Bennun; +Cc: linux-can, Pankaj Sharma [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context 2021-03-05 22:29 ` Marc Kleine-Budde @ 2021-03-08 9:11 ` Torin Cooper-Bennun 2021-03-08 9:16 ` Marc Kleine-Budde 0 siblings, 1 reply; 11+ messages in thread From: Torin Cooper-Bennun @ 2021-03-08 9:11 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: linux-can, Pankaj Sharma 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context 2021-03-08 9:11 ` Torin Cooper-Bennun @ 2021-03-08 9:16 ` Marc Kleine-Budde 2021-03-08 10:31 ` Torin Cooper-Bennun 0 siblings, 1 reply; 11+ messages in thread From: Marc Kleine-Budde @ 2021-03-08 9:16 UTC (permalink / raw) To: Torin Cooper-Bennun; +Cc: linux-can, Pankaj Sharma [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context 2021-03-08 9:16 ` Marc Kleine-Budde @ 2021-03-08 10:31 ` Torin Cooper-Bennun 0 siblings, 0 replies; 11+ messages in thread From: Torin Cooper-Bennun @ 2021-03-08 10:31 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: linux-can, Pankaj Sharma 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-08 10:32 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-05 17:20 [PATCH 0/3] can: m_can: stabilise peripheral m_can RX and TX Torin Cooper-Bennun 2021-03-05 17:20 ` [PATCH 1/3] can: m_can: add infrastructure for internal timestamps Torin Cooper-Bennun 2021-03-05 21:28 ` Marc Kleine-Budde 2021-03-05 17:20 ` [PATCH 2/3] can: m_can: m_can_chip_config(): enable and configure " Torin Cooper-Bennun 2021-03-05 21:34 ` Marc Kleine-Budde 2021-03-08 9:09 ` Torin Cooper-Bennun 2021-03-05 17:20 ` [PATCH 3/3] can: m_can: fix RX path: use rx-offload to ensure skbs are sent from softirq context Torin Cooper-Bennun 2021-03-05 22:29 ` Marc Kleine-Budde 2021-03-08 9:11 ` Torin Cooper-Bennun 2021-03-08 9:16 ` Marc Kleine-Budde 2021-03-08 10:31 ` Torin Cooper-Bennun
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.