[BUG] smctr.c changes in latest BK

[BUG] smctr.c changes in latest BK

[Jochen Friedrich]

Hi,

===== smctr.c 1.15 vs 1.16 =====
--- 1.15/drivers/net/tokenring/smctr.c  Thu Nov 21 23:06:12 2002
+++ 1.16/drivers/net/tokenring/smctr.c  Thu Feb 13 07:23:32 2003
@@ -3064,7 +3064,7 @@
         __u8 r;

         /* Check if node address has been specified by user. (non-0) */
-        for(i = 0; ((i < 6) && (dev->dev_addr[i] == 0)); i++);
+        for(i = 0; ((i < 6) && (dev->dev_addr[i] == 0)); i++)
         {
                 if(i != 6)
                 {

Please revert this one as it is just wrong. As already mentioned here in
LKML (IIRC it was Alan), the semicolon is really intended here.

The above loop just runs until a non-zero byte is found in the MAC
address or all 6 bytes have been checked. A value of i=6 will then
indicate an all-zero MAC address.

However, the block following the for loop is completely unnecessary and
confusing. This patch just removes the irritating braces:

===== smctr.c 1.17 vs edited =====
--- 1.17/drivers/net/tokenring/smctr.c  Thu Feb 13 22:47:11 2003
+++ edited/smctr.c      Thu Feb 13 23:44:07 2003
@@ -3058,26 +3058,25 @@
         __u8 r;

         /* Check if node address has been specified by user. (non-0) */
-        for(i = 0; ((i < 6) && (dev->dev_addr[i] == 0)); i++)
+        for(i = 0; ((i < 6) && (dev->dev_addr[i] == 0)); i++);
+
+        if(i != 6) /* Node addr is not 00:00:00:00:00:00 */
         {
-                if(i != 6)
+                for(i = 0; i < 6; i++)
                 {
-                        for(i = 0; i < 6; i++)
-                        {
-                                r = inb(ioaddr + LAR0 + i);
-                                dev->dev_addr[i] = (char)r;
-                        }
-                        dev->addr_len = 6;
+                        r = inb(ioaddr + LAR0 + i);
+                        dev->dev_addr[i] = (char)r;
                 }
-                else    /* Node addr. not given by user, read it from board. */
+                dev->addr_len = 6;
+        }
+        else    /* Node addr. not given by user, read it from board. */
+        {
+                for(i = 0; i < 6; i++)
                 {
-                        for(i = 0; i < 6; i++)
-                        {
-                                r = inb(ioaddr + LAR0 + i);
-                                dev->dev_addr[i] = (char)r;
-                        }
-                        dev->addr_len = 6;
+                        r = inb(ioaddr + LAR0 + i);
+                        dev->dev_addr[i] = (char)r;
                 }
+                dev->addr_len = 6;
         }

         return (0);

Thanks,
--jochen

Re: [BUG] smctr.c changes in latest BK

[Jochen Friedrich]

Hi,

On Thu, 13 Feb 2003, Jochen Friedrich wrote:

> Hi,
>
> ===== smctr.c 1.15 vs 1.16 =====
> --- 1.15/drivers/net/tokenring/smctr.c  Thu Nov 21 23:06:12 2002
> +++ 1.16/drivers/net/tokenring/smctr.c  Thu Feb 13 07:23:32 2003
> @@ -3064,7 +3064,7 @@
>          __u8 r;
>
>          /* Check if node address has been specified by user. (non-0) */
> -        for(i = 0; ((i < 6) && (dev->dev_addr[i] == 0)); i++);
> +        for(i = 0; ((i < 6) && (dev->dev_addr[i] == 0)); i++)
>          {
>                  if(i != 6)
>                  {
>
> Please revert this one as it is just wrong. As already mentioned here in
> LKML (IIRC it was Alan), the semicolon is really intended here.
>
> The above loop just runs until a non-zero byte is found in the MAC
> address or all 6 bytes have been checked. A value of i=6 will then
> indicate an all-zero MAC address.

After taking a second look, i just recognized that both cases (MAC adress
all-zero or not) are handled exactly the same (by duplicated code), so the
whole stuff is unnecessary.

The whole function just reduces to a simple copy loop:

===== smctr.c 1.17 vs edited =====
--- 1.17/drivers/net/tokenring/smctr.c  Thu Feb 13 22:47:11 2003
+++ edited/smctr.c      Fri Feb 14 00:07:33 2003
@@ -3057,28 +3057,12 @@
         unsigned int i;
         __u8 r;

-        /* Check if node address has been specified by user. (non-0) */
-        for(i = 0; ((i < 6) && (dev->dev_addr[i] == 0)); i++)
+        for(i = 0; i < 6; i++)
         {
-                if(i != 6)
-                {
-                        for(i = 0; i < 6; i++)
-                        {
-                                r = inb(ioaddr + LAR0 + i);
-                                dev->dev_addr[i] = (char)r;
-                        }
-                        dev->addr_len = 6;
-                }
-                else    /* Node addr. not given by user, read it from board. */
-                {
-                        for(i = 0; i < 6; i++)
-                        {
-                                r = inb(ioaddr + LAR0 + i);
-                                dev->dev_addr[i] = (char)r;
-                        }
-                        dev->addr_len = 6;
-                }
+                r = inb(ioaddr + LAR0 + i);
+                dev->dev_addr[i] = (char)r;
         }
+        dev->addr_len = 6;

         return (0);
 }

Thanks,
--jochen

Re: [BUG] smctr.c changes in latest BK

[Jeff Garzik]

Jochen Friedrich wrote:
> After taking a second look, i just recognized that both cases (MAC adress
> all-zero or not) are handled exactly the same (by duplicated code), so the
> whole stuff is unnecessary.
> 
> The whole function just reduces to a simple copy loop:


yep, you're right.  applied.

Re: [BUG] smctr.c changes in latest BK

[Olivier Galibert]

On Fri, Feb 14, 2003 at 12:05:24AM +0100, Jochen Friedrich wrote:
> On Thu, 13 Feb 2003, Jochen Friedrich wrote:
> > Please revert this one as it is just wrong. As already mentioned here in
> > LKML (IIRC it was Alan), the semicolon is really intended here.
> >
> > The above loop just runs until a non-zero byte is found in the MAC
> > address or all 6 bytes have been checked. A value of i=6 will then
> > indicate an all-zero MAC address.
> 
> After taking a second look, i just recognized that both cases (MAC adress
> all-zero or not) are handled exactly the same (by duplicated code), so the
> whole stuff is unnecessary.
> 
> The whole function just reduces to a simple copy loop:

Doesn't that mean that the original function was buggy and it should
not have copied the mac address over if one was user-provided?

  OG.

Re: [BUG] smctr.c changes in latest BK

[Jochen Friedrich]

Hi Olivier,

> Doesn't that mean that the original function was buggy and it should
> not have copied the mac address over if one was user-provided?

Right. But for now i preferred to clean up the offending code. The driver
still needs some serious cleanup (not 64bit clean, lots of IMHO
unnecessary and confusing casts, not using propper reference counting,
and probably some more), so i plan to clean up those first before adding
new features to the driver.

Fortunately, i recently go hold of one of these cards, so i will be able
to test the cleanup on my Alpha for 64bit cleanliness :-)

Cheers,
--jochen