[PATCH update6] drivers: add LCD support

[PATCH update6] drivers: add LCD support

[Miguel Ojeda Sandonis]

Andrew, here it is the coding style fixes. Thanks you.

I think the driver is getting ready for freeze until
2.6.20-rc1 (if anyone sees something wrong), so I will
try to send just minor fixes like this.

After that, in the future, I will work on the Paulo Marques'
idea to reduce CPU waste even more if possible.
---

 - Minor coding style fixes

 Documentation/auxdisplay/cfag12864b-example.c |    4 ++--
 drivers/auxdisplay/cfag12864b.c               |    4 ++--
 drivers/auxdisplay/cfag12864bfb.c             |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

miguelojeda-drivers-add-LCD-support-6-minor-coding-style-fixes
Signed-off-by: Miguel Ojeda Sandonis <maxextreme@gmail.com>
---
diff -uprN -X dontdiff linux-2.6.19-rc1-mod3/Documentation/auxdisplay/cfag12864b-example.c linux/Documentation/auxdisplay/cfag12864b-example.c
--- linux-2.6.19-rc1-mod3/Documentation/auxdisplay/cfag12864b-example.c	2006-11-01 00:39:39.000000000 +0000
+++ linux/Documentation/auxdisplay/cfag12864b-example.c	2006-11-01 01:27:55.000000000 +0000
@@ -203,7 +203,7 @@ void example(unsigned char n)
 	unsigned short i, j;
 	unsigned char matrix[CFAG12864B_WIDTH * CFAG12864B_HEIGHT];
 
-	if(n > EXAMPLES)
+	if (n > EXAMPLES)
 		return;
 
 	printf("Example %i/%i - ", n, EXAMPLES);
@@ -273,7 +273,7 @@ int main(int argc, char *argv[])
 	for (n = 1; n <= EXAMPLES; n++) {
 		example(n);
 		cfag12864b_blit();
-		while(getchar() != '\n');
+		while (getchar() != '\n');
 	}
 
 	cfag12864b_exit();
diff -uprN -X dontdiff linux-2.6.19-rc1-mod3/drivers/auxdisplay/cfag12864b.c linux/drivers/auxdisplay/cfag12864b.c
--- linux-2.6.19-rc1-mod3/drivers/auxdisplay/cfag12864b.c	2006-11-01 00:40:59.000000000 +0000
+++ linux/drivers/auxdisplay/cfag12864b.c	2006-11-01 01:25:12.000000000 +0000
@@ -237,7 +237,7 @@ unsigned char cfag12864b_enable(void)
 
 	mutex_lock(&cfag12864b_mutex);
 
-	if(!cfag12864b_updating) {
+	if (!cfag12864b_updating) {
 		cfag12864b_updating = 1;
 		cfag12864b_queue();
 		ret = 0;
@@ -254,7 +254,7 @@ void cfag12864b_disable(void)
 {
 	mutex_lock(&cfag12864b_mutex);
 
-	if(cfag12864b_updating) {
+	if (cfag12864b_updating) {
 		cfag12864b_updating = 0;
 		cancel_delayed_work(&cfag12864b_work);
 		flush_workqueue(cfag12864b_workqueue);
diff -uprN -X dontdiff linux-2.6.19-rc1-mod3/drivers/auxdisplay/cfag12864bfb.c linux/drivers/auxdisplay/cfag12864bfb.c
--- linux-2.6.19-rc1-mod3/drivers/auxdisplay/cfag12864bfb.c	2006-11-01 00:40:59.000000000 +0000
+++ linux/drivers/auxdisplay/cfag12864bfb.c	2006-11-01 01:26:10.000000000 +0000
@@ -71,7 +71,7 @@ static struct page *cfag12864bfb_vma_nop
 	struct page *page = virt_to_page(cfag12864b_buffer);
 	get_page(page);
 
-	if(type)
+	if (type)
 		*type = VM_FAULT_MINOR;
 
 	return page;
@@ -156,7 +156,7 @@ static int __init cfag12864bfb_init(void
 {
 	int ret;
 
-	if(cfag12864b_enable()) {
+	if (cfag12864b_enable()) {
 		printk(KERN_ERR CFAG12864BFB_NAME ": ERROR: "
 			"can't enable cfag12864b refreshing (being used)\n");
 		return -ENODEV;

Re: [PATCH update6] drivers: add LCD support

[Franck Bui-Huu]

Hi

Miguel Ojeda Sandonis wrote:
> Andrew, here it is the coding style fixes. Thanks you.
> 
> I think the driver is getting ready for freeze until
> 2.6.20-rc1 (if anyone sees something wrong), so I will
> try to send just minor fixes like this.

:)

Again, any thoughts on cache aliasing ?

Thanks
		Franck

Re: [PATCH update6] drivers: add LCD support

[Miguel Ojeda]

On 11/2/06, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> Hi
>
> Miguel Ojeda Sandonis wrote:
> > Andrew, here it is the coding style fixes. Thanks you.
> >
> > I think the driver is getting ready for freeze until
> > 2.6.20-rc1 (if anyone sees something wrong), so I will
> > try to send just minor fixes like this.
>
> :)
>
> Again, any thoughts on cache aliasing ?
>

If you have seen something wrong, please tell. What are you thinking about?

Thanks,
     Miguel Ojeda

Re: [PATCH update6] drivers: add LCD support

[Franck Bui-Huu]

Miguel Ojeda wrote:
> On 11/2/06, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> Hi
>>
>> Miguel Ojeda Sandonis wrote:
>> > Andrew, here it is the coding style fixes. Thanks you.
>> >
>> > I think the driver is getting ready for freeze until
>> > 2.6.20-rc1 (if anyone sees something wrong), so I will
>> > try to send just minor fixes like this.
>>
>> :)
>>
>> Again, any thoughts on cache aliasing ?
>>
> 
> If you have seen something wrong, please tell. What are you thinking about?

Sorry for the short/fast question. I'm wondering how does the cache
behave here. You have 2 virtual addresses that point to the same
location in physical RAM: kernel frame buffer which has a kernel
virtual address and the vma you're returning when an application mmap
the device. This last address is a user virtual address and is
different from the first one.

Now let's say that some of the kernel frame buffer data are in the
data cache and never be invalidate during this example. The
application updates its mmapped frame buffer. During the refresh time,
the kernel take a look to its frame buffer to check if something
change. Since the 'old' data are still in the data cache and are
valid, the kernel will use them instead of the new one set by the
application.

		Franck

Re: [PATCH update6] drivers: add LCD support

[Miguel Ojeda]

On 11/2/06, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> Miguel Ojeda wrote:
> > On 11/2/06, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> >> Hi
> >>
> >> Miguel Ojeda Sandonis wrote:
> >> > Andrew, here it is the coding style fixes. Thanks you.
> >> >
> >> > I think the driver is getting ready for freeze until
> >> > 2.6.20-rc1 (if anyone sees something wrong), so I will
> >> > try to send just minor fixes like this.
> >>
> >> :)
> >>
> >> Again, any thoughts on cache aliasing ?
> >>
> >
> > If you have seen something wrong, please tell. What are you thinking about?
>
> Sorry for the short/fast question. I'm wondering how does the cache
> behave here. You have 2 virtual addresses that point to the same
> location in physical RAM: kernel frame buffer which has a kernel
> virtual address and the vma you're returning when an application mmap
> the device. This last address is a user virtual address and is
> different from the first one.
>
> Now let's say that some of the kernel frame buffer data are in the
> data cache and never be invalidate during this example. The

Sorry, I don't understand what do you mean with this sentence.

> application updates its mmapped frame buffer. During the refresh time,
> the kernel take a look to its frame buffer to check if something
> change. Since the 'old' data are still in the data cache and are
> valid, the kernel will use them instead of the new one set by the
> application.

Anyway: The data cache contains "old" data (last buffer data). Right.
The application has changed the buffer. Also OK. Why memcmp won't find
the differences?

What do you mean with "data cache", "kernel fb data", ...?

Re: [PATCH update6] drivers: add LCD support

[Andrew Morton]

On Thu, 2 Nov 2006 14:44:56 +0000
"Miguel Ojeda" <maxextreme@gmail.com> wrote:

> > Sorry for the short/fast question. I'm wondering how does the cache
> > behave here. You have 2 virtual addresses that point to the same
> > location in physical RAM: kernel frame buffer which has a kernel
> > virtual address and the vma you're returning when an application mmap
> > the device. This last address is a user virtual address and is
> > different from the first one.
> >
> > Now let's say that some of the kernel frame buffer data are in the
> > data cache and never be invalidate during this example. The
> 
> Sorry, I don't understand what do you mean with this sentence.

Some CPU architectures experience what Documentation/cachetlb.txt calls
"virtual aliasing in the data cache".

If you map the same physical page at virtual address A1 and also at another
virtual address A2 then writes to address A1 do not necessarily appear
correctly at address A2.  This because the write to A1 is stuck in the CPU
cache and the CPU hardware doesn't know that read from A2 is accessing the
same page.

The solutions to this are:

a) add lots of flushing everywhere (see Documentation/cachetlb.txt, I
   guess) (this documentation is maddening: it uses the term "flush" for
   both writeback and for invalidation.  In this context,
   flush==writeback).

b) If you select the correct virtual addresses for A1 and A2 (ie: ensure
   that the mmap() handler returns an address which correlates with the
   page's kernel address) then apparently the aliasing goes away.

   So, for example, if the kernel's view of the page is at 0xd0102000
   then you make sure that userspace's address for the page is at
   0xnnn02000 (for some length of nnn).

   I don't know what the mmap handler has to do to arrange for this to
   happen.

c) add `depends on x86' to your Kconfig ;)


davem and rmk are (amongst others) the guys for this stuff.  afaik it isn't
documented anywhere.

Re: [PATCH update6] drivers: add LCD support

[Miguel Ojeda]

On 11/2/06, Andrew Morton <akpm@osdl.org> wrote:
> On Thu, 2 Nov 2006 14:44:56 +0000
> "Miguel Ojeda" <maxextreme@gmail.com> wrote:
>
> > > Now let's say that some of the kernel frame buffer data are in the
> > > data cache and never be invalidate during this example. The
> >
> > Sorry, I don't understand what do you mean with this sentence.
>
> Some CPU architectures experience what Documentation/cachetlb.txt calls
> "virtual aliasing in the data cache".
>
> If you map the same physical page at virtual address A1 and also at another
> virtual address A2 then writes to address A1 do not necessarily appear
> correctly at address A2.  This because the write to A1 is stuck in the CPU
> cache and the CPU hardware doesn't know that read from A2 is accessing the
> same page.
>
> The solutions to this are:
>
> a) add lots of flushing everywhere (see Documentation/cachetlb.txt, I
>    guess) (this documentation is maddening: it uses the term "flush" for
>    both writeback and for invalidation.  In this context,
>    flush==writeback).
>
> b) If you select the correct virtual addresses for A1 and A2 (ie: ensure
>    that the mmap() handler returns an address which correlates with the
>    page's kernel address) then apparently the aliasing goes away.
>
>    So, for example, if the kernel's view of the page is at 0xd0102000
>    then you make sure that userspace's address for the page is at
>    0xnnn02000 (for some length of nnn).
>
>    I don't know what the mmap handler has to do to arrange for this to
>    happen.
>
> c) add `depends on x86' to your Kconfig ;)

I really like portable stuff :)

>
>
> davem and rmk are (amongst others) the guys for this stuff.  afaik it isn't
> documented anywhere.
>

Whoa, thanks you for the long explanation. May 2.6.18-new vmalloc
related functions help correlating userspace & kernel addresses? I
will try them and come with an answer tomorrow.

Quoting http://lwn.net/Articles/2.6-kernel-api/

"Some functions have been added to make it easy for kernel code to
allocate a buffer with vmalloc() and map it into user space. They are:

     void *vmalloc_user(unsigned long size);
     void *vmalloc_32_user(unsigned long size);
     int remap_vmalloc_range(struct vm_area_struct *vma, void *addr,
                             unsigned long pgoff);

The first two functions are a form of vmalloc() which obtain memory
intended to be mapped into user space; among other things, they zero
the entire range to avoid leaking data. vmalloc_32_user() allocates
low memory only. A call to remap_vmalloc_range() will complete the
job; it will refuse, however, to remap memory which has not been
allocated with one of the two functions above."

Re: [PATCH update6] drivers: add LCD support

[Andrew Morton]

On Thu, 2 Nov 2006 19:33:48 +0000
"Miguel Ojeda" <maxextreme@gmail.com> wrote:

> May 2.6.18-new vmalloc
> related functions help correlating userspace & kernel addresses? I
> will try them and come with an answer tomorrow.
> 
> Quoting http://lwn.net/Articles/2.6-kernel-api/
> 
> "Some functions have been added to make it easy for kernel code to
> allocate a buffer with vmalloc() and map it into user space. They are:
> 
>      void *vmalloc_user(unsigned long size);
>      void *vmalloc_32_user(unsigned long size);
>      int remap_vmalloc_range(struct vm_area_struct *vma, void *addr,
>                              unsigned long pgoff);
> 
> The first two functions are a form of vmalloc() which obtain memory
> intended to be mapped into user space; among other things, they zero
> the entire range to avoid leaking data. vmalloc_32_user() allocates
> low memory only. A call to remap_vmalloc_range() will complete the
> job; it will refuse, however, to remap memory which has not been
> allocated with one of the two functions above."

No, it doesn't look like those helper functions are designed to handle this.

I'm really not the person to be asking about this.  I can poke around in
arch/sparc64/kernel/sys_sparc.c:arch_get_unmapped_area() as well as the
next guy, and it seems to be doing the right thing for MAP_FIXED, but
how/whether it handles !MAP_FIXED I do not know.  Ask davem ;)

Re: [PATCH update6] drivers: add LCD support

[David Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Thu, 2 Nov 2006 12:04:12 -0800

> On Thu, 2 Nov 2006 19:33:48 +0000
> "Miguel Ojeda" <maxextreme@gmail.com> wrote:
> 
> > May 2.6.18-new vmalloc
> > related functions help correlating userspace & kernel addresses? I
> > will try them and come with an answer tomorrow.
> > 
> > Quoting http://lwn.net/Articles/2.6-kernel-api/
> > 
> > "Some functions have been added to make it easy for kernel code to
> > allocate a buffer with vmalloc() and map it into user space. They are:
> > 
> >      void *vmalloc_user(unsigned long size);
> >      void *vmalloc_32_user(unsigned long size);
> >      int remap_vmalloc_range(struct vm_area_struct *vma, void *addr,
> >                              unsigned long pgoff);
> > 
> > The first two functions are a form of vmalloc() which obtain memory
> > intended to be mapped into user space; among other things, they zero
> > the entire range to avoid leaking data. vmalloc_32_user() allocates
> > low memory only. A call to remap_vmalloc_range() will complete the
> > job; it will refuse, however, to remap memory which has not been
> > allocated with one of the two functions above."
> 
> No, it doesn't look like those helper functions are designed to handle this.
> 
> I'm really not the person to be asking about this.  I can poke around in
> arch/sparc64/kernel/sys_sparc.c:arch_get_unmapped_area() as well as the
> next guy, and it seems to be doing the right thing for MAP_FIXED, but
> how/whether it handles !MAP_FIXED I do not know.  Ask davem ;)

Unfortunately that code never gets called for MAP_FIXED :-)

I'll comment on these issues and explain what needs to occur,
we have several things that want to do this kind of user/kernel
sharing of ring buffers and similar, so best to get the
infrastructure going to get it right.

As a first approximation, getting remap_vmalloc_range() to do the
right thing is the best way to start this stuff off.

Re: [PATCH update6] drivers: add LCD support

[Miguel Ojeda]

On 11/3/06, David Miller <davem@davemloft.net> wrote:
> From: Andrew Morton <akpm@osdl.org>
> Date: Thu, 2 Nov 2006 12:04:12 -0800
>
> > On Thu, 2 Nov 2006 19:33:48 +0000
> > "Miguel Ojeda" <maxextreme@gmail.com> wrote:
> >
> > > May 2.6.18-new vmalloc
> > > related functions help correlating userspace & kernel addresses? I
> > > will try them and come with an answer tomorrow.
> > >
> > > Quoting http://lwn.net/Articles/2.6-kernel-api/
> > >
> > > "Some functions have been added to make it easy for kernel code to
> > > allocate a buffer with vmalloc() and map it into user space. They are:
> > >
> > >      void *vmalloc_user(unsigned long size);
> > >      void *vmalloc_32_user(unsigned long size);
> > >      int remap_vmalloc_range(struct vm_area_struct *vma, void *addr,
> > >                              unsigned long pgoff);
> > >
> > > The first two functions are a form of vmalloc() which obtain memory
> > > intended to be mapped into user space; among other things, they zero
> > > the entire range to avoid leaking data. vmalloc_32_user() allocates
> > > low memory only. A call to remap_vmalloc_range() will complete the
> > > job; it will refuse, however, to remap memory which has not been
> > > allocated with one of the two functions above."
> >
> > No, it doesn't look like those helper functions are designed to handle this.
> >
> > I'm really not the person to be asking about this.  I can poke around in
> > arch/sparc64/kernel/sys_sparc.c:arch_get_unmapped_area() as well as the
> > next guy, and it seems to be doing the right thing for MAP_FIXED, but
> > how/whether it handles !MAP_FIXED I do not know.  Ask davem ;)
>
> Unfortunately that code never gets called for MAP_FIXED :-)
>
> I'll comment on these issues and explain what needs to occur,
> we have several things that want to do this kind of user/kernel
> sharing of ring buffers and similar, so best to get the
> infrastructure going to get it right.
>
> As a first approximation, getting remap_vmalloc_range() to do the
> right thing is the best way to start this stuff off.
>

Ok, thanks you! If I can help with anything, please tell me.

In the meantime I will play with vmalloc_user() and friends.