linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stefan Richter <stefanr@s5r6.in-berlin.de>
To: npiggin@suse.de
Cc: akpm@linux-foundation.org, krh@redhat.com,
	linux1394-devel@lists.sourceforge.net,
	linux-kernel@vger.kernel.org
Subject: Re: [patch 06/18] ieee1394: nopage
Date: Wed, 05 Dec 2007 14:09:48 +0100	[thread overview]
Message-ID: <4756A31C.5060405@s5r6.in-berlin.de> (raw)
In-Reply-To: <20071205071627.360419000@nick.local0.net>

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/

  reply	other threads:[~2007-12-05 13:10 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-12-05  7:15 [patch 00/18] remove nopage npiggin
2007-12-05  7:15 ` [patch 01/18] ia64: ia32 nopage npiggin
2007-12-05  7:15 ` [patch 02/18] relay: nopage npiggin
2007-12-05  7:15 ` [patch 03/18] drm: nopage npiggin
2007-12-05  9:05   ` Dave Airlie
2007-12-05  9:17     ` Nick Piggin
2007-12-05  7:15 ` [patch 04/18] uio: nopage npiggin
2007-12-05 10:04   ` Hans-Jürgen Koch
2007-12-05 10:10     ` Nick Piggin
2007-12-05 10:25       ` Hans-Jürgen Koch
2007-12-05 10:37         ` Nick Piggin
2007-12-05  7:15 ` [patch 05/18] kvm: nopage npiggin
2007-12-05 10:40   ` Avi Kivity
2007-12-05  7:15 ` [patch 06/18] ieee1394: nopage npiggin
2007-12-05 13:09   ` Stefan Richter [this message]
2007-12-05 13:15     ` Stefan Richter
2007-12-05 23:52       ` Nick Piggin
2007-12-05 23:51     ` Nick Piggin
2007-12-15 13:04       ` Stefan Richter
2007-12-15 13:01   ` Stefan Richter
2007-12-05  7:15 ` [patch 07/18] v4l: nopage npiggin
2007-12-08  0:31   ` Andrew Morton
2007-12-08  9:15     ` Ingo Molnar
2007-12-08 10:15       ` Andrew Morton
2007-12-09 17:10     ` Randy Dunlap
2007-12-10  5:06     ` [patch] x64/page.h: convert some macros to inlines Randy Dunlap
2007-12-10  8:34       ` Ingo Molnar
2007-12-05  7:15 ` [patch 08/18] fb: defio nopage npiggin
2007-12-05  7:15 ` [patch 09/18] agp: alpha nopage npiggin
2007-12-05  7:15 ` [patch 10/18] sg: nopage npiggin
2008-02-08  3:45   ` Douglas Gilbert
2007-12-05  7:15 ` [patch 11/18] ib: nopage npiggin
2007-12-05  7:15 ` [patch 12/18] usb: mon nopage npiggin
2007-12-05 16:39   ` Pete Zaitcev
2007-12-05 23:54     ` Nick Piggin
2007-12-05  7:16 ` [patch 13/18] alsa: nopage npiggin
2007-12-13 15:35   ` Takashi Iwai
2007-12-05  7:16 ` [patch 14/18] oss: via nopage npiggin
2007-12-05  7:16 ` [patch 15/18] alsa: usx2y nopage npiggin
2007-12-13 15:35   ` Takashi Iwai
2007-12-05  7:16 ` [patch 16/18] mm: special mapping nopage npiggin
2007-12-05  7:16 ` [patch 17/18] mm: remove nopage npiggin
2007-12-05 22:47   ` Andrew Morton
2007-12-05 23:23     ` Nick Piggin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4756A31C.5060405@s5r6.in-berlin.de \
    --to=stefanr@s5r6.in-berlin.de \
    --cc=akpm@linux-foundation.org \
    --cc=krh@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux1394-devel@lists.sourceforge.net \
    --cc=npiggin@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).