Re: [PATCH 1/2] r6040: fix multicast operations

[Shawn Lin <shawn@dmp.com.tw>]

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.

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