drivers/net: fixed drivers that support netpoll use ndo_start_xmit()

PATCH Applied DDD 3 participants 4 messages Aug 14, 2009 – Aug 18, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

DDD

author

2009-08-14 05:00 UTC #0 lore raw

PATCH 1/1 drivers/net: fixed drivers that support netpoll use ndo_start_xmit()

The NETPOLL API requires that interrupts remain disabled in netpoll_send_skb(). The use of spin_lock_irq() and spin_unlock_irq() in the NETPOLL API callbacks causes the interrupts to get enabled and can lead to kernel instability.

The solution is to use spin_lock_irqsave() and spin_unlock_restore() to prevent the irqs from getting enabled while in netpoll_send_skb().

Call trace:
netpoll_send_skb()
{
   -> local_irq_save(flags)
     ---> dev->ndo_start_xmit(skb, dev)
         ---> spin_lock_irq()
         ---> spin_unlock_irq() *******here would enable the interrupt.
              ...

-> local_irq_restore(flags)
}

Signed-off-by Dongdong Deng <[email protected]>

Signed-off-by Jason Wessel <[email protected]>

Acked-by Bruce Ashfield <[email protected]>

---
 drivers/net/b44.c              |    5 +++--
 drivers/net/tulip/tulip_core.c |    5 +++--
 drivers/net/ucc_geth.c         |    5 +++--
 drivers/net/via-rhine.c        |    5 +++--
 4 files changed, 12 insertions(+), 8 deletions(-)

drivers/net/b44.c +3 -2

index 36d4d37..bafca67 100644
@@ -952,9 +952,10 @@ static int b44_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	int rc = NETDEV_TX_OK;
 	dma_addr_t mapping;
 	u32 len, entry, ctrl;
+	unsigned long flags;
 
 	len = skb->len;
-	spin_lock_irq(&bp->lock);
+	spin_lock_irqsave(&bp->lock, flags);
 
 	/* This is a hard error, log it. */
 	if (unlikely(TX_BUFFS_AVAIL(bp) < 1)) {

@@ -1027,7 +1028,7 @@ static int b44_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev->trans_start = jiffies;
 
 out_unlock:
-	spin_unlock_irq(&bp->lock);
+	spin_unlock_irqrestore(&bp->lock, flags);
 
 	return rc;

drivers/net/tulip/tulip_core.c +3 -2

index 99a6364..4cf9a65 100644
@@ -652,8 +652,9 @@ tulip_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	int entry;
 	u32 flag;
 	dma_addr_t mapping;
+	unsigned long flags;
 
-	spin_lock_irq(&tp->lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	/* Calculate the next Tx descriptor entry. */
 	entry = tp->cur_tx % TX_RING_SIZE;

@@ -688,7 +689,7 @@ tulip_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Trigger an immediate transmit demand. */
 	iowrite32(0, tp->base_addr + CSR1);
 
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 
 	dev->trans_start = jiffies;

drivers/net/ucc_geth.c +3 -2

index 3b957e6..8a7b8c7 100644
@@ -3111,10 +3111,11 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 __iomem *bd;			/* BD pointer */
 	u32 bd_status;
 	u8 txQ = 0;
+	unsigned long flags;
 
 	ugeth_vdbg("%s: IN", __func__);
 
-	spin_lock_irq(&ugeth->lock);
+	spin_lock_irqsave(&ugeth->lock, flags);
 
 	dev->stats.tx_bytes += skb->len;

@@ -3171,7 +3172,7 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	uccf = ugeth->uccf;
 	out_be16(uccf->p_utodr, UCC_FAST_TOD);
 #endif
-	spin_unlock_irq(&ugeth->lock);
+	spin_unlock_irqrestore(&ugeth->lock, flags);
 
 	return 0;
 }

drivers/net/via-rhine.c +3 -2

index 88c30a5..934f767 100644
@@ -1218,6 +1218,7 @@ static int rhine_start_tx(struct sk_buff *skb, struct net_device *dev)
 	struct rhine_private *rp = netdev_priv(dev);
 	void __iomem *ioaddr = rp->base;
 	unsigned entry;
+	unsigned long flags;
 
 	/* Caution: the write order is important here, set the field
 	   with the "ownership" bits last. */

@@ -1261,7 +1262,7 @@ static int rhine_start_tx(struct sk_buff *skb, struct net_device *dev)
 		cpu_to_le32(TXDESC | (skb->len >= ETH_ZLEN ? skb->len : ETH_ZLEN));
 
 	/* lock eth irq */
-	spin_lock_irq(&rp->lock);
+	spin_lock_irqsave(&rp->lock, flags);
 	wmb();
 	rp->tx_ring[entry].tx_status = cpu_to_le32(DescOwn);
 	wmb();

@@ -1280,7 +1281,7 @@ static int rhine_start_tx(struct sk_buff *skb, struct net_device *dev)
 
 	dev->trans_start = jiffies;
 
-	spin_unlock_irq(&rp->lock);
+	spin_unlock_irqrestore(&rp->lock, flags);
 
 	if (debug > 4) {
 		printk(KERN_DEBUG "%s: Transmit frame #%d queued in slot %d.\n",

Signature · 1 line

1.6.0.4

Matt Mackall

Acked-by on v1 1/1

2009-08-14 15:33 UTC #1 lore raw

Quoted 19 lines · DDD

The NETPOLL API requires that interrupts remain disabled in netpoll_send_skb(). The use of spin_lock_irq() and spin_unlock_irq() in the NETPOLL API callbacks causes the interrupts to get enabled and can lead to kernel instability.
The solution is to use spin_lock_irqsave() and spin_unlock_restore() to prevent the irqs from getting enabled while in netpoll_send_skb().
Call trace:
netpoll_send_skb()
{
   -> local_irq_save(flags)
     ---> dev->ndo_start_xmit(skb, dev)
         ---> spin_lock_irq()
         ---> spin_unlock_irq() *******here would enable the interrupt.
              ...
   -> local_irq_restore(flags)
}

Quoted 3 lines

Signed-off-by Dongdong Deng <[email protected]>
Signed-off-by Jason Wessel <[email protected]>
Acked-by Bruce Ashfield <[email protected]>

Acked-by Matt Mackall <[email protected]>

Perhaps we should also have a WARN_ONCE if start_xmit returns with interrupts enabled?

Signature · 1 line

http://selenic.com : development and support for Mercurial and Linux

David Miller

maintainer Applied

2009-08-14 23:32 UTC #2 lore raw

From: Matt Mackall <[email protected]>
Date: Fri, 14 Aug 2009 10:33:39 -0500

Quoted 8 lines · DDD

Quoted 7 lines
The NETPOLL API requires that interrupts remain disabled in netpoll_send_skb(). The use of spin_lock_irq() and spin_unlock_irq() in the NETPOLL API callbacks causes the interrupts to get enabled and can lead to kernel instability.
The solution is to use spin_lock_irqsave() and spin_unlock_restore() to prevent the irqs from getting enabled while in netpoll_send_skb().

...

Quoted 5 lines

Quoted 3 lines
Signed-off-by Dongdong Deng <[email protected]>
Signed-off-by Jason Wessel <[email protected]>
Acked-by Bruce Ashfield <[email protected]>
Acked-by Matt Mackall <[email protected]>

Applied, thanks everyone.

Quoted 2 lines

Perhaps we should also have a WARN_ONCE if start_xmit returns with interrupts enabled?

Probably a good idea.

DDD

author

2009-08-18 05:47 UTC #3 lore raw

Quoted 16 lines · David Miller

From: Matt Mackall <[email protected]>
Date: Fri, 14 Aug 2009 10:33:39 -0500
Quoted 3 lines · DDD
Quoted 2 lines
The NETPOLL API requires that interrupts remain disabled in netpoll_send_skb(). The use of spin_lock_irq() and
spin_unlock_irq()
Quoted 1 line
Quoted 1 line
in the NETPOLL API callbacks causes the interrupts to get enabled
and
Quoted 3 lines
Quoted 3 lines
can lead to kernel instability.
The solution is to use spin_lock_irqsave() and
spin_unlock_restore()
Quoted 1 line
Quoted 1 line
to prevent the irqs from getting enabled while in
netpoll_send_skb().
...

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Signed-off-by Dongdong Deng <[email protected]>
Signed-off-by Jason Wessel <[email protected]>
Acked-by Bruce Ashfield <[email protected]>
Acked-by Matt Mackall <[email protected]>
Applied, thanks everyone.

Thanks. :-)

Quoted 5 lines

Quoted 3 lines
Perhaps we should also have a WARN_ONCE if start_xmit returns with interrupts enabled?
Probably a good idea.

I think it is a good idea too, thanks for your suggestion, Matt. I will do a tiny patch for it shortly.

Dongdong