[parisc-linux] memcpy_fromio() seems partially broken [with patch]

[parisc-linux] memcpy_fromio() seems partially broken [with patch]

[Helge Deller]

Hi,

while I was playing with a few network-cards in the c3k I found a strange 
behaviour when copying unaligned data with memcpy_fromio() off the PCI bus.

Background: 
I wanted to copy data off the network-card (HP J2585A) from an 4-byte aligned 
PCI address to an 2-byte aligned (b/c of skb_reserve(skb, 2)) memory-address 
with memcpy_fromio().
The current code in CVS for memcpy_fromio() copied byteswapped and/or 
completely wrong data for all copied values, while the following 
memcpy_fromio() worked without problems:

/* Copies a block of memory from a device in an efficient manner.
 * Assumes the device can cope with 32-bit transfers.  If it can't,
 * don't use this function.
 */
void memcpy_fromio(const void *dest, unsigned long src, int count)
{
        u32 value;
        if (((unsigned long)dest & 3) != (src & 3))
                goto wordcopy;
        while (src & 3) {
                *(char *)dest = readb(src++);
                ((char *)dest)++;
                count--;
        }
        while (count > 3) {
                *(u32 *)dest = readl(src);
                dest += 4;
                src += 4;
                count -= 4;
        }
        if (!count)
                return;
 wordcopy:
        if (src & 3)
                goto bytecopy;
 wordcopy_loop:
        value = readl(src);
        *((char*)dest) = (char) (value >> 24);  dest++;
        if (--count == 0) return;
        *((char*)dest) = (char) (value >> 16);  dest++;
        if (--count == 0) return;
        *((char*)dest) = (char) (value >> 8);   dest++;
        if (--count == 0) return;
        *((char*)dest) = (char) (value);  dest++;
        if (count) goto wordcopy_loop;
        return;
 
 bytecopy:
        while (count--) {
                *(char *)dest = readb(src++);
                ((char *)dest)++;
        }
}

The previous version copied all data with readb() while this implementation 
tries to access the (pci-)bus most of the time with readl()s. 
I know, this is not the best or cleanest implementation, but I think we need 
to change memcpy_fromio() in such a manner.

What is your thought about it, or any ideas why the previous version sucked ?

Greetings,
Helge

Re: [parisc-linux] memcpy_fromio() seems partially broken [with patch]

[Matthew Wilcox]

On Fri, Jun 08, 2001 at 11:37:31PM +0200, Helge Deller wrote:
> while I was playing with a few network-cards in the c3k I found a strange 
> behaviour when copying unaligned data with memcpy_fromio() off the PCI bus.
> 
> Background: 
> I wanted to copy data off the network-card (HP J2585A) from an 4-byte aligned 
> PCI address to an 2-byte aligned (b/c of skb_reserve(skb, 2)) memory-address 
> with memcpy_fromio().
> The current code in CVS for memcpy_fromio() copied byteswapped and/or 
> completely wrong data for all copied values, while the following 
> memcpy_fromio() worked without problems:

This is very bizarre.  Following the codepaths, the current code does:

	if (((unsigned long)dest & 3) != (src & 3))
		goto bytecopy;

 bytecopy:
	while (count--) {
		*(char *)dest = readb(src++);
		((char *)dest)++;
	}

and i don't see how that can possibly do byteswapping.  unless byteswapping
is necessary and this code is failing to do that?  Your code is doing:

         if (((unsigned long)dest & 3) != (src & 3))
                 goto wordcopy;

  wordcopy:
         if (src & 3)
                 goto bytecopy;
  wordcopy_loop:
         value = readl(src);
         *((char*)dest) = (char) (value >> 24);  dest++;
         if (--count == 0) return;
         *((char*)dest) = (char) (value >> 16);  dest++;
         if (--count == 0) return;
         *((char*)dest) = (char) (value >> 8);   dest++;
         if (--count == 0) return;
         *((char*)dest) = (char) (value);  dest++;
         if (count) goto wordcopy_loop;
         return;

which is doing a byteswap on the  value returned from readl!  This seems
pretty grotesque to me.  I want to hear from someone (eg grant) who can
say what's actually going on here..

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] memcpy_fromio() seems partially broken [with patch]

[Grant Grundler]

Matthew Wilcox wrote:
> This is very bizarre.  Following the codepaths, the current code does:
...
>  bytecopy:
...
> and i don't see how that can possibly do byteswapping.  unless byteswapping
> is necessary and this code is failing to do that?

That's what it sounds like.

> Your code is doing:
> 
>          if (((unsigned long)dest & 3) != (src & 3))
>                  goto wordcopy;
> 
>   wordcopy:
>          if (src & 3)
>                  goto bytecopy;
>   wordcopy_loop:
...
> which is doing a byteswap on the  value returned from readl!

Exactly.

> This seems
> pretty grotesque to me.

It looks grotesque but the compiler should be able to
do pretty well with that.

>   I want to hear from someone (eg grant) who can
> say what's actually going on here..

The problem seems that the data needs to be swapped and I don't know why.
Perhaps the card is swapping the data?
acenic (and tulip?) has such a mode.

PARISC pci host bus adapters has a "byte is byte" approach to data.
I've heard the term "address invariance" used but not sure if
that's exactly the same thing. The idea here is data DMA'd into memory
is a byte stream and the CPU reading the data from memory byte at a time
from low address to higher address would see the same data.

Normally we expect the consumer of data (eg device driver) from IO space
to perform the swap and not the accessor functions (eg readl).

I need to do more homework to be certain about the following:
  I *think* CPU reading IO space directly needs to be swapped to
  provide the same illusion that DMA provides. The problem (I think)
  is the data is *little* endian across the PCI bus and presented as
  such to the CPU which then stores it *big* endian in memory.

Dino ERS has a nice chapter on this mess. Elroy follows the same rules.
I'll have to read that again.

grant

Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253