| Message ID | 20240726195944.2414812-5-msp@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: m_can: Fix polling and other issues | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Hi Markus, kernel test robot noticed the following build warnings: [auto build test WARNING on mkl-can-next/testing] [also build test WARNING on linus/master v6.10 next-20240726] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Markus-Schneider-Pargmann/can-m_can-Reset-coalescing-during-suspend-resume/20240727-042714 base: https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git testing patch link: https://lore.kernel.org/r/20240726195944.2414812-5-msp%40baylibre.com patch subject: [PATCH 4/7] can: m_can: Do not cancel timer from within timer config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240727/202407271748.gsVE0Hih-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240727/202407271748.gsVE0Hih-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202407271748.gsVE0Hih-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/can/m_can/m_can.c:1205: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * This interrupt handler is called either from the interrupt thread or a vim +1205 drivers/net/can/m_can/m_can.c 1203 1204 /** > 1205 * This interrupt handler is called either from the interrupt thread or a 1206 * hrtimer. This has implications like cancelling a timer won't be possible 1207 * blocking. 1208 */ 1209 static int m_can_interrupt_handler(struct m_can_classdev *cdev) 1210 { 1211 struct net_device *dev = cdev->net; 1212 u32 ir; 1213 int ret; 1214 1215 if (pm_runtime_suspended(cdev->dev)) 1216 return IRQ_NONE; 1217 1218 ir = m_can_read(cdev, M_CAN_IR); 1219 m_can_coalescing_update(cdev, ir); 1220 if (!ir) 1221 return IRQ_NONE; 1222 1223 /* ACK all irqs */ 1224 m_can_write(cdev, M_CAN_IR, ir); 1225 1226 if (cdev->ops->clear_interrupts) 1227 cdev->ops->clear_interrupts(cdev); 1228 1229 /* schedule NAPI in case of 1230 * - rx IRQ 1231 * - state change IRQ 1232 * - bus error IRQ and bus error reporting 1233 */ 1234 if (ir & (IR_RF0N | IR_RF0W | IR_ERR_ALL_30X)) { 1235 cdev->irqstatus = ir; 1236 if (!cdev->is_peripheral) { 1237 m_can_disable_all_interrupts(cdev); 1238 napi_schedule(&cdev->napi); 1239 } else { 1240 ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir); 1241 if (ret < 0) 1242 return ret; 1243 } 1244 } 1245 1246 if (cdev->version == 30) { 1247 if (ir & IR_TC) { 1248 /* Transmission Complete Interrupt*/ 1249 u32 timestamp = 0; 1250 unsigned int frame_len; 1251 1252 if (cdev->is_peripheral) 1253 timestamp = m_can_get_timestamp(cdev); 1254 frame_len = m_can_tx_update_stats(cdev, 0, timestamp); 1255 m_can_finish_tx(cdev, 1, frame_len); 1256 } 1257 } else { 1258 if (ir & (IR_TEFN | IR_TEFW)) { 1259 /* New TX FIFO Element arrived */ 1260 ret = m_can_echo_tx_event(dev); 1261 if (ret != 0) 1262 return ret; 1263 } 1264 } 1265 1266 if (cdev->is_peripheral) 1267 can_rx_offload_threaded_irq_finish(&cdev->offload); 1268 1269 return IRQ_HANDLED; 1270 } 1271
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 42ed7f0fea78..e70c7100a3c9 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,16 @@ 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 +1237,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 +1257,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 +1267,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 +1992,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; - m_can_isr(0, cdev->net); + if (cdev->can.state == CAN_STATE_BUS_OFF || + cdev->can.state == CAN_STATE_STOPPED) + return HRTIMER_NORESTART; + + ret = m_can_interrupt_handler(cdev); + + /* 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));
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 <msp@baylibre.com> --- drivers/net/can/m_can/m_can.c | 58 ++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 15 deletions(-)