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

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

[Joshua Kinard]

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 <kumba@gentoo.org>
---

 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,
 };

 /*

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

[David Miller]

From: Joshua Kinard <kumba@gentoo.org>
Date: Sat, 17 Dec 2011 19:56:29 -0500

> +/* 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;
> +
> +

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.

> +	/* Multicast filter. */
> +	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.

> +		netdev_for_each_mc_addr(ha, dev)
> +			set_bit((ether_crc(ETH_ALEN, ha->addr) >> 26),
> +				    (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.

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

[Joshua Kinard]

On 12/17/2011 21:56, David Miller wrote:

> From: Joshua Kinard <kumba@gentoo.org>
> Date: Sat, 17 Dec 2011 19:56:29 -0500
> 
>> +/* 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;
>> +
>> +
> 
> 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.


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.


>> +	/* Multicast filter. */
>> +	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.


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.


>> +		netdev_for_each_mc_addr(ha, dev)
>> +			set_bit((ether_crc(ETH_ALEN, ha->addr) >> 26),
>> +				    (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.

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
kumba@gentoo.org
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

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

[David Miller]

From: Joshua Kinard <kumba@gentoo.org>
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.

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

[Sergei Shtylyov]

Hello.

On 18-12-2011 4:56, Joshua Kinard wrote:

> 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<kumba@gentoo.org>
> ---

    Some minor nits below...

>   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
[...]
> @@ -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;
> +
> +

    On empty oine would be enough...

>   /*
>    * This structure is private to each device. It is used to pass
>    * packets in and out, so there is place for a packet
> @@ -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);

    Parens not needed here.

> +
> +	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;

     This block is over-indented.

> +	} 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;

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

> +	}
> +
> +	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,

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

WBR, Sergei

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

[Joshua Kinard]

On 12/18/2011 08:26, Sergei Shtylyov wrote:

>> @@ -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;
>> +
>> +
> 
>    On empty oine would be enough...
> 


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


>>   /*
>>    * This structure is private to each device. It is used to pass
>>    * packets in and out, so there is place for a packet
>> @@ -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);
> 
>    Parens not needed here.
> 


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


>> +
>> +    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;
> 
>     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.


>> +    } 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;
> 
>    This last statement is common between all branches, so could be moved out
> of *if*...
> 


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,
>> -    .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,
> 
>    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!

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
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

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

[Joshua Kinard]

On 12/18/2011 00:19, David Miller wrote:

> From: Joshua Kinard <kumba@gentoo.org>
> 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.


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,

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
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

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

[Joshua Kinard]

On 12/17/2011 21:56, David Miller wrote:


>> +		netdev_for_each_mc_addr(ha, dev)
>> +			set_bit((ether_crc(ETH_ALEN, ha->addr) >> 26),
>> +				    (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.
> 


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,

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
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

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

[Joshua Kinard]

SGI IP32 (O2)'s ethernet driver (meth) lacks meth_set_rx_mode, 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 <kumba@gentoo.org>
---

 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>

 #include <asm/ip32/mace.h>
 #include <asm/ip32/ip32_ints.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;

+	/* Multicast filter. */
+	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,
 };

 /*

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

[David Miller]

From: Joshua Kinard <kumba@gentoo.org>
Date: Sat, 24 Dec 2011 20:45:32 -0500

> SGI IP32 (O2)'s ethernet driver (meth) lacks meth_set_rx_mode, 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 <kumba@gentoo.org>

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.

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

[Joshua Kinard]

On 12/26/2011 15:17, David Miller wrote:

> From: Joshua Kinard <kumba@gentoo.org>
> Date: Sat, 24 Dec 2011 20:45:32 -0500
> 
>> SGI IP32 (O2)'s ethernet driver (meth) lacks meth_set_rx_mode, 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 <kumba@gentoo.org>
> 
> 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.


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,

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
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

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

[Joshua Kinard]

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 <kumba@gentoo.org>
---

 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);

 /*
+ * 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
+
+/*
  * 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;
 	/* address of PHY, used by mdio_* functions, initialized in mdio_probe */
@@ -79,6 +87,9 @@ struct meth_private {
 	struct sk_buff *rx_skbs[RX_RING_ENTRIES];
 	unsigned long rx_write;

+	/* Multicast filter. */
+	u64 mcast_filter;
+
 	spinlock_t meth_lock;
 };

@@ -765,6 +776,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 +819,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,
 };

 /*

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

[David Miller]

From: Joshua Kinard <kumba@gentoo.org>
Date: Tue, 27 Dec 2011 00:06:15 -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 <kumba@gentoo.org>

Applied to net-next, thanks.

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

[Stephen Hemminger]

On Tue, 27 Dec 2011 00:06:15 -0500
Joshua Kinard <kumba@gentoo.org> wrote:

> @@ -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;


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

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

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 27 Dec 2011 10:34:08 -0800

> On Tue, 27 Dec 2011 00:06:15 -0500
> Joshua Kinard <kumba@gentoo.org> wrote:
> 
>> @@ -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;
> 
> 
> This device driver writer needs to read:
>   Documentation/volatile-considered-harmful.txt

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

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

[Joshua Kinard]

On 12/27/2011 13:34, Stephen Hemminger wrote:

> On Tue, 27 Dec 2011 00:06:15 -0500
> Joshua Kinard <kumba@gentoo.org> wrote:
> 
>> @@ -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;
> 
> 
> This device driver writer needs to read:
>   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.

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
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

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

[Stephen Hemminger]

On Tue, 27 Dec 2011 16:29:57 -0500
Joshua Kinard <kumba@gentoo.org> wrote:

> 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.

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.

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

[Joshua Kinard]

On 12/27/2011 17:34, Stephen Hemminger wrote:

> On Tue, 27 Dec 2011 16:29:57 -0500
> Joshua Kinard <kumba@gentoo.org> wrote:
> 
>> 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.
> 
> 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.


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.

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
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

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

[Stephen Hemminger]

On Tue, 27 Dec 2011 17:48:32 -0500
Joshua Kinard <kumba@gentoo.org> wrote:

> On 12/27/2011 17:34, Stephen Hemminger wrote:
> 
> > On Tue, 27 Dec 2011 16:29:57 -0500
> > Joshua Kinard <kumba@gentoo.org> wrote:
> > 
> >> 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.
> > 
> > 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.
> 
> 
> 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.
> 

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 :-)