[PATCH] natsemi update 1/4 Use assigned MAC address

[PATCH] natsemi update 1/4 Use assigned MAC address

[Gary N Spiess]

[-- Attachment #1.1: Type: text/plain, Size: 636 bytes --]

Jeff,
This is the first of a series of patches needed to support our product using the DP83815.
This patch accepts the MAC address as a parameter because our implementation does not have an EEPROM.  I tried to upgrade to the new module_param_array() macro, but couldn't find a tutorial on the new param scheme.  To get things working for our development, I use a __setup() wrapper to get "ethaddr=" from the linux command line.
Gary
 oooooooooooooooooooooooooooooooooooooooooooooooooo
 Gary Spiess (Gary.Spiess@Intermec.com)
 MobileLan Wireless Products Group, Intermec Technology Corp
 voice: 319 369-3580  fax: 319 369-3804


[-- Attachment #1.2: Type: text/html, Size: 1005 bytes --]

[-- Attachment #2: natsemi-2.6.6-1.patch --]
[-- Type: application/octet-stream, Size: 2883 bytes --]

--- linux-2.6.6/drivers/net/natsemi.c	2004-06-02 13:31:37.000000000 -0500
+++ linux/drivers/net/natsemi.c	2004-06-02 16:56:38.000000000 -0500
@@ -147,6 +147,7 @@
 
 #include <linux/config.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/timer.h>
@@ -209,6 +210,8 @@
 #define MAX_UNITS 8		/* More are supported, limit only on options */
 static int options[MAX_UNITS];
 static int full_duplex[MAX_UNITS];
+static char *ethaddr[MAX_UNITS] = {NULL, };
+static int ethaddr_num = 0;
 
 /* Operational parameters that are set at compile time. */
 
@@ -256,6 +259,7 @@
 MODULE_PARM(rx_copybreak, "i");
 MODULE_PARM(options, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM(full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
+module_param_array(ethaddr, charp, ethaddr_num, S_IRUGO);
 MODULE_PARM_DESC(max_interrupt_work, 
 	"DP8381x maximum events handled per interrupt");
 MODULE_PARM_DESC(mtu, "DP8381x MTU (all boards)");
@@ -265,6 +269,7 @@
 MODULE_PARM_DESC(options, 
 	"DP8381x: Bits 0-3: media type, bit 17: full duplex");
 MODULE_PARM_DESC(full_duplex, "DP8381x full duplex setting(s) (1)");
+MODULE_PARM_DESC(ethaddr, "DP8381x MAC addr(s) xx:xx:xx:xx:xx:xx");
 
 /*
 				Theory of Operation
@@ -776,13 +781,22 @@
 		goto err_ioremap;
 	}
 
-	/* Work around the dropped serial bit. */
-	prev_eedata = eeprom_read(ioaddr, 6);
-	for (i = 0; i < 3; i++) {
-		int eedata = eeprom_read(ioaddr, i + 7);
-		dev->dev_addr[i*2] = (eedata << 1) + (prev_eedata >> 15);
-		dev->dev_addr[i*2+1] = eedata >> 7;
-		prev_eedata = eedata;
+	if (find_cnt < ethaddr_num)
+	{
+		int i, a[6];
+		sscanf(ethaddr[find_cnt], "%2x:%2x:%2x:%2x:%2x:%2x",
+			&a[0], &a[1], &a[2], &a[3], &a[4], &a[5]);
+		for (i = 6; i--; )
+			dev->dev_addr[i] = (unsigned char)a[i];
+	} else {
+		/* Work around the dropped serial bit. */
+		prev_eedata = eeprom_read(ioaddr, 6);
+		for (i = 0; i < 3; i++) {
+			int eedata = eeprom_read(ioaddr, i + 7);
+			dev->dev_addr[i*2] = (eedata << 1) + (prev_eedata >> 15);
+			dev->dev_addr[i*2+1] = eedata >> 7;
+			prev_eedata = eedata;
+		}
 	}
 
 	dev->base_addr = ioaddr;
@@ -2696,6 +2712,28 @@
 	pci_unregister_driver (&natsemi_driver);
 }
 
+/* permit ethaddr to be specified in the linux command line as
+ * bootargs="ethaddr=xx:xx:xx:xx:xx:xx".
+ */
+static int __init get_mac_addr(char *str)
+{
+	/* can't deal with multiple definitions */
+	if (!ethaddr_num)
+	{
+		static unsigned char tempethaddr[18];
+
+		/* Unsafe to just save the pointer?  I'll make a copy */
+		strncpy(tempethaddr, str, sizeof(tempethaddr));
+		tempethaddr[sizeof(tempethaddr)-1] = 0;
+
+		/* Do what I think module_param_array() should do */
+		ethaddr[ethaddr_num++] = tempethaddr;
+	}
+	return 0;
+}
+
+__setup("ethaddr=", get_mac_addr);
+
 module_init(natsemi_init_mod);
 module_exit(natsemi_exit_mod);
 

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Manfred Spraul]

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

Gary N Spiess wrote:

>Jeff,
>This is the first of a series of patches needed to support our product using the DP83815.
>This patch accepts the MAC address as a parameter because our implementation does not have an EEPROM.  I tried to upgrade to the new module_param_array() macro, but couldn't find a tutorial on the new param scheme.
>
Ok. Appending "natsemi.ethaddr=00:01:02:04:05:06" works.

>  To get things working for our development, I use a __setup() wrapper to get "ethaddr=" from the linux command line.
>  
>
That's not a reason to merge a hack

>+       if (find_cnt < ethaddr_num)
>  
>
I think we should add a special case: if strlen(ethaddr[find_cnt]) is 0, 
then the address from the eeprom is used - this allows to replace one 
mac address if multiple nics are installed.

>+       {
>  
>
Coding style: the { should be in the same line as the if

>+               int i, a[6];
>  
>
i already exists, no need to define another instance.

I've appended an updated patch - could you test it?

But: I'm not sure that the change is required. What about just setting 
the mac to 0, and the actual mac address is set from user space? It's 
possible to set the mac address with

# ifconfig eth2 ether 80:01:02:03:04:05

Is there a reason why you cannot set the mac address from user space?

--
    Manfred

[-- Attachment #2: natsemi-2.6.6-1.patch-new --]
[-- Type: text/plain, Size: 2189 bytes --]

--- 2.6/drivers/net/natsemi.c	2004-05-10 04:31:59.000000000 +0200
+++ build-2.6/drivers/net/natsemi.c	2004-06-06 15:27:12.124436291 +0200
@@ -147,6 +147,7 @@
 
 #include <linux/config.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/timer.h>
@@ -209,6 +210,8 @@
 #define MAX_UNITS 8		/* More are supported, limit only on options */
 static int options[MAX_UNITS];
 static int full_duplex[MAX_UNITS];
+static char *ethaddr[MAX_UNITS] = {NULL, };
+static int ethaddr_num = 0;
 
 /* Operational parameters that are set at compile time. */
 
@@ -256,6 +259,7 @@
 MODULE_PARM(rx_copybreak, "i");
 MODULE_PARM(options, "1-" __MODULE_STRING(MAX_UNITS) "i");
 MODULE_PARM(full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
+module_param_array(ethaddr, charp, ethaddr_num, S_IRUGO);
 MODULE_PARM_DESC(max_interrupt_work, 
 	"DP8381x maximum events handled per interrupt");
 MODULE_PARM_DESC(mtu, "DP8381x MTU (all boards)");
@@ -265,6 +269,7 @@
 MODULE_PARM_DESC(options, 
 	"DP8381x: Bits 0-3: media type, bit 17: full duplex");
 MODULE_PARM_DESC(full_duplex, "DP8381x full duplex setting(s) (1)");
+MODULE_PARM_DESC(ethaddr, "DP8381x MAC addr(s) xx:xx:xx:xx:xx:xx");
 
 /*
 				Theory of Operation
@@ -776,13 +781,21 @@
 		goto err_ioremap;
 	}
 
-	/* Work around the dropped serial bit. */
-	prev_eedata = eeprom_read(ioaddr, 6);
-	for (i = 0; i < 3; i++) {
-		int eedata = eeprom_read(ioaddr, i + 7);
-		dev->dev_addr[i*2] = (eedata << 1) + (prev_eedata >> 15);
-		dev->dev_addr[i*2+1] = eedata >> 7;
-		prev_eedata = eedata;
+	if (find_cnt < ethaddr_num && strlen(ethaddr[find_cnt]) > 0) {
+		int a[6];
+		sscanf(ethaddr[find_cnt], "%2x:%2x:%2x:%2x:%2x:%2x",
+			&a[0], &a[1], &a[2], &a[3], &a[4], &a[5]);
+		for (i = 6; i--; )
+			dev->dev_addr[i] = (unsigned char)a[i];
+	} else {
+		/* Work around the dropped serial bit. */
+		prev_eedata = eeprom_read(ioaddr, 6);
+		for (i = 0; i < 3; i++) {
+			int eedata = eeprom_read(ioaddr, i + 7);
+			dev->dev_addr[i*2] = (eedata << 1) + (prev_eedata >> 15);
+			dev->dev_addr[i*2+1] = eedata >> 7;
+			prev_eedata = eedata;
+		}
 	}
 
 	dev->base_addr = ioaddr;

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Gary N Spiess]

Jeff,

I agree with all comments.  Regarding the total elimination of this patch ...

Only our bootrom knows the MAC address that should be used, and it appends it to the bootargs.  If there is a way for the userspace to take a peek at the bootargs (or a similar mechanism), we can use the ifconfig method to assign the MAC address instead of changing the driver.  I'd prefer it.

Gary



*********** REPLY SEPARATOR  ***********

On 6/6/2004 at 3:30 PM Manfred Spraul wrote:

>Gary N Spiess wrote:
>
>>Jeff,
>>This is the first of a series of patches needed to support our product
>using the DP83815.
>>This patch accepts the MAC address as a parameter because our
>implementation does not have an EEPROM.  I tried to upgrade to the new
>module_param_array() macro, but couldn't find a tutorial on the new param
>scheme.
>>
>Ok. Appending "natsemi.ethaddr=00:01:02:04:05:06" works.
>
>>  To get things working for our development, I use a __setup() wrapper to
>get "ethaddr=" from the linux command line.
>>  
>>
>That's not a reason to merge a hack
>
>>+       if (find_cnt < ethaddr_num)
>>  
>>
>I think we should add a special case: if strlen(ethaddr[find_cnt]) is 0, 
>then the address from the eeprom is used - this allows to replace one 
>mac address if multiple nics are installed.
>
>>+       {
>>  
>>
>Coding style: the { should be in the same line as the if
>
>>+               int i, a[6];
>>  
>>
>i already exists, no need to define another instance.
>
>I've appended an updated patch - could you test it?
>
>But: I'm not sure that the change is required. What about just setting 
>the mac to 0, and the actual mac address is set from user space? It's 
>possible to set the mac address with
>
># ifconfig eth2 ether 80:01:02:03:04:05
>
>Is there a reason why you cannot set the mac address from user space?
>
>--
>    Manfred
>
>
>--- 2.6/drivers/net/natsemi.c	2004-05-10 04:31:59.000000000 +0200
>+++ build-2.6/drivers/net/natsemi.c	2004-06-06 15:27:12.124436291 +0200
>@@ -147,6 +147,7 @@
> 
> #include <linux/config.h>
> #include <linux/module.h>
>+#include <linux/moduleparam.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
> #include <linux/timer.h>
>@@ -209,6 +210,8 @@
> #define MAX_UNITS 8		/* More are supported, limit only on options */
> static int options[MAX_UNITS];
> static int full_duplex[MAX_UNITS];
>+static char *ethaddr[MAX_UNITS] = {NULL, };
>+static int ethaddr_num = 0;
> 
> /* Operational parameters that are set at compile time. */
> 
>@@ -256,6 +259,7 @@
> MODULE_PARM(rx_copybreak, "i");
> MODULE_PARM(options, "1-" __MODULE_STRING(MAX_UNITS) "i");
> MODULE_PARM(full_duplex, "1-" __MODULE_STRING(MAX_UNITS) "i");
>+module_param_array(ethaddr, charp, ethaddr_num, S_IRUGO);
> MODULE_PARM_DESC(max_interrupt_work, 
> 	"DP8381x maximum events handled per interrupt");
> MODULE_PARM_DESC(mtu, "DP8381x MTU (all boards)");
>@@ -265,6 +269,7 @@
> MODULE_PARM_DESC(options, 
> 	"DP8381x: Bits 0-3: media type, bit 17: full duplex");
> MODULE_PARM_DESC(full_duplex, "DP8381x full duplex setting(s) (1)");
>+MODULE_PARM_DESC(ethaddr, "DP8381x MAC addr(s) xx:xx:xx:xx:xx:xx");
> 
> /*
> 				Theory of Operation
>@@ -776,13 +781,21 @@
> 		goto err_ioremap;
> 	}
> 
>-	/* Work around the dropped serial bit. */
>-	prev_eedata = eeprom_read(ioaddr, 6);
>-	for (i = 0; i < 3; i++) {
>-		int eedata = eeprom_read(ioaddr, i + 7);
>-		dev->dev_addr[i*2] = (eedata << 1) + (prev_eedata >> 15);
>-		dev->dev_addr[i*2+1] = eedata >> 7;
>-		prev_eedata = eedata;
>+	if (find_cnt < ethaddr_num && strlen(ethaddr[find_cnt]) > 0) {
>+		int a[6];
>+		sscanf(ethaddr[find_cnt], "%2x:%2x:%2x:%2x:%2x:%2x",
>+			&a[0], &a[1], &a[2], &a[3], &a[4], &a[5]);
>+		for (i = 6; i--; )
>+			dev->dev_addr[i] = (unsigned char)a[i];
>+	} else {
>+		/* Work around the dropped serial bit. */
>+		prev_eedata = eeprom_read(ioaddr, 6);
>+		for (i = 0; i < 3; i++) {
>+			int eedata = eeprom_read(ioaddr, i + 7);
>+			dev->dev_addr[i*2] = (eedata << 1) + (prev_eedata >> 15);
>+			dev->dev_addr[i*2+1] = eedata >> 7;
>+			prev_eedata = eedata;
>+		}
> 	}
> 
> 	dev->base_addr = ioaddr;


 oooooooooooooooooooooooooooooooooooooooooooooooooo
 Gary Spiess (Gary.Spiess@Intermec.com)
 MobileLan Wireless Products Group, Intermec Technology Corp
 voice: 319 369-3580  fax: 319 369-3804

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Stephen Hemminger]

On Mon, 07 Jun 2004 12:58:55 -0500
"Gary N Spiess" <Gary.Spiess@Intermec.com> wrote:

> Jeff,
> 
> I agree with all comments.  Regarding the total elimination of this patch ...
> 
> Only our bootrom knows the MAC address that should be used, and it appends it to the bootargs.  If there is a way for the userspace to take a peek at the bootargs (or a similar mechanism), we can use the ifconfig method to assign the MAC address instead of changing the driver.  I'd prefer it.

/proc/cmdline??

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Gary N Spiess]

Stephen,

Scrap patch 1 of 4, we don't need or want it.  (I'm feeling so sheepish.)

Thanks, Gary

*********** REPLY SEPARATOR  ***********

On 6/7/2004 at 11:16 AM Stephen Hemminger wrote:

>On Mon, 07 Jun 2004 12:58:55 -0500
>"Gary N Spiess" <Gary.Spiess@Intermec.com> wrote:
>
>> Jeff,
>> 
>> I agree with all comments.  Regarding the total elimination of this
>patch ...
>> 
>> Only our bootrom knows the MAC address that should be used, and it
>appends it to the bootargs.  If there is a way for the userspace to take a
>peek at the bootargs (or a similar mechanism), we can use the ifconfig
>method to assign the MAC address instead of changing the driver.  I'd
>prefer it.
>
>/proc/cmdline??


 oooooooooooooooooooooooooooooooooooooooooooooooooo
 Gary Spiess (Gary.Spiess@Intermec.com)
 MobileLan Wireless Products Group, Intermec Technology Corp
 voice: 319 369-3580  fax: 319 369-3804

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Jeff Garzik]

Manfred,

since there was discussion and multiple patches flying about, could you 
resend the patchset, now that the dust has settled?

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Mark Smith]

Hi Manfred, Gary, Stephen,

"But: I'm not sure that the change is required. What about just
setting the mac to 0, and the actual mac address is set from user
space? It's possible to set the mac address with"

Could I suggest if this is the solution implemented, setting the
first octet of the MAC address to 0x02, as in a Locally Assigned
MAC address ?

If an interface with an all zero's MAC address is added to a
bridge, running the Spanning Tree Protocol (STP), it will always
attempt to take over as the root bridge, unless STP root bridge
priorities are being used. This would disrupt traffic on the
attached LAN.

Ideally, assigning "zero" a MAC address which has almost no
chance of disrupting STP or any other protocol could be useful.
Maybe fe:ff:ff:ff:ff:ff ?

I think this would fit in with the "be conservative in what you
send, liberal in what you receive" philosophy.

Regards,
Mark.

ps, please CC on any replies, I'm not subscribed to the list yet.

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Stephen Hemminger]

On Tue, 8 Jun 2004 07:12:52 +0930
Mark Smith <random@72616e646f6d20323030342d30342d31360a.nosense.org> wrote:

> Hi Manfred, Gary, Stephen,
> 
> "But: I'm not sure that the change is required. What about just
> setting the mac to 0, and the actual mac address is set from user
> space? It's possible to set the mac address with"
> 
> Could I suggest if this is the solution implemented, setting the
> first octet of the MAC address to 0x02, as in a Locally Assigned
> MAC address ?
> 
> If an interface with an all zero's MAC address is added to a
> bridge, running the Spanning Tree Protocol (STP), it will always
> attempt to take over as the root bridge, unless STP root bridge
> priorities are being used. This would disrupt traffic on the
> attached LAN.

Actually, it won't let you add any interface to a bridge without
a valid ether address now (ie non-zero and not multicast).

> Ideally, assigning "zero" a MAC address which has almost no
> chance of disrupting STP or any other protocol could be useful.
> Maybe fe:ff:ff:ff:ff:ff ?
> 
> I think this would fit in with the "be conservative in what you
> send, liberal in what you receive" philosophy.
> 
> Regards,
> Mark.
> 
> ps, please CC on any replies, I'm not subscribed to the list yet.

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Mark Smith]

On Mon, 7 Jun 2004 14:46:54 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> On Tue, 8 Jun 2004 07:12:52 +0930
> Mark Smith
> <random@72616e646f6d20323030342d30342d31360a.nosense.org>
> wrote:
> 
> > Hi Manfred, Gary, Stephen,
> > 

<snip>

> > If an interface with an all zero's MAC address is added to a
> > bridge, running the Spanning Tree Protocol (STP), it will
> > always attempt to take over as the root bridge, unless STP
> > root bridge priorities are being used. This would disrupt
> > traffic on the attached LAN.
> 
> Actually, it won't let you add any interface to a bridge
> without a valid ether address now (ie non-zero and not
> multicast).
> 

That's good, I first noticed this issue when I happened to add a
dummy interface into a bridge configuration, and found that its
zero MAC address ended up disrupting the spanning tree. 

I also have just noticed that the dummy interfaces now have
random MAC addresses as per the function you mentioned earlier.
Also good.

Regards,
Mark.

> > 
> > ps, please CC on any replies, I'm not subscribed to the list
> > yet.

Re: [PATCH] natsemi update 1/4 Use assigned MAC address

[Stephen Hemminger]

On Tue, 8 Jun 2004 07:12:52 +0930
Mark Smith <random@72616e646f6d20323030342d30342d31360a.nosense.org> wrote:

> Hi Manfred, Gary, Stephen,
> 
> "But: I'm not sure that the change is required. What about just
> setting the mac to 0, and the actual mac address is set from user
> space? It's possible to set the mac address with"

How about 
	random_ether_addr(dev->dev_addr);

it does the right thing.