Message ID | 20081117072613.GO16240@us.ibm.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Sonny Rao writes: > - if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT, > - size, vma->vm_page_prot)) > + /* check for the case of a small BSR device and map one 4k page for it*/ > + if (dev->bsr_len < PAGE_SIZE && size == PAGE_SIZE) > + ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12, > + vma->vm_page_prot); I think we should be checking that dev->bsr_len == 4096 here. Paul.
On Wed, Nov 19, 2008 at 03:07:04PM +1100, Paul Mackerras wrote: > Sonny Rao writes: > > > - if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT, > > - size, vma->vm_page_prot)) > > + /* check for the case of a small BSR device and map one 4k page for it*/ > > + if (dev->bsr_len < PAGE_SIZE && size == PAGE_SIZE) > > + ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12, > > + vma->vm_page_prot); > > I think we should be checking that dev->bsr_len == 4096 here. > > Paul. Well, dev->bsr_len could be 4096 or 8192
Sonny Rao writes: > On Wed, Nov 19, 2008 at 03:07:04PM +1100, Paul Mackerras wrote: > > I think we should be checking that dev->bsr_len == 4096 here. > > > > Paul. > > Well, dev->bsr_len could be 4096 or 8192 Isn't the dev->bsr_len == 8192 case the one where we'll only map 4096 bytes and therefore not do what the user expected? Sounds to me like we want to return an error for that case. Paul.
On Thu, Nov 20, 2008 at 09:54:21AM +1100, Paul Mackerras wrote: > Sonny Rao writes: > > > On Wed, Nov 19, 2008 at 03:07:04PM +1100, Paul Mackerras wrote: > > > I think we should be checking that dev->bsr_len == 4096 here. > > > > > > Paul. > > > > Well, dev->bsr_len could be 4096 or 8192 > > Isn't the dev->bsr_len == 8192 case the one where we'll only map 4096 > bytes and therefore not do what the user expected? Sounds to me like > we want to return an error for that case. Well, the problem is that we can't tell if the user asked for 4k or 8k (since we only know the size of the VMA). If we fail whenever dev->bsr_len is 8k then the user could never map that device on a 64k page kernel. Is that what we want?
Index: linux/drivers/char/bsr.c =================================================================== --- linux.orig/drivers/char/bsr.c 2008-11-17 00:29:23.000000000 -0600 +++ linux/drivers/char/bsr.c 2008-11-17 00:59:57.000000000 -0600 @@ -27,6 +27,7 @@ #include <linux/cdev.h> #include <linux/list.h> #include <linux/mm.h> +#include <asm/pgtable.h> #include <asm/io.h> /* @@ -115,15 +116,22 @@ { unsigned long size = vma->vm_end - vma->vm_start; struct bsr_dev *dev = filp->private_data; + int ret; - if (size > dev->bsr_len || (size & (PAGE_SIZE-1))) - return -EINVAL; - - vma->vm_flags |= (VM_IO | VM_DONTEXPAND); vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); - if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT, - size, vma->vm_page_prot)) + /* check for the case of a small BSR device and map one 4k page for it*/ + if (dev->bsr_len < PAGE_SIZE && size == PAGE_SIZE) + ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12, + vma->vm_page_prot); + else if (size <= dev->bsr_len) + ret = io_remap_pfn_range(vma, vma->vm_start, + dev->bsr_addr >> PAGE_SHIFT, + size, vma->vm_page_prot); + else + return -EINVAL; + + if (ret) return -EAGAIN; return 0;