[PATCH 0/6] gianfar: Some fixes

[PATCH 0/6] gianfar: Some fixes

[Anton Vorontsov]

Hi all,

Here are some fixes for the gianfar driver, patches on the way.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/6] skbuff: Do not allow skb recycling with disabled IRQs

[Anton Vorontsov]

NAPI drivers try to recycle SKBs in their polling routine, but we
generally don't know the context in which the polling will be called,
and the skb recycling itself may require IRQs to be enabled.

This patch adds irqs_disabled() test to the skb_recycle_check()
routine, so that we'll not let the drivers hit the skb recycling
path with IRQs disabled.

As a side effect, this patch actually disables skb recycling for some
[broken] drivers. E.g. gianfar driver grabs an irqsave spinlock during
TX ring processing, and then tries to recycle an skb, and that caused
the following badness:

nf_conntrack version 0.5.0 (1008 buckets, 4032 max)
------------[ cut here ]------------
Badness at kernel/softirq.c:143
NIP: c003e3c4 LR: c423a528 CTR: c003e344
...
NIP [c003e3c4] local_bh_enable+0x80/0xc4
LR [c423a528] destroy_conntrack+0xd4/0x13c [nf_conntrack]
Call Trace:
[c15d1b60] [c003e32c] local_bh_disable+0x1c/0x34 (unreliable)
[c15d1b70] [c423a528] destroy_conntrack+0xd4/0x13c [nf_conntrack]
[c15d1b80] [c02c6370] nf_conntrack_destroy+0x3c/0x70
--- Exception: c428168c at 0xc15d1c50
LR = 0xc15d1c40
[c15d1ba0] [c0286f3c] skb_release_head_state+0x100/0x104 (unreliable)
[c15d1bb0] [c0288340] skb_recycle_check+0x8c/0x10c
[c15d1bc0] [c01e1688] gfar_poll+0x190/0x384
[c15d1c10] [c02935ac] net_rx_action+0xec/0x22c
[c15d1c50] [c003dd8c] __do_softirq+0xe8/0x224
...

Reported-by: Jon Loeliger <jdl@jdl.com>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 net/core/skbuff.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 80a9616..941bac9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -493,6 +493,9 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
 {
 	struct skb_shared_info *shinfo;
 
+	if (irqs_disabled())
+		return 0;
+
 	if (skb_is_nonlinear(skb) || skb->fclone != SKB_FCLONE_UNAVAILABLE)
 		return 0;
 
-- 
1.6.3.3

[PATCH 2/6] gianfar: Remove 'Interrupt problem!' warning

[Anton Vorontsov]

It is OK to poll with disabled IRQs, so remove the warning.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/gianfar.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 197b358..79c28f5 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -2504,8 +2504,6 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
 				skb_put(skb, pkt_len);
 				dev->stats.rx_bytes += pkt_len;
 
-				if (in_irq() || irqs_disabled())
-					printk("Interrupt problem!\n");
 				gfar_process_frame(dev, skb, amount_pull);
 
 			} else {
-- 
1.6.3.3

[PATCH 3/6] gianfar: Fix build with CONFIG_PM=y

[Anton Vorontsov]

commit fba4ed030cfae7efdb6b79a57b0c5a9d72c9 ("gianfar: Add Multiple
Queue Support") introduced the following build failure:

  CC      gianfar.o
gianfar.c: In function 'gfar_restore':
gianfar.c:1249: error: request for member 'napi' in something not a structure or union

This patch fixes the issue.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/gianfar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 79c28f5..a5b0038 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1246,7 +1246,7 @@ static int gfar_restore(struct device *dev)
 		phy_start(priv->phydev);
 
 	netif_device_attach(ndev);
-	napi_enable(&priv->gfargrp.napi);
+	enable_napi(priv);
 
 	return 0;
 }
-- 
1.6.3.3

RE: [PATCH 3/6] gianfar: Fix build with CONFIG_PM=y

[Kumar Gopalpet-B05799]

=20

>-----Original Message-----
>From: Anton Vorontsov [mailto:avorontsov@ru.mvista.com]=20
>Sent: Wednesday, November 11, 2009 5:41 AM
>To: David Miller
>Cc: Fleming Andy-AFLEMING; Jon Loeliger; Kumar=20
>Gopalpet-B05799; Lennert Buytenhek; Stephen Hemminger;=20
>netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
>Subject: [PATCH 3/6] gianfar: Fix build with CONFIG_PM=3Dy
>
>commit fba4ed030cfae7efdb6b79a57b0c5a9d72c9 ("gianfar: Add=20
>Multiple Queue Support") introduced the following build failure:
>
>  CC      gianfar.o
>gianfar.c: In function 'gfar_restore':
>gianfar.c:1249: error: request for member 'napi' in something=20
>not a structure or union
>
>This patch fixes the issue.
>
>Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>---
> drivers/net/gianfar.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c=20
>index 79c28f5..a5b0038 100644
>--- a/drivers/net/gianfar.c
>+++ b/drivers/net/gianfar.c
>@@ -1246,7 +1246,7 @@ static int gfar_restore(struct device *dev)
> 		phy_start(priv->phydev);
>=20
> 	netif_device_attach(ndev);
>-	napi_enable(&priv->gfargrp.napi);
>+	enable_napi(priv);
>=20
> 	return 0;
> }


I am extreemely sorry for introducing this error, I missed it while
porting my last set of patches.

Thanks for fixing it.


--

Thanks
Sandeep

[PATCH 4/6] gianfar: Fix thinko in gfar_set_rx_stash_index()

[Anton Vorontsov]

We obviously want to write a modified 'temp' value back to the
register, not the saved IRQ flags.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/gianfar_sysfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/gianfar_sysfs.c b/drivers/net/gianfar_sysfs.c
index 3724835..b31c9c8 100644
--- a/drivers/net/gianfar_sysfs.c
+++ b/drivers/net/gianfar_sysfs.c
@@ -186,7 +186,7 @@ static ssize_t gfar_set_rx_stash_index(struct device *dev,
 	temp = gfar_read(&regs->attreli);
 	temp &= ~ATTRELI_EI_MASK;
 	temp |= ATTRELI_EI(index);
-	gfar_write(&regs->attreli, flags);
+	gfar_write(&regs->attreli, temp);
 
 out:
 	unlock_rx_qs(priv);
-- 
1.6.3.3

[PATCH 5/6] gianfar: Fix race between gfar_error() and gfar_start_xmit()

[Anton Vorontsov]

gfar_error() can arrive at the middle of gfar_start_xmit() processing,
and so it can trigger transfers of BDs that we don't yet expect to
be transmitted.

Fix this by locking the tx queues in gfar_error().

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/gianfar.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index a5b0038..fde430a 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -2943,14 +2943,22 @@ static irqreturn_t gfar_error(int irq, void *grp_id)
 		if (events & IEVENT_CRL)
 			dev->stats.tx_aborted_errors++;
 		if (events & IEVENT_XFUN) {
+			unsigned long flags;
+
 			if (netif_msg_tx_err(priv))
 				printk(KERN_DEBUG "%s: TX FIFO underrun, "
 				       "packet dropped.\n", dev->name);
 			dev->stats.tx_dropped++;
 			priv->extra_stats.tx_underrun++;
 
+			local_irq_save(flags);
+			lock_tx_qs(priv);
+
 			/* Reactivate the Tx Queues */
 			gfar_write(&regs->tstat, gfargrp->tstat);
+
+			unlock_tx_qs(priv);
+			local_irq_restore(flags);
 		}
 		if (netif_msg_tx_err(priv))
 			printk(KERN_DEBUG "%s: Transmit Error\n", dev->name);
-- 
1.6.3.3

[PATCH 6/6] gianfar: Revive SKB recycling

[Anton Vorontsov]

Before calling gfar_clean_tx_ring() the driver grabs an irqsave
spinlock, and then tries to recycle skbs. But since
skb_recycle_check() returns 0 with IRQs disabled, we'll never
recycle any skbs.

It appears that gfar_clean_tx_ring() and gfar_start_xmit() are
mostly idependent and can work in parallel, except when they
modify num_txbdfree.

So we can drop the lock from most sections and thus fix the skb
recycling.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/gianfar.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index fde430a..16def13 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1928,14 +1928,11 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* total number of fragments in the SKB */
 	nr_frags = skb_shinfo(skb)->nr_frags;
 
-	spin_lock_irqsave(&tx_queue->txlock, flags);
-
 	/* check if there is space to queue this packet */
 	if ((nr_frags+1) > tx_queue->num_txbdfree) {
 		/* no space, stop the queue */
 		netif_tx_stop_queue(txq);
 		dev->stats.tx_fifo_errors++;
-		spin_unlock_irqrestore(&tx_queue->txlock, flags);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -1999,6 +1996,20 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb);
 
 	/*
+	 * We can work in parallel with gfar_clean_tx_ring(), except
+	 * when modifying num_txbdfree. Note that we didn't grab the lock
+	 * when we were reading the num_txbdfree and checking for available
+	 * space, that's because outside of this function it can only grow,
+	 * and once we've got needed space, it cannot suddenly disappear.
+	 *
+	 * The lock also protects us from gfar_error(), which can modify
+	 * regs->tstat and thus retrigger the transfers, which is why we
+	 * also must grab the lock before setting ready bit for the first
+	 * to be transmitted BD.
+	 */
+	spin_lock_irqsave(&tx_queue->txlock, flags);
+
+	/*
 	 * The powerpc-specific eieio() is used, as wmb() has too strong
 	 * semantics (it requires synchronization between cacheable and
 	 * uncacheable mappings, which eieio doesn't provide and which we
@@ -2225,6 +2236,8 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 	skb_dirtytx = tx_queue->skb_dirtytx;
 
 	while ((skb = tx_queue->tx_skbuff[skb_dirtytx])) {
+		unsigned long flags;
+
 		frags = skb_shinfo(skb)->nr_frags;
 		lbdp = skip_txbd(bdp, frags, base, tx_ring_size);
 
@@ -2269,7 +2282,9 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 			TX_RING_MOD_MASK(tx_ring_size);
 
 		howmany++;
+		spin_lock_irqsave(&tx_queue->txlock, flags);
 		tx_queue->num_txbdfree += frags + 1;
+		spin_unlock_irqrestore(&tx_queue->txlock, flags);
 	}
 
 	/* If we freed a buffer, we can restart transmission, if necessary */
@@ -2548,7 +2563,6 @@ static int gfar_poll(struct napi_struct *napi, int budget)
 	int tx_cleaned = 0, i, left_over_budget = budget;
 	unsigned long serviced_queues = 0;
 	int num_queues = 0;
-	unsigned long flags;
 
 	num_queues = gfargrp->num_rx_queues;
 	budget_per_queue = budget/num_queues;
@@ -2568,14 +2582,7 @@ static int gfar_poll(struct napi_struct *napi, int budget)
 			rx_queue = priv->rx_queue[i];
 			tx_queue = priv->tx_queue[rx_queue->qindex];
 
-			/* If we fail to get the lock,
-			 * don't bother with the TX BDs */
-			if (spin_trylock_irqsave(&tx_queue->txlock, flags)) {
-				tx_cleaned += gfar_clean_tx_ring(tx_queue);
-				spin_unlock_irqrestore(&tx_queue->txlock,
-							flags);
-			}
-
+			tx_cleaned += gfar_clean_tx_ring(tx_queue);
 			rx_cleaned_per_queue = gfar_clean_rx_ring(rx_queue,
 							budget_per_queue);
 			rx_cleaned += rx_cleaned_per_queue;
-- 
1.6.3.3

RE: [PATCH 6/6] gianfar: Revive SKB recycling

[Kumar Gopalpet-B05799]

=20

>-----Original Message-----
>From: Anton Vorontsov [mailto:avorontsov@ru.mvista.com]=20
>Sent: Wednesday, November 11, 2009 5:41 AM
>To: David Miller
>Cc: Fleming Andy-AFLEMING; Jon Loeliger; Kumar=20
>Gopalpet-B05799; Lennert Buytenhek; Stephen Hemminger;=20
>netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
>Subject: [PATCH 6/6] gianfar: Revive SKB recycling
>
>Before calling gfar_clean_tx_ring() the driver grabs an=20
>irqsave spinlock, and then tries to recycle skbs. But since
>skb_recycle_check() returns 0 with IRQs disabled, we'll never=20
>recycle any skbs.
>
>It appears that gfar_clean_tx_ring() and gfar_start_xmit() are=20
>mostly idependent and can work in parallel, except when they=20
>modify num_txbdfree.
>
>So we can drop the lock from most sections and thus fix the=20
>skb recycling.
>
>Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>---
> drivers/net/gianfar.c |   31 +++++++++++++++++++------------
> 1 files changed, 19 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c=20
>index fde430a..16def13 100644
>--- a/drivers/net/gianfar.c
>+++ b/drivers/net/gianfar.c
>@@ -1928,14 +1928,11 @@ static int gfar_start_xmit(struct=20
>sk_buff *skb, struct net_device *dev)
> 	/* total number of fragments in the SKB */
> 	nr_frags =3D skb_shinfo(skb)->nr_frags;
>=20
>-	spin_lock_irqsave(&tx_queue->txlock, flags);
>-
> 	/* check if there is space to queue this packet */
> 	if ((nr_frags+1) > tx_queue->num_txbdfree) {
> 		/* no space, stop the queue */
> 		netif_tx_stop_queue(txq);
> 		dev->stats.tx_fifo_errors++;
>-		spin_unlock_irqrestore(&tx_queue->txlock, flags);
> 		return NETDEV_TX_BUSY;
> 	}
>=20
>@@ -1999,6 +1996,20 @@ static int gfar_start_xmit(struct=20
>sk_buff *skb, struct net_device *dev)
> 	lstatus |=3D BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb);
>=20
> 	/*
>+	 * We can work in parallel with gfar_clean_tx_ring(), except
>+	 * when modifying num_txbdfree. Note that we didn't=20
>grab the lock
>+	 * when we were reading the num_txbdfree and checking=20
>for available
>+	 * space, that's because outside of this function it=20
>can only grow,
>+	 * and once we've got needed space, it cannot suddenly=20
>disappear.
>+	 *
>+	 * The lock also protects us from gfar_error(), which can modify
>+	 * regs->tstat and thus retrigger the transfers, which is why we
>+	 * also must grab the lock before setting ready bit for=20
>the first
>+	 * to be transmitted BD.
>+	 */
>+	spin_lock_irqsave(&tx_queue->txlock, flags);
>+
>+	/*
> 	 * The powerpc-specific eieio() is used, as wmb() has too strong
> 	 * semantics (it requires synchronization between cacheable and
> 	 * uncacheable mappings, which eieio doesn't provide=20
>and which we @@ -2225,6 +2236,8 @@ static int=20
>gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
> 	skb_dirtytx =3D tx_queue->skb_dirtytx;
>=20
> 	while ((skb =3D tx_queue->tx_skbuff[skb_dirtytx])) {
>+		unsigned long flags;
>+
> 		frags =3D skb_shinfo(skb)->nr_frags;
> 		lbdp =3D skip_txbd(bdp, frags, base, tx_ring_size);
>=20
>@@ -2269,7 +2282,9 @@ static int gfar_clean_tx_ring(struct=20
>gfar_priv_tx_q *tx_queue)
> 			TX_RING_MOD_MASK(tx_ring_size);
>=20
> 		howmany++;
>+		spin_lock_irqsave(&tx_queue->txlock, flags);
> 		tx_queue->num_txbdfree +=3D frags + 1;
>+		spin_unlock_irqrestore(&tx_queue->txlock, flags);
> 	}
>=20
> 	/* If we freed a buffer, we can restart transmission,=20
>if necessary */ @@ -2548,7 +2563,6 @@ static int=20
>gfar_poll(struct napi_struct *napi, int budget)
> 	int tx_cleaned =3D 0, i, left_over_budget =3D budget;
> 	unsigned long serviced_queues =3D 0;
> 	int num_queues =3D 0;
>-	unsigned long flags;
>=20
> 	num_queues =3D gfargrp->num_rx_queues;
> 	budget_per_queue =3D budget/num_queues;
>@@ -2568,14 +2582,7 @@ static int gfar_poll(struct napi_struct=20
>*napi, int budget)
> 			rx_queue =3D priv->rx_queue[i];
> 			tx_queue =3D priv->tx_queue[rx_queue->qindex];
>=20
>-			/* If we fail to get the lock,
>-			 * don't bother with the TX BDs */
>-			if=20
>(spin_trylock_irqsave(&tx_queue->txlock, flags)) {
>-				tx_cleaned +=3D=20
>gfar_clean_tx_ring(tx_queue);
>-			=09
>spin_unlock_irqrestore(&tx_queue->txlock,
>-							flags);
>-			}
>-
>+			tx_cleaned +=3D gfar_clean_tx_ring(tx_queue);
> 			rx_cleaned_per_queue =3D=20
>gfar_clean_rx_ring(rx_queue,
> 						=09
>budget_per_queue);
> 			rx_cleaned +=3D rx_cleaned_per_queue;
>--


Anton, we tried some experiments too at our end, and removing the
spinlocks did help improve the performance and recycling was effective
although, I don't have exact numbers to specify.

But overall I agree with you in removing the spinlocks from the
gfar_poll context.


--

Thanks
Sandeep

Re: [PATCH 0/6] gianfar: Some fixes

[Kumar Gala]

On Nov 10, 2009, at 6:10 PM, Anton Vorontsov wrote:

> Hi all,
>
> Here are some fixes for the gianfar driver, patches on the way.
>
> Thanks,

Acked-by: Kumar Gala <galak@kernel.crashing.org>


- k

Re: [PATCH 0/6] gianfar: Some fixes

[David Miller]

From: Kumar Gala <galak@kernel.crashing.org>
Date: Wed, 11 Nov 2009 09:16:41 -0600

> 
> On Nov 10, 2009, at 6:10 PM, Anton Vorontsov wrote:
> 
>> Hi all,
>>
>> Here are some fixes for the gianfar driver, patches on the way.
>>
>> Thanks,
> 
> Acked-by: Kumar Gala <galak@kernel.crashing.org>

All applied to net-next-2.6, thanks.