Patchwork [1/2] mtd: fix wrong usage of ioremap_nocache() in uclinux.c map driver

login
register
mail settings
Submitter Greg Ungerer
Date May 10, 2012, 6:55 a.m.
Message ID <1336632929-26100-1-git-send-email-gerg@snapgear.com>
Download mbox | patch
Permalink /patch/158325/
State New
Headers show

Comments

Greg Ungerer - May 10, 2012, 6:55 a.m.
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>
---
 drivers/mtd/maps/uclinux.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
Artem Bityutskiy - May 11, 2012, 4:05 p.m.
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.
Greg Ungerer - May 12, 2012, 12:10 p.m.
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
Artem Bityutskiy - May 14, 2012, 9:44 a.m.
On Sat, 2012-05-12 at 22:10 +1000, Greg Ungerer wrote:
> Nothing attached?

Sorry, now it is.
Artem Bityutskiy - May 14, 2012, 12:05 p.m.
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!
Greg Ungerer - May 14, 2012, 12:58 p.m.
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
Artem Bityutskiy - May 14, 2012, 1:03 p.m.
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!
David Woodhouse - July 6, 2012, 3:55 p.m.
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.
Greg Ungerer - July 9, 2012, 6:08 a.m.
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
David Woodhouse - July 12, 2012, 7:49 a.m.
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.
Greg Ungerer - July 13, 2012, 6:25 a.m.
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
Artem Bityutskiy - July 18, 2012, 12:12 p.m.
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!
Greg Ungerer - July 19, 2012, 5:44 a.m.
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

Patch

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;
-	}
 }
 
 /****************************************************************************/