Re: [patch 06/18] ieee1394: nopage

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

npiggin@suse.de wrote:
> Convert ieee1394 from nopage to fault.
> Remove redundant vma range checks (correct resource range check is retained).

I never really looked into that part of the 1394 drivers and I'm too
lazy to figure this out myself, so I ask:  What would trip the .fault
handler?  Would be good if I could runtime-test it.

> Signed-off-by: Nick Piggin <npiggin@suse.de>
> Cc: krh@redhat.com

It's an obscure and unimportant detail but I mention it nevertheless:
It's not necessary to Cc krh on drivers/ieee1394 stuff.  He is more into
drivers/firewire. :-)

> Cc: stefanr@s5r6.in-berlin.de
> Cc: linux1394-devel@lists.sourceforge.net
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/ieee1394/dma.c |   39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> Index: linux-2.6/drivers/ieee1394/dma.c
> ===================================================================
> --- linux-2.6.orig/drivers/ieee1394/dma.c
> +++ linux-2.6/drivers/ieee1394/dma.c
> @@ -231,37 +231,32 @@ void dma_region_sync_for_device(struct d
>  
>  #ifdef CONFIG_MMU
>  
> -/* nopage() handler for mmap access */
> +/* fault() handler for mmap access */
>  
> -static struct page *dma_region_pagefault(struct vm_area_struct *area,
> -					 unsigned long address, int *type)
> +static int dma_region_pagefault(struct vm_area_struct *vma,
> +					struct vm_fault *vmf)
>  {
> -	unsigned long offset;
>  	unsigned long kernel_virt_addr;
> -	struct page *ret = NOPAGE_SIGBUS;
>  
> -	struct dma_region *dma = (struct dma_region *)area->vm_private_data;
> +	struct dma_region *dma = (struct dma_region *)vma->vm_private_data;
>  
>  	if (!dma->kvirt)
> -		goto out;
> +		goto error;
>  
> -	if ((address < (unsigned long)area->vm_start) ||
> -	    (address >
> -	     (unsigned long)area->vm_start + (dma->n_pages << PAGE_SHIFT)))
> -		goto out;
> -
> -	if (type)
> -		*type = VM_FAULT_MINOR;
> -	offset = address - area->vm_start;
> -	kernel_virt_addr = (unsigned long)dma->kvirt + offset;
> -	ret = vmalloc_to_page((void *)kernel_virt_addr);
> -	get_page(ret);
> -      out:
> -	return ret;
> +	if (vmf->pgoff >= dma->n_pages)
> +		goto error;
> +
> +	kernel_virt_addr = (unsigned long)dma->kvirt + (vmf->pgoff << PAGE_SHIFT);
> +	vmf->page = vmalloc_to_page((void *)kernel_virt_addr);
> +	get_page(vmf->page);
> +	return 0;
> +
> +      error:
> +	return VM_FAULT_SIGBUS;
>  }

Why not replacing the two 'goto error' by 'return VM_FAULT_SIGBUS'?  If
there is no cleanup after that error jump, I find it sensible to return
early.

>  
>  static struct vm_operations_struct dma_region_vm_ops = {
> -	.nopage = dma_region_pagefault,
> +	.fault = dma_region_pagefault,
>  };
>  
>  /**
> @@ -275,7 +270,7 @@ int dma_region_mmap(struct dma_region *d
>  	if (!dma->kvirt)
>  		return -EINVAL;
>  
> -	/* must be page-aligned */
> +	/* must be page-aligned (XXX: comment is wrong, we could allow pgoff) */
>  	if (vma->vm_pgoff != 0)
>  		return -EINVAL;
>  

Are you sure that the comment is wrong?  Could it be that there are
assumptions elsewhere which require page alignment?  (I should be able
to answer that, but I'm not.)
-- 
Stefan Richter
-=====-=-=== ==-- --=-=
http://arcgraph.de/sr/