From a651261ac74298535f6d6316ebe27beceb6b17b1 Mon Sep 17 00:00:00 2001 From: Markus Schneider-Pargmann Date: Mon, 5 Aug 2024 20:30:41 +0200 Subject: can: m_can: Reset coalescing during suspend/resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During resume the interrupts are limited to IR_RF0N and the chip keeps running. In this case if coalescing is enabled and active we may miss waterlevel interrupts during suspend. It is safer to reset the coalescing by stopping the timer and adding IR_RF0N | IR_TEFN to the interrupts. This is a theoratical issue and probably extremely rare. Cc: Martin Hundebøll Fixes: 4a94d7e31cf5 ("can: m_can: allow keeping the transceiver running in suspend") Signed-off-by: Markus Schneider-Pargmann Link: https://lore.kernel.org/all/20240805183047.305630-2-msp@baylibre.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index cd83c8b5d4b1..31991e2f343e 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -2430,12 +2430,15 @@ int m_can_class_suspend(struct device *dev) netif_device_detach(ndev); /* leave the chip running with rx interrupt enabled if it is - * used as a wake-up source. + * used as a wake-up source. Coalescing needs to be reset then, + * the timer is cancelled here, interrupts are done in resume. */ - if (cdev->pm_wake_source) + if (cdev->pm_wake_source) { + hrtimer_cancel(&cdev->hrtimer); m_can_write(cdev, M_CAN_IE, IR_RF0N); - else + } else { m_can_stop(ndev); + } m_can_clk_stop(cdev); } @@ -2465,6 +2468,13 @@ int m_can_class_resume(struct device *dev) return ret; if (cdev->pm_wake_source) { + /* Restore active interrupts but disable coalescing as + * we may have missed important waterlevel interrupts + * between suspend and resume. Timers are already + * stopped in suspend. Here we enable all interrupts + * again. + */ + cdev->active_interrupts |= IR_RF0N | IR_TEFN; m_can_write(cdev, M_CAN_IE, cdev->active_interrupts); } else { ret = m_can_start(ndev); -- cgit From 6eff1cead75ff330bb33264424c1da6cc7179ab8 Mon Sep 17 00:00:00 2001 From: Markus Schneider-Pargmann Date: Mon, 5 Aug 2024 20:30:42 +0200 Subject: can: m_can: Remove coalesing disable in isr during suspend We don't need to disable coalescing when the interrupt handler executes while the chip is suspended. The coalescing is already reset during suspend. Fixes: 07f25091ca02 ("can: m_can: Implement receive coalescing") Signed-off-by: Markus Schneider-Pargmann Link: https://lore.kernel.org/all/20240805183047.305630-3-msp@baylibre.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 31991e2f343e..ba416c973e8d 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1223,10 +1223,8 @@ static irqreturn_t m_can_isr(int irq, void *dev_id) struct m_can_classdev *cdev = netdev_priv(dev); u32 ir; - if (pm_runtime_suspended(cdev->dev)) { - m_can_coalescing_disable(cdev); + if (pm_runtime_suspended(cdev->dev)) return IRQ_NONE; - } ir = m_can_read(cdev, M_CAN_IR); m_can_coalescing_update(cdev, ir); -- cgit From 40e4552eeef0e3090a5988de15889795936fd38f Mon Sep 17 00:00:00 2001 From: Markus Schneider-Pargmann Date: Mon, 5 Aug 2024 20:30:43 +0200 Subject: can: m_can: Remove m_can_rx_peripheral indirection m_can_rx_peripheral() is a wrapper around m_can_rx_handler() that calls m_can_disable_all_interrupts() on error. The same handling for the same error path is done in m_can_isr() as well. So remove m_can_rx_peripheral() and do the call from m_can_isr() directly. Signed-off-by: Markus Schneider-Pargmann Link: https://lore.kernel.org/all/20240805183047.305630-4-msp@baylibre.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index ba416c973e8d..a37ed376de9b 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1037,22 +1037,6 @@ end: return work_done; } -static int m_can_rx_peripheral(struct net_device *dev, u32 irqstatus) -{ - struct m_can_classdev *cdev = netdev_priv(dev); - int work_done; - - work_done = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, irqstatus); - - /* Don't re-enable interrupts if the driver had a fatal error - * (e.g., FIFO read failure). - */ - if (work_done < 0) - m_can_disable_all_interrupts(cdev); - - return work_done; -} - static int m_can_poll(struct napi_struct *napi, int quota) { struct net_device *dev = napi->dev; @@ -1250,7 +1234,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id) } else { int pkts; - pkts = m_can_rx_peripheral(dev, ir); + pkts = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir); if (pkts < 0) goto out_fail; } -- cgit From 4d5159bfafa8d1a205d8213b7434e0402588b9ed Mon Sep 17 00:00:00 2001 From: Markus Schneider-Pargmann Date: Mon, 5 Aug 2024 20:30:44 +0200 Subject: can: m_can: Do not cancel timer from within timer On setups without interrupts, the interrupt handler is called from a timer callback. For non-peripheral receives napi is scheduled, interrupts are disabled and the timer is canceled with a blocking call. In case of an error this can happen as well. Check if napi is scheduled in the timer callback after the interrupt handler executed. If napi is scheduled, the timer is disabled. It will be reenabled by m_can_poll(). Return error values from the interrupt handler so that interrupt threads and timer callback can deal differently with it. In case of the timer we only disable the timer. The rest will be done when stopping the interface. Fixes: b382380c0d2d ("can: m_can: Add hrtimer to generate software interrupt") Fixes: a163c5761019 ("can: m_can: Start/Cancel polling timer together with interrupts") Signed-off-by: Markus Schneider-Pargmann Link: https://lore.kernel.org/all/20240805183047.305630-5-msp@baylibre.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 57 +++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index a37ed376de9b..5228304779f1 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -487,7 +487,7 @@ static inline void m_can_disable_all_interrupts(struct m_can_classdev *cdev) if (!cdev->net->irq) { dev_dbg(cdev->dev, "Stop hrtimer\n"); - hrtimer_cancel(&cdev->hrtimer); + hrtimer_try_to_cancel(&cdev->hrtimer); } } @@ -1201,11 +1201,15 @@ static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir) HRTIMER_MODE_REL); } -static irqreturn_t m_can_isr(int irq, void *dev_id) +/* This interrupt handler is called either from the interrupt thread or a + * hrtimer. This has implications like cancelling a timer won't be possible + * blocking. + */ +static int m_can_interrupt_handler(struct m_can_classdev *cdev) { - struct net_device *dev = (struct net_device *)dev_id; - struct m_can_classdev *cdev = netdev_priv(dev); + struct net_device *dev = cdev->net; u32 ir; + int ret; if (pm_runtime_suspended(cdev->dev)) return IRQ_NONE; @@ -1232,11 +1236,9 @@ static irqreturn_t m_can_isr(int irq, void *dev_id) m_can_disable_all_interrupts(cdev); napi_schedule(&cdev->napi); } else { - int pkts; - - pkts = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir); - if (pkts < 0) - goto out_fail; + ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir); + if (ret < 0) + return ret; } } @@ -1254,8 +1256,9 @@ static irqreturn_t m_can_isr(int irq, void *dev_id) } else { if (ir & (IR_TEFN | IR_TEFW)) { /* New TX FIFO Element arrived */ - if (m_can_echo_tx_event(dev) != 0) - goto out_fail; + ret = m_can_echo_tx_event(dev); + if (ret != 0) + return ret; } } @@ -1263,16 +1266,31 @@ static irqreturn_t m_can_isr(int irq, void *dev_id) can_rx_offload_threaded_irq_finish(&cdev->offload); return IRQ_HANDLED; +} -out_fail: - m_can_disable_all_interrupts(cdev); - return IRQ_HANDLED; +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); + int ret; + + ret = m_can_interrupt_handler(cdev); + if (ret < 0) { + m_can_disable_all_interrupts(cdev); + return IRQ_HANDLED; + } + + return ret; } static enum hrtimer_restart m_can_coalescing_timer(struct hrtimer *timer) { struct m_can_classdev *cdev = container_of(timer, struct m_can_classdev, hrtimer); + if (cdev->can.state == CAN_STATE_BUS_OFF || + cdev->can.state == CAN_STATE_STOPPED) + return HRTIMER_NORESTART; + irq_wake_thread(cdev->net->irq, cdev->net); return HRTIMER_NORESTART; @@ -1973,8 +1991,17 @@ static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer) { struct m_can_classdev *cdev = container_of(timer, struct m_can_classdev, hrtimer); + int ret; + + if (cdev->can.state == CAN_STATE_BUS_OFF || + cdev->can.state == CAN_STATE_STOPPED) + return HRTIMER_NORESTART; + + ret = m_can_interrupt_handler(cdev); - m_can_isr(0, cdev->net); + /* On error or if napi is scheduled to read, stop the timer */ + if (ret < 0 || napi_is_scheduled(&cdev->napi)) + return HRTIMER_NORESTART; hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL_MS)); -- cgit From a572fea86c9b06cd3e6e89d79d565b52cb7e7cff Mon Sep 17 00:00:00 2001 From: Markus Schneider-Pargmann Date: Mon, 5 Aug 2024 20:30:45 +0200 Subject: can: m_can: disable_all_interrupts, not clear active_interrupts active_interrupts is a cache for the enabled interrupts and not the global masking of interrupts. Do not clear this variable otherwise we may loose the state of the interrupts. Fixes: 07f25091ca02 ("can: m_can: Implement receive coalescing") Signed-off-by: Markus Schneider-Pargmann Link: https://lore.kernel.org/all/20240805183047.305630-6-msp@baylibre.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 5228304779f1..68bd4a00ecca 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -483,7 +483,6 @@ static inline void m_can_disable_all_interrupts(struct m_can_classdev *cdev) { m_can_coalescing_disable(cdev); m_can_write(cdev, M_CAN_ILE, 0x0); - cdev->active_interrupts = 0x0; if (!cdev->net->irq) { dev_dbg(cdev->dev, "Stop hrtimer\n"); -- cgit From 733dbf556cd5b71d5e6f6aa7a93f117b438ab785 Mon Sep 17 00:00:00 2001 From: Markus Schneider-Pargmann Date: Mon, 5 Aug 2024 20:30:46 +0200 Subject: can: m_can: Reset cached active_interrupts on start To force writing the enabled interrupts, reset the active_interrupts cache. Fixes: 07f25091ca02 ("can: m_can: Implement receive coalescing") Signed-off-by: Markus Schneider-Pargmann Link: https://lore.kernel.org/all/20240805183047.305630-7-msp@baylibre.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 68bd4a00ecca..67c4c740c416 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -1541,6 +1541,7 @@ static int m_can_chip_config(struct net_device *dev) else interrupts &= ~(IR_ERR_LEC_31X); } + cdev->active_interrupts = 0; m_can_interrupt_enable(cdev, interrupts); /* route all interrupts to INT0 */ -- cgit From e443d15b949952ee039b731d5c35bcbafa300024 Mon Sep 17 00:00:00 2001 From: Markus Schneider-Pargmann Date: Mon, 5 Aug 2024 20:30:47 +0200 Subject: can: m_can: Limit coalescing to peripheral instances The use of coalescing for non-peripheral chips in the current implementation is limited to non-existing. Disable the possibility to set coalescing through ethtool. Signed-off-by: Markus Schneider-Pargmann Link: https://lore.kernel.org/all/20240805183047.305630-8-msp@baylibre.com Signed-off-by: Marc Kleine-Budde --- drivers/net/can/m_can/m_can.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 67c4c740c416..012c3d22b01d 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -2184,7 +2184,7 @@ static int m_can_set_coalesce(struct net_device *dev, return 0; } -static const struct ethtool_ops m_can_ethtool_ops = { +static const struct ethtool_ops m_can_ethtool_ops_coalescing = { .supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ | ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ | ETHTOOL_COALESCE_TX_USECS_IRQ | @@ -2195,18 +2195,20 @@ static const struct ethtool_ops m_can_ethtool_ops = { .set_coalesce = m_can_set_coalesce, }; -static const struct ethtool_ops m_can_ethtool_ops_polling = { +static const struct ethtool_ops m_can_ethtool_ops = { .get_ts_info = ethtool_op_get_ts_info, }; -static int register_m_can_dev(struct net_device *dev) +static int register_m_can_dev(struct m_can_classdev *cdev) { + struct net_device *dev = cdev->net; + dev->flags |= IFF_ECHO; /* we support local echo */ dev->netdev_ops = &m_can_netdev_ops; - if (dev->irq) - dev->ethtool_ops = &m_can_ethtool_ops; + if (dev->irq && cdev->is_peripheral) + dev->ethtool_ops = &m_can_ethtool_ops_coalescing; else - dev->ethtool_ops = &m_can_ethtool_ops_polling; + dev->ethtool_ops = &m_can_ethtool_ops; return register_candev(dev); } @@ -2392,7 +2394,7 @@ int m_can_class_register(struct m_can_classdev *cdev) if (ret) goto rx_offload_del; - ret = register_m_can_dev(cdev->net); + ret = register_m_can_dev(cdev); if (ret) { dev_err(cdev->dev, "registering %s failed (err=%d)\n", cdev->net->name, ret); -- cgit