forcedeth: mac address fix

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 <[email protected]>

drivers/net/forcedeth.c +24 -2

@@ -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;

Quoted 1 line · Ayaz Abdulla

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 ;-)

Quoted 2 lines

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.

Quoted 1 line

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?

Quoted 2 lines

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

Quoted 60 lines

Signed-off-by Ayaz Abdulla <[email protected]>

drivers/net/forcedeth.c +24 -2

@@ -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;

Quoted 2 lines · Björn Steinbrink

On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:

Quoted 7 lines

Quoted 2 lines

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. :)

Quoted 10 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 11 lines

Quoted 4 lines

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.

Quoted 10 lines · Ayaz Abdulla

Quoted 7 lines · Björn Steinbrink

Quoted 2 lines · Ayaz Abdulla

This critical patch fixes a mac address issue recently introduced. If

Quoted 5 lines

Quoted 3 lines

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. :-)

Quoted 8 lines

Quoted 7 lines

Quoted 1 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.

Quoted 1 line

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.

Quoted 20 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 1 line

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?

Quoted 1 line

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*

Quoted 2 lines

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

Quoted 18 lines · Björn Steinbrink

Quoted 16 lines · Ayaz Abdulla

Quoted 8 lines · Björn Steinbrink

Quoted 4 lines · Ayaz Abdulla

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.

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.