Re: [PATCH] forcedeth: mac address fix

["Björn Steinbrink" <B.Steinbrink@gmx.de>]

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