From: Florian Fainelli <florian@openwrt•org>
To: Shawn Lin <shawn@dmp•com.tw>
Cc: netdev@vger•kernel.org,
Marc Leclerc <marc-leclerc@signaturealpha•com>,
Albert Chen <albert.chen@rdc•com.tw>,
David Miller <davem@davemloft•net>,
Ben Hutchings <ben@decadent•org.uk>
Subject: Re: [PATCH 1/2] r6040: fix multicast operations
Date: Thu, 4 Nov 2010 11:04:24 +0100 [thread overview]
Message-ID: <201011041104.24681.florian@openwrt.org> (raw)
In-Reply-To: <1288788689.1837.141.camel@shawn-desktop>
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.
>
> ===========================================================================
> ================
next prev parent reply other threads:[~2010-11-04 10:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2010-11-11 8:39 ` Shawn Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201011041104.24681.florian@openwrt.org \
--to=florian@openwrt$(echo .)org \
--cc=albert.chen@rdc$(echo .)com.tw \
--cc=ben@decadent$(echo .)org.uk \
--cc=davem@davemloft$(echo .)net \
--cc=marc-leclerc@signaturealpha$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shawn@dmp$(echo .)com.tw \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox