net: meth: Add set_rx_mode hook to fix ICMPv6 neighbor discovery
Subsystems (3)
PATCH 1/1 net: meth: Add set_rx_mode hook to fix ICMPv6 neighbor discovery
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.
---
drivers/net/ethernet/sgi/meth.c | 60 +++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 7 deletions(-)
drivers/net/ethernet/sgi/meth.c +53 -7
@@ -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 Faststatic 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, };/*
David Miller
maintainerFrom: Joshua Kinard <[email protected]>
Date: Sat, 17 Dec 2011 19:56:29 -0500
drivers/net/ethernet/sgi/meth.c static const char *meth_str="SGI O2 Fast · line 66
Quoted 6 lines
drivers/net/ethernet/sgi/meth.c +6
-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 ; + +
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.
Quoted 3 lines
drivers/net/ethernet/sgi/meth.c +3
-0+/* Multicast filter. */ +unsigned long mcast_filter; +
...
Review
Quoted 1 line
(quoted diff) +1
-0+ 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.
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 804
Quoted 3 lines
drivers/net/ethernet/sgi/meth.c +3
-0+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.
Quoted 12 lines · David Miller
From: Joshua Kinard <[email protected]>
Date: Sat, 17 Dec 2011 19:56:29 -0500drivers/net/ethernet/sgi/meth.c static const char *meth_str="SGI O2 Fast · line 66
Quoted 6 lines
drivers/net/ethernet/sgi/meth.c +6
-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 ; + +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.
Quoted 9 lines
Quoted 3 lines
drivers/net/ethernet/sgi/meth.c +3
-0+/* Multicast filter. */ +unsigned long mcast_filter; +...
Review
Quoted 1 line
(quoted diff) +1
-0+ 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.
Quoted 8 lines
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 804
Quoted 3 lines
drivers/net/ethernet/sgi/meth.c +3
-0+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,
Signature · 9 lines
Joshua Kinard Gentoo/MIPS [email protected] 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
David Miller
maintainerFrom: Joshua Kinard <[email protected]>
Date: Sat, 17 Dec 2011 23:37:01 -0500
Quoted 1 line
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.
Quoted 10 lines · David Miller
From: Joshua Kinard <[email protected]>
Date: Sat, 17 Dec 2011 23:37:01 -0500Quoted 1 line
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,
Signature · 9 lines
Joshua Kinard Gentoo/MIPS [email protected] 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
Quoted 7 lines · David Miller
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 804
Quoted 3 lines
drivers/net/ethernet/sgi/meth.c +3
-0+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,
Signature · 9 lines
Joshua Kinard Gentoo/MIPS [email protected] 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
Hello.
Quoted 8 lines · Joshua Kinard
SGI IP32 (O2)'s ethernet driver (meth) lacks a
set_rx_modefunction, 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 protected]>---
Some minor nits below...
Quoted 5 lines
drivers/net/ethernet/sgi/meth.c | 60 +++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 7 deletions(-)drivers/net/ethernet/sgi/meth.c +0
-0
[...]
drivers/net/ethernet/sgi/meth.c static const char *meth_str="SGI O2 Fast · line 66
Quoted 10 lines
drivers/net/ethernet/sgi/meth.c +6
-0@@ -57,6 +58,12 @@ static const char *meth_str="SGI O2 Faststatic 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...
Quoted 5 lines
drivers/net/ethernet/sgi/meth.c +0
-0/* * 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 }
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 785
Quoted 10 lines
drivers/net/ethernet/sgi/meth.c +8
-0} +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.
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 797
Quoted 12 lines
drivers/net/ethernet/sgi/meth.c +12
-0+ +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.
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 806
Quoted 9 lines
drivers/net/ethernet/sgi/meth.c +9
-0+ }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*...
Quoted 13 lines
drivers/net/ethernet/sgi/meth.c +6
-6+ } + +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,
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 822
Quoted 10 lines
drivers/net/ethernet/sgi/meth.c +8
-1- .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
Quoted 4 lines · Sergei Shtylyov
Quoted 4 lines
drivers/net/ethernet/sgi/meth.c +0
-0@@ -57,6 +58,12 @@ static const char *meth_str="SGI O2 Faststatic int timeout =TX_TIMEOUT ;module_param (timeout, int,0 );
Quoted 9 lines
drivers/net/ethernet/sgi/meth.c static const char *meth_str="SGI O2 Fast · line 66
Quoted 6 lines
drivers/net/ethernet/sgi/meth.c +6
-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.
Quoted 7 lines
Quoted 7 lines
drivers/net/ethernet/sgi/meth.c +0
-0/* * 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 } }
Quoted 11 lines
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device
Quoted 8 lines
drivers/net/ethernet/sgi/meth.c +8
-0+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 :)
Quoted 13 lines
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 797
Quoted 12 lines
drivers/net/ethernet/sgi/meth.c +12
-0+ +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;
Quoted 2 lines
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.
Quoted 4 lines
Quoted 4 lines
drivers/net/ethernet/sgi/meth.c +4
-0+ }else { +struct netdev_hw_addr *ha; + priv->mac_ctrl |=METH_ACCEPT_MCAST ; +
Quoted 9 lines
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 806
Quoted 5 lines
drivers/net/ethernet/sgi/meth.c +5
-0+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.
Quoted 17 lines
Quoted 17 lines
drivers/net/ethernet/sgi/meth.c +9
-7+ } + +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,
Quoted 8 lines
drivers/net/ethernet/sgi/meth.c static int meth_ioctl(struct net_device · line 822
Quoted 6 lines
drivers/net/ethernet/sgi/meth.c +5
-0+ .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!
Signature · 9 lines
Joshua Kinard Gentoo/MIPS [email protected] 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
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.
---
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.hb/arch/mips/include/asm/ip32/mace.h
arch/mips/include/asm/ip32/mace.h +26 -26
@@ -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; };/*
drivers/net/ethernet/sgi/meth.c +56 -6
117 lines not on this page · raw message
David Miller
maintainerFrom: Joshua Kinard <[email protected]>
Date: Sat, 24 Dec 2011 20:45:32 -0500
Quoted 7 lines
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 <[email protected]>
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.
Quoted 9 lines · David Miller
From: Joshua Kinard <[email protected]>
Date: Sat, 24 Dec 2011 20:45:32 -0500Quoted 6 lines
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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Joshua Kinard <[email protected]>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,
Signature · 9 lines
Joshua Kinard Gentoo/MIPS [email protected] 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
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.
---
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.hb/arch/mips/include/asm/ip32/mace.h
arch/mips/include/asm/ip32/mace.h +1 -1
@@ -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;
drivers/net/ethernet/sgi/meth.c +47 -1
83 lines not on this page · raw message
From: Joshua Kinard <[email protected]>
Date: Tue, 27 Dec 2011 00:06:15 -0500
Quoted 7 lines
SGI IP32 (O2)'s ethernet driver (meth) lacks a
set_rx_modefunction, 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 protected]>
Applied to net-next, thanks.
arch/mips/include/asm/ip32/mace.h struct mace_video { · line 101
Quoted 9 lines · On Tue, 27 Dec 2011 00:06:15 -0500 Joshua Kinard <[email protected]> wrote:
arch/mips/include/asm/ip32/mace.h +1
-1@@ -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
David Miller
maintainerFrom: Stephen Hemminger <[email protected]>
Date: Tue, 27 Dec 2011 10:34:08 -0800
Quoted 16 lines
arch/mips/include/asm/ip32/mace.h struct mace_video { · line 101
Quoted 9 lines · On Tue, 27 Dec 2011 00:06:15 -0500 Joshua Kinard <[email protected]> wrote:
arch/mips/include/asm/ip32/mace.h +1
-1@@ -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.
Quoted 16 lines · Stephen Hemminger
arch/mips/include/asm/ip32/mace.h struct mace_video { · line 101
Quoted 9 lines · On Tue, 27 Dec 2011 00:06:15 -0500 Joshua Kinard <[email protected]> wrote:
arch/mips/include/asm/ip32/mace.h +1
-1@@ -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.
Signature · 9 lines
Joshua Kinard Gentoo/MIPS [email protected] 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
Quoted 3 lines · On Tue, 27 Dec 2011 16:29:57 -0500 Joshua Kinard <[email protected]> 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.
Quoted 11 lines · Stephen Hemminger
Quoted 3 lines · On Tue, 27 Dec 2011 16:29:57 -0500 Joshua Kinard <[email protected]> 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.
Signature · 9 lines
Joshua Kinard Gentoo/MIPS [email protected] 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
Quoted 15 lines · On Tue, 27 Dec 2011 17:48:32 -0500 Joshua Kinard <[email protected]> wrote:
Quoted 11 lines · Stephen Hemminger
Quoted 3 lines · On Tue, 27 Dec 2011 16:29:57 -0500 Joshua Kinard <[email protected]> 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.
Quoted 5 lines
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 :-)