Re: [PATCH] forcedeth: mac address fix

[Ayaz Abdulla <aabdulla@nvidia.com>]

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