[PATCH] forcedeth: mac address fix

[PATCH] forcedeth: mac address fix

[Ayaz Abdulla]

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

This critical patch fixes a mac address issue recently introduced. If 
the device's mac address was in correct order and the flag 
NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the flag would 
get cleared. During next load, the mac address would get reversed 
because the flag is missing.

As it has been indicated previously, the flag is cleared across a low 
power transition. Therefore, the driver should set the mac address back 
into the reversed order when clearing the flag.

Also, the driver should set back the flag after a low power transition 
to protect against kexec command calling nv_probe a second time.

Signed-off-by: Ayaz Abdulla <aabdulla@nvidia.com>


[-- Attachment #2: patch-forcedeth-macaddrfix --]
[-- Type: text/plain, Size: 2236 bytes --]

--- old/drivers/net/forcedeth.c	2008-03-31 15:25:05.000000000 -0700
+++ new/drivers/net/forcedeth.c	2008-03-31 15:41:51.000000000 -0700
@@ -5317,8 +5317,7 @@
 
 	/* check the workaround bit for correct mac address order */
 	txreg = readl(base + NvRegTransmitPoll);
-	if ((txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) ||
-	    (id->driver_data & DEV_HAS_CORRECT_MACADDR)) {
+	if (id->driver_data & DEV_HAS_CORRECT_MACADDR) {
 		/* mac address is already in correct order */
 		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
 		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
@@ -5326,6 +5325,22 @@
 		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
 		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
 		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
+	} else if (txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) {
+		/* mac address is already in correct order */
+		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
+		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
+		dev->dev_addr[2] = (np->orig_mac[0] >> 16) & 0xff;
+		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
+		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
+		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
+		/*
+		 * Set orig mac address back to the reversed version.
+		 * This flag will be cleared during low power transition.
+		 * Therefore, we should always put back the reversed address.
+		 */
+		np->orig_mac[0] = (dev->dev_addr[5] << 0) + (dev->dev_addr[4] << 8) +
+			(dev->dev_addr[3] << 16) + (dev->dev_addr[2] << 24);
+		np->orig_mac[1] = (dev->dev_addr[1] << 0) + (dev->dev_addr[0] << 8);
 	} else {
 		/* need to reverse mac address to correct order */
 		dev->dev_addr[0] = (np->orig_mac[1] >>  8) & 0xff;
@@ -5596,7 +5611,9 @@
 static int nv_resume(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
+	u8 __iomem *base = get_hwbase(dev);
 	int rc = 0;
+	u32 txreg;
 
 	if (!netif_running(dev))
 		goto out;
@@ -5607,6 +5624,11 @@
 	pci_restore_state(pdev);
 	pci_enable_wake(pdev, PCI_D0, 0);
 
+	/* restore mac address reverse flag */
+	txreg = readl(base + NvRegTransmitPoll);
+	txreg |= NVREG_TRANSMITPOLL_MAC_ADDR_REV;
+	writel(txreg, base + NvRegTransmitPoll);
+
 	rc = nv_open(dev);
 out:
 	return rc;

Re: [PATCH] forcedeth: mac address fix

[Björn Steinbrink]

On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:
> This critical patch fixes a mac address issue recently introduced. If  

Does "recently" mean my commit 2e3884b5b16795c03a7bf295797c1b2402885b88?
If so, I like to be told directly when I break stuff ;-)

> the device's mac address was in correct order and the flag  
> NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the flag would  
> get cleared. During next load, the mac address would get reversed  
> because the flag is missing.

Hm, but nv_remove also writes back the reversed mac address. I don't see
how a plain remove/probe cycle would mess things up.

> As it has been indicated previously, the flag is cleared across a low  
> power transition. Therefore, the driver should set the mac address back  
> into the reversed order when clearing the flag.

That's what nv_remove is supposed to do. Is there a case where nv_remove
is not called?

> Also, the driver should set back the flag after a low power transition  
> to protect against kexec command calling nv_probe a second time.

Sounds like suspend stopped calling nv_remove? That would make sense
then. I never checked whether suspend ever actually did call nv_remove
(I think), but as my patch worked, it basically must have done so, at
least in the past, right?

Thanks,
Björn

>
> Signed-off-by: Ayaz Abdulla <aabdulla@nvidia.com>
>

> --- old/drivers/net/forcedeth.c	2008-03-31 15:25:05.000000000 -0700
> +++ new/drivers/net/forcedeth.c	2008-03-31 15:41:51.000000000 -0700
> @@ -5317,8 +5317,7 @@
>  
>  	/* check the workaround bit for correct mac address order */
>  	txreg = readl(base + NvRegTransmitPoll);
> -	if ((txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) ||
> -	    (id->driver_data & DEV_HAS_CORRECT_MACADDR)) {
> +	if (id->driver_data & DEV_HAS_CORRECT_MACADDR) {
>  		/* mac address is already in correct order */
>  		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
>  		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
> @@ -5326,6 +5325,22 @@
>  		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
>  		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
>  		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
> +	} else if (txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) {
> +		/* mac address is already in correct order */
> +		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
> +		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
> +		dev->dev_addr[2] = (np->orig_mac[0] >> 16) & 0xff;
> +		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
> +		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
> +		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
> +		/*
> +		 * Set orig mac address back to the reversed version.
> +		 * This flag will be cleared during low power transition.
> +		 * Therefore, we should always put back the reversed address.
> +		 */
> +		np->orig_mac[0] = (dev->dev_addr[5] << 0) + (dev->dev_addr[4] << 8) +
> +			(dev->dev_addr[3] << 16) + (dev->dev_addr[2] << 24);
> +		np->orig_mac[1] = (dev->dev_addr[1] << 0) + (dev->dev_addr[0] << 8);
>  	} else {
>  		/* need to reverse mac address to correct order */
>  		dev->dev_addr[0] = (np->orig_mac[1] >>  8) & 0xff;
> @@ -5596,7 +5611,9 @@
>  static int nv_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	u8 __iomem *base = get_hwbase(dev);
>  	int rc = 0;
> +	u32 txreg;
>  
>  	if (!netif_running(dev))
>  		goto out;
> @@ -5607,6 +5624,11 @@
>  	pci_restore_state(pdev);
>  	pci_enable_wake(pdev, PCI_D0, 0);
>  
> +	/* restore mac address reverse flag */
> +	txreg = readl(base + NvRegTransmitPoll);
> +	txreg |= NVREG_TRANSMITPOLL_MAC_ADDR_REV;
> +	writel(txreg, base + NvRegTransmitPoll);
> +
>  	rc = nv_open(dev);
>  out:
>  	return rc;

Re: [PATCH] forcedeth: mac address fix

[Ayaz Abdulla]

Björn Steinbrink wrote:
> On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:
> 
>>This critical patch fixes a mac address issue recently introduced. If  
> 
> 
> Does "recently" mean my commit 2e3884b5b16795c03a7bf295797c1b2402885b88?
> If so, I like to be told directly when I break stuff ;-)
> 
Thats why I cc'd you. :)

> 
>>the device's mac address was in correct order and the flag  
>>NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the flag would  
>>get cleared. During next load, the mac address would get reversed  
>>because the flag is missing.
> 
> 
> Hm, but nv_remove also writes back the reversed mac address. I don't see
> how a plain remove/probe cycle would mess things up.
> 
> 

For example, NVREG_TRANSMITPOLL_MAC_ADDR_REV is set. That would mean 
that orig_mac will be stored with correct address. Then you call 
nv_remove (via ifdown) which set orig_mac back into the register and 
will clear the flag. On the next nv_probe (via ifup), you would perform 
the logic to reverse the mac address. But it is still in correct order.

>>As it has been indicated previously, the flag is cleared across a low  
>>power transition. Therefore, the driver should set the mac address back  
>>into the reversed order when clearing the flag.
> 
> 
> That's what nv_remove is supposed to do. Is there a case where nv_remove
> is not called?
> 

Sorry for the confusion. I was merely stating what needs to be done as 
the full solution. This logic was already in place by your patch.

> 
>>Also, the driver should set back the flag after a low power transition  
>>to protect against kexec command calling nv_probe a second time.
> 
> 
> Sounds like suspend stopped calling nv_remove? That would make sense
> then. I never checked whether suspend ever actually did call nv_remove
> (I think), but as my patch worked, it basically must have done so, at
> least in the past, right?
> 

My understanding is that nv_suspend will call nv_close and then 
nv_resume will call nv_open. I don't think nv_probe/nv_remove is called 
during the low power transitions.

We want to set back the flag in nv_resume in case kexec is call after 
anytime nv_resume is called. Otherwise, nv_probe (via kexec ?) will 
think it needs to reverse the address.

> Thanks,
> Björn
> 
> 
>>Signed-off-by: Ayaz Abdulla <aabdulla@nvidia.com>
>>
> 
>>--- old/drivers/net/forcedeth.c	2008-03-31 15:25:05.000000000 -0700
>>+++ new/drivers/net/forcedeth.c	2008-03-31 15:41:51.000000000 -0700
>>@@ -5317,8 +5317,7 @@
>> 
>> 	/* check the workaround bit for correct mac address order */
>> 	txreg = readl(base + NvRegTransmitPoll);
>>-	if ((txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) ||
>>-	    (id->driver_data & DEV_HAS_CORRECT_MACADDR)) {
>>+	if (id->driver_data & DEV_HAS_CORRECT_MACADDR) {
>> 		/* mac address is already in correct order */
>> 		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
>> 		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
>>@@ -5326,6 +5325,22 @@
>> 		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
>> 		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
>> 		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
>>+	} else if (txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) {
>>+		/* mac address is already in correct order */
>>+		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
>>+		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
>>+		dev->dev_addr[2] = (np->orig_mac[0] >> 16) & 0xff;
>>+		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
>>+		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
>>+		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;
>>+		/*
>>+		 * Set orig mac address back to the reversed version.
>>+		 * This flag will be cleared during low power transition.
>>+		 * Therefore, we should always put back the reversed address.
>>+		 */
>>+		np->orig_mac[0] = (dev->dev_addr[5] << 0) + (dev->dev_addr[4] << 8) +
>>+			(dev->dev_addr[3] << 16) + (dev->dev_addr[2] << 24);
>>+		np->orig_mac[1] = (dev->dev_addr[1] << 0) + (dev->dev_addr[0] << 8);
>> 	} else {
>> 		/* need to reverse mac address to correct order */
>> 		dev->dev_addr[0] = (np->orig_mac[1] >>  8) & 0xff;
>>@@ -5596,7 +5611,9 @@
>> static int nv_resume(struct pci_dev *pdev)
>> {
>> 	struct net_device *dev = pci_get_drvdata(pdev);
>>+	u8 __iomem *base = get_hwbase(dev);
>> 	int rc = 0;
>>+	u32 txreg;
>> 
>> 	if (!netif_running(dev))
>> 		goto out;
>>@@ -5607,6 +5624,11 @@
>> 	pci_restore_state(pdev);
>> 	pci_enable_wake(pdev, PCI_D0, 0);
>> 
>>+	/* restore mac address reverse flag */
>>+	txreg = readl(base + NvRegTransmitPoll);
>>+	txreg |= NVREG_TRANSMITPOLL_MAC_ADDR_REV;
>>+	writel(txreg, base + NvRegTransmitPoll);
>>+
>> 	rc = nv_open(dev);
>> out:
>> 	return rc;
> 
> 

Re: [PATCH] forcedeth: mac address fix

[Ayaz Abdulla]

Björn Steinbrink wrote:
> On 2008.03.31 19:24:24 -0500, Ayaz Abdulla wrote:
> 
>>
>>Björn Steinbrink wrote:
>>
>>>On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:
>>>
>>>
>>>>This critical patch fixes a mac address issue recently introduced. If 
>>>> 
>>>
>>>
>>>Does "recently" mean my commit 2e3884b5b16795c03a7bf295797c1b2402885b88?
>>>If so, I like to be told directly when I break stuff ;-)
>>>
>>
>>Thats why I cc'd you. :)
> 
> 
> OK, it's just that "recently broken" can mean about anything ;-) So I
> would have welcomed a "broken by xxx" in the commit message, or a small
> note below the --- line. :-)
> 
> 
>>>>the device's mac address was in correct order and the flag   
>>>>NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the flag 
>>>>would  get cleared. During next load, the mac address would get 
>>>>reversed  because the flag is missing.
>>>
>>>
>>>Hm, but nv_remove also writes back the reversed mac address. I don't see
>>>how a plain remove/probe cycle would mess things up.
>>>
>>>
>>
>>For example, NVREG_TRANSMITPOLL_MAC_ADDR_REV is set. That would mean  
>>that orig_mac will be stored with correct address. Then you call  
>>nv_remove (via ifdown) which set orig_mac back into the register and  
>>will clear the flag. On the next nv_probe (via ifup), you would perform  
>>the logic to reverse the mac address. But it is still in correct order.
> 
> 
> OK, that's the case when we had two consecutive nv_probe calls, without
> a call to nv_remove in between, right? So yeah, kexec + rmmod + modprobe
> breaks. AFAICT.
> 
Actually, I just realized the case I am looking at is different then 
ifdown/ifup. But it looks like you got it: kexec (nv_probe) + rmmod 
(nv_remove) + modprobe(nv_probe). I have seen it with 
insmod/rmmod/insmod since I don't know how kexec works.

For clarity, here is the data path.

On the first insmod, it will copy the mac address and store it in orig_mac:

	np->orig_mac[0] = readl(base + NvRegMacAddrA);
	np->orig_mac[1] = readl(base + NvRegMacAddrB);

Then, it will go into the "if" block because 
NVREG_TRANSMITPOLL_MAC_ADDR_REV is set:

	if ((txreg & NVREG_TRANSMITPOLL_MAC_ADDR_REV) ||
	    (id->driver_data & DEV_HAS_CORRECT_MACADDR)) {
		/* mac address is already in correct order */
		dev->dev_addr[0] = (np->orig_mac[0] >>  0) & 0xff;
		dev->dev_addr[1] = (np->orig_mac[0] >>  8) & 0xff;
		dev->dev_addr[2] = (np->orig_mac[0] >> 16) & 0xff;
		dev->dev_addr[3] = (np->orig_mac[0] >> 24) & 0xff;
		dev->dev_addr[4] = (np->orig_mac[1] >>  0) & 0xff;
		dev->dev_addr[5] = (np->orig_mac[1] >>  8) & 0xff;


Now, if you do a rmmod, nv_remove gets called. You store the correct 
address back into the hw register (since orig_mac was correct) but clear 
the "correct" flag:

	writel(np->orig_mac[0], base + NvRegMacAddrA);
	writel(np->orig_mac[1], base + NvRegMacAddrB);
	writel(readl(base + NvRegTransmitPoll) & ~NVREG_TRANSMITPOLL_MAC_ADDR_REV,
	       base + NvRegTransmitPoll);

Now, on next insmod, it will save the mac address into orig_mac (which 
is the correct one). But, this time it will take the "else" path because 
the previous nv_remove cleared the flag:


		/* need to reverse mac address to correct order */
		dev->dev_addr[0] = (np->orig_mac[1] >>  8) & 0xff;
		dev->dev_addr[1] = (np->orig_mac[1] >>  0) & 0xff;
		dev->dev_addr[2] = (np->orig_mac[0] >> 24) & 0xff;
		dev->dev_addr[3] = (np->orig_mac[0] >> 16) & 0xff;
		dev->dev_addr[4] = (np->orig_mac[0] >>  8) & 0xff;
		dev->dev_addr[5] = (np->orig_mac[0] >>  0) & 0xff;
		writel(txreg|NVREG_TRANSMITPOLL_MAC_ADDR_REV, base + NvRegTransmitPoll);

This will cause the mac address to be reversed! That is how your patch 
has had some negative effect.



> 
>>>>As it has been indicated previously, the flag is cleared across a low 
>>>> power transition. Therefore, the driver should set the mac address 
>>>>back  into the reversed order when clearing the flag.
>>>
>>>
>>>That's what nv_remove is supposed to do. Is there a case where nv_remove
>>>is not called?
>>>
>>
>>Sorry for the confusion. I was merely stating what needs to be done as  
>>the full solution. This logic was already in place by your patch.
>>
>>
>>>>Also, the driver should set back the flag after a low power 
>>>>transition  to protect against kexec command calling nv_probe a 
>>>>second time.
>>>
>>>
>>>Sounds like suspend stopped calling nv_remove? That would make sense
>>>then. I never checked whether suspend ever actually did call nv_remove
>>>(I think), but as my patch worked, it basically must have done so, at
>>>least in the past, right?
>>>
>>
>>My understanding is that nv_suspend will call nv_close and then  
>>nv_resume will call nv_open. I don't think nv_probe/nv_remove is called  
>>during the low power transitions.
> 
> 
> Hm, then I fail to see why my patch had any effect. I only touched
> nv_probe and nv_remove, and it solved the mac reversal on suspend
> problem... *confused*
> 
AFAICT nv_remove is not called during the power transitions.

> 
>>We want to set back the flag in nv_resume in case kexec is call after  
>>anytime nv_resume is called. Otherwise, nv_probe (via kexec ?) will  
>>think it needs to reverse the address.
> 
> 
> Hmhm, that also somehow conflicts with the fact that my patch did
> anything... I think. 
> 
> Björn

Re: [PATCH] forcedeth: mac address fix

[Björn Steinbrink]

On 2008.03.31 19:24:24 -0500, Ayaz Abdulla wrote:
>
>
> Björn Steinbrink wrote:
>> On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:
>>
>>> This critical patch fixes a mac address issue recently introduced. If 
>>>  
>>
>>
>> Does "recently" mean my commit 2e3884b5b16795c03a7bf295797c1b2402885b88?
>> If so, I like to be told directly when I break stuff ;-)
>>
> Thats why I cc'd you. :)

OK, it's just that "recently broken" can mean about anything ;-) So I
would have welcomed a "broken by xxx" in the commit message, or a small
note below the --- line. :-)

>>> the device's mac address was in correct order and the flag   
>>> NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the flag 
>>> would  get cleared. During next load, the mac address would get 
>>> reversed  because the flag is missing.
>>
>>
>> Hm, but nv_remove also writes back the reversed mac address. I don't see
>> how a plain remove/probe cycle would mess things up.
>>
>>
>
> For example, NVREG_TRANSMITPOLL_MAC_ADDR_REV is set. That would mean  
> that orig_mac will be stored with correct address. Then you call  
> nv_remove (via ifdown) which set orig_mac back into the register and  
> will clear the flag. On the next nv_probe (via ifup), you would perform  
> the logic to reverse the mac address. But it is still in correct order.

OK, that's the case when we had two consecutive nv_probe calls, without
a call to nv_remove in between, right? So yeah, kexec + rmmod + modprobe
breaks. AFAICT.

>>> As it has been indicated previously, the flag is cleared across a low 
>>>  power transition. Therefore, the driver should set the mac address 
>>> back  into the reversed order when clearing the flag.
>>
>>
>> That's what nv_remove is supposed to do. Is there a case where nv_remove
>> is not called?
>>
>
> Sorry for the confusion. I was merely stating what needs to be done as  
> the full solution. This logic was already in place by your patch.
>
>>
>>> Also, the driver should set back the flag after a low power 
>>> transition  to protect against kexec command calling nv_probe a 
>>> second time.
>>
>>
>> Sounds like suspend stopped calling nv_remove? That would make sense
>> then. I never checked whether suspend ever actually did call nv_remove
>> (I think), but as my patch worked, it basically must have done so, at
>> least in the past, right?
>>
>
> My understanding is that nv_suspend will call nv_close and then  
> nv_resume will call nv_open. I don't think nv_probe/nv_remove is called  
> during the low power transitions.

Hm, then I fail to see why my patch had any effect. I only touched
nv_probe and nv_remove, and it solved the mac reversal on suspend
problem... *confused*

> We want to set back the flag in nv_resume in case kexec is call after  
> anytime nv_resume is called. Otherwise, nv_probe (via kexec ?) will  
> think it needs to reverse the address.

Hmhm, that also somehow conflicts with the fact that my patch did
anything... I think. 

Björn

Re: [PATCH] forcedeth: mac address fix

[Björn Steinbrink]

On 2008.03.31 21:39:00 -0500, Ayaz Abdulla wrote:
> Björn Steinbrink wrote:
>> On 2008.03.31 19:24:24 -0500, Ayaz Abdulla wrote:
>>> Björn Steinbrink wrote:
>>>> On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:
>>>>> the device's mac address was in correct order and the flag    
>>>>> NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the 
>>>>> flag would  get cleared. During next load, the mac address would 
>>>>> get reversed  because the flag is missing.
>>>>
>>>> Hm, but nv_remove also writes back the reversed mac address. I don't see
>>>> how a plain remove/probe cycle would mess things up.
>>>
>>> For example, NVREG_TRANSMITPOLL_MAC_ADDR_REV is set. That would mean  
>>> that orig_mac will be stored with correct address. Then you call   
>>> nv_remove (via ifdown) which set orig_mac back into the register and  
>>> will clear the flag. On the next nv_probe (via ifup), you would 
>>> perform  the logic to reverse the mac address. But it is still in 
>>> correct order.
>>
>> OK, that's the case when we had two consecutive nv_probe calls, without
>> a call to nv_remove in between, right? So yeah, kexec + rmmod + modprobe
>> breaks. AFAICT.
>
> Actually, I just realized the case I am looking at is different then  
> ifdown/ifup. But it looks like you got it: kexec (nv_probe) + rmmod  
> (nv_remove) + modprobe(nv_probe). I have seen it with  
> insmod/rmmod/insmod since I don't know how kexec works.

I don't quite see how a plain insmod/rmmod/insmod causes that, but
anyway, I can see how the patch fixes a problem, so let's keep it at
that for now :-)

>>> My understanding is that nv_suspend will call nv_close and then   
>>> nv_resume will call nv_open. I don't think nv_probe/nv_remove is 
>>> called  during the low power transitions.
>>
>> Hm, then I fail to see why my patch had any effect. I only touched
>> nv_probe and nv_remove, and it solved the mac reversal on suspend
>> problem... *confused*
>
> AFAICT nv_remove is not called during the power transitions.

After thinking through that a bit, I realized that the affected users
might have been using some userspace stuff to wrap the suspend/resume
cycle that did remove the forcedeth module prior to suspend. IIRC the
old hibernate shell script does that. That would explain why my patch
did the trick for them.

Anyway, the patch looks good to me.

Björn

[PATCH] forcedeth: mac address fix

[Stanislav O. Bezzubtsev]

Set second bit of randomly generated mac. That marks MAC
as locally assigned. IEEE802.3, section one, 3.2.3.

Signed-off-by: Stanislav O. Bezzubtsev <stas@lvk.cs.msu.su>
---
 drivers/net/forcedeth.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index e1da466..b404c7a 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -5821,7 +5821,7 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
 		        dev->dev_addr);
 		dev_printk(KERN_ERR, &pci_dev->dev,
 			"Please complain to your hardware vendor. Switching to a random MAC.\n");
-		dev->dev_addr[0] = 0x00;
+		dev->dev_addr[0] = 0x02; /* set local assignment bit */
 		dev->dev_addr[1] = 0x00;
 		dev->dev_addr[2] = 0x6c;
 		get_random_bytes(&dev->dev_addr[3], 3);
-- 
1.6.5

Re: [PATCH] forcedeth: mac address fix

[David Miller]

From: "Stanislav O\. Bezzubtsev" <stas@lvk.cs.msu.su>
Date: Fri, 13 Nov 2009 15:22:55 +0300

> Set second bit of randomly generated mac. That marks MAC
> as locally assigned. IEEE802.3, section one, 3.2.3.
> 
> Signed-off-by: Stanislav O. Bezzubtsev <stas@lvk.cs.msu.su>

Do you know why this driver even has this bug?

It's because it does not use random_ether_addr() and tries to
do it by hand all by itself.

Please fix this bug for real by having the driver use the proper
interface to calculate a randomized ethernet address.

Thank you.

[PATCH] forcedeth: mac address fix

[Stanislav O. Bezzubtsev]

Use the existing random_ether_addr() to generate random MAC
instead of doing it by-hand.

Signed-off-by: Stanislav O. Bezzubtsev <stas@lvk.cs.msu.su>
---
 drivers/net/forcedeth.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index e1da466..3116601 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -5821,10 +5821,7 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
 		        dev->dev_addr);
 		dev_printk(KERN_ERR, &pci_dev->dev,
 			"Please complain to your hardware vendor. Switching to a random MAC.\n");
-		dev->dev_addr[0] = 0x00;
-		dev->dev_addr[1] = 0x00;
-		dev->dev_addr[2] = 0x6c;
-		get_random_bytes(&dev->dev_addr[3], 3);
+		random_ether_addr(dev->dev_addr);
 	}
 
 	dprintk(KERN_DEBUG "%s: MAC Address %pM\n",
-- 
1.6.5

Re: [PATCH] forcedeth: mac address fix

[David Miller]

From: "Stanislav O\. Bezzubtsev" <stas@lvk.cs.msu.su>
Date: Sat, 14 Nov 2009 11:31:25 +0300

> Use the existing random_ether_addr() to generate random MAC
> instead of doing it by-hand.
> 
> Signed-off-by: Stanislav O. Bezzubtsev <stas@lvk.cs.msu.su>

Applied, thank you.