Subject: [PATCH] net: meth: Add set_rx_mode hook to fix ICMPv6 neighbor discovery

From: Joshua Kinard <[email-masked]>
Date: Sat, 17 Dec 2011 19:56:29 -0500

SGI IP32 (O2)'s ethernet driver (meth) lacks a set_rx_mode function, which
prevents IPv6 from working completely because any ICMPv6 neighbor
solicitation requests aren't picked up by the driver.  So the machine can
ping out and connect to other systems, but other systems will have a very
hard time connecting to the O2.

Signed-off-by: Joshua Kinard <[email-masked]>
---

 drivers/net/ethernet/sgi/meth.c |   60 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 7 deletions(-)

--- a/drivers/net/ethernet/sgi/meth.c	2011-12-17 15:51:44.569166824 -0500
+++ b/drivers/net/ethernet/sgi/meth.c	2011-12-17 15:51:20.259167050 -0500
@@ -28,6 +28,7 @@
 #include <linux/tcp.h>         /* struct tcphdr */
 #include <linux/skbuff.h>
 #include <linux/mii.h>         /* MII definitions */
+#include <linux/crc32.h>

 #include <asm/ip32/mace.h>
 #include <asm/ip32/ip32_ints.h>
@@ -57,6 +58,12 @@ static const char *meth_str="SGI O2 Fast
 static int timeout = TX_TIMEOUT;
 module_param(timeout, int, 0);

+/* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
+ * MACE Ethernet uses a 64 element hash table based on the Ethernet CRC.
+ */
+static int multicast_filter_limit = 32;
+
+
 /*
  * This structure is private to each device. It is used to pass
  * packets in and out, so there is place for a packet
@@ -79,6 +86,9 @@ struct meth_private {
 	struct sk_buff *rx_skbs[RX_RING_ENTRIES];
 	unsigned long rx_write;

+	/* Multicast filter. */
+	unsigned long mcast_filter;
+
 	spinlock_t meth_lock;
 };

@@ -765,15 +775,51 @@ static int meth_ioctl(struct net_device
 	}
 }

+static void meth_set_rx_mode(struct net_device *dev)
+{
+	struct meth_private *priv = netdev_priv(dev);
+	unsigned long flags;
+
+	netif_stop_queue(dev);
+	spin_lock_irqsave(&priv->meth_lock, flags);
+	priv->mac_ctrl &= ~(METH_PROMISC);
+
+	if (dev->flags & IFF_PROMISC) {
+		priv->mac_ctrl |= METH_PROMISC;
+		priv->mcast_filter = 0xffffffffffffffffUL;
+		mace->eth.mac_ctrl = priv->mac_ctrl;
+		mace->eth.mcast_filter = priv->mcast_filter;
+	} else if ((netdev_mc_count(dev) > multicast_filter_limit) ||
+			   (dev->flags & IFF_ALLMULTI)) {
+			priv->mac_ctrl |= METH_ACCEPT_AMCAST;
+			priv->mcast_filter = 0xffffffffffffffffUL;
+			mace->eth.mac_ctrl = priv->mac_ctrl;
+			mace->eth.mcast_filter = priv->mcast_filter;
+	} else {
+		struct netdev_hw_addr *ha;
+		priv->mac_ctrl |= METH_ACCEPT_MCAST;
+
+		netdev_for_each_mc_addr(ha, dev)
+			set_bit((ether_crc(ETH_ALEN, ha->addr) >> 26),
+				    (volatile long unsigned int *)&priv->mcast_filter);
+
+		mace->eth.mcast_filter = priv->mcast_filter;
+	}
+
+	spin_unlock_irqrestore(&priv->meth_lock, flags);
+	netif_wake_queue(dev);
+}
+
 static const struct net_device_ops meth_netdev_ops = {
-	.ndo_open		= meth_open,
-	.ndo_stop		= meth_release,
-	.ndo_start_xmit		= meth_tx,
-	.ndo_do_ioctl		= meth_ioctl,
-	.ndo_tx_timeout		= meth_tx_timeout,
-	.ndo_change_mtu		= eth_change_mtu,
-	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_open				= meth_open,
+	.ndo_stop				= meth_release,
+	.ndo_start_xmit			= meth_tx,
+	.ndo_do_ioctl			= meth_ioctl,
+	.ndo_tx_timeout			= meth_tx_timeout,
+	.ndo_change_mtu			= eth_change_mtu,
+	.ndo_validate_addr		= eth_validate_addr,
 	.ndo_set_mac_address	= eth_mac_addr,
+	.ndo_set_rx_mode    	= meth_set_rx_mode,
 };

 /*

---

From: David Miller <[email-masked]>
Date: Sat, 17 Dec 2011 21:56:30 -0500 (EST)

From: Joshua Kinard <[email-masked]>
Date: Sat, 17 Dec 2011 19:56:29 -0500

> +static int multicast_filter_limit = 32;

Unnecessary empty line, only one is sufficient.  I also don't see a reason
to even define this value.  If it's a constant then use a const type.

> +	unsigned long mcast_filter;
 ...
> +		priv->mcast_filter = 0xffffffffffffffffUL;

You're assuming that unsigned long is 64-bits here.  You need to use a
type which matches your expections regardless of the architecture that
the code is built on.

> +				    (volatile long unsigned int *)&priv->mcast_filter);

This makes an assumption not only about the size of the "unsigned long"
type, but also of the endianness of the architecture this runs on.

Please recode this to remove both assumptions.

---

From: Joshua Kinard <[email-masked]>
Date: Sat, 17 Dec 2011 23:37:01 -0500

> to even define this value.  If it's a constant then use a const type.

Lifted straight out of another driver already in the tree and checked
against the docs.  I can spin a new patch to constify it, but the same fix
is needed for several other drivers, too.

> type which matches your expections regardless of the architecture that

MACE Ethernet only ever appears on the SGI O2 systems.  It's part of the
MACE chip and doesn't exist (as far as I know) in any kind of standalone
form.  It's virtually impossible for it to appear outside of any other
architecture/machine.

That said, would using 'u64' over 'unsigned long' work?  The O2 codebase is
far from pretty, and would need a LOT of cleanups along similar lines.  This
code simply matches what is already existing in-tree.

> Please recode this to remove both assumptions.

See note above regarding the 'unsigned long' bit.  The endian assumption is
not directly visible to me, however.  What, specifically, is incorrect?  The
call to ether_crc?  The bitwise right-shift?  set_bit?

I lifted this out of au1000_eth.c (which is a little-endian MIPS device, if
I recall correctly), and all the digging I could do states that the Ethernet
CRC algorithm is LE anyways (ether_crc() calls crc32_le, bitrev32, and
such).  I couldn't find anything big-endian about it, even when I tested it
against several other code samples that computed the 6-bit hash key from the
Dst MAC address.

Thanks,

-- 
Joshua Kinard
Gentoo/MIPS
[email-masked]
4096R/D25D95E3 2011-03-28

"The past tempts us, the present confuses us, the future frightens us.  And
our lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

---

From: David Miller <[email-masked]>
Date: Sun, 18 Dec 2011 00:19:25 -0500 (EST)

From: Joshua Kinard <[email-masked]>
Date: Sat, 17 Dec 2011 23:37:01 -0500

> MACE Ethernet only ever appears on the SGI O2 systems.

That has no bearing on my feedback, we simply do not put non-portable
code like this into the tree at this point.

Just because this driver has been maintained in an non-portable manner
up to this point, doesn't mean we continue doing that.

---

From: Sergei Shtylyov <[email-masked]>
Date: Sun, 18 Dec 2011 17:26:47 +0400

Hello.

> Signed-off-by: Joshua Kinard<[email-masked]>
> ---

    Some minor nits below...

> +++ b/drivers/net/ethernet/sgi/meth.c	2011-12-17 15:51:20.259167050 -0500
[...]
> +static int multicast_filter_limit = 32;

    On empty oine would be enough...

> +	priv->mac_ctrl&= ~(METH_PROMISC);

    Parens not needed here.

> +			mace->eth.mcast_filter = priv->mcast_filter;

     This block is over-indented.

> +		mace->eth.mcast_filter = priv->mcast_filter;

    This last statement is common between all branches, so could be moved out 
of *if*...

> +	.ndo_set_rx_mode    	= meth_set_rx_mode,

    The intializer values are not aligned now, and they were before the patch.

WBR, Sergei

---

From: Joshua Kinard <[email-masked]>
Date: Sun, 18 Dec 2011 09:35:39 -0500

>    On empty oine would be enough...

Fixed, as Dave pointed out.  I converted it and the driver name to a macro
anyways.

>> +    priv->mac_ctrl&= ~(METH_PROMISC);

Yeah, I am a habitual parenthesis abuser.  You should see the RTC driver I
re-wrote :)

>     This block is over-indented.

Weird.  The editor I was using had the tabs set to an equivalent of 4
spaces, so it lined up for me *originally*, but after the patch was applied,
it was out of alignment, too.  I think I got it fixed this time, though.
Not sure what was causing that.

>    This last statement is common between all branches, so could be moved out

Done.

>    The intializer values are not aligned now, and they were before the patch.

Yeah, same problem as above.  Not sure how my tabs got mangled.  Should be
fixed in the next revision once I test it.

Thanks!

---

From: Joshua Kinard <[email-masked]>
Date: Sun, 18 Dec 2011 09:40:24 -0500

> up to this point, doesn't mean we continue doing that.

Agreed.  However, I was simply trying to fix a problem that prevents IPv6
from working, not fix every little thing wrong with the code.  While I want
to tackle re-writing the driver to be more in line with existing network
drivers, that's a future project.  I'm still new to driver
development/kernel work in general, so I'm learning here.

Thanks for the feedback, though,

---

From: Joshua Kinard <[email-masked]>
Date: Sun, 18 Dec 2011 10:13:29 -0500

> type, but also of the endianness of the architecture this runs on.

Can you give me some tips on this one?  au1000_eth.c does the same thing,
and I'm not seeing what the endian issue is exactly.  Is it the >> 26 part
or the use of ether_crc?  I see there's an ether_crc_le, too, and some
drivers also do the >> 26 bit on it as well.

Which is correct?  The few drivers I've looked at don't exactly spell out
this part of the code, and are usually doing something different because
most seem to access the multicast filter register in either 8-bits or
32-bits.  MACE ethernet seems to be one of the few doing it in full 64-bits.

Thanks,

---

From: Joshua Kinard <[email-masked]>
Date: Sat, 24 Dec 2011 20:45:32 -0500

SGI IP32 (O2)'s ethernet driver (meth) lacks meth_set_rx_mode, which

 arch/mips/include/asm/ip32/mace.h |   52 +++++++++++++++----------------
 drivers/net/ethernet/sgi/meth.c   |   62 ++++++++++++++++++++++++++++++++++----
 2 files changed, 82 insertions(+), 32 deletions(-)

diff -Naurp a/arch/mips/include/asm/ip32/mace.h
b/arch/mips/include/asm/ip32/mace.h
--- a/arch/mips/include/asm/ip32/mace.h	2011-12-24 16:19:46.703561171 -0500
+++ b/arch/mips/include/asm/ip32/mace.h	2011-12-24 16:27:40.613556791 -0500
@@ -95,35 +95,35 @@ struct mace_video {
  * Ethernet interface
  */
 struct mace_ethernet {
-	volatile unsigned long mac_ctrl;
-	volatile unsigned long int_stat;
-	volatile unsigned long dma_ctrl;
-	volatile unsigned long timer;
-	volatile unsigned long tx_int_al;
-	volatile unsigned long rx_int_al;
-	volatile unsigned long tx_info;
-	volatile unsigned long tx_info_al;
-	volatile unsigned long rx_buff;
-	volatile unsigned long rx_buff_al1;
-	volatile unsigned long rx_buff_al2;
-	volatile unsigned long diag;
-	volatile unsigned long phy_data;
-	volatile unsigned long phy_regs;
-	volatile unsigned long phy_trans_go;
-	volatile unsigned long backoff_seed;
+	volatile u64 mac_ctrl;
+	volatile u64 int_stat;
+	volatile u64 dma_ctrl;
+	volatile u64 timer;
+	volatile u64 tx_int_al;
+	volatile u64 rx_int_al;
+	volatile u64 tx_info;
+	volatile u64 tx_info_al;
+	volatile u64 rx_buff;
+	volatile u64 rx_buff_al1;
+	volatile u64 rx_buff_al2;
+	volatile u64 diag;
+	volatile u64 phy_data;
+	volatile u64 phy_regs;
+	volatile u64 phy_trans_go;
+	volatile u64 backoff_seed;
 	/*===================================*/
-	volatile unsigned long imq_reserved[4];
-	volatile unsigned long mac_addr;
-	volatile unsigned long mac_addr2;
-	volatile unsigned long mcast_filter;
-	volatile unsigned long tx_ring_base;
+	volatile u64 imq_reserved[4];
+	volatile u64 mac_addr;
+	volatile u64 mac_addr2;
+	volatile u64 mcast_filter;
+	volatile u64 tx_ring_base;
 	/* Following are read-only registers for debugging */
-	volatile unsigned long tx_pkt1_hdr;
-	volatile unsigned long tx_pkt1_ptr[3];
-	volatile unsigned long tx_pkt2_hdr;
-	volatile unsigned long tx_pkt2_ptr[3];
+	volatile u64 tx_pkt1_hdr;
+	volatile u64 tx_pkt1_ptr[3];
+	volatile u64 tx_pkt2_hdr;
+	volatile u64 tx_pkt2_ptr[3];
 	/*===================================*/
-	volatile unsigned long rx_fifo;
+	volatile u64 rx_fifo;
 };

 /*
diff -Naurp a/drivers/net/ethernet/sgi/meth.c b/drivers/net/ethernet/sgi/meth.c
--- a/drivers/net/ethernet/sgi/meth.c	2011-12-24 16:20:06.743560985 -0500
+++ b/drivers/net/ethernet/sgi/meth.c	2011-12-24 16:27:18.743556993 -0500
@@ -28,6 +28,7 @@
 #include <linux/tcp.h>         /* struct tcphdr */
 #include <linux/skbuff.h>
 #include <linux/mii.h>         /* MII definitions */
+#include <linux/crc32.h>

@@ -48,9 +49,6 @@
 #define MFE_RX_DEBUG 0
 #endif

-
-static const char *meth_str="SGI O2 Fast Ethernet";
-
 /* The maximum time waited (in jiffies) before assuming a Tx failed. (400ms) */
 #define TX_TIMEOUT (400*HZ/1000)

@@ -58,27 +56,44 @@ static int timeout = TX_TIMEOUT;
 module_param(timeout, int, 0);

 /*
+ * Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
+ * MACE Ethernet uses a 64 element hash table based on the Ethernet CRC.
+ */
+#define METH_MCF_LIMIT 32
+
+/* Driver name */
+#define METH_DRV_NAME "SGI O2 Fast Ethernet"
+
+/*
  * This structure is private to each device. It is used to pass
  * packets in and out, so there is place for a packet
  */
 struct meth_private {
 	/* in-memory copy of MAC Control register */
-	unsigned long mac_ctrl;
+	u64 mac_ctrl;
+
 	/* in-memory copy of DMA Control register */
-	unsigned long dma_ctrl;
+	u64 dma_ctrl;
+
 	/* address of PHY, used by mdio_* functions, initialized in mdio_probe */
 	unsigned long phy_addr;
+
+	/* TX bits */
 	tx_packet *tx_ring;
 	dma_addr_t tx_ring_dma;
 	struct sk_buff *tx_skbs[TX_RING_ENTRIES];
 	dma_addr_t tx_skb_dmas[TX_RING_ENTRIES];
 	unsigned long tx_read, tx_write, tx_count;

+	/* RX bits */
 	rx_packet *rx_ring[RX_RING_ENTRIES];
 	dma_addr_t rx_ring_dmas[RX_RING_ENTRIES];
 	struct sk_buff *rx_skbs[RX_RING_ENTRIES];
 	unsigned long rx_write;

+	u64 mcast_filter;
+
 	spinlock_t meth_lock;
 };

@@ -323,7 +338,7 @@ static int meth_open(struct net_device *
 	if (ret < 0)
 		goto out_free_tx_ring;

-	ret = request_irq(dev->irq, meth_interrupt, 0, meth_str, dev);
+	ret = request_irq(dev->irq, meth_interrupt, 0, METH_DRV_NAME, dev);
 	if (ret) {
 		printk(KERN_ERR "%s: Can't get irq %d\n", dev->name, dev->irq);
 		goto out_free_rx_ring;
@@ -765,6 +780,40 @@ static int meth_ioctl(struct net_device
 	}
 }

+static void meth_set_rx_mode(struct net_device *dev)
+{
+	struct meth_private *priv = netdev_priv(dev);
+	unsigned long flags;
+
+	netif_stop_queue(dev);
+	spin_lock_irqsave(&priv->meth_lock, flags);
+	priv->mac_ctrl &= ~METH_PROMISC;
+
+	if (dev->flags & IFF_PROMISC) {
+		priv->mac_ctrl |= METH_PROMISC;
+		priv->mcast_filter = 0xffffffffffffffffUL;
+	} else if ((netdev_mc_count(dev) > METH_MCF_LIMIT) ||
+		   (dev->flags & IFF_ALLMULTI)) {
+		priv->mac_ctrl |= METH_ACCEPT_AMCAST;
+		priv->mcast_filter = 0xffffffffffffffffUL;
+	} else {
+		struct netdev_hw_addr *ha;
+		priv->mac_ctrl |= METH_ACCEPT_MCAST;
+
+		netdev_for_each_mc_addr(ha, dev)
+			set_bit((ether_crc(ETH_ALEN, ha->addr) >> 26),
+			        (volatile unsigned long *)&priv->mcast_filter);
+	}
+
+	/* Write the changes to the chip registers. */
+	mace->eth.mac_ctrl = priv->mac_ctrl;
+	mace->eth.mcast_filter = priv->mcast_filter;
+
+	/* Done! */
+	spin_unlock_irqrestore(&priv->meth_lock, flags);
+	netif_wake_queue(dev);
+}
+
 static const struct net_device_ops meth_netdev_ops = {
 	.ndo_open		= meth_open,
 	.ndo_stop		= meth_release,
@@ -774,6 +823,7 @@ static const struct net_device_ops meth_
 	.ndo_change_mtu		= eth_change_mtu,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_set_mac_address	= eth_mac_addr,
+	.ndo_set_rx_mode    	= meth_set_rx_mode,
 };

 /*

---

From: David Miller <[email-masked]>
Date: Mon, 26 Dec 2011 15:17:13 -0500 (EST)

From: Joshua Kinard <[email-masked]>
Date: Sat, 24 Dec 2011 20:45:32 -0500

> Signed-off-by: Joshua Kinard <[email-masked]>

Lots of completely unrelated changes here, the IRQ name string has
nothing to do with specifically fixing the ICMPv6 neighbour discovery
bug.

Do not mix changes like this, one change per patch please, thanks.

---

From: Joshua Kinard <[email-masked]>
Date: Mon, 26 Dec 2011 23:54:39 -0500

> Do not mix changes like this, one change per patch please, thanks.

Noted.  I dropped the name change patch and minimized the changes to the
mace registers to just the two registers needed for this specific case.
I'll send two separate patches for the remaining registers and the name bit
to just linux-mips later on.  Patch to follow.

Thanks,

---

From: Joshua Kinard <[email-masked]>
Date: Tue, 27 Dec 2011 00:06:15 -0500

arch/mips/include/asm/ip32/mace.h |    2 -
 drivers/net/ethernet/sgi/meth.c   |   48 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

diff -Naurp a/arch/mips/include/asm/ip32/mace.h
b/arch/mips/include/asm/ip32/mace.h
--- a/arch/mips/include/asm/ip32/mace.h	2011-12-24 16:19:46.703561171 -0500
+++ b/arch/mips/include/asm/ip32/mace.h	2011-12-26 20:04:15.281839510 -0500
@@ -95,7 +95,7 @@ struct mace_video {
  * Ethernet interface
  */
 struct mace_ethernet {
-	volatile unsigned long mac_ctrl;
+	volatile u64 mac_ctrl;
 	volatile unsigned long int_stat;
 	volatile unsigned long dma_ctrl;
 	volatile unsigned long timer;
diff -Naurp a/drivers/net/ethernet/sgi/meth.c b/drivers/net/ethernet/sgi/meth.c
--- a/drivers/net/ethernet/sgi/meth.c	2011-12-24 16:20:06.743560985 -0500
+++ b/drivers/net/ethernet/sgi/meth.c	2011-12-26 20:03:53.471839710 -0500
@@ -28,6 +28,7 @@
 #include <linux/tcp.h>         /* struct tcphdr */
 #include <linux/skbuff.h>
 #include <linux/mii.h>         /* MII definitions */
+#include <linux/crc32.h>

 #include <asm/ip32/mace.h>
 #include <asm/ip32/ip32_ints.h>
@@ -58,12 +59,19 @@ static int timeout = TX_TIMEOUT;
 module_param(timeout, int, 0);

 /*
 	unsigned long dma_ctrl;
@@ -79,6 +87,9 @@ struct meth_private {

@@ -765,6 +776,40 @@ static int meth_ioctl(struct net_device
 	}
 }

@@ -774,6 +819,7 @@ static const struct net_device_ops meth_

 /*

---

From: David Miller <[email-masked]>
Date: Tue, 27 Dec 2011 13:17:43 -0500 (EST)

From: Joshua Kinard <[email-masked]>
Date: Tue, 27 Dec 2011 00:06:15 -0500

> Signed-off-by: Joshua Kinard <[email-masked]>

Applied to net-next, thanks.

---

From: Stephen Hemminger <[email-masked]>
Date: Tue, 27 Dec 2011 10:34:08 -0800

On Tue, 27 Dec 2011 00:06:15 -0500

>  	volatile unsigned long timer;

This device driver writer needs to read:
  Documentation/volatile-considered-harmful.txt

---

From: David Miller <[email-masked]>
Date: Tue, 27 Dec 2011 13:52:09 -0500 (EST)

From: Stephen Hemminger <[email-masked]>
Date: Tue, 27 Dec 2011 10:34:08 -0800

>   Documentation/volatile-considered-harmful.txt

This driver has a lot of problems, some of which I've made Joshua aware of
already.

---

From: Joshua Kinard <[email-masked]>
Date: Tue, 27 Dec 2011 16:29:57 -0500

>   Documentation/volatile-considered-harmful.txt

MIPS I/O registers are always memory-mapped, and to prevent the compiler
from trying to over-optimize, volatile is used to make sure we always read a
value from the hardware and not from some cached value.

See MIPS Run (2nd Ed), pp 307, section 10.5.2 highlights an example of this,
which is viewable here:
http://books.google.com/books?id=kk8G2gK4Tw8C&pg=PA307&lpg=PA308#v=onepage&q&f=false

But other than that, yeah, this driver needs to pretty much be stripped down
to the nuts and bolts and re-written.  Maybe something to tackle in the
future.  I still haven't gotten around to submitting the RTC driver for O2's
(that I re-wrote from a patch sent into LKML years ago) upstream yet.

---

From: Stephen Hemminger <[email-masked]>
Date: Tue, 27 Dec 2011 14:34:41 -0800

On Tue, 27 Dec 2011 16:29:57 -0500

> value from the hardware and not from some cached value.

Almost every other network driver had memory mapped register.
The problem is volatile is that the compiler is stupid and wrong.
Using explicit barriers is preferred and ensures correct and fast
code.

---

From: Joshua Kinard <[email-masked]>
Date: Tue, 27 Dec 2011 17:48:32 -0500

> Using explicit barriers is preferred and ensures correct and fast

I am somewhat new to driver development, so I do not know all the tricks of
the trade just yet.  Do you have references to doing explicit barriers that
I can look at?  Might be worth trying on the RTC driver I have to get the
hang of them.

---

From: Stephen Hemminger <[email-masked]>
Date: Tue, 27 Dec 2011 16:29:03 -0800

On Tue, 27 Dec 2011 17:48:32 -0500

> I can look at?  Might be worth trying on the RTC driver I have to get the

Start by reading volatile considered harmful and memory barriers in kernel
Documentation directory. Paul does a better job of explaining it than
I could ever do :-)