From: Anton Vorontsov <hidden> Date: 2010-03-03 18:19:01
Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB
recycling") gianfar driver sooner or later stops transmitting any
packets on SMP machines.
start_xmit() prepares new skb for transmitting, generally it does
three things:
1. sets up all BDs (marks them ready to send), except the first one.
2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring()
would cleanup it later.
3. sets up the first BD, i.e. marks it ready.
Here is what clean_tx_ring() does:
1. reads skbs from tx_queue->tx_skbuff
2. checks if the *last* BD is ready. If it's still ready [to send]
then it it isn't transmitted, so clean_tx_ring() returns.
Otherwise it actually cleanups BDs. All is OK.
Now, if there is just one BD, code flow:
- start_xmit(): stores skb into tx_skbuff. Note that the first BD
(which is also the last one) isn't marked as ready, yet.
- clean_tx_ring(): sees that skb is not null, *and* its lstatus
says that it is NOT ready (like if BD was sent), so it cleans
it up (bad!)
- start_xmit(): marks BD as ready [to send], but it's too late.
We can fix this simply by reordering lstatus/tx_skbuff writes.
Reported-by: Martyn Welch <redacted>
Bisected-by: Paul Gortmaker [off-list ref]
Signed-off-by: Anton Vorontsov <redacted>
Tested-by: Paul Gortmaker <redacted>
Tested-by: Martyn Welch <redacted>
Cc: Sandeep Gopalpet <redacted>
Cc: Stable <redacted> [2.6.33]
---
drivers/net/gianfar.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
@@ -2021,7 +2021,6 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)}/* setup the TxBD length and buffer pointer for the first BD */-tx_queue->tx_skbuff[tx_queue->skb_curtx]=skb;txbdp_start->bufPtr=dma_map_single(&priv->ofdev->dev,skb->data,skb_headlen(skb),DMA_TO_DEVICE);
@@ -2053,6 +2052,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)txbdp_start->lstatus=lstatus;+eieio();/* force lstatus write before tx_skbuff */++tx_queue->tx_skbuff[tx_queue->skb_curtx]=skb;+/* Update the current skb pointer to the next entry we will use*(wrappingifnecessary)*/tx_queue->skb_curtx=(tx_queue->skb_curtx+1)&
From: David Miller <davem@davemloft.net> Date: 2010-03-04 08:41:39
From: Anton Vorontsov <redacted>
Date: Wed, 3 Mar 2010 21:18:58 +0300
Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB
recycling") gianfar driver sooner or later stops transmitting any
packets on SMP machines.
start_xmit() prepares new skb for transmitting, generally it does
three things:
1. sets up all BDs (marks them ready to send), except the first one.
2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring()
would cleanup it later.
3. sets up the first BD, i.e. marks it ready.
Here is what clean_tx_ring() does:
1. reads skbs from tx_queue->tx_skbuff
2. checks if the *last* BD is ready. If it's still ready [to send]
then it it isn't transmitted, so clean_tx_ring() returns.
Otherwise it actually cleanups BDs. All is OK.
Now, if there is just one BD, code flow:
- start_xmit(): stores skb into tx_skbuff. Note that the first BD
(which is also the last one) isn't marked as ready, yet.
- clean_tx_ring(): sees that skb is not null, *and* its lstatus
says that it is NOT ready (like if BD was sent), so it cleans
it up (bad!)
- start_xmit(): marks BD as ready [to send], but it's too late.
We can fix this simply by reordering lstatus/tx_skbuff writes.
Reported-by: Martyn Welch <redacted>
Bisected-by: Paul Gortmaker [off-list ref]
Signed-off-by: Anton Vorontsov <redacted>
Tested-by: Paul Gortmaker <redacted>
Tested-by: Martyn Welch <redacted>
From: Kumar Gala <hidden> Date: 2010-03-04 16:34:29
On Mar 4, 2010, at 2:41 AM, David Miller wrote:
From: Anton Vorontsov <redacted>
Date: Wed, 3 Mar 2010 21:18:58 +0300
=20
quoted
Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB
recycling") gianfar driver sooner or later stops transmitting any
packets on SMP machines.
=20
start_xmit() prepares new skb for transmitting, generally it does
three things:
=20
1. sets up all BDs (marks them ready to send), except the first one.
2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring()
would cleanup it later.
3. sets up the first BD, i.e. marks it ready.
=20
Here is what clean_tx_ring() does:
=20
1. reads skbs from tx_queue->tx_skbuff
2. checks if the *last* BD is ready. If it's still ready [to send]
then it it isn't transmitted, so clean_tx_ring() returns.
Otherwise it actually cleanups BDs. All is OK.
=20
Now, if there is just one BD, code flow:
=20
- start_xmit(): stores skb into tx_skbuff. Note that the first BD
(which is also the last one) isn't marked as ready, yet.
- clean_tx_ring(): sees that skb is not null, *and* its lstatus
says that it is NOT ready (like if BD was sent), so it cleans
it up (bad!)
- start_xmit(): marks BD as ready [to send], but it's too late.
=20
We can fix this simply by reordering lstatus/tx_skbuff writes.
=20
Reported-by: Martyn Welch <redacted>
Bisected-by: Paul Gortmaker [off-list ref]
Signed-off-by: Anton Vorontsov <redacted>
Tested-by: Paul Gortmaker <redacted>
Tested-by: Martyn Welch <redacted>
=20
Applied.
Anton,
Once this makes it into Linus's tree can you make sure we get it added =
to -stable.
- k=
On Wed, Mar 3, 2010 at 8:18 PM, Anton Vorontsov
[off-list ref] wrote:
quoted hunk
Starting with commit a3bc1f11e9b867a4f49505 ("gianfar: Revive SKB
recycling") gianfar driver sooner or later stops transmitting any
packets on SMP machines.
start_xmit() prepares new skb for transmitting, generally it does
three things:
1. sets up all BDs (marks them ready to send), except the first one.
2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring()
=A0 would cleanup it later.
3. sets up the first BD, i.e. marks it ready.
Here is what clean_tx_ring() does:
1. reads skbs from tx_queue->tx_skbuff
2. checks if the *last* BD is ready. If it's still ready [to send]
=A0 then it it isn't transmitted, so clean_tx_ring() returns.
=A0 Otherwise it actually cleanups BDs. All is OK.
Now, if there is just one BD, code flow:
- start_xmit(): stores skb into tx_skbuff. Note that the first BD
=A0(which is also the last one) isn't marked as ready, yet.
- clean_tx_ring(): sees that skb is not null, *and* its lstatus
=A0says that it is NOT ready (like if BD was sent), so it cleans
=A0it up (bad!)
- start_xmit(): marks BD as ready [to send], but it's too late.
We can fix this simply by reordering lstatus/tx_skbuff writes.
Reported-by: Martyn Welch <redacted>
Bisected-by: Paul Gortmaker [off-list ref]
Signed-off-by: Anton Vorontsov <redacted>
Tested-by: Paul Gortmaker <redacted>
Tested-by: Martyn Welch <redacted>
Cc: Sandeep Gopalpet <redacted>
Cc: Stable <redacted> [2.6.33]
---
=A0drivers/net/gianfar.c | =A0 =A05 ++++-
=A01 files changed, 4 insertions(+), 1 deletions(-)
This patch also makes gianfar work stable on mpc8313 with 2.6.33/RT_PREEMPT=
.
WIthout it, I see exactly the same problems as reported by Anton on SMP.
/Esben
--=20
Esben Haabendal, Senior Software Consultant
Dor=E9Development ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark
Phone: +45 51 92 53 93, E-mail: eha@doredevelopment.dk
WWW: http://www.doredevelopment.dk