Problems with dma_mmap_writecombine on mach-pxa

Problems with dma_mmap_writecombine on mach-pxa

[Frank Buss]

I'm trying to use the pxafb driver on mach-pxa, but I can't mmap the
framebuffer memory. I can access it from the driver, filling the entire
screen, but when I access the pointer returned from mmap from a user space
program, the following two things happens:

- the vm_pgoff is ignored and I get the start of the internal buffer, which
caused writing to the palette and DMA descriptors

- when I change the location of the framebuffer to the start of the internal
buffer, I can write to the screen, but only to the first 4 k; any write
after this address is ignored, but no segfault is generated.

Any ideas what I can do to find the reason? I don't think that it is a
kernel bug, but perhaps a wrong configuration for my platform.

-- 
Frank Buß, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

Re: Problems with dma_mmap_writecombine on mach-pxa

[Russell King]

On Thu, Feb 17, 2005 at 06:51:49PM +0100, Frank Buss wrote:
> I'm trying to use the pxafb driver on mach-pxa, but I can't mmap the
> framebuffer memory. I can access it from the driver, filling the entire
> screen, but when I access the pointer returned from mmap from a user space
> program, the following two things happens:
> 
> - the vm_pgoff is ignored and I get the start of the internal buffer, which
> caused writing to the palette and DMA descriptors
> 
> - when I change the location of the framebuffer to the start of the internal
> buffer, I can write to the screen, but only to the first 4 k; any write
> after this address is ignored, but no segfault is generated.
> 
> Any ideas what I can do to find the reason? I don't think that it is a
> kernel bug, but perhaps a wrong configuration for my platform.

Please try this (and revert your changes):

===== arch/arm/mm/consistent.c 1.27 vs edited =====
--- 1.27/arch/arm/mm/consistent.c	2005-01-21 05:02:14 +00:00
+++ edited/arch/arm/mm/consistent.c	2005-02-17 18:11:50 +00:00
@@ -284,13 +284,15 @@
 	spin_unlock_irqrestore(&consistent_lock, flags);
 
 	if (c) {
+		unsigned long off = vma->vm_pgoff;
+
 		kern_size = (c->vm_end - c->vm_start) >> PAGE_SHIFT;
 
-		if (vma->vm_pgoff < kern_size &&
-		    user_size <= (kern_size - vma->vm_pgoff)) {
+		if (off < kern_size &&
+		    user_size <= (kern_size - off)) {
 			vma->vm_flags |= VM_RESERVED;
 			ret = remap_pfn_range(vma, vma->vm_start,
-					      page_to_pfn(c->vm_pages),
+					      page_to_pfn(c->vm_pages) + off,
 					      user_size, vma->vm_page_prot);
 		}
 	}


-- 
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: Problems with dma_mmap_writecombine on mach-pxa

[Frank Buss]

> Please try this (and revert your changes):

thanks, this could fix the bug with the vm_pgoff, but I don't think this
will fix the problem with the ignored memory access after the first
PAGE_SIZE from the mapped memory. I'll try it tommorow, when I'm again at my
customers site, where I have access to the board.

-- 
Frank Buß, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

Re: Problems with dma_mmap_writecombine on mach-pxa

[Russell King]

On Thu, Feb 17, 2005 at 08:14:34PM +0100, Frank Buss wrote:
> > Please try this (and revert your changes):
> 
> thanks, this could fix the bug with the vm_pgoff, but I don't think this
> will fix the problem with the ignored memory access after the first
> PAGE_SIZE from the mapped memory. I'll try it tommorow, when I'm again at my
> customers site, where I have access to the board.

Since we map the whole lot in one go, if you get one page, there's no
reason why you shouldn't get the lot.  This is why I'm wondering if
it has something to do with your other modifications.

-- 
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

[TTY] 2 points seems strange to me.

[Franck Bui-Huu]

Hi,

Looking at TTY code, I noticed a weird test done in "opost_bock"
located in n_tty.c file. I don't understand why the following test is
done at the start of the function:
if (nr > sizeof(buf))
        nr = sizeof(buf);
Actually it limits the size of processing blocks to 4 bytes and I can't
find a reason why.

Second point, a lot of serial drivers call in their interrupt handler
"tty_flip_buffer_push" function. This function must no be called
in interrupt context. Why is it done anyway ?

Thanks for your answers.

       Franck

Re: [TTY] 2 points seems strange to me.

[Paul Fulghum]

Franck Bui-Huu wrote:
> Looking at TTY code, I noticed a weird test done in "opost_bock"
> located in n_tty.c file. I don't understand why the following test is
> done at the start of the function:
> if (nr > sizeof(buf))
>        nr = sizeof(buf);
> Actually it limits the size of processing blocks to 4 bytes and I can't
> find a reason why.

No, it limits the size to 80 bytes,
which is the size of buf.

sizeof returns the size of the char array buf[80]
(standard C)

> Second point, a lot of serial drivers call in their interrupt handler
> "tty_flip_buffer_push" function. This function must no be called
> in interrupt context. Why is it done anyway ?

Calling tty_flip_buffer_push() is fine from interrupt
as long as tty->low_latency is not set. It just queues
work for later.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [TTY] 2 points seems strange to me.

[Paulo Marques]

Paul Fulghum wrote:
> Franck Bui-Huu wrote:
> 
>> Looking at TTY code, I noticed a weird test done in "opost_bock"
>> located in n_tty.c file. I don't understand why the following test is
>> done at the start of the function:
>> if (nr > sizeof(buf))
>>        nr = sizeof(buf);
>> Actually it limits the size of processing blocks to 4 bytes and I can't
>> find a reason why.
> 
> 
> No, it limits the size to 80 bytes,
> which is the size of buf.
> 
> sizeof returns the size of the char array buf[80]
> (standard C)

Looking at the code, I think Franck is right. buf is a "const unsigned 
char *" for which sizeof(buf) is the size of a pointer.

This certainly looks like a bug.

Since the function doesn't guarantee that nr bytes are written, and the 
caller must handle the case of fewer bytes, this probably went unnoticed.

-- 
Paulo Marques - www.grupopie.com

All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

Re: [TTY] 2 points seems strange to me.

[Paul Fulghum]

Paulo Marques wrote:
> Paul Fulghum wrote:
>> No, it limits the size to 80 bytes,
>> which is the size of buf.
>>
>> sizeof returns the size of the char array buf[80]
>> (standard C)
> 
> Looking at the code, I think Franck is right. buf is a "const unsigned 
> char *" for which sizeof(buf) is the size of a pointer.

What kernel version are you looking at?
I'm looking at 2.4.20 n_tty.c opost_block() and
buf is a char array.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [TTY] 2 points seems strange to me.

[Paulo Marques]

Paul Fulghum wrote:
> Paulo Marques wrote:
> 
>> Paul Fulghum wrote:
>>
>>> No, it limits the size to 80 bytes,
>>> which is the size of buf.
>>>
>>> sizeof returns the size of the char array buf[80]
>>> (standard C)
>>
>>
>> Looking at the code, I think Franck is right. buf is a "const unsigned 
>> char *" for which sizeof(buf) is the size of a pointer.
> 
> 
> What kernel version are you looking at?
> I'm looking at 2.4.20 n_tty.c opost_block() and
> buf is a char array.

Oops, this should have been clarified sooner...

I was looking at a 2.6.11-rc2 tree I have hanging around here. So maybe 
the "buf" type was changed and the condition remained, and that produced 
the bug.

-- 
Paulo Marques - www.grupopie.com

All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

Re: [TTY] 2 points seems strange to me.

[linux-os]

On Fri, 18 Feb 2005, Paul Fulghum wrote:

> Paulo Marques wrote:
>> Paul Fulghum wrote:
>>> No, it limits the size to 80 bytes,
>>> which is the size of buf.
>>> 
>>> sizeof returns the size of the char array buf[80]
>>> (standard C)
>> 
>> Looking at the code, I think Franck is right. buf is a "const unsigned char 
>> *" for which sizeof(buf) is the size of a pointer.
>
> What kernel version are you looking at?
> I'm looking at 2.4.20 n_tty.c opost_block() and
> buf is a char array.
>
> -- 
> Paul Fulghum
> Microgate Systems, Ltd.

Ahaa!  That's how the bug got introduced. It used to be an
array and then it got changed to a pointer! linux-2.4.26
also shows a local array.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.10 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.

Re: [TTY] 2 points seems strange to me.

[Paulo Marques]

linux-os wrote:
> On Fri, 18 Feb 2005, Paul Fulghum wrote:
> 
>> Paulo Marques wrote:
>>
>>> Paul Fulghum wrote:
>>>
>>>> No, it limits the size to 80 bytes,
>>>> which is the size of buf.
>>>>
>>>> sizeof returns the size of the char array buf[80]
>>>> (standard C)
>>>
>>>
>>> Looking at the code, I think Franck is right. buf is a "const 
>>> unsigned char *" for which sizeof(buf) is the size of a pointer.
>>
>>
>> What kernel version are you looking at?
>> I'm looking at 2.4.20 n_tty.c opost_block() and
>> buf is a char array.
>>
>> -- 
>> Paul Fulghum
>> Microgate Systems, Ltd.
> 
> 
> Ahaa!  That's how the bug got introduced. It used to be an
> array and then it got changed to a pointer! linux-2.4.26
> also shows a local array.

Yes, just looked at the revision history in linux.bkbits.net and Linus 
just fixed this 67 hours ago... So we're too late :)

-- 
Paulo Marques - www.grupopie.com

All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

Re: [TTY] 2 points seems strange to me.

[Franck Bui-Huu]

>>
>> Ahaa!  That's how the bug got introduced. It used to be an
>> array and then it got changed to a pointer! linux-2.4.26
>> also shows a local array.
>
>
> Yes, just looked at the revision history in linux.bkbits.net and Linus 
> just fixed this 67 hours ago... So we're too late :)
>
ok, maybe next time :)

          Franck

Re: [TTY] 2 points seems strange to me.

[linux-os]

On Fri, 18 Feb 2005, Paulo Marques wrote:
> Franck Bui-Huu wrote:
> 
>> Looking at TTY code, I noticed a weird test done in "opost_bock"
>> located in n_tty.c file. I don't understand why the following test is
>> done at the start of the function:
>> if (nr > sizeof(buf))
>>        nr = sizeof(buf);
>> Actually it limits the size of processing blocks to 4 bytes and I can't
>> find a reason why.
> 
> No, it limits the size to 80 bytes,
> which is the size of buf.
> 
> sizeof returns the size of the char array buf[80]
> (standard C)
>

Huh?? "buf" is a pointer. On this system pointers are 4-bytes
in length. What "standard C" are you using? Frank found a bug.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.10 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.

Re: [TTY] 2 points seems strange to me.

[Franck Bui-Huu]

>> Second point, a lot of serial drivers call in their interrupt handler
>> "tty_flip_buffer_push" function. This function must no be called
>> in interrupt context. Why is it done anyway ?
>
>
> Calling tty_flip_buffer_push() is fine from interrupt
> as long as tty->low_latency is not set. It just queues
> work for later.
>
I was looking at driver for 8250 in 8250.c file and at the end
of "receive_chars" interrupt handler, it calls "tty_flip_buffer_push"
even if "tty->low_latency" is set since no such test is done before
the call...
I was also wondering why not always calling "schedule_delayed_work"
whatever the state of "tty->latency"?

          Franck

Re: [TTY] 2 points seems strange to me.

[Paul Fulghum]

Franck Bui-Huu wrote:
> I was looking at driver for 8250 in 8250.c file and at the end
> of "receive_chars" interrupt handler, it calls "tty_flip_buffer_push"
> even if "tty->low_latency" is set since no such test is done before
> the call...

Yes this is a known problem. In the case of SMP kernel
and low_latency, and 8250.c, this causes a dead lock. This has
resurfaced again and again for many months from
different sources.

There is disagreement on what code is
in error and what should be fixed. So it stays broken.

> I was also wondering why not always calling "schedule_delayed_work"
> whatever the state of "tty->latency"?

If low_latency is set and you are not calling
from interrupt, then calling work directly speeds processing.

I submitted a patch that forced schedule_delayed_work
if in interrupt context to avoid this problem. It was rejected.

-- 
Paul Fulghum
Microgate Systems, Ltd.

RE: Problems with dma_mmap_writecombine on mach-pxa

[Frank Buss]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> Since we map the whole lot in one go, if you get one page, there's no
> reason why you shouldn't get the lot.  This is why I'm wondering if
> it has something to do with your other modifications.

your patch works, thanks, but only for the problem with the ignored offset, 
as expected. Now I can use the original pxafb driver, but with the same 
problem: All writes from user space in the mmap'ed region after the first 
4096 bytes are ignored.

Perhaps it is not a kernel bug, but a configuration problem with the 
platform, because it is a in-house developed platform. Now a colleague is 
working on this problem and reading a book about the Linux 2.6 kernel 
details over weekend and learning how to use the kernel debugger (until now 
we have used printk and flashing new zImages every time, which is very time 
consuming). I'll post the solution next week, if we'll found one.

-- 
Frank Buß, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

RE: Problems with dma_mmap_writecombine on mach-pxa

[Frank Buss]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> Since we map the whole lot in one go, if you get one page, there's no
> reason why you shouldn't get the lot.  This is why I'm wondering if
> it has something to do with your other modifications.

my colleage has found the bug: in the function dma_mmap in 
arch/arm/mm/consistent.c the call to remap_pfn_range uses 
user_size in PAGE_SIZE units, but looks like it is expected 
in bytes. When using (user_size << PAGE_SHIFT), it works.

I don't know, where to fix it: Should the lower level calls 
get the size in bytes (most function arguments in Linux 
kernel sources are not commented), this means fixing the 
dma_mmap, or should PAGE_SIZE be used, then the lower level 
functions needs to be fixed.

-- 
Frank Buß, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de