* [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks
@ 2024-05-08 13:45 Jakub Kicinski
2024-05-08 14:32 ` Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-05-08 13:45 UTC (permalink / raw
To: davem
Cc: netdev, edumazet, pabeni, Jakub Kicinski, Erhard Furtner, robh,
elder, wei.fang, bhupesh.sharma, benh
Erhard reports netpoll warnings from sungem:
netpoll_send_skb_on_dev(): eth0 enabled interrupts in poll (gem_start_xmit+0x0/0x398)
WARNING: CPU: 1 PID: 1 at net/core/netpoll.c:370 netpoll_send_skb+0x1fc/0x20c
gem_poll_controller() disables interrupts, which may sleep.
We can't sleep in netpoll, it has interrupts disabled completely.
Strangely, gem_poll_controller() doesn't even poll the completions,
and instead acts as if an interrupt has fired so it just schedules
NAPI and exits. None of this has been necessary for years, since
netpoll invokes NAPI directly.
Fixes: fe09bb619096 ("sungem: Spring cleaning and GRO support")
Reported-and-tested-by: Erhard Furtner <erhard_f@mailbox.org>
Link: https://lore.kernel.org/all/20240428125306.2c3080ef@legion
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: robh@kernel.org
CC: elder@kernel.org
CC: wei.fang@nxp.com
CC: bhupesh.sharma@linaro.org
CC: benh@kernel.crashing.org
---
drivers/net/ethernet/sun/sungem.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
index 9bd1df8308d2..d3a2fbb14140 100644
--- a/drivers/net/ethernet/sun/sungem.c
+++ b/drivers/net/ethernet/sun/sungem.c
@@ -949,17 +949,6 @@ static irqreturn_t gem_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
-#ifdef CONFIG_NET_POLL_CONTROLLER
-static void gem_poll_controller(struct net_device *dev)
-{
- struct gem *gp = netdev_priv(dev);
-
- disable_irq(gp->pdev->irq);
- gem_interrupt(gp->pdev->irq, dev);
- enable_irq(gp->pdev->irq);
-}
-#endif
-
static void gem_tx_timeout(struct net_device *dev, unsigned int txqueue)
{
struct gem *gp = netdev_priv(dev);
@@ -2839,9 +2828,6 @@ static const struct net_device_ops gem_netdev_ops = {
.ndo_change_mtu = gem_change_mtu,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = gem_set_mac_address,
-#ifdef CONFIG_NET_POLL_CONTROLLER
- .ndo_poll_controller = gem_poll_controller,
-#endif
};
static int gem_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
--
2.45.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks
2024-05-08 13:45 [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks Jakub Kicinski
@ 2024-05-08 14:32 ` Eric Dumazet
2024-05-09 11:10 ` Wei Fang
2024-05-11 1:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2024-05-08 14:32 UTC (permalink / raw
To: Jakub Kicinski
Cc: davem, netdev, pabeni, Erhard Furtner, robh, elder, wei.fang,
bhupesh.sharma, benh
On Wed, May 8, 2024 at 3:45 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Erhard reports netpoll warnings from sungem:
>
> netpoll_send_skb_on_dev(): eth0 enabled interrupts in poll (gem_start_xmit+0x0/0x398)
> WARNING: CPU: 1 PID: 1 at net/core/netpoll.c:370 netpoll_send_skb+0x1fc/0x20c
>
> gem_poll_controller() disables interrupts, which may sleep.
> We can't sleep in netpoll, it has interrupts disabled completely.
> Strangely, gem_poll_controller() doesn't even poll the completions,
> and instead acts as if an interrupt has fired so it just schedules
> NAPI and exits. None of this has been necessary for years, since
> netpoll invokes NAPI directly.
>
> Fixes: fe09bb619096 ("sungem: Spring cleaning and GRO support")
> Reported-and-tested-by: Erhard Furtner <erhard_f@mailbox.org>
> Link: https://lore.kernel.org/all/20240428125306.2c3080ef@legion
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks
2024-05-08 13:45 [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks Jakub Kicinski
2024-05-08 14:32 ` Eric Dumazet
@ 2024-05-09 11:10 ` Wei Fang
2024-05-09 18:51 ` Jakub Kicinski
2024-05-11 1:30 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Wei Fang @ 2024-05-09 11:10 UTC (permalink / raw
To: Jakub Kicinski, davem@davemloft.net
Cc: netdev@vger.kernel.org, edumazet@google.com, pabeni@redhat.com,
Erhard Furtner, robh@kernel.org, elder@kernel.org,
bhupesh.sharma@linaro.org, benh@kernel.crashing.org
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2024年5月8日 21:45
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org; edumazet@google.com; pabeni@redhat.com;
> Jakub Kicinski <kuba@kernel.org>; Erhard Furtner <erhard_f@mailbox.org>;
> robh@kernel.org; elder@kernel.org; Wei Fang <wei.fang@nxp.com>;
> bhupesh.sharma@linaro.org; benh@kernel.crashing.org
> Subject: [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid
> deadlocks
>
> Erhard reports netpoll warnings from sungem:
>
> netpoll_send_skb_on_dev(): eth0 enabled interrupts in poll
> (gem_start_xmit+0x0/0x398)
> WARNING: CPU: 1 PID: 1 at net/core/netpoll.c:370
> netpoll_send_skb+0x1fc/0x20c
>
> gem_poll_controller() disables interrupts, which may sleep.
> We can't sleep in netpoll, it has interrupts disabled completely.
> Strangely, gem_poll_controller() doesn't even poll the completions, and
> instead acts as if an interrupt has fired so it just schedules NAPI and exits.
> None of this has been necessary for years, since netpoll invokes NAPI directly.
>
Thanks for reminder.
The fec driver should have the same issue, but I don't know much about netpoll,
is ndo_poll_controller() no needed anymore? so it can be safely deleted from
device driver?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks
2024-05-09 11:10 ` Wei Fang
@ 2024-05-09 18:51 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-05-09 18:51 UTC (permalink / raw
To: Wei Fang
Cc: davem@davemloft.net, netdev@vger.kernel.org, edumazet@google.com,
pabeni@redhat.com, Erhard Furtner, robh@kernel.org,
elder@kernel.org, bhupesh.sharma@linaro.org,
benh@kernel.crashing.org
On Thu, 9 May 2024 11:10:46 +0000 Wei Fang wrote:
> Thanks for reminder.
> The fec driver should have the same issue, but I don't know much about netpoll,
> is ndo_poll_controller() no needed anymore? so it can be safely deleted from
> device driver?
If the driver uses NAPI for tx completions - implementing
ndo_poll_controller is not necessary.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks
2024-05-08 13:45 [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks Jakub Kicinski
2024-05-08 14:32 ` Eric Dumazet
2024-05-09 11:10 ` Wei Fang
@ 2024-05-11 1:30 ` patchwork-bot+netdevbpf
2024-05-23 23:53 ` Benjamin Herrenschmidt
2 siblings, 1 reply; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-05-11 1:30 UTC (permalink / raw
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, erhard_f, robh, elder, wei.fang,
bhupesh.sharma, benh
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 8 May 2024 06:45:04 -0700 you wrote:
> Erhard reports netpoll warnings from sungem:
>
> netpoll_send_skb_on_dev(): eth0 enabled interrupts in poll (gem_start_xmit+0x0/0x398)
> WARNING: CPU: 1 PID: 1 at net/core/netpoll.c:370 netpoll_send_skb+0x1fc/0x20c
>
> gem_poll_controller() disables interrupts, which may sleep.
> We can't sleep in netpoll, it has interrupts disabled completely.
> Strangely, gem_poll_controller() doesn't even poll the completions,
> and instead acts as if an interrupt has fired so it just schedules
> NAPI and exits. None of this has been necessary for years, since
> netpoll invokes NAPI directly.
>
> [...]
Here is the summary with links:
- [net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks
https://git.kernel.org/netdev/net/c/ac0a230f719b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks
2024-05-11 1:30 ` patchwork-bot+netdevbpf
@ 2024-05-23 23:53 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2024-05-23 23:53 UTC (permalink / raw
To: patchwork-bot+netdevbpf, Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, erhard_f, robh, elder, wei.fang,
bhupesh.sharma
On Sat, 2024-05-11 at 01:30 +0000, patchwork-bot+netdevbpf@kernel.org
wrote:
> Hello:
>
> This patch was applied to netdev/net.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
>
> On Wed, 8 May 2024 06:45:04 -0700 you wrote:
> > Erhard reports netpoll warnings from sungem:
> >
> > netpoll_send_skb_on_dev(): eth0 enabled interrupts in poll
> > (gem_start_xmit+0x0/0x398)
> > WARNING: CPU: 1 PID: 1 at net/core/netpoll.c:370
> > netpoll_send_skb+0x1fc/0x20c
> >
> > gem_poll_controller() disables interrupts, which may sleep.
> > We can't sleep in netpoll, it has interrupts disabled completely.
> > Strangely, gem_poll_controller() doesn't even poll the completions,
> > and instead acts as if an interrupt has fired so it just schedules
> > NAPI and exits. None of this has been necessary for years, since
> > netpoll invokes NAPI directly.
Well, I wrote that in 2011 so ... :-) But yeah, sounds good.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-24 0:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-08 13:45 [PATCH net] eth: sungem: remove .ndo_poll_controller to avoid deadlocks Jakub Kicinski
2024-05-08 14:32 ` Eric Dumazet
2024-05-09 11:10 ` Wei Fang
2024-05-09 18:51 ` Jakub Kicinski
2024-05-11 1:30 ` patchwork-bot+netdevbpf
2024-05-23 23:53 ` Benjamin Herrenschmidt
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.