All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] r6040: fix multicast operations
@ 2010-10-20 21:09 Florian Fainelli
  2010-10-21  2:55 ` Ben Hutchings
  2010-11-03 12:51 ` Shawn Lin
  0 siblings, 2 replies; 8+ messages in thread
From: Florian Fainelli @ 2010-10-20 21:09 UTC (permalink / raw)
  To: netdev, Shawn Lin, Marc Leclerc, Albert Chen, David Miller

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 <marc-leclerc@signaturealpha.com>
Tested-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
Signed-off-by: Shawn Lin <shawn@dmp.com.tw>
Signed-off-by: Albert Chen <albert.chen@rdc.com.tw>
Signed-off-by: Florian Fainelli <florian@openwrt.org>
CC: stable@kernel.org
---
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;
 
-	/* 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++;
+		}
+	}
+	/* 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);
 			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);
-		}
-		i++;
-	}
+	iowrite16(lp->mcr0, ioaddr);
+
+	spin_unlock_irqrestore(&lp->lock, flags);
 }
 
 static void netdev_get_drvinfo(struct net_device *dev,

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] r6040: fix multicast operations
  2010-10-20 21:09 [PATCH 1/2] r6040: fix multicast operations Florian Fainelli
@ 2010-10-21  2:55 ` Ben Hutchings
  2010-10-21  8:27   ` Shawn Lin
  2010-11-03 12:51 ` Shawn Lin
  1 sibling, 1 reply; 8+ messages in thread
From: Ben Hutchings @ 2010-10-21  2:55 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, Shawn Lin, Marc Leclerc, Albert Chen, David Miller

[-- Attachment #1: Type: text/plain, Size: 5893 bytes --]

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 <marc-leclerc@signaturealpha.com>
> Tested-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
> Signed-off-by: Shawn Lin <shawn@dmp.com.tw>
> Signed-off-by: Albert Chen <albert.chen@rdc.com.tw>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> 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
<http://article.gmane.org/gmane.linux.network/174926> 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] r6040: fix multicast operations
  2010-10-21  2:55 ` Ben Hutchings
@ 2010-10-21  8:27   ` Shawn Lin
  2010-10-26 14:02     ` Ben Hutchings
  0 siblings, 1 reply; 8+ messages in thread
From: Shawn Lin @ 2010-10-21  8:27 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Florian Fainelli, netdev, Marc Leclerc, Albert Chen, David Miller

Dear Ben,

I help to answer your questions.

On Thu, 2010-10-21 at 03:55 +0100, Ben Hutchings wrote:
> 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 <marc-leclerc@signaturealpha.com>
> > Tested-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
> > Signed-off-by: Shawn Lin <shawn@dmp.com.tw>
> > Signed-off-by: Albert Chen <albert.chen@rdc.com.tw>
> > Signed-off-by: Florian Fainelli <florian@openwrt.org>
> > 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?

Unused exact match entries? I am not sure which entries are you
mentioned.

There are five match entries in the code:

1) if (dev->flags & IFF_PROMISC)
then write a hardware flag to enable promiscuous function.


2) if (netdev_mc_count(dev) <= 3)
There are two hardware features could be used to filter multicast
frames:

[1] Three extra MAC/Multicast registers that could be used to get the
exact frames from specified multicast addresses.

[2] Multicast hash table registers.

I follow the original code to write exact multicast addresses to the
three registers when they are not greater than 3.


3) if (netdev_mc_empty(dev))
because we masked the multicast hash table flag before examine all
conditions, we only need to clear the addresses in the three
MAC/Multicast registers.


4) if (dev->flags & IFF_ALLMULTI)
then enable multicast hash table function and write 0xffff to all
multicast hash table registers.


5) else
to enablue multicast hash table and calculate corresponding bit
according to each multicast address.

> 
> > +	}
> > +	/* 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?

This hash alogorithm is provided by RDC's engineers.

We also verified on different hardware platforms.

I will double confirm it with RDC's engineers.

> 
> >  			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
> <http://article.gmane.org/gmane.linux.network/174926> 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
> > 
> > 
> 

Hope some issues have been clarified.

--
Regards,

Shawn Lin


===========================================================================================
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. 
If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. 
Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of DM&P Group is strictly prohibited; and any information in this email irrelevant to the official business of DM&P Group shall be deemed as neither given nor endorsed by DM&P Group.

===========================================================================================

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] r6040: fix multicast operations
  2010-10-21  8:27   ` Shawn Lin
@ 2010-10-26 14:02     ` Ben Hutchings
  2010-10-27  1:19       ` Shawn Lin
  0 siblings, 1 reply; 8+ messages in thread
From: Ben Hutchings @ 2010-10-26 14:02 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Florian Fainelli, netdev, Marc Leclerc, Albert Chen, David Miller

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

On Thu, Oct 21, 2010 at 04:27:34PM +0800, Shawn Lin wrote:
[...]
> > > +	/* 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?
> 
> Unused exact match entries? I am not sure which entries are you
> mentioned.

If there are 1 or 2 addresses in the multicast list then some of the
exact match entries will be used and some will not.  But the loop
above does not clear the unused entries.

[...]
> 2) if (netdev_mc_count(dev) <= 3)
> There are two hardware features could be used to filter multicast
> frames:
[...]
> 3) if (netdev_mc_empty(dev))
> because we masked the multicast hash table flag before examine all
> conditions, we only need to clear the addresses in the three
> MAC/Multicast registers.
[...]

But why is this so different from the case of 1-3 addresses?  I would
write these two cases as:

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++;
	}
	while (i < MCAST_MAX) {
		iowrite16(0, ioaddr + MID_1L + 8 * i);
		iowrite16(0, ioaddr + MID_1M + 8 * i);
		iowrite16(0, ioaddr + MID_1H + 8 * i);
		i++;
	}
} 

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] r6040: fix multicast operations
  2010-10-26 14:02     ` Ben Hutchings
@ 2010-10-27  1:19       ` Shawn Lin
  0 siblings, 0 replies; 8+ messages in thread
From: Shawn Lin @ 2010-10-27  1:19 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Florian Fainelli, netdev, Marc Leclerc, Albert Chen, David Miller

On Tue, 2010-10-26 at 15:02 +0100, Ben Hutchings wrote:
> On Thu, Oct 21, 2010 at 04:27:34PM +0800, Shawn Lin wrote:
> [...]
> > > > +	/* 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?
> > 
> > Unused exact match entries? I am not sure which entries are you
> > mentioned.
> 
> If there are 1 or 2 addresses in the multicast list then some of the
> exact match entries will be used and some will not.  But the loop
> above does not clear the unused entries.
> 

Yes, you are right.

> [...]
> > 2) if (netdev_mc_count(dev) <= 3)
> > There are two hardware features could be used to filter multicast
> > frames:
> [...]
> > 3) if (netdev_mc_empty(dev))
> > because we masked the multicast hash table flag before examine all
> > conditions, we only need to clear the addresses in the three
> > MAC/Multicast registers.
> [...]
> 
> But why is this so different from the case of 1-3 addresses?  I would
> write these two cases as:
> 
> 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++;
> 	}
> 	while (i < MCAST_MAX) {
> 		iowrite16(0, ioaddr + MID_1L + 8 * i);
> 		iowrite16(0, ioaddr + MID_1M + 8 * i);
> 		iowrite16(0, ioaddr + MID_1H + 8 * i);
> 		i++;
> 	}
> } 
> 
> Ben.
> 

Thanks.

I will fix the issues you mentioned and resubmit the code.

--
Regards,

Shawn Lin


===========================================================================================
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. 
If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. 
Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of DM&P Group is strictly prohibited; and any information in this email irrelevant to the official business of DM&P Group shall be deemed as neither given nor endorsed by DM&P Group.

===========================================================================================

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] r6040: fix multicast operations
  2010-10-20 21:09 [PATCH 1/2] r6040: fix multicast operations Florian Fainelli
  2010-10-21  2:55 ` Ben Hutchings
@ 2010-11-03 12:51 ` Shawn Lin
  2010-11-04 10:04   ` Florian Fainelli
  1 sibling, 1 reply; 8+ messages in thread
From: Shawn Lin @ 2010-11-03 12:51 UTC (permalink / raw)
  To: netdev, Marc Leclerc, Albert Chen, David Miller, Florian Fainelli

On Wed, 2010-10-20 at 23:09 +0200, 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)

I suggest to modify the comment as follows.

3) The hashing of the address was not correct, due to an invalid
 substraction (15 - (crc & 0x0f)) instead of (crc & 0x0f) and an
 incorrect crc algorithm (ether_crc_le) instead of (ether_crc).

[...]

The original code I submitted to Florian has some issues mentioned by Ben Hutchings.

This revision fixes these issues and another issue about the sequence of configuring multicast hash table registers.

The correct sequence is to enable multicast function before write values to hash table registers. I have verified it on my platform.

The hash algorithm is provided by hardware designers. I also re-confirmed it with RDC's engineer.

Please let me know if anyone has questions.

The version is for net-next-2.6:

---
diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
index 0b014c8..e88e171 100644
--- a/drivers/net/r6040.c
+++ b/drivers/net/r6040.c
@@ -69,6 +69,8 @@
 
 /* MAC registers */
 #define MCR0		0x00	/* Control register 0 */
+#define  PROMISC	0x0020  /* Promiscuous mode */
+#define  HASH_EN	0x0100  /* Enable multicast hash table function */
 #define MCR1		0x04	/* Control register 1 */
 #define  MAC_RST	0x0001	/* Reset the MAC */
 #define MBCR		0x08	/* Bus control */
@@ -851,77 +853,84 @@ 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;
+	u16 hash_table[4] = { 0, };
 
-	/* 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;
+	lp->mcr0 = ioread16(ioaddr + MCR0) & ~(PROMISC | HASH_EN);
+
+	/* Promiscuous Mode */
 	if (dev->flags & IFF_PROMISC) {
-		reg |= 0x0020;
-		lp->mcr0 |= 0x0020;
+		lp->mcr0 |= PROMISC;
 	}
-	/* 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 |= HASH_EN;
+
+		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 the hash table */
-	if (netdev_mc_count(dev) > MCAST_MAX) {
-		u16 hash_table[4];
+		for (i = 0; i < 4; i++)
+			hash_table[i] = 0xffff;
+	}
+	/* Use internal multicast address registers if the number of
+	 * multicast addresses is not greater than MCAST_MAX. */
+	else if (netdev_mc_count(dev) <= MCAST_MAX) {
+		i = 0;
+		netdev_for_each_mc_addr(ha, dev) {
+			u16 *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++;
+		}
+		while (i < MCAST_MAX) {
+			iowrite16(0, ioaddr + MID_1L + 8 * i);
+			iowrite16(0, ioaddr + MID_1M + 8 * i);
+			iowrite16(0, ioaddr + MID_1H + 8 * i);
+			i++;
+		}
+	}
+	/* Otherwise, Enable multicast hash table function. */
+	else {
 		u32 crc;
 
-		for (i = 0; i < 4; i++)
-			hash_table[i] = 0;
+		lp->mcr0 |= HASH_EN;
 
-		netdev_for_each_mc_addr(ha, dev) {
-			char *addrs = ha->addr;
+		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);
+		}
 
-			if (!(*addrs & 1))
-				continue;
+		/* Build multicast hash table */
+		netdev_for_each_mc_addr(ha, dev) {
+			u8 *addrs = ha->addr;
 
-			crc = ether_crc_le(6, addrs);
+			crc = ether_crc(ETH_ALEN, addrs);
 			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(lp->mcr0, ioaddr + MCR0);
+
+	/* Fill the MAC hash tables with their values */
+	if (lp->mcr0 && HASH_EN) {
 		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)
-			break;
-		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++;
-	}
-	while (i < MCAST_MAX) {
-		iowrite16(0xffff, ioaddr + MID_1L + 8 * i);
-		iowrite16(0xffff, ioaddr + MID_1M + 8 * i);
-		iowrite16(0xffff, ioaddr + MID_1H + 8 * i);
-		i++;
-	}
+
+	spin_unlock_irqrestore(&lp->lock, flags);
 }
 
 static void netdev_get_drvinfo(struct net_device *dev,
---

The version is for 2.6.32.y and 2.6.27.y:

---
diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
index 9ee9f01..f9af419 100644
--- a/drivers/net/r6040.c
+++ b/drivers/net/r6040.c
@@ -69,6 +69,8 @@
 
 /* MAC registers */
 #define MCR0		0x00	/* Control register 0 */
+#define  PROMISC	0x0020  /* Promiscuous mode */
+#define  HASH_EN	0x0100  /* Enable multicast hash table function */
 #define MCR1		0x04	/* Control register 1 */
 #define  MAC_RST	0x0001	/* Reset the MAC */
 #define MBCR		0x08	/* Bus control */
@@ -935,76 +937,88 @@ 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 dev_mc_list *dmi = dev->mc_list;
 	int i;
+	u16 hash_table[4] = { 0, };
 
-	/* 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;
+	lp->mcr0 = ioread16(ioaddr + MCR0) & ~(PROMISC | HASH_EN);
+
+	/* Promiscuous Mode */
 	if (dev->flags & IFF_PROMISC) {
-		reg |= 0x0020;
-		lp->mcr0 |= 0x0020;
+		lp->mcr0 |= PROMISC;
 	}
-	/* Too many multicast addresses
-	 * accept all traffic */
-	else if ((dev->mc_count > MCAST_MAX)
-		|| (dev->flags & IFF_ALLMULTI))
-		reg |= 0x0020;
+	/* Enable multicast hash table function to
+	 * receive all multicast packets. */
+	else if (dev->flags & IFF_ALLMULTI) {
+		lp->mcr0 |= HASH_EN;
+
+		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(reg, ioaddr);
-	spin_unlock_irqrestore(&lp->lock, flags);
+		for (i = 0; i < 4; i++)
+			hash_table[i] = 0xffff;
+	}
+	/* Use internal multicast address registers if the number of
+	 * multicast addresses is not greater than MCAST_MAX. */
+	else if (dev->mc_count <= MCAST_MAX) {
+		i = 0;
+		while (i < dev->mc_count) {
+			u16 *adrp = (u16 *) dmi->dmi_addr;
+			dmi = dmi->next;
 
-	/* Build the hash table */
-	if (dev->mc_count > MCAST_MAX) {
-		u16 hash_table[4];
+			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++;
+		}
+		while (i < MCAST_MAX) {
+			iowrite16(0, ioaddr + MID_1L + 8 * i);
+			iowrite16(0, ioaddr + MID_1M + 8 * i);
+			iowrite16(0, ioaddr + MID_1H + 8 * i);
+			i++;
+		}
+	}
+	/* Otherwise, Enable multicast hash table function. */
+	else {
 		u32 crc;
 
-		for (i = 0; i < 4; i++)
-			hash_table[i] = 0;
+		lp->mcr0 |= HASH_EN;
 
-		for (i = 0; i < dev->mc_count; i++) {
-			char *addrs = dmi->dmi_addr;
+		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 */
+		for (i = 0; i < dev->mc_count; i++) {
+			u8 *addrs = dmi->dmi_addr;
 			dmi = dmi->next;
 
-			if (!(*addrs & 1))
-				continue;
-
-			crc = ether_crc_le(6, addrs);
+			crc = ether_crc(ETH_ALEN, addrs);
 			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(lp->mcr0, ioaddr + MCR0);
+
+	/* Fill the MAC hash tables with their values */
+	if (lp->mcr0 && HASH_EN) {
 		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 */
-	dmi = dev->mc_list;
-	for (i = 0, dmi; (i < dev->mc_count) && (i < MCAST_MAX); i++) {
-		adrp = (u16 *)dmi->dmi_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);
-		dmi = dmi->next;
-	}
-	for (i = dev->mc_count; i < MCAST_MAX; i++) {
-		iowrite16(0xffff, ioaddr + MID_1L + 8*i);
-		iowrite16(0xffff, ioaddr + MID_1M + 8*i);
-		iowrite16(0xffff, ioaddr + MID_1H + 8*i);
-	}
+
+	spin_unlock_irqrestore(&lp->lock, flags);
 }
 
 static void netdev_get_drvinfo(struct net_device *dev,
---



===========================================================================================
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. 
If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. 
Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of DM&P Group is strictly prohibited; and any information in this email irrelevant to the official business of DM&P Group shall be deemed as neither given nor endorsed by DM&P Group.

===========================================================================================

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] r6040: fix multicast operations
  2010-11-03 12:51 ` Shawn Lin
@ 2010-11-04 10:04   ` Florian Fainelli
  2010-11-11  8:39     ` Shawn Lin
  0 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2010-11-04 10:04 UTC (permalink / raw)
  To: Shawn Lin; +Cc: netdev, Marc Leclerc, Albert Chen, David Miller, Ben Hutchings

Hello Shawn,

On Wednesday 03 November 2010 13:51:29 Shawn Lin wrote:
> On Wed, 2010-10-20 at 23:09 +0200, 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)
> 
> I suggest to modify the comment as follows.
> 
> 3) The hashing of the address was not correct, due to an invalid
>  substraction (15 - (crc & 0x0f)) instead of (crc & 0x0f) and an
>  incorrect crc algorithm (ether_crc_le) instead of (ether_crc).
> 
> [...]
> 
> The original code I submitted to Florian has some issues mentioned by Ben
> Hutchings.
> 
> This revision fixes these issues and another issue about the sequence of
> configuring multicast hash table registers.
> 
> The correct sequence is to enable multicast function before write values to
> hash table registers. I have verified it on my platform.
> 
> The hash algorithm is provided by hardware designers. I also re-confirmed
> it with RDC's engineer.
> 
> Please let me know if anyone has questions.
> 
> The version is for net-next-2.6:

Please resubmit the patch with your Signed-off-by tag and the Tested-by: 
to keep track of the issue. Thank you!

> 
> ---
> diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
> index 0b014c8..e88e171 100644
> --- a/drivers/net/r6040.c
> +++ b/drivers/net/r6040.c
> @@ -69,6 +69,8 @@
> 
>  /* MAC registers */
>  #define MCR0		0x00	/* Control register 0 */
> +#define  PROMISC	0x0020  /* Promiscuous mode */
> +#define  HASH_EN	0x0100  /* Enable multicast hash table function */
>  #define MCR1		0x04	/* Control register 1 */
>  #define  MAC_RST	0x0001	/* Reset the MAC */
>  #define MBCR		0x08	/* Bus control */
> @@ -851,77 +853,84 @@ 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;
> +	u16 hash_table[4] = { 0, };
> 
> -	/* 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;
> +	lp->mcr0 = ioread16(ioaddr + MCR0) & ~(PROMISC | HASH_EN);
> +
> +	/* Promiscuous Mode */
>  	if (dev->flags & IFF_PROMISC) {
> -		reg |= 0x0020;
> -		lp->mcr0 |= 0x0020;
> +		lp->mcr0 |= PROMISC;
>  	}
> -	/* 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 |= HASH_EN;
> +
> +		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 the hash table */
> -	if (netdev_mc_count(dev) > MCAST_MAX) {
> -		u16 hash_table[4];
> +		for (i = 0; i < 4; i++)
> +			hash_table[i] = 0xffff;
> +	}
> +	/* Use internal multicast address registers if the number of
> +	 * multicast addresses is not greater than MCAST_MAX. */
> +	else if (netdev_mc_count(dev) <= MCAST_MAX) {
> +		i = 0;
> +		netdev_for_each_mc_addr(ha, dev) {
> +			u16 *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++;
> +		}
> +		while (i < MCAST_MAX) {
> +			iowrite16(0, ioaddr + MID_1L + 8 * i);
> +			iowrite16(0, ioaddr + MID_1M + 8 * i);
> +			iowrite16(0, ioaddr + MID_1H + 8 * i);
> +			i++;
> +		}
> +	}
> +	/* Otherwise, Enable multicast hash table function. */
> +	else {
>  		u32 crc;
> 
> -		for (i = 0; i < 4; i++)
> -			hash_table[i] = 0;
> +		lp->mcr0 |= HASH_EN;
> 
> -		netdev_for_each_mc_addr(ha, dev) {
> -			char *addrs = ha->addr;
> +		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);
> +		}
> 
> -			if (!(*addrs & 1))
> -				continue;
> +		/* Build multicast hash table */
> +		netdev_for_each_mc_addr(ha, dev) {
> +			u8 *addrs = ha->addr;
> 
> -			crc = ether_crc_le(6, addrs);
> +			crc = ether_crc(ETH_ALEN, addrs);
>  			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(lp->mcr0, ioaddr + MCR0);
> +
> +	/* Fill the MAC hash tables with their values */
> +	if (lp->mcr0 && HASH_EN) {
>  		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)
> -			break;
> -		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++;
> -	}
> -	while (i < MCAST_MAX) {
> -		iowrite16(0xffff, ioaddr + MID_1L + 8 * i);
> -		iowrite16(0xffff, ioaddr + MID_1M + 8 * i);
> -		iowrite16(0xffff, ioaddr + MID_1H + 8 * i);
> -		i++;
> -	}
> +
> +	spin_unlock_irqrestore(&lp->lock, flags);
>  }
> 
>  static void netdev_get_drvinfo(struct net_device *dev,
> ---
> 
> The version is for 2.6.32.y and 2.6.27.y:
> 
> ---
> diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
> index 9ee9f01..f9af419 100644
> --- a/drivers/net/r6040.c
> +++ b/drivers/net/r6040.c
> @@ -69,6 +69,8 @@
> 
>  /* MAC registers */
>  #define MCR0		0x00	/* Control register 0 */
> +#define  PROMISC	0x0020  /* Promiscuous mode */
> +#define  HASH_EN	0x0100  /* Enable multicast hash table function */
>  #define MCR1		0x04	/* Control register 1 */
>  #define  MAC_RST	0x0001	/* Reset the MAC */
>  #define MBCR		0x08	/* Bus control */
> @@ -935,76 +937,88 @@ 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 dev_mc_list *dmi = dev->mc_list;
>  	int i;
> +	u16 hash_table[4] = { 0, };
> 
> -	/* 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;
> +	lp->mcr0 = ioread16(ioaddr + MCR0) & ~(PROMISC | HASH_EN);
> +
> +	/* Promiscuous Mode */
>  	if (dev->flags & IFF_PROMISC) {
> -		reg |= 0x0020;
> -		lp->mcr0 |= 0x0020;
> +		lp->mcr0 |= PROMISC;
>  	}
> -	/* Too many multicast addresses
> -	 * accept all traffic */
> -	else if ((dev->mc_count > MCAST_MAX)
> -		|| (dev->flags & IFF_ALLMULTI))
> -		reg |= 0x0020;
> +	/* Enable multicast hash table function to
> +	 * receive all multicast packets. */
> +	else if (dev->flags & IFF_ALLMULTI) {
> +		lp->mcr0 |= HASH_EN;
> +
> +		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(reg, ioaddr);
> -	spin_unlock_irqrestore(&lp->lock, flags);
> +		for (i = 0; i < 4; i++)
> +			hash_table[i] = 0xffff;
> +	}
> +	/* Use internal multicast address registers if the number of
> +	 * multicast addresses is not greater than MCAST_MAX. */
> +	else if (dev->mc_count <= MCAST_MAX) {
> +		i = 0;
> +		while (i < dev->mc_count) {
> +			u16 *adrp = (u16 *) dmi->dmi_addr;
> +			dmi = dmi->next;
> 
> -	/* Build the hash table */
> -	if (dev->mc_count > MCAST_MAX) {
> -		u16 hash_table[4];
> +			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++;
> +		}
> +		while (i < MCAST_MAX) {
> +			iowrite16(0, ioaddr + MID_1L + 8 * i);
> +			iowrite16(0, ioaddr + MID_1M + 8 * i);
> +			iowrite16(0, ioaddr + MID_1H + 8 * i);
> +			i++;
> +		}
> +	}
> +	/* Otherwise, Enable multicast hash table function. */
> +	else {
>  		u32 crc;
> 
> -		for (i = 0; i < 4; i++)
> -			hash_table[i] = 0;
> +		lp->mcr0 |= HASH_EN;
> 
> -		for (i = 0; i < dev->mc_count; i++) {
> -			char *addrs = dmi->dmi_addr;
> +		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 */
> +		for (i = 0; i < dev->mc_count; i++) {
> +			u8 *addrs = dmi->dmi_addr;
>  			dmi = dmi->next;
> 
> -			if (!(*addrs & 1))
> -				continue;
> -
> -			crc = ether_crc_le(6, addrs);
> +			crc = ether_crc(ETH_ALEN, addrs);
>  			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(lp->mcr0, ioaddr + MCR0);
> +
> +	/* Fill the MAC hash tables with their values */
> +	if (lp->mcr0 && HASH_EN) {
>  		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 */
> -	dmi = dev->mc_list;
> -	for (i = 0, dmi; (i < dev->mc_count) && (i < MCAST_MAX); i++) {
> -		adrp = (u16 *)dmi->dmi_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);
> -		dmi = dmi->next;
> -	}
> -	for (i = dev->mc_count; i < MCAST_MAX; i++) {
> -		iowrite16(0xffff, ioaddr + MID_1L + 8*i);
> -		iowrite16(0xffff, ioaddr + MID_1M + 8*i);
> -		iowrite16(0xffff, ioaddr + MID_1H + 8*i);
> -	}
> +
> +	spin_unlock_irqrestore(&lp->lock, flags);
>  }
> 
>  static void netdev_get_drvinfo(struct net_device *dev,
> ---
> 
> 
> 
> ===========================================================================
> ================ The privileged confidential information contained in this
> email is intended for use only by the addressees as indicated by the
> original sender of this email. If you are not the addressee indicated in
> this email or are not responsible for delivery of the email to such a
> person, please kindly reply to the sender indicating this fact and delete
> all copies of it from your computer and network server immediately. Your
> cooperation is highly appreciated. It is advised that any unauthorized use
> of confidential information of DM&P Group is strictly prohibited; and any
> information in this email irrelevant to the official business of DM&P
> Group shall be deemed as neither given nor endorsed by DM&P Group.
> 
> ===========================================================================
> ================

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] r6040: fix multicast operations
  2010-11-04 10:04   ` Florian Fainelli
@ 2010-11-11  8:39     ` Shawn Lin
  0 siblings, 0 replies; 8+ messages in thread
From: Shawn Lin @ 2010-11-11  8:39 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, Marc Leclerc, Albert Chen, David Miller, Ben Hutchings

The original code does not work well when the number of mulitcast
address to handle is greater than MCAST_MAX. It only enable promiscous
mode instead of multicast hash table mode, so the hash table function
will not be activated and all multicast frames will be recieved in this
condition.

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) and an
incorrect crc algorithm (ether_crc_le) instead of (ether_crc).

4) If necessary, we should set HASH_EN flag in MCR0 to enable multicast
hash table function.


The version is for net-next-2.6:

Reported-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
Tested-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
Signed-off-by: Shawn Lin <shawn@dmp.com.tw>
Signed-off-by: Albert Chen <albert.chen@rdc.com.tw>
---
diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
index 0b014c8..e88e171 100644
--- a/drivers/net/r6040.c
+++ b/drivers/net/r6040.c
@@ -69,6 +69,8 @@
 
 /* MAC registers */
 #define MCR0           0x00    /* Control register 0 */
+#define  PROMISC       0x0020  /* Promiscuous mode */
+#define  HASH_EN       0x0100  /* Enable multicast hash table function
*/
 #define MCR1           0x04    /* Control register 1 */
 #define  MAC_RST       0x0001  /* Reset the MAC */
 #define MBCR           0x08    /* Bus control */
@@ -851,77 +853,84 @@ 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;
+       u16 hash_table[4] = { 0, };
 
-       /* 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;
+       lp->mcr0 = ioread16(ioaddr + MCR0) & ~(PROMISC | HASH_EN);
+
+       /* Promiscuous Mode */
        if (dev->flags & IFF_PROMISC) {
-               reg |= 0x0020;
-               lp->mcr0 |= 0x0020;
+               lp->mcr0 |= PROMISC;
        }
-       /* 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 |= HASH_EN;
+
+               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 the hash table */
-       if (netdev_mc_count(dev) > MCAST_MAX) {
-               u16 hash_table[4];
+               for (i = 0; i < 4; i++)
+                       hash_table[i] = 0xffff;
+       }
+       /* Use internal multicast address registers if the number of
+        * multicast addresses is not greater than MCAST_MAX. */
+       else if (netdev_mc_count(dev) <= MCAST_MAX) {
+               i = 0;
+               netdev_for_each_mc_addr(ha, dev) {
+                       u16 *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++;
+               }
+               while (i < MCAST_MAX) {
+                       iowrite16(0, ioaddr + MID_1L + 8 * i);
+                       iowrite16(0, ioaddr + MID_1M + 8 * i);
+                       iowrite16(0, ioaddr + MID_1H + 8 * i);
+                       i++;
+               }
+       }
+       /* Otherwise, Enable multicast hash table function. */
+       else {
                u32 crc;
 
-               for (i = 0; i < 4; i++)
-                       hash_table[i] = 0;
+               lp->mcr0 |= HASH_EN;
 
-               netdev_for_each_mc_addr(ha, dev) {
-                       char *addrs = ha->addr;
+               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);
+               }
 
-                       if (!(*addrs & 1))
-                               continue;
+               /* Build multicast hash table */
+               netdev_for_each_mc_addr(ha, dev) {
+                       u8 *addrs = ha->addr;
 
-                       crc = ether_crc_le(6, addrs);
+                       crc = ether_crc(ETH_ALEN, addrs);
                        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(lp->mcr0, ioaddr + MCR0);
+
+       /* Fill the MAC hash tables with their values */
+       if (lp->mcr0 && HASH_EN) {
                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)
-                       break;
-               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++;
-       }
-       while (i < MCAST_MAX) {
-               iowrite16(0xffff, ioaddr + MID_1L + 8 * i);
-               iowrite16(0xffff, ioaddr + MID_1M + 8 * i);
-               iowrite16(0xffff, ioaddr + MID_1H + 8 * i);
-               i++;
-       }
+
+       spin_unlock_irqrestore(&lp->lock, flags);
 }
 
 static void netdev_get_drvinfo(struct net_device *dev,
---


The version is for 2.6.32.y and 2.6.27.y:

Reported-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
Tested-by: Marc Leclerc <marc-leclerc@signaturealpha.com>
Signed-off-by: Shawn Lin <shawn@dmp.com.tw>
Signed-off-by: Albert Chen <albert.chen@rdc.com.tw>
---
diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
index 9ee9f01..f9af419 100644
--- a/drivers/net/r6040.c
+++ b/drivers/net/r6040.c
@@ -69,6 +69,8 @@
 
 /* MAC registers */
 #define MCR0           0x00    /* Control register 0 */
+#define  PROMISC       0x0020  /* Promiscuous mode */
+#define  HASH_EN       0x0100  /* Enable multicast hash table function
*/
 #define MCR1           0x04    /* Control register 1 */
 #define  MAC_RST       0x0001  /* Reset the MAC */
 #define MBCR           0x08    /* Bus control */
@@ -935,76 +937,88 @@ 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 dev_mc_list *dmi = dev->mc_list;
        int i;
+       u16 hash_table[4] = { 0, };
 
-       /* 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;
+       lp->mcr0 = ioread16(ioaddr + MCR0) & ~(PROMISC | HASH_EN);
+
+       /* Promiscuous Mode */
        if (dev->flags & IFF_PROMISC) {
-               reg |= 0x0020;
-               lp->mcr0 |= 0x0020;
+               lp->mcr0 |= PROMISC;
        }
-       /* Too many multicast addresses
-        * accept all traffic */
-       else if ((dev->mc_count > MCAST_MAX)
-               || (dev->flags & IFF_ALLMULTI))
-               reg |= 0x0020;
+       /* Enable multicast hash table function to
+        * receive all multicast packets. */
+       else if (dev->flags & IFF_ALLMULTI) {
+               lp->mcr0 |= HASH_EN;
+
+               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(reg, ioaddr);
-       spin_unlock_irqrestore(&lp->lock, flags);
+               for (i = 0; i < 4; i++)
+                       hash_table[i] = 0xffff;
+       }
+       /* Use internal multicast address registers if the number of
+        * multicast addresses is not greater than MCAST_MAX. */
+       else if (dev->mc_count <= MCAST_MAX) {
+               i = 0;
+               while (i < dev->mc_count) {
+                       u16 *adrp = (u16 *) dmi->dmi_addr;
+                       dmi = dmi->next;
 
-       /* Build the hash table */
-       if (dev->mc_count > MCAST_MAX) {
-               u16 hash_table[4];
+                       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++;
+               }
+               while (i < MCAST_MAX) {
+                       iowrite16(0, ioaddr + MID_1L + 8 * i);
+                       iowrite16(0, ioaddr + MID_1M + 8 * i);
+                       iowrite16(0, ioaddr + MID_1H + 8 * i);
+                       i++;
+               }
+       }
+       /* Otherwise, Enable multicast hash table function. */
+       else {
                u32 crc;
 
-               for (i = 0; i < 4; i++)
-                       hash_table[i] = 0;
+               lp->mcr0 |= HASH_EN;
 
-               for (i = 0; i < dev->mc_count; i++) {
-                       char *addrs = dmi->dmi_addr;
+               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 */
+               for (i = 0; i < dev->mc_count; i++) {
+                       u8 *addrs = dmi->dmi_addr;
                        dmi = dmi->next;
 
-                       if (!(*addrs & 1))
-                               continue;
-
-                       crc = ether_crc_le(6, addrs);
+                       crc = ether_crc(ETH_ALEN, addrs);
                        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(lp->mcr0, ioaddr + MCR0);
+
+       /* Fill the MAC hash tables with their values */
+       if (lp->mcr0 && HASH_EN) {
                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 */
-       dmi = dev->mc_list;
-       for (i = 0, dmi; (i < dev->mc_count) && (i < MCAST_MAX); i++) {
-               adrp = (u16 *)dmi->dmi_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);
-               dmi = dmi->next;
-       }
-       for (i = dev->mc_count; i < MCAST_MAX; i++) {
-               iowrite16(0xffff, ioaddr + MID_1L + 8*i);
-               iowrite16(0xffff, ioaddr + MID_1M + 8*i);
-               iowrite16(0xffff, ioaddr + MID_1H + 8*i);
-       }
+
+       spin_unlock_irqrestore(&lp->lock, flags);
 }
 
 static void netdev_get_drvinfo(struct net_device *dev,
---



===========================================================================================
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. 
If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. 
Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of DM&P Group is strictly prohibited; and any information in this email irrelevant to the official business of DM&P Group shall be deemed as neither given nor endorsed by DM&P Group.

===========================================================================================

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2010-11-11  8:39 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-20 21:09 [PATCH 1/2] r6040: fix multicast operations Florian Fainelli
2010-10-21  2:55 ` Ben Hutchings
2010-10-21  8:27   ` Shawn Lin
2010-10-26 14:02     ` Ben Hutchings
2010-10-27  1:19       ` Shawn Lin
2010-11-03 12:51 ` Shawn Lin
2010-11-04 10:04   ` Florian Fainelli
2010-11-11  8:39     ` Shawn Lin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.