Re: [PATCH] forcedeth: mac address fix

[Ayaz Abdulla <aabdulla@nvidia.com>]

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