b44: fix misalignment and wasted space in rx handling

PATCH Applied Felix Fietkau 3 participants 4 messages Jan 9, 2009 – Jan 10, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Felix Fietkau

author

2009-01-09 12:40 UTC #0 lore raw

PATCH v2 1/1 b44: fix misalignment and wasted space in rx handling

Broadcom 4400 puts a header of configurable size (apparently needs to be at least 28 bytes) in front of received packets. When handling this, the previous code accidentally added the offset 30 *twice* for the software and once for the hardware, thereby cancelling out the IP alignment effect of the 30 byte padding and wasting an additional 30 bytes of memory per packet.

This patch fixes this problem and improves routing throughput by about 30% on MIPS, where unaligned access is expensive.

Signed-off-by Felix Fietkau <[email protected]>

---
Sorry about the line wrapping in the previous mail. Should be fixed now

drivers/net/b44.c +6 -7

@@ -73,8 +73,8 @@
 	  (BP)->tx_cons - (BP)->tx_prod - TX_RING_GAP(BP))
 #define NEXT_TX(N)		(((N) + 1) & (B44_TX_RING_SIZE - 1))
 
-#define RX_PKT_OFFSET		30
-#define RX_PKT_BUF_SZ		(1536 + RX_PKT_OFFSET + 64)
+#define RX_PKT_OFFSET		(RX_HEADER_LEN + 2)
+#define RX_PKT_BUF_SZ		(1536 + RX_PKT_OFFSET)
 
 /* minimum number of free TX descriptors required to wake up TX process */
 #define B44_TX_WAKEUP_THRESH		(B44_TX_RING_SIZE / 4)

@@ -682,7 +682,6 @@ static int b44_alloc_rx_skb(struct b44 *
 	}
 
 	rh = (struct rx_header *) skb->data;
-	skb_reserve(skb, RX_PKT_OFFSET);
 
 	rh->len = 0;
 	rh->flags = 0;

@@ -693,13 +692,13 @@ static int b44_alloc_rx_skb(struct b44 *
 	if (src_map != NULL)
 		src_map->skb = NULL;
 
-	ctrl  = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));
+	ctrl = (DESC_CTRL_LEN & RX_PKT_BUF_SZ);
 	if (dest_idx == (B44_RX_RING_SIZE - 1))
 		ctrl |= DESC_CTRL_EOT;
 
 	dp = &bp->rx_ring[dest_idx];
 	dp->ctrl = cpu_to_le32(ctrl);
-	dp->addr = cpu_to_le32((u32) mapping + RX_PKT_OFFSET + bp->dma_offset);
+	dp->addr = cpu_to_le32((u32) mapping + bp->dma_offset);
 
 	if (bp->flags & B44_FLAG_RX_RING_HACK)
 		b44_sync_dma_desc_for_device(bp->sdev, bp->rx_ring_dma,

@@ -809,8 +808,8 @@ static int b44_rx(struct b44 *bp, int bu
 			ssb_dma_unmap_single(bp->sdev, map,
 					     skb_size, DMA_FROM_DEVICE);
 			/* Leave out rx_header */
-                	skb_put(skb, len + RX_PKT_OFFSET);
-            	        skb_pull(skb, RX_PKT_OFFSET);
+			skb_put(skb, len + RX_PKT_OFFSET);
+			skb_pull(skb, RX_PKT_OFFSET);
 		} else {
 			struct sk_buff *copy_skb;

Michael Buesch

2009-01-09 22:27 UTC #1 lore raw

Quoted 14 lines · On Friday 09 January 2009 13:39:57 Felix Fietkau wrote:

drivers/net/b44.c +2 -2

@@ -73,8 +73,8 @@
 	  (BP)->tx_cons - (BP)->tx_prod - TX_RING_GAP(BP))
 #define NEXT_TX(N)		(((N) + 1) & (B44_TX_RING_SIZE - 1))
 
-#define RX_PKT_OFFSET		30
-#define RX_PKT_BUF_SZ		(1536 + RX_PKT_OFFSET + 64)
+#define RX_PKT_OFFSET		(RX_HEADER_LEN + 2)
+#define RX_PKT_BUF_SZ		(1536 + RX_PKT_OFFSET)
 
 /* minimum number of free TX descriptors required to wake up TX process */
 #define B44_TX_WAKEUP_THRESH		(B44_TX_RING_SIZE / 4)

drivers/net/b44.c static int b44_alloc_rx_skb(struct b44 * · line 685

Quoted 4 lines

drivers/net/b44.c +0 -1

@@ -682,7 +682,6 @@ static int b44_alloc_rx_skb(struct b44 *
 	}
 
 	rh = (struct rx_header *) skb->data;
-	skb_reserve(skb, RX_PKT_OFFSET);

Looks correct.

drivers/net/b44.c static int b44_alloc_rx_skb(struct b44 * · line 695

Quoted 8 lines

drivers/net/b44.c +1 -1

 	rh->len = 0;
 	rh->flags = 0;

@@ -693,13 +692,13 @@ static int b44_alloc_rx_skb(struct b44 *
 	if (src_map != NULL)
 		src_map->skb = NULL;
 
-	ctrl  = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));
+	ctrl = (DESC_CTRL_LEN & RX_PKT_BUF_SZ);

Are you sure this is right? b43 has the same DMA engine, and we subtract the offset from the buffer size there. So we end up with the descriptor control field telling the space available for the frame payload.

So I think the correct code would be

	ctrl = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));

drivers/net/b44.c static int b44_alloc_rx_skb(struct b44 * · line 701

Quoted 7 lines

drivers/net/b44.c +1 -1

 	if (dest_idx == (B44_RX_RING_SIZE - 1))
 		ctrl |= DESC_CTRL_EOT;
 
 	dp = &bp->rx_ring[dest_idx];
 	dp->ctrl = cpu_to_le32(ctrl);
-	dp->addr = cpu_to_le32((u32) mapping + RX_PKT_OFFSET + bp->dma_offset);
+	dp->addr = cpu_to_le32((u32) mapping + bp->dma_offset);

This looks correct.

Signature · 1 line

Greetings, Michael.

Felix Fietkau

author

2009-01-09 22:55 UTC #2 lore raw

Quoted 28 lines · Michael Buesch

Quoted 14 lines · On Friday 09 January 2009 13:39:57 Felix Fietkau wrote:

drivers/net/b44.c +2 -2

@@ -73,8 +73,8 @@
 	  (BP)->tx_cons - (BP)->tx_prod - TX_RING_GAP(BP))
 #define NEXT_TX(N)		(((N) + 1) & (B44_TX_RING_SIZE - 1))
 
-#define RX_PKT_OFFSET		30
-#define RX_PKT_BUF_SZ		(1536 + RX_PKT_OFFSET + 64)
+#define RX_PKT_OFFSET		(RX_HEADER_LEN + 2)
+#define RX_PKT_BUF_SZ		(1536 + RX_PKT_OFFSET)
 
 /* minimum number of free TX descriptors required to wake up TX process */
 #define B44_TX_WAKEUP_THRESH		(B44_TX_RING_SIZE / 4)

drivers/net/b44.c static int b44_alloc_rx_skb(struct b44 * · line 685

Quoted 4 lines

drivers/net/b44.c +0 -1

@@ -682,7 +682,6 @@ static int b44_alloc_rx_skb(struct b44 *
 	}
 
 	rh = (struct rx_header *) skb->data;
-	skb_reserve(skb, RX_PKT_OFFSET);

Looks correct.

Quoted 6 lines

drivers/net/b44.c +0 -0

 	rh->len = 0;
 	rh->flags = 0;

@@ -693,13 +692,13 @@ static int b44_alloc_rx_skb(struct b44 *
 	if (src_map != NULL)
 		src_map->skb = NULL;

Quoted 6 lines

drivers/net/b44.c static int b44_alloc_rx_skb(struct b44 * · line 695
Quoted 2 lines
drivers/net/b44.c +1 -1
-	ctrl  = (DESC_CTRL_LEN & (RX_PKT_BUF_SZ - RX_PKT_OFFSET));
+	ctrl = (DESC_CTRL_LEN & RX_PKT_BUF_SZ);
Are you sure this is right? b43 has the same DMA engine, and we subtract the offset from the buffer size there. So we end up with the descriptor control field telling the space available for the frame payload.

Quote from a Datasheet for the BCM5365 that I found online:

"This field is the length, in bytes, of the data buffer associated with this descriptor. A descriptor with a BufCount value greater than 0x1000 causes a descriptor protocol error."

I checked the public hnddma.c code and it also appears to not subtract the packet offset either, so I think my version is correct.

- Felix

David Miller

maintainer Applied

2009-01-10 07:08 UTC #3 lore raw

In reply to Felix Fietkau

From: Felix Fietkau <[email protected]>
Date: Fri, 09 Jan 2009 13:39:57 +0100

Quoted 11 lines

Broadcom 4400 puts a header of configurable size (apparently needs to be at least 28 bytes) in front of received packets. When handling this, the previous code accidentally added the offset 30 *twice* for the software and once for the hardware, thereby cancelling out the IP alignment effect of the 30 byte padding and wasting an additional 30 bytes of memory per packet.
This patch fixes this problem and improves routing throughput by about 30% on MIPS, where unaligned access is expensive.
Signed-off-by Felix Fietkau <[email protected]>

Applied.