| Message ID | 4EED3A3D.9080503@gentoo.org |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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,
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,
--- 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, }; /*
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html