Jon Mason [off-list ref] :
[...]

I never said it was pretty, simply a working snapshot of my test driver.  I 
figured that I had been promising you this update for so long, I better send 
it out before you get upset with me.  ;-)

No, no, I meant mangled as in "kmail fcuked the tabs again".

[...]

The change to opts1 seems cleaner to me.  As the only the that the adapter 
needs to know is the RingEnd.  I can try removing it and see if it makes any 
difference.

Until there is a stability or performance reason for it, I will not take
the change:
- it would add one extra parameter in any future problem report;
- neither me nor you can reproduce on it the testing done on the current form.

I would lazily label it as too much work with too low a priority.

Opts2 is VERY necessary to zero.  Without this zero, the adapter will change 
the partition point randomly of each packet (random because I have yet to 
determine the rhyme or reason).  I didn't want to spend too much time trying 
to determine the bahavior of this since I found out that zeroing it solved 
the problem.  If you can think of a reason why this doesn't work, I can 
always go back and try to figure it out.

Ok, this one really changes the behavior of the driver.

[...]

See above for reason.  I can re-order the operations and a mb (rmd I would 
think).  

So this one changes the behavior of the driver as well, right ?

The window may be narrow but I'll sleep more quietly if the ordering is
modified and the memory barrier added.

[...]

quoted

Why is rtl8169_rx_csum() moved around ?

Seemed to make more sense to me to have it after the 
pci_dma_sync_single_for_cpu.  If you disagree, I can move it back.  

rtl8169_rx_csum() only uses the consistent DMA mapping so it does not
care.

[skb_put/memcpy duplication]

Me too, but I couldn't think of a better way.  Suggestions welcomed.

Nothing fancy. Something like:
	len  = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
	memcpy(skb->data, sk_buff[0]->tail, len);
	skb_put(skb, len);

quoted

5 - rtl8169_return_to_asic() in rtl8169_rx_copy() seems suspect
    when FirstFrag is true and the skb allocation failed.

In this case the desc is only a portion of the whole packet.  I thought it 
best to toss the whole packet if the driver couldn't alloc a skb, and return 
the descriptor so that it can be used again.  

1 - the packet could be under rx_copybreak as well;
2 - the driver currently defers the loss of a packet until there is no memory
    pointed by the current descriptor used for Rx DMA from the adapter. If it
    is refilled soon enough, no one notices it.

Actually, talking about this gave me an idea.  Since desc_part is the largest 
skb we will ever need (aside from when they are combined), it would be more 
efficient to use that instead of the full size.  I will create a patch for 
this (and the mb changes) and resubmit it.  Let me know if you want me to 
return other things to their original spot.

I am not sure I understand exactly what you mean. If you are saying that
the max size is known and can be allocated in advance so that only the
rx_copybreak calls for an instant allocation, we are saying the same thing
(actually this should have been point 3- above :o) ).

It will make my life easier if the patch only contains the minimal amount of
changes (easier reviewing, testing, confidence, blah blah, it's boring but
you got the idea).

quoted

6 - skb_put() ends duplicated. It should be possible to avoid it.

Is it?  I was specifically trying to aviod that.  Where do you see that?

->
+   if (rtl8169_rx_copy(&skb, pkt_size, desc, tp)) {
     pci_action = pci_unmap_single;
     tp->Rx_skbuff[entry] = NULL;
+    skb_put(skb, pkt_size);
    }

-> rtl8169_rx_copy()
Lots of skb_put()

Don't bother too much about this one. I had expected to keep the
skb->dev = ...; skb_put(...); skb->proto = ... sequence as is to
minimize the changes but it is not necessarily doable/sane.

--
Ueimor