Subject: Re: Re: pcnet32.c bug?

From: "Zhang Fuxin" <[email-masked]>
Date: Thu, 30 May 2002 23:47:26 +0800

hi,
>>--- drivers/net/pcnet32.c	19 Mar 2002 16:40:55 -0000	1.1.1.1.2.1.2.6
>>+++ drivers/net/pcnet32.c	29 May 2002 09:57:33 -0000	1.13.4.2
>>@@ -1343,6 +1351,10 @@
>>  		if (!rx_in_place) {
>>  		    skb_reserve(skb,2); /* 16 byte align */
>>  		    skb_put(skb,pkt_len);	/* Make room */
>>+                    pci_dma_sync_single(lp->pci_dev,
>>+				    lp->rx_skbuff[entry]->tail,
>>+				    pkt_len,
>>+				    PCI_DMA_FROMDEVICE);
>>  		    eth_copy_and_sum(skb,
>>  				     (unsigned char 
>>*)(lp->rx_skbuff[entry]->tail),
>>  				     pkt_len,0);

Because many mips ports do wback_inv for pci_dma_sync_single,how can
this help?
I think it just mean to invalidate cache contents the cpu has for this
buffer,and thus cpu can be sure to read data dmaed to memory by the device
And now we are doing write back,won't that be overwriting the buffer with 
stale data in cache?

so,the problem is,can we use wback_inv instead of inv in pci_dma_sync_single
when the direction is FROMDEVICE? I don't think so,but that would mean many
current drivers are broken...

i guess the fact that current driver are working is because no mips machine
has big enough cache to survive the data to the point of the buffer's reuse.

Correct solution should be something like Will Jhun's proposal? 

that is:
  each transition i.e. (FROM_DEVICE operation)
	pci_map_single()      - device now owns the buffer [invalidate]
	[DMA]
	pci_dma_sync_single() - driver now owns it         [no invalidate]
	[driver touches buffer]
	pci_dma_prep_single() - device owns it once again  [invalidate]
	[DMA] ...

I hope i am totally wrong:)

BTW: i am struggling with eepro100 driver these days,and now i have
got an NAPI version of it running. The rx ring often got messed up
with the old pci_dma_sync_xx logic,before i change it.

>
>Note that there is another problem with cache line invalidation and 
>the use of pci_dma_sync_single(), where one can get stale entries in 
>the cache when the buffer is next re-used for DMA.
>
>My co-worker Will Jhun <mailto:[email-masked]> just sent 
>e-mail on the subject of problems with the cache invalidation 
>routines last Saturday, with Message-ID: 
><[email-masked]>
>
>Kevin

>-- 

= = = = = = = = = = = = = = = = = = = =

Best Regards
---------------------------------------
Zhang Fuxin
System Architecture Lab
Institute of Computing Technology
Chinese Academy of Sciences,China
http://www.ict.ac.cn
 
			　　　　　　　　　2002-05-30

---

From: William Jhun <[email-masked]>
Date: Thu, 30 May 2002 10:37:33 -0700

> current drivers are broken...

If I understand this correctly, a writeback means only modified
cachelines (i.e. data cache lines with a dirty bit, like 'W', set) will
be written back to main memory.  Since the driver never actually writes
to any of these buffers (and their contents are invalidated on the
pci_map_single()), nothing should ever be written back to main memory.
If this were not the case, you would surely see packet corruption as the
stale cachelines from a re-used buffer would be written back. I tend not
to think that it's just coincidence and that MIPS caches tend to be
small...

So, writeback-invalidate is not incorrect, but it's not efficient for
all platforms. Ralf explained to me that some CPUs cannot do indexed
invalidates separately from writebacks.

I think all three (dma_cache_wback, dma_cache_wback_inv,
dma_cache_inv) of these calls should be implemented for all CPUs and
default to dma_cache_wback_inv() if not available. I can come up with a
patch if people agree (that wback_inv is a suitable replacement for
either _inv or _wback; MIPS-specific code than can be written to use
whatever is most optimal if present on that architecture...

Thanks,
Will