All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 1/9] bgmac: simplify tx ring index handling
@ 2015-04-13 23:42 Felix Fietkau
  2015-04-13 23:42 ` [PATCH v6 2/9] bgmac: leave interrupts disabled as long as there is work to do Felix Fietkau
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

Keep incrementing ring->start and ring->end instead of pointing it to
the actual ring slot entry. This simplifies the calculation of the
number of free slots.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 44 ++++++++++++++---------------------
 drivers/net/ethernet/broadcom/bgmac.h |  6 ++---
 2 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index fa8f9e1..73374b4 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -142,11 +142,10 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 {
 	struct device *dma_dev = bgmac->core->dma_dev;
 	struct net_device *net_dev = bgmac->net_dev;
-	struct bgmac_slot_info *slot = &ring->slots[ring->end];
-	int free_slots;
+	int index = ring->end % BGMAC_TX_RING_SLOTS;
+	struct bgmac_slot_info *slot = &ring->slots[index];
 	int nr_frags;
 	u32 flags;
-	int index = ring->end;
 	int i;
 
 	if (skb->len > BGMAC_DESC_CTL1_LEN) {
@@ -158,13 +157,7 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 		skb_checksum_help(skb);
 
 	nr_frags = skb_shinfo(skb)->nr_frags;
-
-	if (ring->start <= ring->end)
-		free_slots = ring->start - ring->end + BGMAC_TX_RING_SLOTS;
-	else
-		free_slots = ring->start - ring->end;
-
-	if (free_slots <= nr_frags + 1) {
+	if (ring->end - ring->start + nr_frags + 1 >= BGMAC_TX_RING_SLOTS) {
 		bgmac_err(bgmac, "TX ring is full, queue should be stopped!\n");
 		netif_stop_queue(net_dev);
 		return NETDEV_TX_BUSY;
@@ -200,7 +193,7 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 	}
 
 	slot->skb = skb;
-
+	ring->end += nr_frags + 1;
 	netdev_sent_queue(net_dev, skb->len);
 
 	wmb();
@@ -208,13 +201,12 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 	/* Increase ring->end to point empty slot. We tell hardware the first
 	 * slot it should *not* read.
 	 */
-	ring->end = (index + 1) % BGMAC_TX_RING_SLOTS;
 	bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_TX_INDEX,
 		    ring->index_base +
-		    ring->end * sizeof(struct bgmac_dma_desc));
+		    (ring->end % BGMAC_TX_RING_SLOTS) *
+		    sizeof(struct bgmac_dma_desc));
 
-	free_slots -= nr_frags + 1;
-	if (free_slots < 8)
+	if (ring->end - ring->start >= BGMAC_TX_RING_SLOTS - 8)
 		netif_stop_queue(net_dev);
 
 	return NETDEV_TX_OK;
@@ -256,17 +248,17 @@ static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring)
 	empty_slot &= BGMAC_DMA_TX_STATDPTR;
 	empty_slot /= sizeof(struct bgmac_dma_desc);
 
-	while (ring->start != empty_slot) {
-		struct bgmac_slot_info *slot = &ring->slots[ring->start];
-		u32 ctl1 = le32_to_cpu(ring->cpu_base[ring->start].ctl1);
-		int len = ctl1 & BGMAC_DESC_CTL1_LEN;
+	while (ring->start != ring->end) {
+		int slot_idx = ring->start % BGMAC_TX_RING_SLOTS;
+		struct bgmac_slot_info *slot = &ring->slots[slot_idx];
+		u32 ctl1;
+		int len;
 
-		if (!slot->dma_addr) {
-			bgmac_err(bgmac, "Hardware reported transmission for empty TX ring slot %d! End of ring: %d\n",
-				  ring->start, ring->end);
-			goto next;
-		}
+		if (slot_idx == empty_slot)
+			break;
 
+		ctl1 = le32_to_cpu(ring->cpu_base[slot_idx].ctl1);
+		len = ctl1 & BGMAC_DESC_CTL1_LEN;
 		if (ctl1 & BGMAC_DESC_CTL0_SOF)
 			/* Unmap no longer used buffer */
 			dma_unmap_single(dma_dev, slot->dma_addr, len,
@@ -284,10 +276,8 @@ static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring)
 			slot->skb = NULL;
 		}
 
-next:
 		slot->dma_addr = 0;
-		if (++ring->start >= BGMAC_TX_RING_SLOTS)
-			ring->start = 0;
+		ring->start++;
 		freed = true;
 	}
 
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 3ad965f..5a198d5 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -414,10 +414,10 @@ enum bgmac_dma_ring_type {
  * empty.
  */
 struct bgmac_dma_ring {
-	u16 num_slots;
-	u16 start;
-	u16 end;
+	u32 start;
+	u32 end;
 
+	u16 num_slots;
 	u16 mmio_base;
 	struct bgmac_dma_desc *cpu_base;
 	dma_addr_t dma_base;
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 2/9] bgmac: leave interrupts disabled as long as there is work to do
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-14  5:40   ` Rafał Miłecki
  2015-04-13 23:42 ` [PATCH v6 3/9] bgmac: set received skb headroom to NET_SKB_PAD Felix Fietkau
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

Always poll rx and tx during NAPI poll instead of relying on the status
of the first interrupt. This prevents bgmac_poll from leaving unfinished
work around until the next IRQ.
In my tests this makes bridging/routing throughput under heavy load more
stable and ensures that no new IRQs arrive as long as bgmac_poll uses up
the entire budget.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 31 ++++++++++---------------------
 drivers/net/ethernet/broadcom/bgmac.h |  1 -
 2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 73374b4..30464d0 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1105,8 +1105,6 @@ static void bgmac_chip_reset(struct bgmac *bgmac)
 	bgmac_phy_init(bgmac);
 
 	netdev_reset_queue(bgmac->net_dev);
-
-	bgmac->int_status = 0;
 }
 
 static void bgmac_chip_intrs_on(struct bgmac *bgmac)
@@ -1221,14 +1219,13 @@ static irqreturn_t bgmac_interrupt(int irq, void *dev_id)
 	if (!int_status)
 		return IRQ_NONE;
 
-	/* Ack */
-	bgmac_write(bgmac, BGMAC_INT_STATUS, int_status);
+	int_status &= ~(BGMAC_IS_TX0 | BGMAC_IS_RX);
+	if (int_status)
+		bgmac_err(bgmac, "Unknown IRQs: 0x%08X\n", int_status);
 
 	/* Disable new interrupts until handling existing ones */
 	bgmac_chip_intrs_off(bgmac);
 
-	bgmac->int_status = int_status;
-
 	napi_schedule(&bgmac->napi);
 
 	return IRQ_HANDLED;
@@ -1237,25 +1234,17 @@ static irqreturn_t bgmac_interrupt(int irq, void *dev_id)
 static int bgmac_poll(struct napi_struct *napi, int weight)
 {
 	struct bgmac *bgmac = container_of(napi, struct bgmac, napi);
-	struct bgmac_dma_ring *ring;
 	int handled = 0;
 
-	if (bgmac->int_status & BGMAC_IS_TX0) {
-		ring = &bgmac->tx_ring[0];
-		bgmac_dma_tx_free(bgmac, ring);
-		bgmac->int_status &= ~BGMAC_IS_TX0;
-	}
+	/* Ack */
+	bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);
 
-	if (bgmac->int_status & BGMAC_IS_RX) {
-		ring = &bgmac->rx_ring[0];
-		handled += bgmac_dma_rx_read(bgmac, ring, weight);
-		bgmac->int_status &= ~BGMAC_IS_RX;
-	}
+	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0]);
+	handled += bgmac_dma_rx_read(bgmac, &bgmac->rx_ring[0], weight);
 
-	if (bgmac->int_status) {
-		bgmac_err(bgmac, "Unknown IRQs: 0x%08X\n", bgmac->int_status);
-		bgmac->int_status = 0;
-	}
+	/* Poll again if more events arrived in the meantime */
+	if (bgmac_read(bgmac, BGMAC_INT_STATUS) & (BGMAC_IS_TX0 | BGMAC_IS_RX))
+		return handled;
 
 	if (handled < weight) {
 		napi_complete(napi);
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 5a198d5..abd50d1 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -452,7 +452,6 @@ struct bgmac {
 
 	/* Int */
 	u32 int_mask;
-	u32 int_status;
 
 	/* Current MAC state */
 	int mac_speed;
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 3/9] bgmac: set received skb headroom to NET_SKB_PAD
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
  2015-04-13 23:42 ` [PATCH v6 2/9] bgmac: leave interrupts disabled as long as there is work to do Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-13 23:42 ` [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling Felix Fietkau
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

A packet buffer offset of 30 bytes is inefficient, because the first 2
bytes end up in a different cacheline.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 14 ++++++++------
 drivers/net/ethernet/broadcom/bgmac.h |  4 +++-
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 30464d0..9a6742e 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -342,13 +342,13 @@ static int bgmac_dma_rx_skb_for_slot(struct bgmac *bgmac,
 		return -ENOMEM;
 
 	/* Poison - if everything goes fine, hardware will overwrite it */
-	rx = buf;
+	rx = buf + BGMAC_RX_BUF_OFFSET;
 	rx->len = cpu_to_le16(0xdead);
 	rx->flags = cpu_to_le16(0xbeef);
 
 	/* Map skb for the DMA */
-	dma_addr = dma_map_single(dma_dev, buf, BGMAC_RX_BUF_SIZE,
-				  DMA_FROM_DEVICE);
+	dma_addr = dma_map_single(dma_dev, buf + BGMAC_RX_BUF_OFFSET,
+				  BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
 	if (dma_mapping_error(dma_dev, dma_addr)) {
 		bgmac_err(bgmac, "DMA mapping error\n");
 		put_page(virt_to_head_page(buf));
@@ -399,7 +399,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 	while (ring->start != ring->end) {
 		struct device *dma_dev = bgmac->core->dma_dev;
 		struct bgmac_slot_info *slot = &ring->slots[ring->start];
-		struct bgmac_rx_header *rx = slot->buf;
+		struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET;
 		struct sk_buff *skb;
 		void *buf = slot->buf;
 		u16 len, flags;
@@ -450,8 +450,10 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 					 BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
 
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
-			skb_put(skb, BGMAC_RX_FRAME_OFFSET + len);
-			skb_pull(skb, BGMAC_RX_FRAME_OFFSET);
+			skb_put(skb, BGMAC_RX_FRAME_OFFSET +
+				BGMAC_RX_BUF_OFFSET + len);
+			skb_pull(skb, BGMAC_RX_FRAME_OFFSET +
+				 BGMAC_RX_BUF_OFFSET);
 
 			skb_checksum_none_assert(skb);
 			skb->protocol = eth_type_trans(skb, bgmac->net_dev);
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index abd50d1..3db0d52 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -360,9 +360,11 @@
 
 #define BGMAC_RX_HEADER_LEN			28		/* Last 24 bytes are unused. Well... */
 #define BGMAC_RX_FRAME_OFFSET			30		/* There are 2 unused bytes between header and real data */
+#define BGMAC_RX_BUF_OFFSET			(NET_SKB_PAD + NET_IP_ALIGN - \
+						 BGMAC_RX_FRAME_OFFSET)
 #define BGMAC_RX_MAX_FRAME_SIZE			1536		/* Copied from b44/tg3 */
 #define BGMAC_RX_BUF_SIZE			(BGMAC_RX_FRAME_OFFSET + BGMAC_RX_MAX_FRAME_SIZE)
-#define BGMAC_RX_ALLOC_SIZE			(SKB_DATA_ALIGN(BGMAC_RX_BUF_SIZE) + \
+#define BGMAC_RX_ALLOC_SIZE			(SKB_DATA_ALIGN(BGMAC_RX_BUF_SIZE + BGMAC_RX_BUF_OFFSET) + \
 						 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
 
 #define BGMAC_BFL_ENETROBO			0x0010		/* has ephy roboswitch spi */
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
  2015-04-13 23:42 ` [PATCH v6 2/9] bgmac: leave interrupts disabled as long as there is work to do Felix Fietkau
  2015-04-13 23:42 ` [PATCH v6 3/9] bgmac: set received skb headroom to NET_SKB_PAD Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-14  5:55   ` Rafał Miłecki
  2015-04-13 23:42 ` [PATCH v6 5/9] bgmac: add check for oversized packets Felix Fietkau
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

Allocate a new buffer before processing the completed one. If allocation
fails, reuse the old buffer.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 70 +++++++++++++++++------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 9a6742e..f897e62 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -382,6 +382,19 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac,
 	dma_desc->ctl1 = cpu_to_le32(ctl1);
 }
 
+static void bgmac_dma_rx_poison_buf(struct device *dma_dev,
+				    struct bgmac_slot_info *slot)
+{
+	struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET;
+
+	dma_sync_single_for_cpu(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
+				DMA_FROM_DEVICE);
+	rx->len = cpu_to_le16(0xdead);
+	rx->flags = cpu_to_le16(0xdead);
+	dma_sync_single_for_device(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
+				   DMA_FROM_DEVICE);
+}
+
 static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			     int weight)
 {
@@ -404,51 +417,32 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 		void *buf = slot->buf;
 		u16 len, flags;
 
-		/* Unmap buffer to make it accessible to the CPU */
-		dma_sync_single_for_cpu(dma_dev, slot->dma_addr,
-					BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
+		do {
+			/* Prepare new skb as replacement */
+			if (bgmac_dma_rx_skb_for_slot(bgmac, slot)) {
+				bgmac_dma_rx_poison_buf(dma_dev, slot);
+				break;
+			}
 
-		/* Get info from the header */
-		len = le16_to_cpu(rx->len);
-		flags = le16_to_cpu(rx->flags);
+			/* Unmap buffer to make it accessible to the CPU */
+			dma_unmap_single(dma_dev, slot->dma_addr,
+					 BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
 
-		do {
-			dma_addr_t old_dma_addr = slot->dma_addr;
-			int err;
+			/* Get info from the header */
+			len = le16_to_cpu(rx->len);
+			flags = le16_to_cpu(rx->flags);
 
 			/* Check for poison and drop or pass the packet */
 			if (len == 0xdead && flags == 0xbeef) {
 				bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n",
 					  ring->start);
-				dma_sync_single_for_device(dma_dev,
-							   slot->dma_addr,
-							   BGMAC_RX_BUF_SIZE,
-							   DMA_FROM_DEVICE);
+				put_page(virt_to_head_page(buf));
 				break;
 			}
 
 			/* Omit CRC. */
 			len -= ETH_FCS_LEN;
 
-			/* Prepare new skb as replacement */
-			err = bgmac_dma_rx_skb_for_slot(bgmac, slot);
-			if (err) {
-				/* Poison the old skb */
-				rx->len = cpu_to_le16(0xdead);
-				rx->flags = cpu_to_le16(0xbeef);
-
-				dma_sync_single_for_device(dma_dev,
-							   slot->dma_addr,
-							   BGMAC_RX_BUF_SIZE,
-							   DMA_FROM_DEVICE);
-				break;
-			}
-			bgmac_dma_rx_setup_desc(bgmac, ring, ring->start);
-
-			/* Unmap old skb, we'll pass it to the netfif */
-			dma_unmap_single(dma_dev, old_dma_addr,
-					 BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
-
 			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
 			skb_put(skb, BGMAC_RX_FRAME_OFFSET +
 				BGMAC_RX_BUF_OFFSET + len);
@@ -461,6 +455,8 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			handled++;
 		} while (0);
 
+		bgmac_dma_rx_setup_desc(bgmac, ring, ring->start);
+
 		if (++ring->start >= BGMAC_RX_RING_SLOTS)
 			ring->start = 0;
 
@@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
 
 	for (i = 0; i < ring->num_slots; i++) {
 		slot = &ring->slots[i];
-		if (!slot->buf)
+		if (!slot->dma_addr)
 			continue;
 
-		if (slot->dma_addr)
-			dma_unmap_single(dma_dev, slot->dma_addr,
-					 BGMAC_RX_BUF_SIZE,
-					 DMA_FROM_DEVICE);
+		dma_unmap_single(dma_dev, slot->dma_addr,
+				 BGMAC_RX_BUF_SIZE,
+				 DMA_FROM_DEVICE);
 		put_page(virt_to_head_page(slot->buf));
+		slot->dma_addr = 0;
 	}
 }
 
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 5/9] bgmac: add check for oversized packets
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
                   ` (2 preceding siblings ...)
  2015-04-13 23:42 ` [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-14  5:56   ` Rafał Miłecki
  2015-04-13 23:42 ` [PATCH v6 6/9] bgmac: increase rx ring size from 511 to 512 Felix Fietkau
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

In very rare cases, the MAC can catch an internal buffer that is bigger
than it's supposed to be. Instead of crashing the kernel, simply pass
the buffer back to the hardware

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index f897e62..eafdbca 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -440,6 +440,13 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 				break;
 			}
 
+			if (len > BGMAC_RX_ALLOC_SIZE) {
+				bgmac_err(bgmac, "Found oversized packet at slot %d, DMA issue!\n",
+					  ring->start);
+				put_page(virt_to_head_page(buf));
+				break;
+			}
+
 			/* Omit CRC. */
 			len -= ETH_FCS_LEN;
 
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 6/9] bgmac: increase rx ring size from 511 to 512
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
                   ` (3 preceding siblings ...)
  2015-04-13 23:42 ` [PATCH v6 5/9] bgmac: add check for oversized packets Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-14  5:57   ` Rafał Miłecki
  2015-04-13 23:42 ` [PATCH v6 7/9] bgmac: simplify dma init/cleanup Felix Fietkau
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

Limiting it to 511 looks like a failed attempt at leaving one descriptor
empty to allow the hardware to stop processing a buffer that has not
been prepared yet. However, this doesn't work because this affects the
total ring size as well

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 3db0d52..e45e303 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -356,7 +356,7 @@
 #define BGMAC_MAX_RX_RINGS			1
 
 #define BGMAC_TX_RING_SLOTS			128
-#define BGMAC_RX_RING_SLOTS			512 - 1		/* Why -1? Well, Broadcom does that... */
+#define BGMAC_RX_RING_SLOTS			512
 
 #define BGMAC_RX_HEADER_LEN			28		/* Last 24 bytes are unused. Well... */
 #define BGMAC_RX_FRAME_OFFSET			30		/* There are 2 unused bytes between header and real data */
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 7/9] bgmac: simplify dma init/cleanup
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
                   ` (4 preceding siblings ...)
  2015-04-13 23:42 ` [PATCH v6 6/9] bgmac: increase rx ring size from 511 to 512 Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-14  7:24   ` Rafał Miłecki
  2015-04-13 23:42 ` [PATCH v6 8/9] bgmac: fix DMA rx corruption Felix Fietkau
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

Instead of allocating buffers at device init time and initializing
descriptors at device open, do both at the same time (during open).
Free all buffers when closing the device.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 70 +++++++++++++++++------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index eafdbca..aad80a7 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -557,18 +557,26 @@ static void bgmac_dma_ring_desc_free(struct bgmac *bgmac,
 			  ring->dma_base);
 }
 
-static void bgmac_dma_free(struct bgmac *bgmac)
+static void bgmac_dma_cleanup(struct bgmac *bgmac)
 {
 	int i;
 
-	for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
+	for (i = 0; i < BGMAC_MAX_TX_RINGS; i++)
 		bgmac_dma_tx_ring_free(bgmac, &bgmac->tx_ring[i]);
-		bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i]);
-	}
-	for (i = 0; i < BGMAC_MAX_RX_RINGS; i++) {
+
+	for (i = 0; i < BGMAC_MAX_RX_RINGS; i++)
 		bgmac_dma_rx_ring_free(bgmac, &bgmac->rx_ring[i]);
+}
+
+static void bgmac_dma_free(struct bgmac *bgmac)
+{
+	int i;
+
+	for (i = 0; i < BGMAC_MAX_TX_RINGS; i++)
+		bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i]);
+
+	for (i = 0; i < BGMAC_MAX_RX_RINGS; i++)
 		bgmac_dma_ring_desc_free(bgmac, &bgmac->rx_ring[i]);
-	}
 }
 
 static int bgmac_dma_alloc(struct bgmac *bgmac)
@@ -616,8 +624,6 @@ static int bgmac_dma_alloc(struct bgmac *bgmac)
 	}
 
 	for (i = 0; i < BGMAC_MAX_RX_RINGS; i++) {
-		int j;
-
 		ring = &bgmac->rx_ring[i];
 		ring->num_slots = BGMAC_RX_RING_SLOTS;
 		ring->mmio_base = ring_base[i];
@@ -640,15 +646,6 @@ static int bgmac_dma_alloc(struct bgmac *bgmac)
 			ring->index_base = lower_32_bits(ring->dma_base);
 		else
 			ring->index_base = 0;
-
-		/* Alloc RX slots */
-		for (j = 0; j < ring->num_slots; j++) {
-			err = bgmac_dma_rx_skb_for_slot(bgmac, &ring->slots[j]);
-			if (err) {
-				bgmac_err(bgmac, "Can't allocate skb for slot in RX ring\n");
-				goto err_dma_free;
-			}
-		}
 	}
 
 	return 0;
@@ -658,10 +655,10 @@ err_dma_free:
 	return -ENOMEM;
 }
 
-static void bgmac_dma_init(struct bgmac *bgmac)
+static int bgmac_dma_init(struct bgmac *bgmac)
 {
 	struct bgmac_dma_ring *ring;
-	int i;
+	int i, err;
 
 	for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
 		ring = &bgmac->tx_ring[i];
@@ -693,8 +690,13 @@ static void bgmac_dma_init(struct bgmac *bgmac)
 		if (ring->unaligned)
 			bgmac_dma_rx_enable(bgmac, ring);
 
-		for (j = 0; j < ring->num_slots; j++)
+		for (j = 0; j < ring->num_slots; j++) {
+			err = bgmac_dma_rx_skb_for_slot(bgmac, &ring->slots[j]);
+			if (err)
+				return err;
+
 			bgmac_dma_rx_setup_desc(bgmac, ring, j);
+		}
 
 		bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX,
 			    ring->index_base +
@@ -703,6 +705,8 @@ static void bgmac_dma_init(struct bgmac *bgmac)
 		ring->start = 0;
 		ring->end = 0;
 	}
+
+	return 0;
 }
 
 /**************************************************
@@ -1178,11 +1182,8 @@ static void bgmac_enable(struct bgmac *bgmac)
 }
 
 /* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipinit */
-static void bgmac_chip_init(struct bgmac *bgmac, bool full_init)
+static void bgmac_chip_init(struct bgmac *bgmac)
 {
-	struct bgmac_dma_ring *ring;
-	int i;
-
 	/* 1 interrupt per received frame */
 	bgmac_write(bgmac, BGMAC_INT_RECV_LAZY, 1 << BGMAC_IRL_FC_SHIFT);
 
@@ -1200,16 +1201,7 @@ static void bgmac_chip_init(struct bgmac *bgmac, bool full_init)
 
 	bgmac_write(bgmac, BGMAC_RXMAX_LENGTH, 32 + ETHER_MAX_LEN);
 
-	if (full_init) {
-		bgmac_dma_init(bgmac);
-		if (1) /* FIXME: is there any case we don't want IRQs? */
-			bgmac_chip_intrs_on(bgmac);
-	} else {
-		for (i = 0; i < BGMAC_MAX_RX_RINGS; i++) {
-			ring = &bgmac->rx_ring[i];
-			bgmac_dma_rx_enable(bgmac, ring);
-		}
-	}
+	bgmac_chip_intrs_on(bgmac);
 
 	bgmac_enable(bgmac);
 }
@@ -1269,8 +1261,13 @@ static int bgmac_open(struct net_device *net_dev)
 	int err = 0;
 
 	bgmac_chip_reset(bgmac);
+
+	err = bgmac_dma_init(bgmac);
+	if (err)
+		goto err_out;
+
 	/* Specs say about reclaiming rings here, but we do that in DMA init */
-	bgmac_chip_init(bgmac, true);
+	bgmac_chip_init(bgmac);
 
 	err = request_irq(bgmac->core->irq, bgmac_interrupt, IRQF_SHARED,
 			  KBUILD_MODNAME, net_dev);
@@ -1283,8 +1280,10 @@ static int bgmac_open(struct net_device *net_dev)
 	phy_start(bgmac->phy_dev);
 
 	netif_carrier_on(net_dev);
+	return 0;
 
 err_out:
+	bgmac_dma_cleanup(bgmac);
 	return err;
 }
 
@@ -1301,6 +1300,7 @@ static int bgmac_stop(struct net_device *net_dev)
 	free_irq(bgmac->core->irq, net_dev);
 
 	bgmac_chip_reset(bgmac);
+	bgmac_dma_cleanup(bgmac);
 
 	return 0;
 }
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 8/9] bgmac: fix DMA rx corruption
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
                   ` (5 preceding siblings ...)
  2015-04-13 23:42 ` [PATCH v6 7/9] bgmac: simplify dma init/cleanup Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-13 23:42 ` [PATCH v6 9/9] bgmac: drop ring->num_slots Felix Fietkau
  2015-04-14  5:38 ` [PATCH v6 1/9] bgmac: simplify tx ring index handling Rafał Miłecki
  8 siblings, 0 replies; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

The driver needs to inform the hardware about the first invalid (not yet
filled) rx slot, by writing its DMA descriptor pointer offset to the
BGMAC_DMA_RX_INDEX register.

This register was set to a value exceeding the rx ring size, effectively
allowing the hardware constant access to the full ring, regardless of
which slots are initialized.

To fix this issue, always mark the last filled rx slot as invalid.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index aad80a7..a07a62e 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -362,6 +362,16 @@ static int bgmac_dma_rx_skb_for_slot(struct bgmac *bgmac,
 	return 0;
 }
 
+static void bgmac_dma_rx_update_index(struct bgmac *bgmac,
+				      struct bgmac_dma_ring *ring)
+{
+	dma_wmb();
+
+	bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX,
+		    ring->index_base +
+		    ring->end * sizeof(struct bgmac_dma_desc));
+}
+
 static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac,
 				    struct bgmac_dma_ring *ring, int desc_idx)
 {
@@ -380,6 +390,8 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac,
 	dma_desc->addr_high = cpu_to_le32(upper_32_bits(ring->slots[desc_idx].dma_addr));
 	dma_desc->ctl0 = cpu_to_le32(ctl0);
 	dma_desc->ctl1 = cpu_to_le32(ctl1);
+
+	ring->end = desc_idx;
 }
 
 static void bgmac_dma_rx_poison_buf(struct device *dma_dev,
@@ -407,9 +419,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 	end_slot &= BGMAC_DMA_RX_STATDPTR;
 	end_slot /= sizeof(struct bgmac_dma_desc);
 
-	ring->end = end_slot;
-
-	while (ring->start != ring->end) {
+	while (ring->start != end_slot) {
 		struct device *dma_dev = bgmac->core->dma_dev;
 		struct bgmac_slot_info *slot = &ring->slots[ring->start];
 		struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET;
@@ -471,6 +481,8 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			break;
 	}
 
+	bgmac_dma_rx_update_index(bgmac, ring);
+
 	return handled;
 }
 
@@ -690,6 +702,8 @@ static int bgmac_dma_init(struct bgmac *bgmac)
 		if (ring->unaligned)
 			bgmac_dma_rx_enable(bgmac, ring);
 
+		ring->start = 0;
+		ring->end = 0;
 		for (j = 0; j < ring->num_slots; j++) {
 			err = bgmac_dma_rx_skb_for_slot(bgmac, &ring->slots[j]);
 			if (err)
@@ -698,12 +712,7 @@ static int bgmac_dma_init(struct bgmac *bgmac)
 			bgmac_dma_rx_setup_desc(bgmac, ring, j);
 		}
 
-		bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX,
-			    ring->index_base +
-			    ring->num_slots * sizeof(struct bgmac_dma_desc));
-
-		ring->start = 0;
-		ring->end = 0;
+		bgmac_dma_rx_update_index(bgmac, ring);
 	}
 
 	return 0;
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v6 9/9] bgmac: drop ring->num_slots
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
                   ` (6 preceding siblings ...)
  2015-04-13 23:42 ` [PATCH v6 8/9] bgmac: fix DMA rx corruption Felix Fietkau
@ 2015-04-13 23:42 ` Felix Fietkau
  2015-04-14  6:38   ` Rafał Miłecki
  2015-04-14  5:38 ` [PATCH v6 1/9] bgmac: simplify tx ring index handling Rafał Miłecki
  8 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2015-04-13 23:42 UTC (permalink / raw)
  To: netdev; +Cc: zajec5, hauke

The ring size is always known at compile time, so make the code a bit
more efficient

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/ethernet/broadcom/bgmac.c | 27 ++++++++++++++-------------
 drivers/net/ethernet/broadcom/bgmac.h |  3 +--
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index a07a62e..30968e8 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -123,7 +123,7 @@ bgmac_dma_tx_add_buf(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 	struct bgmac_dma_desc *dma_desc;
 	u32 ctl1;
 
-	if (i == ring->num_slots - 1)
+	if (i == BGMAC_TX_RING_SLOTS - 1)
 		ctl0 |= BGMAC_DESC_CTL0_EOT;
 
 	ctl1 = len & BGMAC_DESC_CTL1_LEN;
@@ -378,7 +378,7 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac,
 	struct bgmac_dma_desc *dma_desc = ring->cpu_base + desc_idx;
 	u32 ctl0 = 0, ctl1 = 0;
 
-	if (desc_idx == ring->num_slots - 1)
+	if (desc_idx == BGMAC_RX_RING_SLOTS - 1)
 		ctl0 |= BGMAC_DESC_CTL0_EOT;
 	ctl1 |= BGMAC_RX_BUF_SIZE & BGMAC_DESC_CTL1_LEN;
 	/* Is there any BGMAC device that requires extension? */
@@ -516,7 +516,7 @@ static void bgmac_dma_tx_ring_free(struct bgmac *bgmac,
 	struct bgmac_slot_info *slot;
 	int i;
 
-	for (i = 0; i < ring->num_slots; i++) {
+	for (i = 0; i < BGMAC_TX_RING_SLOTS; i++) {
 		int len = dma_desc[i].ctl1 & BGMAC_DESC_CTL1_LEN;
 
 		slot = &ring->slots[i];
@@ -541,7 +541,7 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
 	struct bgmac_slot_info *slot;
 	int i;
 
-	for (i = 0; i < ring->num_slots; i++) {
+	for (i = 0; i < BGMAC_RX_RING_SLOTS; i++) {
 		slot = &ring->slots[i];
 		if (!slot->dma_addr)
 			continue;
@@ -555,7 +555,8 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
 }
 
 static void bgmac_dma_ring_desc_free(struct bgmac *bgmac,
-				     struct bgmac_dma_ring *ring)
+				     struct bgmac_dma_ring *ring,
+				     int num_slots)
 {
 	struct device *dma_dev = bgmac->core->dma_dev;
 	int size;
@@ -564,7 +565,7 @@ static void bgmac_dma_ring_desc_free(struct bgmac *bgmac,
 	    return;
 
 	/* Free ring of descriptors */
-	size = ring->num_slots * sizeof(struct bgmac_dma_desc);
+	size = num_slots * sizeof(struct bgmac_dma_desc);
 	dma_free_coherent(dma_dev, size, ring->cpu_base,
 			  ring->dma_base);
 }
@@ -585,10 +586,12 @@ static void bgmac_dma_free(struct bgmac *bgmac)
 	int i;
 
 	for (i = 0; i < BGMAC_MAX_TX_RINGS; i++)
-		bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i]);
+		bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i],
+					 BGMAC_TX_RING_SLOTS);
 
 	for (i = 0; i < BGMAC_MAX_RX_RINGS; i++)
-		bgmac_dma_ring_desc_free(bgmac, &bgmac->rx_ring[i]);
+		bgmac_dma_ring_desc_free(bgmac, &bgmac->rx_ring[i],
+					 BGMAC_RX_RING_SLOTS);
 }
 
 static int bgmac_dma_alloc(struct bgmac *bgmac)
@@ -611,11 +614,10 @@ static int bgmac_dma_alloc(struct bgmac *bgmac)
 
 	for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
 		ring = &bgmac->tx_ring[i];
-		ring->num_slots = BGMAC_TX_RING_SLOTS;
 		ring->mmio_base = ring_base[i];
 
 		/* Alloc ring of descriptors */
-		size = ring->num_slots * sizeof(struct bgmac_dma_desc);
+		size = BGMAC_TX_RING_SLOTS * sizeof(struct bgmac_dma_desc);
 		ring->cpu_base = dma_zalloc_coherent(dma_dev, size,
 						     &ring->dma_base,
 						     GFP_KERNEL);
@@ -637,11 +639,10 @@ static int bgmac_dma_alloc(struct bgmac *bgmac)
 
 	for (i = 0; i < BGMAC_MAX_RX_RINGS; i++) {
 		ring = &bgmac->rx_ring[i];
-		ring->num_slots = BGMAC_RX_RING_SLOTS;
 		ring->mmio_base = ring_base[i];
 
 		/* Alloc ring of descriptors */
-		size = ring->num_slots * sizeof(struct bgmac_dma_desc);
+		size = BGMAC_RX_RING_SLOTS * sizeof(struct bgmac_dma_desc);
 		ring->cpu_base = dma_zalloc_coherent(dma_dev, size,
 						     &ring->dma_base,
 						     GFP_KERNEL);
@@ -704,7 +705,7 @@ static int bgmac_dma_init(struct bgmac *bgmac)
 
 		ring->start = 0;
 		ring->end = 0;
-		for (j = 0; j < ring->num_slots; j++) {
+		for (j = 0; j < BGMAC_RX_RING_SLOTS; j++) {
 			err = bgmac_dma_rx_skb_for_slot(bgmac, &ring->slots[j]);
 			if (err)
 				return err;
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index e45e303..db27feb 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -419,11 +419,10 @@ struct bgmac_dma_ring {
 	u32 start;
 	u32 end;
 
-	u16 num_slots;
-	u16 mmio_base;
 	struct bgmac_dma_desc *cpu_base;
 	dma_addr_t dma_base;
 	u32 index_base; /* Used for unaligned rings only, otherwise 0 */
+	u16 mmio_base;
 	bool unaligned;
 
 	struct bgmac_slot_info slots[BGMAC_RX_RING_SLOTS];
-- 
2.2.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/9] bgmac: simplify tx ring index handling
  2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
                   ` (7 preceding siblings ...)
  2015-04-13 23:42 ` [PATCH v6 9/9] bgmac: drop ring->num_slots Felix Fietkau
@ 2015-04-14  5:38 ` Rafał Miłecki
  8 siblings, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  5:38 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> @@ -158,13 +157,7 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
>                 skb_checksum_help(skb);
>
>         nr_frags = skb_shinfo(skb)->nr_frags;
> -
> -       if (ring->start <= ring->end)
> -               free_slots = ring->start - ring->end + BGMAC_TX_RING_SLOTS;
> -       else
> -               free_slots = ring->start - ring->end;
> -
> -       if (free_slots <= nr_frags + 1) {
> +       if (ring->end - ring->start + nr_frags + 1 >= BGMAC_TX_RING_SLOTS) {
>                 bgmac_err(bgmac, "TX ring is full, queue should be stopped!\n");
>                 netif_stop_queue(net_dev);
>                 return NETDEV_TX_BUSY;

Regarding my previous report if following problem:
ring->end:0x00 ring->start:0xfd (ring->end - ring->start + nr_frags + 1):-252
ring->end:0x00 ring->start:0xff (ring->end - ring->start + nr_frags + 1):-254
ring->end:0x00 ring->start:0xff (ring->end - ring->start + nr_frags + 1):-254
ring->end:0x00 ring->start:0xff (ring->end - ring->start + nr_frags + 1):-254
ring->end:0x00 ring->start:0xfe (ring->end - ring->start + nr_frags + 1):-253
ring->end:0x00 ring->start:0xff (ring->end - ring->start + nr_frags + 1):-254
ring->end:0x00 ring->start:0xfe (ring->end - ring->start + nr_frags + 1):-253
ring->end:0x00 ring->start:0xfd (ring->end - ring->start + nr_frags + 1):-252
ring->end:0x00 ring->start:0xfd (ring->end - ring->start + nr_frags + 1):-252
(with hacked *_RING_SLOTS and u8 start, end;)

My hacked code didn't work, because using u8 killed your trick with
using the way of treating numbers (32b limit) in condition.

When using u32 your trick seems to work:
bgmac: [bgmac_dma_tx_add] ring->end:0x00000000 ring->start:0xfffffff9
bgmac: [bgmac_dma_tx_add] (ring->end - ring->start + nr_frags + 1):8

bgmac: [bgmac_dma_tx_add] ring->end:0x00000001 ring->start:0xfffffff9
bgmac: [bgmac_dma_tx_add] (ring->end - ring->start + nr_frags + 1):9

bgmac: [bgmac_dma_tx_add] ring->end:0x00000002 ring->start:0xfffffff9
bgmac: [bgmac_dma_tx_add] (ring->end - ring->start + nr_frags + 1):10

If you'll be sending V7, what about adding something like
/* Ring start and end are u32, subtraction gives number of free slots */
Would that be OK? To point this way the code is OK and help understand why?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 2/9] bgmac: leave interrupts disabled as long as there is work to do
  2015-04-13 23:42 ` [PATCH v6 2/9] bgmac: leave interrupts disabled as long as there is work to do Felix Fietkau
@ 2015-04-14  5:40   ` Rafał Miłecki
  0 siblings, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  5:40 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> Always poll rx and tx during NAPI poll instead of relying on the status
> of the first interrupt. This prevents bgmac_poll from leaving unfinished
> work around until the next IRQ.
> In my tests this makes bridging/routing throughput under heavy load more
> stable and ensures that no new IRQs arrive as long as bgmac_poll uses up
> the entire budget.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>

Acked-by: Rafał Miłecki <zajec5@gmail.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling
  2015-04-13 23:42 ` [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling Felix Fietkau
@ 2015-04-14  5:55   ` Rafał Miłecki
  2015-04-14  9:13     ` Felix Fietkau
  0 siblings, 1 reply; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  5:55 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> @@ -382,6 +382,19 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac,
>         dma_desc->ctl1 = cpu_to_le32(ctl1);
>  }
>
> +static void bgmac_dma_rx_poison_buf(struct device *dma_dev,
> +                                   struct bgmac_slot_info *slot)
> +{
> +       struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET;
> +
> +       dma_sync_single_for_cpu(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
> +                               DMA_FROM_DEVICE);
> +       rx->len = cpu_to_le16(0xdead);
> +       rx->flags = cpu_to_le16(0xdead);
> +       dma_sync_single_for_device(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
> +                                  DMA_FROM_DEVICE);
> +}

flags should be 0xbeef


> @@ -404,51 +417,32 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>                 void *buf = slot->buf;
>                 u16 len, flags;
>
> -               /* Unmap buffer to make it accessible to the CPU */
> -               dma_sync_single_for_cpu(dma_dev, slot->dma_addr,
> -                                       BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
> +               do {
> +                       /* Prepare new skb as replacement */
> +                       if (bgmac_dma_rx_skb_for_slot(bgmac, slot)) {
> +                               bgmac_dma_rx_poison_buf(dma_dev, slot);
> +                               break;
> +                       }

So you just replaced slot->dma_addr with a mapping of new skb data.


>
> -               /* Get info from the header */
> -               len = le16_to_cpu(rx->len);
> -               flags = le16_to_cpu(rx->flags);
> +                       /* Unmap buffer to make it accessible to the CPU */
> +                       dma_unmap_single(dma_dev, slot->dma_addr,
> +                                        BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);

Now you unmap the new sbk data instead of the old one you want to access.
That's why the old code included old_dma_addr.


> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
>
>         for (i = 0; i < ring->num_slots; i++) {
>                 slot = &ring->slots[i];
> -               if (!slot->buf)
> +               if (!slot->dma_addr)
>                         continue;

I think it breaks error path of bgmac_dma_alloc. It may happen that
during DMA alloc we alloc skb and then we fail to map it. In such case
buf should be freed. Please trace how bgmac_dma_free is used in
bgmac_dma_alloc.


>
> -               if (slot->dma_addr)
> -                       dma_unmap_single(dma_dev, slot->dma_addr,
> -                                        BGMAC_RX_BUF_SIZE,
> -                                        DMA_FROM_DEVICE);
> +               dma_unmap_single(dma_dev, slot->dma_addr,
> +                                BGMAC_RX_BUF_SIZE,
> +                                DMA_FROM_DEVICE);
>                 put_page(virt_to_head_page(slot->buf));
> +               slot->dma_addr = 0;
>         }
>  }

-- 
Rafał

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 5/9] bgmac: add check for oversized packets
  2015-04-13 23:42 ` [PATCH v6 5/9] bgmac: add check for oversized packets Felix Fietkau
@ 2015-04-14  5:56   ` Rafał Miłecki
  0 siblings, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  5:56 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> In very rare cases, the MAC can catch an internal buffer that is bigger
> than it's supposed to be. Instead of crashing the kernel, simply pass
> the buffer back to the hardware
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>

Acked-by: Rafał Miłecki <zajec5@gmail.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 6/9] bgmac: increase rx ring size from 511 to 512
  2015-04-13 23:42 ` [PATCH v6 6/9] bgmac: increase rx ring size from 511 to 512 Felix Fietkau
@ 2015-04-14  5:57   ` Rafał Miłecki
  0 siblings, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  5:57 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> Limiting it to 511 looks like a failed attempt at leaving one descriptor
> empty to allow the hardware to stop processing a buffer that has not
> been prepared yet. However, this doesn't work because this affects the
> total ring size as well
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>

Acked-by: Rafał Miłecki <zajec5@gmail.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 9/9] bgmac: drop ring->num_slots
  2015-04-13 23:42 ` [PATCH v6 9/9] bgmac: drop ring->num_slots Felix Fietkau
@ 2015-04-14  6:38   ` Rafał Miłecki
  0 siblings, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  6:38 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> The ring size is always known at compile time, so make the code a bit
> more efficient
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>

Acked-by: Rafał Miłecki <zajec5@gmail.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 7/9] bgmac: simplify dma init/cleanup
  2015-04-13 23:42 ` [PATCH v6 7/9] bgmac: simplify dma init/cleanup Felix Fietkau
@ 2015-04-14  7:24   ` Rafał Miłecki
  0 siblings, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  7:24 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
> Instead of allocating buffers at device init time and initializing
> descriptors at device open, do both at the same time (during open).
> Free all buffers when closing the device.
>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  drivers/net/ethernet/broadcom/bgmac.c | 70 +++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index eafdbca..aad80a7 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -557,18 +557,26 @@ static void bgmac_dma_ring_desc_free(struct bgmac *bgmac,
>                           ring->dma_base);
>  }
>
> -static void bgmac_dma_free(struct bgmac *bgmac)
> +static void bgmac_dma_cleanup(struct bgmac *bgmac)
>  {
>         int i;
>
> -       for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
> +       for (i = 0; i < BGMAC_MAX_TX_RINGS; i++)
>                 bgmac_dma_tx_ring_free(bgmac, &bgmac->tx_ring[i]);
> -               bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i]);
> -       }
> -       for (i = 0; i < BGMAC_MAX_RX_RINGS; i++) {
> +
> +       for (i = 0; i < BGMAC_MAX_RX_RINGS; i++)
>                 bgmac_dma_rx_ring_free(bgmac, &bgmac->rx_ring[i]);
> +}
> +
> +static void bgmac_dma_free(struct bgmac *bgmac)
> +{
> +       int i;
> +
> +       for (i = 0; i < BGMAC_MAX_TX_RINGS; i++)
> +               bgmac_dma_ring_desc_free(bgmac, &bgmac->tx_ring[i]);
> +
> +       for (i = 0; i < BGMAC_MAX_RX_RINGS; i++)
>                 bgmac_dma_ring_desc_free(bgmac, &bgmac->rx_ring[i]);
> -       }
>  }
>
>  static int bgmac_dma_alloc(struct bgmac *bgmac)
> @@ -693,8 +690,13 @@ static void bgmac_dma_init(struct bgmac *bgmac)
>                 if (ring->unaligned)
>                         bgmac_dma_rx_enable(bgmac, ring);
>
> -               for (j = 0; j < ring->num_slots; j++)
> +               for (j = 0; j < ring->num_slots; j++) {
> +                       err = bgmac_dma_rx_skb_for_slot(bgmac, &ring->slots[j]);
> +                       if (err)
> +                               return err;

I don't think I like this logic. Instead of allowing caller to just
detect an error and handle it, you require it to do a cleanup. I think
bgmac_dma_init shouldn't leave any mess after failing.

If you e.g. call alloc_netdev and it fails, you don't care about
freeing whatever is allocated before failing.

I think you should call bgmac_dma_cleanup directly in a bgmac_dma_init
in case of error.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling
  2015-04-14  5:55   ` Rafał Miłecki
@ 2015-04-14  9:13     ` Felix Fietkau
  2015-04-14  9:19       ` Rafał Miłecki
  0 siblings, 1 reply; 21+ messages in thread
From: Felix Fietkau @ 2015-04-14  9:13 UTC (permalink / raw)
  To: Rafał Miłecki; +Cc: Network Development, Hauke Mehrtens

On 2015-04-14 07:55, Rafał Miłecki wrote:
> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
>> @@ -382,6 +382,19 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac,
>>         dma_desc->ctl1 = cpu_to_le32(ctl1);
>>  }
>>
>> +static void bgmac_dma_rx_poison_buf(struct device *dma_dev,
>> +                                   struct bgmac_slot_info *slot)
>> +{
>> +       struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET;
>> +
>> +       dma_sync_single_for_cpu(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
>> +                               DMA_FROM_DEVICE);
>> +       rx->len = cpu_to_le16(0xdead);
>> +       rx->flags = cpu_to_le16(0xdead);
>> +       dma_sync_single_for_device(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
>> +                                  DMA_FROM_DEVICE);
>> +}
> 
> flags should be 0xbeef
Right.

>> @@ -404,51 +417,32 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>                 void *buf = slot->buf;
>>                 u16 len, flags;
>>
>> -               /* Unmap buffer to make it accessible to the CPU */
>> -               dma_sync_single_for_cpu(dma_dev, slot->dma_addr,
>> -                                       BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
>> +               do {
>> +                       /* Prepare new skb as replacement */
>> +                       if (bgmac_dma_rx_skb_for_slot(bgmac, slot)) {
>> +                               bgmac_dma_rx_poison_buf(dma_dev, slot);
>> +                               break;
>> +                       }
> 
> So you just replaced slot->dma_addr with a mapping of new skb data.
> 
> 
>>
>> -               /* Get info from the header */
>> -               len = le16_to_cpu(rx->len);
>> -               flags = le16_to_cpu(rx->flags);
>> +                       /* Unmap buffer to make it accessible to the CPU */
>> +                       dma_unmap_single(dma_dev, slot->dma_addr,
>> +                                        BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
> 
> Now you unmap the new sbk data instead of the old one you want to access.
> That's why the old code included old_dma_addr.
Stupid mistake, I should be more careful with late night coding ;)

>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
>>
>>         for (i = 0; i < ring->num_slots; i++) {
>>                 slot = &ring->slots[i];
>> -               if (!slot->buf)
>> +               if (!slot->dma_addr)
>>                         continue;
> 
> I think it breaks error path of bgmac_dma_alloc. It may happen that
> during DMA alloc we alloc skb and then we fail to map it. In such case
> buf should be freed. Please trace how bgmac_dma_free is used in
> bgmac_dma_alloc.
I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer
allocation and dma mapping. If dma mapping fails, the buffer is freed
before any part of the slot is overwritten.

>> -               if (slot->dma_addr)
>> -                       dma_unmap_single(dma_dev, slot->dma_addr,
>> -                                        BGMAC_RX_BUF_SIZE,
>> -                                        DMA_FROM_DEVICE);
>> +               dma_unmap_single(dma_dev, slot->dma_addr,
>> +                                BGMAC_RX_BUF_SIZE,
>> +                                DMA_FROM_DEVICE);
>>                 put_page(virt_to_head_page(slot->buf));
>> +               slot->dma_addr = 0;
>>         }
>>  }

- Felix

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling
  2015-04-14  9:13     ` Felix Fietkau
@ 2015-04-14  9:19       ` Rafał Miłecki
  2015-04-14  9:26         ` Felix Fietkau
  0 siblings, 1 reply; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  9:19 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2015-04-14 07:55, Rafał Miłecki wrote:
>> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
>>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
>>>
>>>         for (i = 0; i < ring->num_slots; i++) {
>>>                 slot = &ring->slots[i];
>>> -               if (!slot->buf)
>>> +               if (!slot->dma_addr)
>>>                         continue;
>>
>> I think it breaks error path of bgmac_dma_alloc. It may happen that
>> during DMA alloc we alloc skb and then we fail to map it. In such case
>> buf should be freed. Please trace how bgmac_dma_free is used in
>> bgmac_dma_alloc.
> I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer
> allocation and dma mapping. If dma mapping fails, the buffer is freed
> before any part of the slot is overwritten.

Oops, I think I just spotted a memory leak then.

If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an
error without freeing a skb.

-- 
Rafał

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling
  2015-04-14  9:19       ` Rafał Miłecki
@ 2015-04-14  9:26         ` Felix Fietkau
  2015-04-14  9:32           ` Rafał Miłecki
  2015-04-14  9:37           ` Rafał Miłecki
  0 siblings, 2 replies; 21+ messages in thread
From: Felix Fietkau @ 2015-04-14  9:26 UTC (permalink / raw)
  To: Rafał Miłecki; +Cc: Network Development, Hauke Mehrtens

On 2015-04-14 11:19, Rafał Miłecki wrote:
> On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote:
>> On 2015-04-14 07:55, Rafał Miłecki wrote:
>>> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
>>>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
>>>>
>>>>         for (i = 0; i < ring->num_slots; i++) {
>>>>                 slot = &ring->slots[i];
>>>> -               if (!slot->buf)
>>>> +               if (!slot->dma_addr)
>>>>                         continue;
>>>
>>> I think it breaks error path of bgmac_dma_alloc. It may happen that
>>> during DMA alloc we alloc skb and then we fail to map it. In such case
>>> buf should be freed. Please trace how bgmac_dma_free is used in
>>> bgmac_dma_alloc.
>> I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer
>> allocation and dma mapping. If dma mapping fails, the buffer is freed
>> before any part of the slot is overwritten.
> 
> Oops, I think I just spotted a memory leak then.
> 
> If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an
> error without freeing a skb.
It neither allocates, nor frees an skb - I should probably rename that
function in a later patch round.

Allocation does:
	buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE);

When handling a DMA mapping error, it does this:
	put_page(virt_to_head_page(buf));

Where's the memory leak?

- Felix

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling
  2015-04-14  9:26         ` Felix Fietkau
@ 2015-04-14  9:32           ` Rafał Miłecki
  2015-04-14  9:37           ` Rafał Miłecki
  1 sibling, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  9:32 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 11:26, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2015-04-14 11:19, Rafał Miłecki wrote:
>> On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote:
>>> On 2015-04-14 07:55, Rafał Miłecki wrote:
>>>> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
>>>>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
>>>>>
>>>>>         for (i = 0; i < ring->num_slots; i++) {
>>>>>                 slot = &ring->slots[i];
>>>>> -               if (!slot->buf)
>>>>> +               if (!slot->dma_addr)
>>>>>                         continue;
>>>>
>>>> I think it breaks error path of bgmac_dma_alloc. It may happen that
>>>> during DMA alloc we alloc skb and then we fail to map it. In such case
>>>> buf should be freed. Please trace how bgmac_dma_free is used in
>>>> bgmac_dma_alloc.
>>> I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer
>>> allocation and dma mapping. If dma mapping fails, the buffer is freed
>>> before any part of the slot is overwritten.
>>
>> Oops, I think I just spotted a memory leak then.
>>
>> If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an
>> error without freeing a skb.
> It neither allocates, nor frees an skb - I should probably rename that
> function in a later patch round.
>
> Allocation does:
>         buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE);
>
> When handling a DMA mapping error, it does this:
>         put_page(virt_to_head_page(buf));
>
> Where's the memory leak?

I mean the buf, not skb, sorry:
buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE);

if (dma_mapping_error(dma_dev, dma_addr)) {
        LACKING free(buf) HERE
        return -ENOMEM;
}

-- 
Rafał

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling
  2015-04-14  9:26         ` Felix Fietkau
  2015-04-14  9:32           ` Rafał Miłecki
@ 2015-04-14  9:37           ` Rafał Miłecki
  1 sibling, 0 replies; 21+ messages in thread
From: Rafał Miłecki @ 2015-04-14  9:37 UTC (permalink / raw)
  To: Felix Fietkau; +Cc: Network Development, Hauke Mehrtens

On 14 April 2015 at 11:26, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2015-04-14 11:19, Rafał Miłecki wrote:
>> On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote:
>>> On 2015-04-14 07:55, Rafał Miłecki wrote:
>>>> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
>>>>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
>>>>>
>>>>>         for (i = 0; i < ring->num_slots; i++) {
>>>>>                 slot = &ring->slots[i];
>>>>> -               if (!slot->buf)
>>>>> +               if (!slot->dma_addr)
>>>>>                         continue;
>>>>
>>>> I think it breaks error path of bgmac_dma_alloc. It may happen that
>>>> during DMA alloc we alloc skb and then we fail to map it. In such case
>>>> buf should be freed. Please trace how bgmac_dma_free is used in
>>>> bgmac_dma_alloc.
>>> I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer
>>> allocation and dma mapping. If dma mapping fails, the buffer is freed
>>> before any part of the slot is overwritten.
>>
>> Oops, I think I just spotted a memory leak then.
>>
>> If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an
>> error without freeing a skb.
> It neither allocates, nor frees an skb - I should probably rename that
> function in a later patch round.
>
> Allocation does:
>         buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE);
>
> When handling a DMA mapping error, it does this:
>         put_page(virt_to_head_page(buf));
>
> Where's the memory leak?

Oooh, sorry. I didn't understand put_page. I just expected direct
kfree here. It's alright then, sorry.

-- 
Rafał

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-04-14  9:37 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-13 23:42 [PATCH v6 1/9] bgmac: simplify tx ring index handling Felix Fietkau
2015-04-13 23:42 ` [PATCH v6 2/9] bgmac: leave interrupts disabled as long as there is work to do Felix Fietkau
2015-04-14  5:40   ` Rafał Miłecki
2015-04-13 23:42 ` [PATCH v6 3/9] bgmac: set received skb headroom to NET_SKB_PAD Felix Fietkau
2015-04-13 23:42 ` [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling Felix Fietkau
2015-04-14  5:55   ` Rafał Miłecki
2015-04-14  9:13     ` Felix Fietkau
2015-04-14  9:19       ` Rafał Miłecki
2015-04-14  9:26         ` Felix Fietkau
2015-04-14  9:32           ` Rafał Miłecki
2015-04-14  9:37           ` Rafał Miłecki
2015-04-13 23:42 ` [PATCH v6 5/9] bgmac: add check for oversized packets Felix Fietkau
2015-04-14  5:56   ` Rafał Miłecki
2015-04-13 23:42 ` [PATCH v6 6/9] bgmac: increase rx ring size from 511 to 512 Felix Fietkau
2015-04-14  5:57   ` Rafał Miłecki
2015-04-13 23:42 ` [PATCH v6 7/9] bgmac: simplify dma init/cleanup Felix Fietkau
2015-04-14  7:24   ` Rafał Miłecki
2015-04-13 23:42 ` [PATCH v6 8/9] bgmac: fix DMA rx corruption Felix Fietkau
2015-04-13 23:42 ` [PATCH v6 9/9] bgmac: drop ring->num_slots Felix Fietkau
2015-04-14  6:38   ` Rafał Miłecki
2015-04-14  5:38 ` [PATCH v6 1/9] bgmac: simplify tx ring index handling Rafał Miłecki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.