Skip to contentSkip to conversation

eth: sungem: remove .ndo_poll_controller to avoid deadlocks

PATCH Applied Jakub Kicinski 5 participants 6 messages May 8, 2024 – May 24, 2024

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Jakub Kicinski

author maintainer
#0 lore raw

PATCH 1/1 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.

Fixes fe09bb619096 ("sungem: Spring cleaning and GRO support")
Reported-and-tested-by Erhard Furtner <[email protected]>
Link https://lore.kernel.org/all/20240428125306.2c3080ef@legion open here
Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 drivers/net/ethernet/sun/sungem.c | 14 --------------
 1 file changed, 14 deletions(-)
drivers/net/ethernet/sun/sungem.c +0 -14
index 9bd1df8308d2..d3a2fbb14140 100644
@@ -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)
Signature · 1 line
2.45.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 13 lines · Jakub Kicinski

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.

Quoted 4 lines
Fixes fe09bb619096 ("sungem: Spring cleaning and GRO support")
Reported-and-tested-by Erhard Furtner <[email protected]>
Link https://lore.kernel.org/all/20240428125306.2c3080ef@legion open here
Signed-off-by Jakub Kicinski <[email protected]>
Reviewed-by Eric Dumazet <[email protected]>
#2 lore raw
In reply to Jakub Kicinski
Quoted 16 lines

-----Original Message-----
From: Jakub Kicinski <[email protected]>
Sent: 2024年5月8日 21:45
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; Jakub Kicinski <[email protected]>; Erhard Furtner <[email protected]>; [email protected]; [email protected]; Wei Fang <[email protected]>; [email protected]; [email protected]
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)

Quoted 9 lines
  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?

author maintainer
#3 lore raw
Quoted 4 lines · 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.

CI Applied
#4 lore raw
In reply to Jakub Kicinski
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 5 lines · 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
Quoted 8 lines

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!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
#5 lore raw
Quoted 19 lines · On Sat, 2024-05-11 at 01:30 +0000, [email protected] wrote:

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 13 lines · 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.