Re: [PATCH] fix e100 rx path on ARM (was [PATCH] e100 rx: or s and el bits)

[David Acker <dacker@roinet.com>]

Milton Miller wrote:
> 
> On Jun 1, 2007, at 3:45 PM, David Acker wrote:
>> Ok, I took a stab at coding and testing these ideas.  Below is a patch 
>> against 2.6.22-rc3.
>> Let me know what you think.
> 
> I think you got most of the ideas.   As Auke noted, your coding style is 
> showing again.   And your mailer again munged whitespace (fixed by 
> s/^<space><space>/<space>/ s/^$/<space>/).
Sorry about the coding style.  I instinctively followed what was there 
instead of kernel coding convention.  I will look into how whitespace is 
getting screwed up.

>> Find a buffer that is complete with rx->el not set and rx->s0 set.
>>     It appears that hardware can read the rfd's el-bit, then software 
>> can clear the rfd el-bit and set the rfd size, and then hardware can 
>> come in and read the size.
> 
> Yes, since the size is after the EL flag in the descriptor, this can 
> happen since the pci read is not atomic.
> 
>> I am reading the status back, although I don't think that I have to in 
>> this instance.
> 
> Actually, you are reading it when the rfd still has EL set.  Since the 
> cpu will never encounter that case, the if condition is never satisfied.
In my tests, every time I found a completed rfd with the el-bit set, the 
receiver was in the out of resources state.


> How about creating a state unknown, for when we think we should check 
> the device if its running.
> If we are in this state and then encounter a received packet without s0 
> set, we can set it back
> to running.   We set it when we rx a packet with s0 set.
> 
> We then move both io_status reads to the caller.
I can look into that as I clean this up.


>> I am testing a version of this code patched against 2.6.18.4 on my PXA 
>> 255 based system.  I will let you all know how it goes.
The testing I did so far did well.  I will try to get some more going 
tonight, hopefully on a cleaned up patch.


> I'm assuming this is why the cleanup of the receiver start to always 
> start on rx_to_clean got dropped again. :-)
Yep.  I will get that in the next patch.


> Also, I would like a few sentences in the Driver Operation section IV 
> Receive big comment.  Something like
> 
> In order to keep updates to the RFD link field from colliding with 
> hardware writes to mark packets complete, we use the feature that 
> hardware will not write to a size 0 descriptor and mark the previous 
> packet as end-of-list (EL).   After updating the link, we remove EL and 
> only then restore the size such that hardware may use the 
> previous-to-end RFD.
> 
> at the end of the first paragraph, and insert software before "no 
> locking is required" in the second.
Sounds good to me.

I will see if I can get into a cleaned up patch today and get it out by 
tomorrow.  Thanks for dealing with me...I have been around kernel code 
for awhile but posting official patches to linux is new to me.
-Ack