xgmac: use bufsz not dma_buf_sz in rx_refill

PATCH In review Kyle McMartin 5 participants 9 messages Aug 29, 2013 – Aug 30, 2013

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Kyle McMartin

author

2013-08-29 20:11 UTC #0 lore raw

PATCH 1/1 xgmac: use bufsz not dma_buf_sz in rx_refill

Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)

Signed-off-by Kyle McMartin <[email protected]>

drivers/net/ethernet/calxeda/xgmac.c +1 -1

@@ -686,7 +686,7 @@ static void xgmac_rx_refill(struct xgmac_priv *priv)
 			priv->rx_skbuff[entry] = skb;
 			paddr = dma_map_single(priv->device, skb->data,
 					       bufsz, DMA_FROM_DEVICE);
-			desc_set_buf_addr(p, paddr, priv->dma_buf_sz);
+			desc_set_buf_addr(p, paddr, bufsz);
 		}
 
 		netdev_dbg(priv->dev, "rx ring: head %d, tail %d\n",

Rob Herring

2013-08-29 20:31 UTC #1 lore raw

Quoted 5 lines · Kyle McMartin

Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)

Please see my patch series of fixes:

http://www.spinics.net/lists/netdev/msg248076.html

drivers/net/ethernet/calxeda/xgmac.c static void xgmac_rx_refill(struct xgmac_priv *priv) · line 689

Quoted 10 lines

Signed-off-by Kyle McMartin <[email protected]>

drivers/net/ethernet/calxeda/xgmac.c +1 -1

@@ -686,7 +686,7 @@ static void xgmac_rx_refill(struct xgmac_priv *priv)
 			priv->rx_skbuff[entry] = skb;
 			paddr = dma_map_single(priv->device, skb->data,
 					       bufsz, DMA_FROM_DEVICE);
-			desc_set_buf_addr(p, paddr, priv->dma_buf_sz);
+			desc_set_buf_addr(p, paddr, bufsz);

This is not right because the h/w wants the size to include the 2 bytes skipped at the beginning for NET_IP_ALIGN. The h/w may also corrupt 1-7 bytes of data at the head or tail if the frame is not 8 byte aligned and padded. We are relying that skb allocations are aligned.

Rob

Kyle McMartin

2013-08-29 20:26 UTC #2 lore raw

Quoted 11 lines · Rob Herring

Quoted 5 lines · Kyle McMartin
Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)
Please see my patch series of fixes:
http://www.spinics.net/lists/netdev/msg248076.html

Hah, thanks. Yeah, I wondered wtf was up with get_buf_len, but figured the hardware must have been doing something subtle. ;-)

I'll apply these to our Fedora tree and pass them along for testing.

Thanks Rob.

--Kyle

David Miller

maintainer

2013-08-29 20:38 UTC #3 lore raw

In reply to Rob Herring

From: Rob Herring <[email protected]>
Date: Thu, 29 Aug 2013 15:24:30 -0500

Quoted 10 lines · Kyle McMartin

Quoted 5 lines
Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)
Please see my patch series of fixes:
http://www.spinics.net/lists/netdev/msg248076.html

Which need to be resubmitted with feedback intergrated.

I knew your series existed, but Kyle submitted a fix in the correct format meanwhile so I applied it.

Sergei Shtylyov

2013-08-29 20:31 UTC #4 lore raw

In reply to Kyle McMartin

Hello.

Quoted 2 lines · Kyle McMartin

Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of

Please also specify that commit's summary line in parens.

Quoted 5 lines

priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)
Signed-off-by Kyle McMartin <[email protected]>

WBR, Sergei

David Miller

maintainer

2013-08-29 20:34 UTC #5 lore raw

In reply to Kyle McMartin

From: Kyle McMartin <[email protected]>
Date: Thu, 29 Aug 2013 16:11:51 -0400

Quoted 7 lines

Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)
Signed-off-by Kyle McMartin <[email protected]>

Applied and queued up for -stable, thanks.

Kyle McMartin

2013-08-29 20:35 UTC #6 lore raw

Quoted 9 lines · David Miller

From: Kyle McMartin <[email protected]>
Date: Thu, 29 Aug 2013 16:11:51 -0400
Quoted 6 lines
Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)

Quoted 3 lines

Quoted 1 line
Signed-off-by Kyle McMartin <[email protected]>
Applied and queued up for -stable, thanks.

Heh, too fast David, Rob nak'd it. :)

Sorry about that.

--Kyle

David Miller

maintainer

2013-08-29 20:38 UTC #7 lore raw

From: Kyle McMartin <[email protected]>
Date: Thu, 29 Aug 2013 16:35:23 -0400

Quoted 10 lines · David Miller

Quoted 9 lines
From: Kyle McMartin <[email protected]>
Date: Thu, 29 Aug 2013 16:11:51 -0400
Quoted 6 lines
Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by Kyle McMartin <[email protected]>
Applied and queued up for -stable, thanks.
Heh, too fast David, Rob nak'd it. :)
Sorry about that.

If Rob had resubmitted his series in a timely manner this wouldn't have happened.

I reverted.

Rob Herring

2013-08-30 03:17 UTC #8 lore raw

Quoted 15 lines · David Miller

From: Kyle McMartin <[email protected]>
Date: Thu, 29 Aug 2013 16:35:23 -0400
Quoted 12 lines · David Miller
Quoted 11 lines
From: Kyle McMartin <[email protected]>
Date: Thu, 29 Aug 2013 16:11:51 -0400
Quoted 7 lines
Noticed while debugging another issue that DMA debug turned up, looks like commit ef468d23 missed a case when switching to bufsz instead of priv->dma_buf_sz? (tbf, the whole handling of DMA buffers seems slightly suspect since we're not tracking the size, but trusting the hardware between map and unmap...)
Signed-off-by Kyle McMartin <[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Applied and queued up for -stable, thanks.
Heh, too fast David, Rob nak'd it. :)
Sorry about that.
If Rob had resubmitted his series in a timely manner this wouldn't have happened.

I was giving them more time hoping to have some feedback from Lennert. Anyways, I've just resubmitted v2 which addresses Ben's comments.

Rob