diff mbox series

[net,v2,05/12] net/sonic: Fix receive buffer handling

Message ID e20133bf43ec6f5967a3330dacaf38f653bf3061.1579641728.git.fthain@telegraphics.com.au
State Changes Requested
Delegated to: David Miller
Headers show
Series Fixes for SONIC ethernet driver | expand

Commit Message

Finn Thain Jan. 21, 2020, 9:22 p.m. UTC
The SONIC can sometimes advance its rx buffer pointer (RRP register)
without advancing its rx descriptor pointer (CRDA register). As a result
the index of the current rx descriptor may not equal that of the current
rx buffer. The driver mistakenly assumes that they are always equal.
This assumption leads to incorrect packet lengths and possible packet
duplication. Avoid this by calling a new function to locate the buffer
corresponding to a given descriptor.

Fixes: efcce839360f ("[PATCH] macsonic/jazzsonic network drivers update")
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/net/ethernet/natsemi/sonic.c | 36 ++++++++++++++++++++++++----
 drivers/net/ethernet/natsemi/sonic.h |  5 ++--
 2 files changed, 34 insertions(+), 7 deletions(-)

Comments

Stephen Hemminger Jan. 21, 2020, 10:23 p.m. UTC | #1
On Wed, 22 Jan 2020 08:22:08 +1100
Finn Thain <fthain@telegraphics.com.au> wrote:

>  
> +/* Return the array index corresponding to a given Receive Buffer pointer. */
> +
> +static inline int index_from_addr(struct sonic_local *lp, dma_addr_t addr,
> +				  unsigned int last)

Why the blank line between comment and the start of the function?

Also, the kernel standard is not to use the inline keyword on functions
and let the compiler decide to inline if it wants to. The compiler is much
smarter at knowing the architectural limitations than humans are.
Finn Thain Jan. 21, 2020, 11:53 p.m. UTC | #2
On Tue, 21 Jan 2020, Stephen Hemminger wrote:

> On Wed, 22 Jan 2020 08:22:08 +1100
> Finn Thain <fthain@telegraphics.com.au> wrote:
> 
> >  
> > +/* Return the array index corresponding to a given Receive Buffer pointer. */
> > +
> > +static inline int index_from_addr(struct sonic_local *lp, dma_addr_t addr,
> > +				  unsigned int last)
> 
> Why the blank line between comment and the start of the function?
> 

The driver mostly uses this style:

/*
 * We have a good packet(s), pass it/them up the network stack.
 */
static void sonic_rx(struct net_device *dev)
{
}

To my eyes, style I used is the closest readable approximation of the 
existing style that doesn't upset checkpatch.

Anyway, I will remove the blank lines.

> Also, the kernel standard is not to use the inline keyword on functions 
> and let the compiler decide to inline if it wants to.

OK.

Thanks for your review.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/natsemi/sonic.c b/drivers/net/ethernet/natsemi/sonic.c
index 5ba705ad7d4e..3387f7bc1a80 100644
--- a/drivers/net/ethernet/natsemi/sonic.c
+++ b/drivers/net/ethernet/natsemi/sonic.c
@@ -408,6 +408,22 @@  static irqreturn_t sonic_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/* Return the array index corresponding to a given Receive Buffer pointer. */
+
+static inline int index_from_addr(struct sonic_local *lp, dma_addr_t addr,
+				  unsigned int last)
+{
+	unsigned int i = last;
+
+	do {
+		i = (i + 1) & SONIC_RRS_MASK;
+		if (addr == lp->rx_laddr[i])
+			return i;
+	} while (i != last);
+
+	return -ENOENT;
+}
+
 /*
  * We have a good packet(s), pass it/them up the network stack.
  */
@@ -427,6 +443,16 @@  static void sonic_rx(struct net_device *dev)
 
 		status = sonic_rda_get(dev, entry, SONIC_RD_STATUS);
 		if (status & SONIC_RCR_PRX) {
+			u32 addr = (sonic_rda_get(dev, entry,
+						  SONIC_RD_PKTPTR_H) << 16) |
+				   sonic_rda_get(dev, entry, SONIC_RD_PKTPTR_L);
+			int i = index_from_addr(lp, addr, entry);
+
+			if (i < 0) {
+				WARN_ONCE(1, "failed to find buffer!\n");
+				break;
+			}
+
 			/* Malloc up new buffer. */
 			new_skb = netdev_alloc_skb(dev, SONIC_RBSIZE + 2);
 			if (new_skb == NULL) {
@@ -448,7 +474,7 @@  static void sonic_rx(struct net_device *dev)
 
 			/* now we have a new skb to replace it, pass the used one up the stack */
 			dma_unmap_single(lp->device, lp->rx_laddr[entry], SONIC_RBSIZE, DMA_FROM_DEVICE);
-			used_skb = lp->rx_skb[entry];
+			used_skb = lp->rx_skb[i];
 			pkt_len = sonic_rda_get(dev, entry, SONIC_RD_PKTLEN);
 			skb_trim(used_skb, pkt_len);
 			used_skb->protocol = eth_type_trans(used_skb, dev);
@@ -457,13 +483,13 @@  static void sonic_rx(struct net_device *dev)
 			lp->stats.rx_bytes += pkt_len;
 
 			/* and insert the new skb */
-			lp->rx_laddr[entry] = new_laddr;
-			lp->rx_skb[entry] = new_skb;
+			lp->rx_laddr[i] = new_laddr;
+			lp->rx_skb[i] = new_skb;
 
 			bufadr_l = (unsigned long)new_laddr & 0xffff;
 			bufadr_h = (unsigned long)new_laddr >> 16;
-			sonic_rra_put(dev, entry, SONIC_RR_BUFADR_L, bufadr_l);
-			sonic_rra_put(dev, entry, SONIC_RR_BUFADR_H, bufadr_h);
+			sonic_rra_put(dev, i, SONIC_RR_BUFADR_L, bufadr_l);
+			sonic_rra_put(dev, i, SONIC_RR_BUFADR_H, bufadr_h);
 		} else {
 			/* This should only happen, if we enable accepting broken packets. */
 		}
diff --git a/drivers/net/ethernet/natsemi/sonic.h b/drivers/net/ethernet/natsemi/sonic.h
index 9e4ff8dd032d..e6d47e45c5c2 100644
--- a/drivers/net/ethernet/natsemi/sonic.h
+++ b/drivers/net/ethernet/natsemi/sonic.h
@@ -275,8 +275,9 @@ 
 #define SONIC_NUM_RDS   SONIC_NUM_RRS /* number of receive descriptors */
 #define SONIC_NUM_TDS   16            /* number of transmit descriptors */
 
-#define SONIC_RDS_MASK  (SONIC_NUM_RDS-1)
-#define SONIC_TDS_MASK  (SONIC_NUM_TDS-1)
+#define SONIC_RRS_MASK  (SONIC_NUM_RRS - 1)
+#define SONIC_RDS_MASK  (SONIC_NUM_RDS - 1)
+#define SONIC_TDS_MASK  (SONIC_NUM_TDS - 1)
 
 #define SONIC_RBSIZE	1520          /* size of one resource buffer */