[RFC] Buggy e100.c on ARM / DMA sync interfaces

[RFC] Buggy e100.c on ARM / DMA sync interfaces

[Russell King]

I have a report which indicates that the e100.c driver does not work on
ARM when caches are in writeback mode.  The symptoms indicate that the
card is unable to communicate properly.

Looking up port of RPC 100003/2 on 10.2.40.5
portmap: server 10.2.40.5 not responding, timed out
Root-NFS: Unable to get nfsd port number from server, using default
Looking up port of RPC 100005/1 on 10.2.40.5
portmap: server 10.2.40.5 not responding, timed out

Looking over the e100.c code, I find the following in e100_rx_alloc_skb():

        rx->skb->dev = nic->netdev;
        skb_reserve(rx->skb, rx_offset);
        memcpy(rx->skb->data, &nic->blank_rfd, sizeof(struct rfd));
        rx->dma_addr = pci_map_single(nic->pdev, rx->skb->data,
                RFD_BUF_LEN, PCI_DMA_BIDIRECTIONAL);

        /* Link the RFD to end of RFA by linking previous RFD to
         * this one, and clearing EL bit of previous.  */
        if(rx->prev->skb) {
                struct rfd *prev_rfd = (struct rfd *)rx->prev->skb->data;
                put_unaligned(cpu_to_le32(rx->dma_addr),
                        (u32 *)&prev_rfd->link);
                wmb();
                prev_rfd->command &= ~cpu_to_le16(cb_el);
                pci_dma_sync_single_for_device(nic->pdev, rx->prev->dma_addr,
                        sizeof(struct rfd), PCI_DMA_TODEVICE);
        }

This can't work - and I suspect anyone using the *dma_sync* functions
will be in for the same problem.  Why?

Cache lines.  They have a defined size and are not merely a single byte
or a single word.  If you modify even one single bit, you stand the
chance of writing back the whole cache line, possibly overwriting data
which the device has updated since the cache line was read.

Therefore, if you're going to use the dma_sync functions to modify data
owned by the remote device, you _must_ stop the remote device accessing
the surrounding data _before_ touching it.

With the above code on ARM, it effectively means that we will read the
whole struct rfd and some other data into cache, modify the command
field, and then write _at least_ the whole struct rfd back out, all
with the chip's DMA still running.

_That_ can't be good.

Note - I'm not saying that this is the cause of the above problem, but
that this is something I have spotted while reading through the driver
to ascertain why it possibly could not be working.

Comments?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [RFC] Buggy e100.c on ARM / DMA sync interfaces

[James Bottomley]

> This can't work - and I suspect anyone using the *dma_sync* functions
> will be in for the same problem.  Why?
> 
> Cache lines.  They have a defined size and are not merely a single byte
> or a single word.  If you modify even one single bit, you stand the
> chance of writing back the whole cache line, possibly overwriting data
> which the device has updated since the cache line was read.

Hang on, the PCI (and DMA) API explicitly states that to call sync on a
dma mapped area, you must do it for the *whole* of the area.  The API's
also transfer ownership of the area (i.e. only the device *or* the
driver may touch it depending on who has the ownership). This is
designedly to get us out of cache line interference problems.

So the particular problem here is an incorrect *abuse* of the API. 
There is a section in the black magic part of the DMA API that allows
you partially to sync a mapped area (dma_sync_single_range).  However,
it's in the experts only section and it explains that if you do this,
you're responsible for preventing cache line interference and provides
another API (dma_get_cache_alignment) explicitly so the driver knows
what the cache line boundaries are.

> Therefore, if you're going to use the dma_sync functions to modify data
> owned by the remote device, you _must_ stop the remote device accessing
> the surrounding data _before_ touching it.

Precisely, hence the ownership concept.

> With the above code on ARM, it effectively means that we will read the
> whole struct rfd and some other data into cache, modify the command
> field, and then write _at least_ the whole struct rfd back out, all
> with the chip's DMA still running.
> 
> _That_ can't be good.
> 
> Note - I'm not saying that this is the cause of the above problem, but
> that this is something I have spotted while reading through the driver
> to ascertain why it possibly could not be working.
> 
> Comments?

Clearly the e100 needs fixing.  Either by migrating it to the expert
API. Or, in this case, could we not just pad the structure with
appropriate L1_CACHE_ALIGNMENT tags?

James