* [RFC PATCH] mtd: allow mmap for ram devices in memory address space. @ 2014-03-06 7:32 Philippe De Muyter 2014-03-06 21:39 ` Brian Norris 0 siblings, 1 reply; 5+ messages in thread From: Philippe De Muyter @ 2014-03-06 7:32 UTC (permalink / raw) To: linux-mtd; +Cc: David Howells, Markus Niebel Implement mmap for mtd-ram, by using mtd->_get_unmapped_area, as asked by an ancient comment, to get the physical address (if any) in memory of the mtd device. Therefore, mapram_unmapped_area had to be changed to return the physical address, not the virtual one, For the NOMMU case, that makes no difference, but for the MMU case, this is needed by mapram_unmapped_area to implement mmap. Signed-off-by: Philippe De Muyter <phdm@macqel.be> Cc: David Howells <dhowells@redhat.com> Cc: Markus Niebel <list-09@tqsc.de> --- drivers/mtd/chips/map_ram.c | 2 +- drivers/mtd/mtdchar.c | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/chips/map_ram.c b/drivers/mtd/chips/map_ram.c index 991c2a1..e93d147 100644 --- a/drivers/mtd/chips/map_ram.c +++ b/drivers/mtd/chips/map_ram.c @@ -92,7 +92,7 @@ static unsigned long mapram_unmapped_area(struct mtd_info *mtd, unsigned long flags) { struct map_info *map = mtd->priv; - return (unsigned long) map->virt + offset; + return (unsigned long) map->phys + offset; } static int mapram_read (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 2147e73..00eac7a 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -1112,13 +1112,23 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma) #ifdef CONFIG_MMU struct mtd_file_info *mfi = file->private_data; struct mtd_info *mtd = mfi->mtd; - struct map_info *map = mtd->priv; + loff_t offset = vma->vm_pgoff << PAGE_SHIFT; + loff_t len = vma->vm_end - vma->vm_start; + if (offset > mtd->size - len) + return (unsigned long) -EINVAL; + if (mtd->_get_unmapped_area) { + phys_addr_t start; + + start = mtd->_get_unmapped_area(mtd, len, offset, vma->vm_flags); + return vm_iomap_memory(vma, start, len); + } /* This is broken because it assumes the MTD device is map-based and that mtd->priv is a valid struct map_info. It should be replaced with something that uses the mtd_get_unmapped_area() operation properly. */ if (0 /*mtd->type == MTD_RAM || mtd->type == MTD_ROM*/) { + struct map_info *map = mtd->priv; #ifdef pgprot_noncached if (file->f_flags & O_DSYNC || map->phys >= __pa(high_memory)) vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); -- 1.7.5.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] mtd: allow mmap for ram devices in memory address space. 2014-03-06 7:32 [RFC PATCH] mtd: allow mmap for ram devices in memory address space Philippe De Muyter @ 2014-03-06 21:39 ` Brian Norris 2014-03-07 10:44 ` [PATCH v2] mtd: mtdchar: " Philippe De Muyter 0 siblings, 1 reply; 5+ messages in thread From: Brian Norris @ 2014-03-06 21:39 UTC (permalink / raw) To: Philippe De Muyter; +Cc: David Howells, Markus Niebel, linux-mtd On Thu, Mar 06, 2014 at 08:32:26AM +0100, Philippe De Muyter wrote: > Implement mmap for mtd-ram, by using mtd->_get_unmapped_area, as asked by > an ancient comment, to get the physical address (if any) in memory of the > mtd device. > > Therefore, mapram_unmapped_area had to be changed to return the physical > address, not the virtual one, For the NOMMU case, that makes no difference, > but for the MMU case, this is needed by mapram_unmapped_area to implement mmap. > > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > Cc: David Howells <dhowells@redhat.com> > Cc: Markus Niebel <list-09@tqsc.de> > --- > drivers/mtd/chips/map_ram.c | 2 +- > drivers/mtd/mtdchar.c | 12 +++++++++++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/chips/map_ram.c b/drivers/mtd/chips/map_ram.c > index 991c2a1..e93d147 100644 > --- a/drivers/mtd/chips/map_ram.c > +++ b/drivers/mtd/chips/map_ram.c > @@ -92,7 +92,7 @@ static unsigned long mapram_unmapped_area(struct mtd_info *mtd, > unsigned long flags) > { > struct map_info *map = mtd->priv; > - return (unsigned long) map->virt + offset; > + return (unsigned long) map->phys + offset; > } > > static int mapram_read (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) > diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c > index 2147e73..00eac7a 100644 > --- a/drivers/mtd/mtdchar.c > +++ b/drivers/mtd/mtdchar.c > @@ -1112,13 +1112,23 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma) > #ifdef CONFIG_MMU > struct mtd_file_info *mfi = file->private_data; > struct mtd_info *mtd = mfi->mtd; > - struct map_info *map = mtd->priv; > + loff_t offset = vma->vm_pgoff << PAGE_SHIFT; > + loff_t len = vma->vm_end - vma->vm_start; > > + if (offset > mtd->size - len) > + return (unsigned long) -EINVAL; What's the cast doing? This function returns 'int'. And anyway, you're duplicating the checks that are already in mtd_get_unmapped_area(). See below. > + if (mtd->_get_unmapped_area) { > + phys_addr_t start; > + > + start = mtd->_get_unmapped_area(mtd, len, offset, vma->vm_flags); You should be using mtd_get_unmapped_area(), as the comment below says. The underscore in front of the function pointer is to prevent people from using them directly. You can check for -EOPNOTSUPP after calling mtd_get_unmapped_area(). > + return vm_iomap_memory(vma, start, len); > + } > /* This is broken because it assumes the MTD device is map-based > and that mtd->priv is a valid struct map_info. It should be > replaced with something that uses the mtd_get_unmapped_area() > operation properly. */ > if (0 /*mtd->type == MTD_RAM || mtd->type == MTD_ROM*/) { > + struct map_info *map = mtd->priv; You're adding dead code. It's behind an 'if (0)'. Could you perhaps take a harder look at this code and fix up or kill any cruft? > #ifdef pgprot_noncached > if (file->f_flags & O_DSYNC || map->phys >= __pa(high_memory)) > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); Brian ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] mtd: mtdchar: allow mmap for ram devices in memory address space. 2014-03-06 21:39 ` Brian Norris @ 2014-03-07 10:44 ` Philippe De Muyter 2014-03-07 13:54 ` [PATCH v3] " Philippe De Muyter 0 siblings, 1 reply; 5+ messages in thread From: Philippe De Muyter @ 2014-03-07 10:44 UTC (permalink / raw) To: linux-mtd; +Cc: David Howells, Markus Niebel, Brian Norris use mtd_get_unmapped_area, as asked by an ancient comment, to get the physical address (if any) in memory of the mtd device. Therefore, mapram_unmapped_area had to be changed to return the physical address, not the virtual one, For the NOMMU case, that makes no difference, but for the MMU case, it is needed by mapram_unmapped_area to implement mmap. Signed-off-by: Philippe De Muyter <phdm@macqel.be> Cc: David Howells <dhowells@redhat.com> Cc: Markus Niebel <Markus.Niebel@tqs.de> Cc: Brian Norris <computersforpeace@gmail.com> --- v2: cleanup following Brian's comments drivers/mtd/chips/map_ram.c | 2 +- drivers/mtd/mtdchar.c | 22 ++++++++-------------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/drivers/mtd/chips/map_ram.c b/drivers/mtd/chips/map_ram.c index 991c2a1..e93d147 100644 --- a/drivers/mtd/chips/map_ram.c +++ b/drivers/mtd/chips/map_ram.c @@ -92,7 +92,7 @@ static unsigned long mapram_unmapped_area(struct mtd_info *mtd, unsigned long flags) { struct map_info *map = mtd->priv; - return (unsigned long) map->virt + offset; + return (unsigned long) map->phys + offset; } static int mapram_read (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 2147e73..a1c7096 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -1112,20 +1112,14 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma) #ifdef CONFIG_MMU struct mtd_file_info *mfi = file->private_data; struct mtd_info *mtd = mfi->mtd; - struct map_info *map = mtd->priv; - - /* This is broken because it assumes the MTD device is map-based - and that mtd->priv is a valid struct map_info. It should be - replaced with something that uses the mtd_get_unmapped_area() - operation properly. */ - if (0 /*mtd->type == MTD_RAM || mtd->type == MTD_ROM*/) { -#ifdef pgprot_noncached - if (file->f_flags & O_DSYNC || map->phys >= __pa(high_memory)) - vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); -#endif - return vm_iomap_memory(vma, map->phys, map->size); - } - return -ENODEV; + loff_t offset = vma->vm_pgoff << PAGE_SHIFT; + loff_t len = vma->vm_end - vma->vm_start; + phys_addr_t start; + + if (offset > mtd->size - len) + return -EINVAL; + start = mtd_get_unmapped_area(mtd, len, offset, vma->vm_flags); + return start == -EOPNOTSUPP ? -ENODEV : vm_iomap_memory(vma, start, len); #else return vma->vm_flags & VM_SHARED ? 0 : -EACCES; #endif -- 1.7.5.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3] mtd: mtdchar: allow mmap for ram devices in memory address space. 2014-03-07 10:44 ` [PATCH v2] mtd: mtdchar: " Philippe De Muyter @ 2014-03-07 13:54 ` Philippe De Muyter 2015-11-21 0:25 ` Brian Norris 0 siblings, 1 reply; 5+ messages in thread From: Philippe De Muyter @ 2014-03-07 13:54 UTC (permalink / raw) To: linux-mtd; +Cc: David Howells, Markus Niebel, Brian Norris use mtd_get_unmapped_area, as asked by an ancient comment, to get the physical address (if any) in memory of the mtd device. Therefore, mapram_unmapped_area had to be changed to return the physical address, not the virtual one, For the NOMMU case, that makes no difference, but for the MMU case, it is needed by mapram_unmapped_area to implement mmap. Signed-off-by: Philippe De Muyter <phdm@macqel.be> Cc: David Howells <dhowells@redhat.com> Cc: Markus Niebel <Markus.Niebel@tqs.de> Cc: Brian Norris <computersforpeace@gmail.com> --- v3: avoid duplicating test done by mtd_get_unmapped_area drivers/mtd/chips/map_ram.c | 2 +- drivers/mtd/mtdchar.c | 24 ++++++++++-------------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/drivers/mtd/chips/map_ram.c b/drivers/mtd/chips/map_ram.c index 991c2a1..e93d147 100644 --- a/drivers/mtd/chips/map_ram.c +++ b/drivers/mtd/chips/map_ram.c @@ -92,7 +92,7 @@ static unsigned long mapram_unmapped_area(struct mtd_info *mtd, unsigned long flags) { struct map_info *map = mtd->priv; - return (unsigned long) map->virt + offset; + return (unsigned long) map->phys + offset; } static int mapram_read (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 2147e73..5956246 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -1112,20 +1112,16 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma) #ifdef CONFIG_MMU struct mtd_file_info *mfi = file->private_data; struct mtd_info *mtd = mfi->mtd; - struct map_info *map = mtd->priv; - - /* This is broken because it assumes the MTD device is map-based - and that mtd->priv is a valid struct map_info. It should be - replaced with something that uses the mtd_get_unmapped_area() - operation properly. */ - if (0 /*mtd->type == MTD_RAM || mtd->type == MTD_ROM*/) { -#ifdef pgprot_noncached - if (file->f_flags & O_DSYNC || map->phys >= __pa(high_memory)) - vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); -#endif - return vm_iomap_memory(vma, map->phys, map->size); - } - return -ENODEV; + loff_t offset = vma->vm_pgoff << PAGE_SHIFT; + loff_t len = vma->vm_end - vma->vm_start; + phys_addr_t start; + + start = mtd_get_unmapped_area(mtd, len, offset, vma->vm_flags); + if (start == -EOPNOTSUPP) + return -ENODEV; + if (start == -EINVAL) + return -EINVAL; + return vm_iomap_memory(vma, start, len); #else return vma->vm_flags & VM_SHARED ? 0 : -EACCES; #endif -- 1.7.5.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] mtd: mtdchar: allow mmap for ram devices in memory address space. 2014-03-07 13:54 ` [PATCH v3] " Philippe De Muyter @ 2015-11-21 0:25 ` Brian Norris 0 siblings, 0 replies; 5+ messages in thread From: Brian Norris @ 2015-11-21 0:25 UTC (permalink / raw) To: Philippe De Muyter; +Cc: linux-mtd, David Howells, Markus Niebel In case this gets resurrected... On Fri, Mar 07, 2014 at 02:54:39PM +0100, Philippe De Muyter wrote: > use mtd_get_unmapped_area, as asked by an ancient comment, to get > the physical address (if any) in memory of the mtd device. > > Therefore, mapram_unmapped_area had to be changed to return the physical > address, not the virtual one, For the NOMMU case, that makes no difference, > but for the MMU case, it is needed by mapram_unmapped_area to implement mmap. > > Signed-off-by: Philippe De Muyter <phdm@macqel.be> > Cc: David Howells <dhowells@redhat.com> > Cc: Markus Niebel <Markus.Niebel@tqs.de> > Cc: Brian Norris <computersforpeace@gmail.com> > --- > v3: avoid duplicating test done by mtd_get_unmapped_area > > drivers/mtd/chips/map_ram.c | 2 +- > drivers/mtd/mtdchar.c | 24 ++++++++++-------------- > 2 files changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/mtd/chips/map_ram.c b/drivers/mtd/chips/map_ram.c > index 991c2a1..e93d147 100644 > --- a/drivers/mtd/chips/map_ram.c > +++ b/drivers/mtd/chips/map_ram.c > @@ -92,7 +92,7 @@ static unsigned long mapram_unmapped_area(struct mtd_info *mtd, > unsigned long flags) > { > struct map_info *map = mtd->priv; > - return (unsigned long) map->virt + offset; > + return (unsigned long) map->phys + offset; > } > > static int mapram_read (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) > diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c > index 2147e73..5956246 100644 > --- a/drivers/mtd/mtdchar.c > +++ b/drivers/mtd/mtdchar.c > @@ -1112,20 +1112,16 @@ static int mtdchar_mmap(struct file *file, struct vm_area_struct *vma) > #ifdef CONFIG_MMU > struct mtd_file_info *mfi = file->private_data; > struct mtd_info *mtd = mfi->mtd; > - struct map_info *map = mtd->priv; > - > - /* This is broken because it assumes the MTD device is map-based > - and that mtd->priv is a valid struct map_info. It should be > - replaced with something that uses the mtd_get_unmapped_area() > - operation properly. */ > - if (0 /*mtd->type == MTD_RAM || mtd->type == MTD_ROM*/) { > -#ifdef pgprot_noncached > - if (file->f_flags & O_DSYNC || map->phys >= __pa(high_memory)) > - vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > -#endif > - return vm_iomap_memory(vma, map->phys, map->size); > - } > - return -ENODEV; > + loff_t offset = vma->vm_pgoff << PAGE_SHIFT; > + loff_t len = vma->vm_end - vma->vm_start; > + phys_addr_t start; > + > + start = mtd_get_unmapped_area(mtd, len, offset, vma->vm_flags); Hmm, you're storing an 'unsigned long' in 'phys_addr_t', then treating it as unsigned for the error checking. Is that guaranteed to work right? Especially if sizeof(phys_addr_t) != sizeof(unsigned long)? e.g., if phys_addr_t is 64 bits (with PAE?), but unsigned long is 32-bits, we might see: start = mtd_get_unmapped_area(); // -EOPNOTSUPP But -EOPNOTSUPP in a 32-bit unsigned long is only: 0xffffffa1 which extends to: 0x00000000ffffffa1 and doesn't match favorably with '-EOPNOTSUPP' when you compare it later. I think it makes more sense for 'start' to either match the return type of mtd_get_unmapped_area() (i.e., unsigned long), or else just be a signed type, so we don't have the unsigned/signed comparison issues below. Regards, Brian > + if (start == -EOPNOTSUPP) > + return -ENODEV; > + if (start == -EINVAL) > + return -EINVAL; > + return vm_iomap_memory(vma, start, len); > #else > return vma->vm_flags & VM_SHARED ? 0 : -EACCES; > #endif > -- > 1.7.5.3 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-11-21 0:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-06 7:32 [RFC PATCH] mtd: allow mmap for ram devices in memory address space Philippe De Muyter 2014-03-06 21:39 ` Brian Norris 2014-03-07 10:44 ` [PATCH v2] mtd: mtdchar: " Philippe De Muyter 2014-03-07 13:54 ` [PATCH v3] " Philippe De Muyter 2015-11-21 0:25 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.