| Message ID | 20230810083716.29653-1-repk@triplefau.lt (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: stmmac: remove disable_irq() from ->ndo_poll_controller() | expand |
On Thu, 10 Aug 2023 10:37:16 +0200 Remi Pommarel wrote: > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 4727f7be4f86..bbe509abc5dc 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -5958,8 +5958,8 @@ static void stmmac_poll_controller(struct net_device *dev) > for (i = 0; i < priv->plat->tx_queues_to_use; i++) > stmmac_msi_intr_tx(0, &priv->dma_conf.tx_queue[i]); > } else { > - disable_irq(dev->irq); > - stmmac_interrupt(dev->irq, dev); > + if (disable_hardirq(dev->irq)) > + stmmac_interrupt(dev->irq, dev); > enable_irq(dev->irq); Implementing .ndo_poll_controller is only needed if driver doesn't use NAPI. This driver seems to use NAPI on all paths, AFAICT you can simply delete this function completely.
On Fri, Aug 11, 2023 at 06:20:25PM -0700, Jakub Kicinski wrote: > On Thu, 10 Aug 2023 10:37:16 +0200 Remi Pommarel wrote: > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 4727f7be4f86..bbe509abc5dc 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -5958,8 +5958,8 @@ static void stmmac_poll_controller(struct net_device *dev) > > for (i = 0; i < priv->plat->tx_queues_to_use; i++) > > stmmac_msi_intr_tx(0, &priv->dma_conf.tx_queue[i]); > > } else { > > - disable_irq(dev->irq); > > - stmmac_interrupt(dev->irq, dev); > > + if (disable_hardirq(dev->irq)) > > + stmmac_interrupt(dev->irq, dev); > > enable_irq(dev->irq); > > Implementing .ndo_poll_controller is only needed if driver doesn't use > NAPI. This driver seems to use NAPI on all paths, AFAICT you can simply > delete this function completely. Looks like since [0] you are right. Will send a new PATCH removing stmmac_poll_controller. Thanks. [0]: ac3d9dd034e5 ("netpoll: make ndo_poll_controller() optional")
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 4727f7be4f86..bbe509abc5dc 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -5958,8 +5958,8 @@ static void stmmac_poll_controller(struct net_device *dev) for (i = 0; i < priv->plat->tx_queues_to_use; i++) stmmac_msi_intr_tx(0, &priv->dma_conf.tx_queue[i]); } else { - disable_irq(dev->irq); - stmmac_interrupt(dev->irq, dev); + if (disable_hardirq(dev->irq)) + stmmac_interrupt(dev->irq, dev); enable_irq(dev->irq); } }
Using netconsole netpoll_poll_dev could be called from interrupt context, thus using disable_irq() would cause the following kernel warning with CONFIG_DEBUG_ATOMIC_SLEEP enabled: BUG: sleeping function called from invalid context at kernel/irq/manage.c:137 in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 10, name: ksoftirqd/0 CPU: 0 PID: 10 Comm: ksoftirqd/0 Tainted: G W 5.15.42-00075-g816b502b2298-dirty #117 Hardware name: aml (r1) (DT) Call trace: dump_backtrace+0x0/0x270 show_stack+0x14/0x20 dump_stack_lvl+0x8c/0xac dump_stack+0x18/0x30 ___might_sleep+0x150/0x194 __might_sleep+0x64/0xbc synchronize_irq+0x8c/0x150 disable_irq+0x2c/0x40 stmmac_poll_controller+0x140/0x1a0 netpoll_poll_dev+0x6c/0x220 netpoll_send_skb+0x308/0x390 netpoll_send_udp+0x418/0x760 write_msg+0x118/0x140 [netconsole] console_unlock+0x404/0x500 vprintk_emit+0x118/0x250 dev_vprintk_emit+0x19c/0x1cc dev_printk_emit+0x90/0xa8 __dev_printk+0x78/0x9c _dev_warn+0xa4/0xbc ath10k_warn+0xe8/0xf0 [ath10k_core] ath10k_htt_txrx_compl_task+0x790/0x7fc [ath10k_core] ath10k_pci_napi_poll+0x98/0x1f4 [ath10k_pci] __napi_poll+0x58/0x1f4 net_rx_action+0x504/0x590 _stext+0x1b8/0x418 run_ksoftirqd+0x74/0xa4 smpboot_thread_fn+0x210/0x3c0 kthread+0x1fc/0x210 ret_from_fork+0x10/0x20 Commit [0] introcuded disable_hardirq() to address this situation, so use it here to avoid above warning. [0] 02cea395866 ("genirq: Provide disable_hardirq()") Cc: <stable@vger.kernel.org> Signed-off-by: Remi Pommarel <repk@triplefau.lt> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)