[PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[Nick Holloway]

Use vm_insert_page() instead of remap_pfn_range(), and remove
the PageReserved fiddling.

Signed-off-by: Nick Holloway <Nick.Holloway@pyrites.org.uk>

---

Although the cpia driver functioned correctly after printing out the
"incomplete pfn remapping" message, I thought I would have a go at the
trivial conversion'' as I have access to the hardware.

Driver has been tested with a parport CPIA camera (using "motion").

 cpia.c |   22 ++++------------------
 1 files changed, 4 insertions(+), 18 deletions(-)

--- linux-2.6.15-rc4/drivers/media/video/cpia.c~	2005-12-03 10:04:33.000000000 +0000
+++ linux-2.6.15-rc4/drivers/media/video/cpia.c	2005-12-05 11:20:57.000000000 +0000
@@ -219,7 +219,6 @@ static void set_flicker(struct cam_param
 static void *rvmalloc(unsigned long size)
 {
 	void *mem;
-	unsigned long adr;
 
 	size = PAGE_ALIGN(size);
 	mem = vmalloc_32(size);
@@ -227,29 +226,15 @@ static void *rvmalloc(unsigned long size
 		return NULL;
 
 	memset(mem, 0, size); /* Clear the ram out, no junk to the user */
-	adr = (unsigned long) mem;
-	while (size > 0) {
-		SetPageReserved(vmalloc_to_page((void *)adr));
-		adr += PAGE_SIZE;
-		size -= PAGE_SIZE;
-	}
 
 	return mem;
 }
 
 static void rvfree(void *mem, unsigned long size)
 {
-	unsigned long adr;
-
 	if (!mem)
 		return;
 
-	adr = (unsigned long) mem;
-	while ((long) size > 0) {
-		ClearPageReserved(vmalloc_to_page((void *)adr));
-		adr += PAGE_SIZE;
-		size -= PAGE_SIZE;
-	}
 	vfree(mem);
 }
 
@@ -3753,7 +3738,8 @@ static int cpia_mmap(struct file *file, 
 	struct video_device *dev = file->private_data;
 	unsigned long start = vma->vm_start;
 	unsigned long size  = vma->vm_end - vma->vm_start;
-	unsigned long page, pos;
+	unsigned long pos;
+	struct page* page;
 	struct cam_data *cam = dev->priv;
 	int retval;
 
@@ -3781,8 +3767,8 @@ static int cpia_mmap(struct file *file, 
 
 	pos = (unsigned long)(cam->frame_buf);
 	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
+		page = vmalloc_to_page((void *)pos);
+		if (vm_insert_page(vma, start, page)) {
 			up(&cam->busy_lock);
 			return -EAGAIN;
 		}

-- 
 `O O'  | Nick.Holloway@pyrites.org.uk
// ^ \\ | http://www.pyrites.org.uk/

Re: [PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[Hugh Dickins]

On Mon, 5 Dec 2005, Nick Holloway wrote:

> Use vm_insert_page() instead of remap_pfn_range(), and remove
> the PageReserved fiddling.
> 
> Signed-off-by: Nick Holloway <Nick.Holloway@pyrites.org.uk>
> 
> ---
> 
> Although the cpia driver functioned correctly after printing out the
> "incomplete pfn remapping" message, I thought I would have a go at the
> trivial conversion'' as I have access to the hardware.
> 
> Driver has been tested with a parport CPIA camera (using "motion").

That patch looks good, thanks for making and testing it.

A couple of minor points which you may reasonably feel go beyond
what you were attempting:

rvfree becomes totally pointless (since vfree checks for NULL itself),
so it would be better to delete rvfree and replace the rvfree calls
by vfree calls (removing the size argument).

pos would be better off as a u8* matching frame_buf, than an unsigned
long that has to be cast this way and that.

And a third point which makes me hesitate.  It used to set PAGE_SHARED
(read-write access) on all the page table entries, whether the mmap
was MAP_PRIVATE or MAP_SHARED, PROT_WRITE or not.  That was wrong, and
Linus intentionally does not permit that mistake with vm_insert_page.

And the change _probably_ causes no trouble for anyone; but it _might_
cause trouble somewhere: it could be that userspace needs correcting
(to ask for shared write access where it wasn't asking before).
I've no idea whether write access makes sense with this driver.

So personally I'm rather reluctant to recommend a change of this kind
late in a release cycle (and I'd prefer that you didn't have to endure
the noisy warnings at this stage too, but Linus put them in,
so I think he wants them to stay).

Mauro, is this drivers/media/video/cpia.c under your maintainership?
If the maintainer wants such a patch to go in now, then I'd agree
with him; but I wouldn't be pushing it myself.

Later on, perhaps 2.6.16-rc-mm and early 2.6.17, I'd like to go over
lots of SetPageReserved drivers, to remove that and convert them over.
I'm sure various other little fixups will suggest themselves in that
exercise (things like adding VM_DONTEXPAND, removing VM_RESERVED and
VM_SHM, adding helpers for vmalloc and high-order loops).

Hugh

>  cpia.c |   22 ++++------------------
>  1 files changed, 4 insertions(+), 18 deletions(-)
> 
> --- linux-2.6.15-rc4/drivers/media/video/cpia.c~	2005-12-03 10:04:33.000000000 +0000
> +++ linux-2.6.15-rc4/drivers/media/video/cpia.c	2005-12-05 11:20:57.000000000 +0000
> @@ -219,7 +219,6 @@ static void set_flicker(struct cam_param
>  static void *rvmalloc(unsigned long size)
>  {
>  	void *mem;
> -	unsigned long adr;
>  
>  	size = PAGE_ALIGN(size);
>  	mem = vmalloc_32(size);
> @@ -227,29 +226,15 @@ static void *rvmalloc(unsigned long size
>  		return NULL;
>  
>  	memset(mem, 0, size); /* Clear the ram out, no junk to the user */
> -	adr = (unsigned long) mem;
> -	while (size > 0) {
> -		SetPageReserved(vmalloc_to_page((void *)adr));
> -		adr += PAGE_SIZE;
> -		size -= PAGE_SIZE;
> -	}
>  
>  	return mem;
>  }
>  
>  static void rvfree(void *mem, unsigned long size)
>  {
> -	unsigned long adr;
> -
>  	if (!mem)
>  		return;
>  
> -	adr = (unsigned long) mem;
> -	while ((long) size > 0) {
> -		ClearPageReserved(vmalloc_to_page((void *)adr));
> -		adr += PAGE_SIZE;
> -		size -= PAGE_SIZE;
> -	}
>  	vfree(mem);
>  }
>  
> @@ -3753,7 +3738,8 @@ static int cpia_mmap(struct file *file, 
>  	struct video_device *dev = file->private_data;
>  	unsigned long start = vma->vm_start;
>  	unsigned long size  = vma->vm_end - vma->vm_start;
> -	unsigned long page, pos;
> +	unsigned long pos;
> +	struct page* page;
>  	struct cam_data *cam = dev->priv;
>  	int retval;
>  
> @@ -3781,8 +3767,8 @@ static int cpia_mmap(struct file *file, 
>  
>  	pos = (unsigned long)(cam->frame_buf);
>  	while (size > 0) {
> -		page = vmalloc_to_pfn((void *)pos);
> -		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
> +		page = vmalloc_to_page((void *)pos);
> +		if (vm_insert_page(vma, start, page)) {
>  			up(&cam->busy_lock);
>  			return -EAGAIN;
>  		}
> 
> -- 
>  `O O'  | Nick.Holloway@pyrites.org.uk
> // ^ \\ | http://www.pyrites.org.uk/
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[Christoph Hellwig]

>  	pos = (unsigned long)(cam->frame_buf);
>  	while (size > 0) {
> -		page = vmalloc_to_pfn((void *)pos);
> -		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
> +		page = vmalloc_to_page((void *)pos);
> +		if (vm_insert_page(vma, start, page)) {

it would be nicer to do the arithmetis on pos as pointers rather than unsigned
long.  Also you might want to use alloc_pages + vmap instead of vmalloc so that
you already have a page array.  Or we should provide a helper that walks over
a vmalloc'ed region and calls vmalloc_to_page + vm_insert_page.  Either way
this type of code is duplicated far too much and we'd really need some better
interface for it.

Re: [PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[Nick Holloway]

On Tue, Dec 06, 2005 at 06:31:26PM +0000, Hugh Dickins wrote:
> On Mon, 5 Dec 2005, Nick Holloway wrote:
> > Although the cpia driver functioned correctly after printing out the
> > "incomplete pfn remapping" message, I thought I would have a go at the
> > trivial conversion'' as I have access to the hardware.
> 
> A couple of minor points which you may reasonably feel go beyond
> what you were attempting:
> 
> rvfree becomes totally pointless (since vfree checks for NULL itself),
> so it would be better to delete rvfree and replace the rvfree calls
> by vfree calls (removing the size argument).

I could see that would be the next step in the cleanup, but I wanted to
perform the minumum changes, so it was clear what I had done.

> pos would be better off as a u8* matching frame_buf, than an unsigned
> long that has to be cast this way and that.

I agree.  I couldn't see any logical reason for dealing with it as
"unsigned long", and wondered about switching to "void*".  On the other
hand, the machine this was being tested on was remote, and the scope
for a bad change that locked up would have halted development.

> And a third point which makes me hesitate.  It used to set PAGE_SHARED
> (read-write access) on all the page table entries, whether the mmap
> was MAP_PRIVATE or MAP_SHARED, PROT_WRITE or not.  That was wrong, and
> Linus intentionally does not permit that mistake with vm_insert_page.
> 
> And the change _probably_ causes no trouble for anyone; but it _might_
> cause trouble somewhere: it could be that userspace needs correcting
> (to ask for shared write access where it wasn't asking before).
> I've no idea whether write access makes sense with this driver.

I did wonder about that too.  The application I tested with does:

    mmap(..., PROT_READ|PROT_WRITE, MAP_SHARED, ... );

This seems to be a common incantation for video4linux clients.  It would
also seem to be the wrong thing for the mmap to not be MAP_SHARED.

> So personally I'm rather reluctant to recommend a change of this kind
> late in a release cycle (and I'd prefer that you didn't have to endure
> the noisy warnings at this stage too, but Linus put them in,
> so I think he wants them to stay).

I'm not concerned with the warnings -- I just fancied a quick kernel hack,
and had the hardware to test.

-- 
 `O O'  | Nick.Holloway@pyrites.org.uk
// ^ \\ | http://www.pyrites.org.uk/

Re: [PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[Nick Holloway]

On Tue, Dec 06, 2005 at 07:10:12PM +0000, Christoph Hellwig wrote:
> >     pos = (unsigned long)(cam->frame_buf);
> >     while (size > 0) {
> > -           page = vmalloc_to_pfn((void *)pos);
> > -           if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
> > +           page = vmalloc_to_page((void *)pos);
> > +           if (vm_insert_page(vma, start, page)) {
>
> it would be nicer to do the arithmetis on pos as pointers rather than unsigned
> long.  Also you might want to use alloc_pages + vmap instead of vmalloc so that
> you already have a page array.  Or we should provide a helper that walks over
> a vmalloc'ed region and calls vmalloc_to_page + vm_insert_page.  Either way
> this type of code is duplicated far too much and we'd really need some better
> interface for it.

As I said in my previous mail, the patch was just switching to use
vm_insert_page, and not any other cleanups.

I agree that a helper is a good idea, as the vmalloc, SetPageReserved,
remap_pfn_range (was remap_page_range in 2.4) pattern has been copied
and pasted across many video4linux drivers.

The cpia driver could do with other cleanups.

        - It doesn't have a sysfs release callback (so says warning printk).
	- The colourspace conversion has been disabled, but should be
	  ripped out.
	- Needs to support V4L2 API

-- 
 `O O'  | Nick.Holloway@pyrites.org.uk
// ^ \\ | http://www.pyrites.org.uk/

Re: [PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[Nick Piggin]

Nick Holloway wrote:
> On Tue, Dec 06, 2005 at 07:10:12PM +0000, Christoph Hellwig wrote:
> 
>>>    pos = (unsigned long)(cam->frame_buf);
>>>    while (size > 0) {
>>>-           page = vmalloc_to_pfn((void *)pos);
>>>-           if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED)) {
>>>+           page = vmalloc_to_page((void *)pos);
>>>+           if (vm_insert_page(vma, start, page)) {
>>
>>it would be nicer to do the arithmetis on pos as pointers rather than unsigned
>>long.  Also you might want to use alloc_pages + vmap instead of vmalloc so that
>>you already have a page array.  Or we should provide a helper that walks over
>>a vmalloc'ed region and calls vmalloc_to_page + vm_insert_page.  Either way
>>this type of code is duplicated far too much and we'd really need some better
>>interface for it.
> 
> 
> As I said in my previous mail, the patch was just switching to use
> vm_insert_page, and not any other cleanups.
> 
> I agree that a helper is a good idea, as the vmalloc, SetPageReserved,
> remap_pfn_range (was remap_page_range in 2.4) pattern has been copied
> and pasted across many video4linux drivers.
> 
> The cpia driver could do with other cleanups.
> 
>         - It doesn't have a sysfs release callback (so says warning printk).
> 	- The colourspace conversion has been disabled, but should be
> 	  ripped out.
> 	- Needs to support V4L2 API
> 

- remove the last traces of rvmalloc (which is an oft repeated code
   sequence in drivers, means something like vmalloc + SetPageReserved)

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH 2.6.15-rc4 1/1] cpia: use vm_insert_page() instead of remap_pfn_range()

[Mauro Carvalho Chehab]

Em Ter, 2005-12-06 às 18:31 +0000, Hugh Dickins escreveu:
> On Mon, 5 Dec 2005, Nick Holloway wrote:
> 

> Mauro, is this drivers/media/video/cpia.c under your maintainership?
> If the maintainer wants such a patch to go in now, then I'd agree
> with him; but I wouldn't be pushing it myself.
	Good question... yes and no... v4l subsystem is under my concern, but I
never touched this driver. Also, it doesn't use videodev.c. Maybe we
should address this question to v4l mailing list (I'm c/c).


	Your comments seems to be pertinent, IMHO. Btw, we have also a similar
patch for em28xx driver at our quilt tree:
	http://linuxtv.org/downloads/quilt

	under:

patches/v4l_dvb_3113_convert_em28xx_to_use_vm_insert_page_instead_of_remap_pfn_range.patch

	Would you please review it also? I've tested it and it seems to work
properly.

Cheers, 
Mauro.