On Wed, 2010-10-20 at 22:25 +0100, Florian Fainelli wrote: > This patch fixes the following issues with the r6040 NIC operating in > multicast: > > 1) When the IFF_ALLMULTI flag is set, we should write 0xffff to the NIC hash > table registers to make it process multicast traffic > 2) When the number of multicast address to handle is smaller than MCAST_MAX > we should use the NIC multicast registers MID1_{L,M,H}. > 3) The hashing of the address was not correct, due to an invalid substraction > (15 - (crc & 0x0f)) instead of (crc & 0x0f) > Reported-by: Marc Leclerc > Tested-by: Marc Leclerc > Signed-off-by: Shawn Lin > Signed-off-by: Albert Chen > Signed-off-by: Florian Fainelli > CC: stable@kernel.org Remember you'll need to provide a different version for 2.6.27.y and 2.6.32.y. > --- > diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c > index 68a8419..3843363 100644 > --- a/drivers/net/r6040.c > +++ b/drivers/net/r6040.c > @@ -852,74 +852,90 @@ static void r6040_multicast_list(struct net_device *dev) > struct r6040_private *lp = netdev_priv(dev); > void __iomem *ioaddr = lp->base; > u16 *adrp; > - u16 reg; > unsigned long flags; > struct netdev_hw_addr *ha; > int i; > > - /* MAC Address */ > - adrp = (u16 *)dev->dev_addr; > - iowrite16(adrp[0], ioaddr + MID_0L); > - iowrite16(adrp[1], ioaddr + MID_0M); > - iowrite16(adrp[2], ioaddr + MID_0H); > - > - /* Promiscous Mode */ > spin_lock_irqsave(&lp->lock, flags); > > /* Clear AMCP & PROM bits */ > - reg = ioread16(ioaddr) & ~0x0120; > - if (dev->flags & IFF_PROMISC) { > - reg |= 0x0020; > + lp->mcr0 = ioread16(ioaddr) & ~0x0120; > + > + /* Promiscuous Mode */ > + if (dev->flags & IFF_PROMISC) > lp->mcr0 |= 0x0020; > - } > - /* Too many multicast addresses > - * accept all traffic */ > - else if ((netdev_mc_count(dev) > MCAST_MAX) || > - (dev->flags & IFF_ALLMULTI)) > - reg |= 0x0020; > > - iowrite16(reg, ioaddr); > - spin_unlock_irqrestore(&lp->lock, flags); > + /* Enable multicast hash table function to > + * receive all multicast packets. > + */ > + else if (dev->flags & IFF_ALLMULTI) { > + lp->mcr0 |= 0x0100; Please give these flags names. > > - /* Build the hash table */ > - if (netdev_mc_count(dev) > MCAST_MAX) { > - u16 hash_table[4]; > + for (i = 0; i < MCAST_MAX ; i++) { > + iowrite16(0, ioaddr + MID_1L + 8 * i); > + iowrite16(0, ioaddr + MID_1M + 8 * i); > + iowrite16(0, ioaddr + MID_1H + 8 * i); > + } > + > + iowrite16(0xffff, ioaddr + MAR0); > + iowrite16(0xffff, ioaddr + MAR1); > + iowrite16(0xffff, ioaddr + MAR2); > + iowrite16(0xffff, ioaddr + MAR3); > + } > + > + /* Use internal multicast address registers > + * if the number of multicast addresses is not greater than MCAST_MAX. > + */ > + else if (netdev_mc_empty(dev)) { > + for (i = 0; i < MCAST_MAX ; i++) { > + iowrite16(0, ioaddr + MID_1L + 8 * i); > + iowrite16(0, ioaddr + MID_1M + 8 * i); > + iowrite16(0, ioaddr + MID_1H + 8 * i); > + } > + } else if (netdev_mc_count(dev) <= MCAST_MAX) { > + i = 0; > + netdev_for_each_mc_addr(ha, dev) { > + adrp = (u16 *) ha->addr; > + iowrite16(adrp[0], ioaddr + MID_1L + 8 * i); > + iowrite16(adrp[1], ioaddr + MID_1M + 8 * i); > + iowrite16(adrp[2], ioaddr + MID_1H + 8 * i); > + i++; > + } What about the unused exact match entries? And why is the empty case special? > + } > + /* Otherwise, Enable multicast hash table function. */ > + else { > + u16 hash_table[4] = { 0, }; > u32 crc; > > - for (i = 0; i < 4; i++) > - hash_table[i] = 0; > + lp->mcr0 |= 0x0100; > > + for (i = 0; i < MCAST_MAX ; i++) { > + iowrite16(0, ioaddr + MID_1L + 8 * i); > + iowrite16(0, ioaddr + MID_1M + 8 * i); > + iowrite16(0, ioaddr + MID_1H + 8 * i); > + } > + > + /* Build multicast hash table */ > netdev_for_each_mc_addr(ha, dev) { > char *addrs = ha->addr; > > if (!(*addrs & 1)) > continue; > > - crc = ether_crc_le(6, addrs); > + crc = ether_crc(ETH_ALEN, addrs); You're reversing the order of bits in the CRC, which is not mentioned in the commit message; are you sure that's right? > crc >>= 26; > - hash_table[crc >> 4] |= 1 << (15 - (crc & 0xf)); > + hash_table[crc >> 4] |= 1 << (crc & 0xf); > } > + > /* Fill the MAC hash tables with their values */ > iowrite16(hash_table[0], ioaddr + MAR0); > iowrite16(hash_table[1], ioaddr + MAR1); > iowrite16(hash_table[2], ioaddr + MAR2); > iowrite16(hash_table[3], ioaddr + MAR3); > } > - /* Multicast Address 1~4 case */ > - i = 0; > - netdev_for_each_mc_addr(ha, dev) { > - if (i < MCAST_MAX) { > - adrp = (u16 *) ha->addr; > - iowrite16(adrp[0], ioaddr + MID_1L + 8 * i); > - iowrite16(adrp[1], ioaddr + MID_1M + 8 * i); > - iowrite16(adrp[2], ioaddr + MID_1H + 8 * i); > - } else { > - iowrite16(0xffff, ioaddr + MID_1L + 8 * i); > - iowrite16(0xffff, ioaddr + MID_1M + 8 * i); > - iowrite16(0xffff, ioaddr + MID_1H + 8 * i); > - } This conflicts with my patch in which Dave has already applied (but not pushed out). Ben. > - i++; > - } > + iowrite16(lp->mcr0, ioaddr); > + > + spin_unlock_irqrestore(&lp->lock, flags); > } > > static void netdev_get_drvinfo(struct net_device *dev, > -- > 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 > > -- Ben Hutchings Once a job is fouled up, anything done to improve it makes it worse.