Re: Poss. bug in tulip driver since 2.4.7

[Prasanna Meda <pmeda@akamai.com>]

Andrew Morton wrote:

> Prasanna Meda <pmeda@akamai.com> wrote:
> >
> > The inner for loop shown below was not
> >  supposed to be  inside  the  outside  loop.
> >  They  also use  the  same index i.
> >  Due to  this, when mc_count is more than
> >  14,  with non ASIX chips,  panics, corruptions
> >  and  denial of services to multicast addresses
> >  can  result!
> >
> >  http://lxr.linux.no/source/drivers/net/tulip/tulip_core.c#L1055
>
> So can you confirm that the driver works correctly with this change?
>
> --- 25/drivers/net/tulip/tulip_core.c~tulip-hash-fix    2003-11-11 18:51:52.000000000 -0800
> +++ 25-akpm/drivers/net/tulip/tulip_core.c      2003-11-11 18:52:31.000000000 -0800
> @@ -979,12 +979,13 @@ static void build_setup_frame_hash(u16 *
>         for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
>              i++, mclist = mclist->next) {
>                 int index = ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x1ff;
> +               int j;
>
>                 set_bit_le(index, hash_table);
>
> -               for (i = 0; i < 32; i++) {
> -                       *setup_frm++ = hash_table[i];
> -                       *setup_frm++ = hash_table[i];
> +               for (j = 0; j < 32; j++) {
> +                       *setup_frm++ = hash_table[j];
> +                       *setup_frm++ = hash_table[j];
>                 }
>                 setup_frm = &tp->setup_frame[13*6];
>         }

No,  you need to bring the for loop outside the loop.
 - Otherwise we need to reset the setup_frame to
tp->setup_frame after every loop.
 - You do not need to set the setup_frm for every
mc address, we can set once after the complete
has_table is ready.
And also the  2.4 code missed a bit in tx_flags.

We did the following change that makes it identical
to 2.2 tulip driver, and it worked.

--- Linux/drivers/net/tulip/tulip_core.c  Fri Oct 10 20:22:29 2003
+++ linux/drivers/net/tulip/tulip_core.c        Fri Oct 10 20:28:19 2003
@@ -1054,19 +1054,19 @@

        memset(hash_table, 0, sizeof(hash_table));
        set_bit_le(255, hash_table);                    /* Broadcast entry */

        /* This should work on big-endian machines as well. */
       for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
             i++, mclist = mclist->next) {
                int index = ether_crc_le(ETH_ALEN, mclist->dmi_addr) & 0x1ff;

                set_bit_le(index, hash_table);
+       }

-               for (i = 0; i < 32; i++) {
-                       *setup_frm++ = hash_table[i];
-                       *setup_frm++ = hash_table[i];
-               }
-               setup_frm = &tp->setup_frame[13*6];
+       for (i = 0; i < 32; i++) {
+               *setup_frm++ = hash_table[i];
+               *setup_frm++ = hash_table[i];
        }
+       setup_frm = &tp->setup_frame[13*6];

        /* Fill the final entry with our physical address. */
        eaddrs = (u16 *)dev->dev_addr;
@@ -1166,11 +1168,13 @@
                }
        } else {
                unsigned long flags;
+               u32 tx_flags = 0x08000000 | 192;

                /* Note that only the low-address shortword of setup_frame is valid!
                   The values are doubled for big-endian architectures. */
                if (dev->mc_count > 14) { /* Must use a multicast hash table. */
                        build_setup_frame_hash(tp->setup_frame, dev);
+                       tx_flags = 0x08400000 | 192;
                } else {
                        build_setup_frame_perfect(tp->setup_frame, dev);
                }
@@ -1180,7 +1184,6 @@
                if (tp->cur_tx - tp->dirty_tx > TX_RING_SIZE - 2) {
                        /* Same setup recently queued, we need not add it. */
                } else {
-                       u32 tx_flags = 0x08000000 | 192;
                        unsigned int entry;
                        int dummy = -1;