| Message ID | 1336632929-26100-1-git-send-email-gerg@snapgear.com |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote: > From: Greg Ungerer <gerg@uclinux.org> > > The uclinux.c mapping driver uses ioremap_nocache() to map its physical > mapping address to a system virtual address. Problem is that the region > it is mapping is not device memory. It is ordinary system RAM. On most > non-MMU systems this doesn't matter, and the mapping is always a 1:1 > translation of the address. > > But if we want to use the uclinux.c mapping driver on real MMU enabled > systems we should be using phys_to_virt() for the translation, since that > is really what we are doing. So change it to do that. > > Signed-off-by: Greg Ungerer <gerg@uclinux.org> How can I compile-test this? Please, suggest a defconfig which compiles. I created one but it does not build: dedekind@blue:~/git/l2-mtd$ make ARCH=m68k CROSS_COMPILE=m68k-linux- CHK include/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh CHK include/generated/compile.h CC arch/m68k/kernel/time.o arch/m68k/kernel/time.c:90:5: error: redefinition of 'arch_gettimeoffset' include/linux/time.h:145:19: note: previous definition of 'arch_gettimeoffset' was here make[1]: *** [arch/m68k/kernel/time.o] Error 1 make: *** [arch/m68k/kernel] Error 2 It is attached.
Hi Artem, On 05/12/2012 02:05 AM, Artem Bityutskiy wrote: > On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote: >> From: Greg Ungerer<gerg@uclinux.org> >> >> The uclinux.c mapping driver uses ioremap_nocache() to map its physical >> mapping address to a system virtual address. Problem is that the region >> it is mapping is not device memory. It is ordinary system RAM. On most >> non-MMU systems this doesn't matter, and the mapping is always a 1:1 >> translation of the address. >> >> But if we want to use the uclinux.c mapping driver on real MMU enabled >> systems we should be using phys_to_virt() for the translation, since that >> is really what we are doing. So change it to do that. >> >> Signed-off-by: Greg Ungerer<gerg@uclinux.org> > > How can I compile-test this? Please, suggest a defconfig which compiles. > I created one but it does not build: > > dedekind@blue:~/git/l2-mtd$ make ARCH=m68k CROSS_COMPILE=m68k-linux- > CHK include/linux/version.h > CHK include/generated/utsrelease.h > CALL scripts/checksyscalls.sh > CHK include/generated/compile.h > CC arch/m68k/kernel/time.o > arch/m68k/kernel/time.c:90:5: error: redefinition of 'arch_gettimeoffset' > include/linux/time.h:145:19: note: previous definition of 'arch_gettimeoffset' was here > make[1]: *** [arch/m68k/kernel/time.o] Error 1 > make: *** [arch/m68k/kernel] Error 2 > > It is attached. Nothing attached? In any case I would suggest: make ARCH=m68k CROSS_COMPILE=m68k-linux- m5208evb_defconfig Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close, FAX: +61 7 3891 3630 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Sat, 2012-05-12 at 22:10 +1000, Greg Ungerer wrote:
> Nothing attached?
Sorry, now it is.
Hi, I have few requests On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote: > @@ -80,7 +80,6 @@ static int __init uclinux_mtd_init(void) > mtd = do_map_probe("map_ram", mapp); > if (!mtd) { > printk("uclinux[mtd]: failed to find a mapping?\n"); KERN_ERR prefixe is missing. Please, fix other printks in this file while on it. > - iounmap(mapp->virt); > return(-ENXIO); > } > > @@ -103,10 +102,8 @@ static void __exit uclinux_mtd_cleanup(void) > map_destroy(uclinux_ram_mtdinfo); > uclinux_ram_mtdinfo = NULL; > } > - if (uclinux_ram_map.virt) { > - iounmap((void *) uclinux_ram_map.virt); > + if (uclinux_ram_map.virt) > uclinux_ram_map.virt = 0; > - } The "if" statements are redundant - could you please kill them? Would you please be kind to address these sparse warnings while you work on this rarely used file: drivers/mtd/maps/uclinux.c:27:17: warning: symbol 'uclinux_ram_map' was not declared. Should it be static? [sparse] drivers/mtd/maps/uclinux.c:49:15: warning: incorrect type in assignment (different address spaces) [sparse] drivers/mtd/maps/uclinux.c:49:15: expected void *<noident> [sparse] drivers/mtd/maps/uclinux.c:49:15: got void [noderef] <asn:2>* [sparse] drivers/mtd/maps/uclinux.c:71:20: warning: incorrect type in assignment (different address spaces) [sparse] drivers/mtd/maps/uclinux.c:71:20: expected void [noderef] <asn:2>*virt [sparse] drivers/mtd/maps/uclinux.c:71:20: got void * [sparse] drivers/mtd/maps/uclinux.c:73:27: warning: Using plain integer as NULL pointer [sparse] drivers/mtd/maps/uclinux.c:106:40: warning: Using plain integer as NULL pointer [sparse] Thanks!
Hi Artem, On 05/14/2012 10:05 PM, Artem Bityutskiy wrote: > I have few requests > > On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote: >> @@ -80,7 +80,6 @@ static int __init uclinux_mtd_init(void) >> mtd = do_map_probe("map_ram", mapp); >> if (!mtd) { >> printk("uclinux[mtd]: failed to find a mapping?\n"); > > KERN_ERR prefixe is missing. Please, fix other printks in this file > while on it. > >> - iounmap(mapp->virt); >> return(-ENXIO); >> } >> >> @@ -103,10 +102,8 @@ static void __exit uclinux_mtd_cleanup(void) >> map_destroy(uclinux_ram_mtdinfo); >> uclinux_ram_mtdinfo = NULL; >> } >> - if (uclinux_ram_map.virt) { >> - iounmap((void *) uclinux_ram_map.virt); >> + if (uclinux_ram_map.virt) >> uclinux_ram_map.virt = 0; >> - } > > The "if" statements are redundant - could you please kill them? > > Would you please be kind to address these sparse warnings while you work > on this rarely used file: > > drivers/mtd/maps/uclinux.c:27:17: warning: symbol 'uclinux_ram_map' was not declared. Should it be static? [sparse] > drivers/mtd/maps/uclinux.c:49:15: warning: incorrect type in assignment (different address spaces) [sparse] > drivers/mtd/maps/uclinux.c:49:15: expected void *<noident> [sparse] > drivers/mtd/maps/uclinux.c:49:15: got void [noderef]<asn:2>* [sparse] > drivers/mtd/maps/uclinux.c:71:20: warning: incorrect type in assignment (different address spaces) [sparse] > drivers/mtd/maps/uclinux.c:71:20: expected void [noderef]<asn:2>*virt [sparse] > drivers/mtd/maps/uclinux.c:71:20: got void * [sparse] > drivers/mtd/maps/uclinux.c:73:27: warning: Using plain integer as NULL pointer [sparse] > drivers/mtd/maps/uclinux.c:106:40: warning: Using plain integer as NULL pointer [sparse] Sure thing, I can clean all those up. Are you happy to take a single cleanup patch on top of the ioremap_nocache fix patch? Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close, FAX: +61 7 3891 3630 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Mon, 2012-05-14 at 22:58 +1000, Greg Ungerer wrote: > > drivers/mtd/maps/uclinux.c:27:17: warning: symbol 'uclinux_ram_map' was not declared. Should it be static? [sparse] > > drivers/mtd/maps/uclinux.c:49:15: warning: incorrect type in assignment (different address spaces) [sparse] > > drivers/mtd/maps/uclinux.c:49:15: expected void *<noident> [sparse] > > drivers/mtd/maps/uclinux.c:49:15: got void [noderef]<asn:2>* [sparse] > > drivers/mtd/maps/uclinux.c:71:20: warning: incorrect type in assignment (different address spaces) [sparse] > > drivers/mtd/maps/uclinux.c:71:20: expected void [noderef]<asn:2>*virt [sparse] > > drivers/mtd/maps/uclinux.c:71:20: got void * [sparse] > > drivers/mtd/maps/uclinux.c:73:27: warning: Using plain integer as NULL pointer [sparse] > > drivers/mtd/maps/uclinux.c:106:40: warning: Using plain integer as NULL pointer [sparse] > > Sure thing, I can clean all those up. Are you happy to take a single > cleanup patch on top of the ioremap_nocache fix patch? Yes, thanks!
On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote: > But if we want to use the uclinux.c mapping driver on real MMU enabled > systems we should be using phys_to_virt() for the translation, since > that is really what we are doing. So change it to do that. That seems wrong. On a highmem page, phys_to_virt() isn't valid. So at the very least, any usage of phys_to_virt() needs a stonking great comment explaining why it's always safe because it can never be used ona a highmem page.
Hi David, On 07/07/12 01:55, David Woodhouse wrote: > On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote: >> But if we want to use the uclinux.c mapping driver on real MMU enabled >> systems we should be using phys_to_virt() for the translation, since >> that is really what we are doing. So change it to do that. > > That seems wrong. On a highmem page, phys_to_virt() isn't valid. So at > the very least, any usage of phys_to_virt() needs a stonking great > comment explaining why it's always safe because it can never be used ona > a highmem page. The only VM based arch this driver can be configured for currently is m68k (actually it is even more specific, only CONFIG_COLDFIRE). And that doesn't support HIGHMEM. Can the kernels data region (and the area immediately after it) be in high memory? Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Mon, 2012-07-09 at 16:08 +1000, Greg Ungerer wrote: > > The only VM based arch this driver can be configured for currently > is m68k (actually it is even more specific, only CONFIG_COLDFIRE). > And that doesn't support HIGHMEM. OK, can we at least have a comment to that effect in the code, alongside that phys_to_virt() call so that nobody is tempted to copy it into new code? And preferably also an '#ifdef CONFIG_HIGHMEM / #error' in the code or a (redundant) !HIGHMEM dependency in Kconfig just to make sure, in case it's ever enabled on other architectures or in case HIGHMEM ever comes to m68k. > Can the kernels data region (and the area immediately after it) be in > high memory? No, that'll be in the directly mapped region.
On 12/07/12 17:49, David Woodhouse wrote: > On Mon, 2012-07-09 at 16:08 +1000, Greg Ungerer wrote: >> >> The only VM based arch this driver can be configured for currently >> is m68k (actually it is even more specific, only CONFIG_COLDFIRE). >> And that doesn't support HIGHMEM. > > OK, can we at least have a comment to that effect in the code, alongside > that phys_to_virt() call so that nobody is tempted to copy it into new > code? And preferably also an '#ifdef CONFIG_HIGHMEM / #error' in the > code or a (redundant) !HIGHMEM dependency in Kconfig just to make sure, > in case it's ever enabled on other architectures or in case HIGHMEM ever > comes to m68k. I'll come up with something and send a patch. >> Can the kernels data region (and the area immediately after it) be in >> high memory? > > No, that'll be in the directly mapped region. Then does it matter whether HIGHMEM is enabled or not? The phys address here is _ebss, the end of the kernels bss section. Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote:
> From: Greg Ungerer <gerg@uclinux.org>
Hi Greg, I had these 2 patches in my l2-mtd.git tree, but dwmw2 decided
to not pick them, so I dropped them as well. Please, re-send new
versions, thanks!
Hi Artem, On 18/07/12 22:12, Artem Bityutskiy wrote: > On Thu, 2012-05-10 at 16:55 +1000, gerg@snapgear.com wrote: >> From: Greg Ungerer <gerg@uclinux.org> > > Hi Greg, I had these 2 patches in my l2-mtd.git tree, but dwmw2 decided > to not pick them, so I dropped them as well. Please, re-send new > versions, thanks! Ok, done. Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c index cfff454..6d43c75 100644 --- a/drivers/mtd/maps/uclinux.c +++ b/drivers/mtd/maps/uclinux.c @@ -68,10 +68,10 @@ static int __init uclinux_mtd_init(void) printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n", (int) mapp->phys, (int) mapp->size); - mapp->virt = ioremap_nocache(mapp->phys, mapp->size); + mapp->virt = phys_to_virt(mapp->phys); if (mapp->virt == 0) { - printk("uclinux[mtd]: ioremap_nocache() failed\n"); + printk("uclinux[mtd]: no virtual mapping?\n"); return(-EIO); } @@ -80,7 +80,6 @@ static int __init uclinux_mtd_init(void) mtd = do_map_probe("map_ram", mapp); if (!mtd) { printk("uclinux[mtd]: failed to find a mapping?\n"); - iounmap(mapp->virt); return(-ENXIO); } @@ -103,10 +102,8 @@ static void __exit uclinux_mtd_cleanup(void) map_destroy(uclinux_ram_mtdinfo); uclinux_ram_mtdinfo = NULL; } - if (uclinux_ram_map.virt) { - iounmap((void *) uclinux_ram_map.virt); + if (uclinux_ram_map.virt) uclinux_ram_map.virt = 0; - } } /****************************************************************************/