Re: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29

Re: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29

[Jörn Engel]

On Thu, 30 June 2005 23:41:40 +0100, tpoynor@infradead.org wrote:
> 
> --- NEW FILE mainstone-flash.c ---

Since this one is new, would you mind some comments?

> /*
>  * $Id:  $
>  *
>  * Map driver for the Mainstone developer platform.
>  *
>  * Author:	Nicolas Pitre
>  * Copyright:	(C) 2001 MontaVista Software Inc.
>  * 
>  * This program is free software; you can redistribute it and/or modify
>  * it under the terms of the GNU General Public License version 2 as
>  * published by the Free Software Foundation.
>  */
> 
> #include <linux/module.h>
> #include <linux/types.h>
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/dma-mapping.h>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/map.h>
> #include <linux/mtd/partitions.h>
> #include <asm/io.h>
> #include <asm/hardware.h>
> #include <asm/arch/pxa-regs.h>
> #include <asm/arch/mainstone.h>
> 
> 
> #define ROM_ADDR	0x00000000
> #define FLASH_ADDR	0x04000000
> 
> #define WINDOW_SIZE 	0x04000000
> 
> static void mainstone_map_inval_cache(struct map_info *map, unsigned long from, ssize_t len)

80 columns?

> {
> 	consistent_sync((char *)map->cached + from, len, DMA_FROM_DEVICE);
> }
> 
> static struct map_info mainstone_maps[2] = { {
> 	.size =		WINDOW_SIZE,
> 	.phys =		PXA_CS0_PHYS,
> 	.inval_cache = 	mainstone_map_inval_cache,
> }, {
> 	.size =		WINDOW_SIZE,
> 	.phys =		PXA_CS1_PHYS,
> 	.inval_cache = 	mainstone_map_inval_cache,
> } };
> 
> static struct mtd_partition mainstone_partitions[] = {
> 	{
> 		.name =		"Bootloader",
> 		.size =		0x00040000,
> 		.offset =	0,
> 		.mask_flags =	MTD_WRITEABLE  /* force read-only */
> 	},{
> 		.name =		"Kernel",
> 		.size =		0x00400000,
> 		.offset =	0x00040000,
> 	},{
> 		.name =		"Filesystem",
> 		.size =		MTDPART_SIZ_FULL,
> 		.offset =	0x00440000
> 	}
> };
> 
> static struct mtd_info *mymtds[2];
> static struct mtd_partition *parsed_parts[2];
> static int nr_parsed_parts[2];
> 
> static const char *probes[] = { "RedBoot", "cmdlinepart", NULL };
> 
> static int __init init_mainstone(void)
> {
> 	int SW7 = 0;  /* FIXME: get from SCR (Mst doc section 3.2.1.1) */
> 	int ret = 0, i;
> 
> 	mainstone_maps[0].bankwidth = (BOOT_DEF & 1) ? 2 : 4;
> 	mainstone_maps[1].bankwidth = 4;
> 
> 	/* Compensate for SW7 which swaps the flash banks */
> 	mainstone_maps[SW7].name = "processor flash";
> 	mainstone_maps[SW7 ^ 1].name = "main board flash";
> 
> 	printk(KERN_NOTICE "Mainstone configured to boot from %s\n",
> 	       mainstone_maps[0].name);
> 
> 	for (i = 0; i < 2; i++) {

Bunch of line below exceed 80 columns again...

> 		mainstone_maps[i].virt = ioremap(mainstone_maps[i].phys, WINDOW_SIZE);
> 		if (!mainstone_maps[i].virt) {
> 			printk(KERN_WARNING "Failed to ioremap %s\n", mainstone_maps[i].name);
> 			if (!ret)
> 				ret = -ENOMEM;
> 			continue;
> 		}
> 		mainstone_maps[i].cached = ioremap_cached(mainstone_maps[i].phys, WINDOW_SIZE);
> 		if (!mainstone_maps[i].cached)
> 			printk(KERN_WARNING "Failed to ioremap cached %s\n", mainstone_maps[i].name);

Shouldn't we error out instead of printing a message and continuing
on?

> 		simple_map_init(&mainstone_maps[i]);
> 
> 		printk(KERN_NOTICE "Probing %s at physical address 0x%08lx (%d-bit bankwidth)\n",
> 		       mainstone_maps[i].name, mainstone_maps[i].phys, 
> 		       mainstone_maps[i].bankwidth * 8);
> 
> 		mymtds[i] = do_map_probe("cfi_probe", &mainstone_maps[i]);
> 		
> 		if (!mymtds[i]) {
> 			iounmap((void *)mainstone_maps[i].virt);
> 			if (mainstone_maps[i].cached)
> 				iounmap(mainstone_maps[i].cached);

This is broken.  After above code, mainstone_maps[i].virt can be NULL.
So either you need a NULL check for both or for none.

> 			if (!ret)
> 				ret = -EIO;
> 			continue;
> 		}
> 		mymtds[i]->owner = THIS_MODULE;
> 
> 		ret = parse_mtd_partitions(mymtds[i], probes,
> 					   &parsed_parts[i], 0);
> 
> 		if (ret > 0)
> 			nr_parsed_parts[i] = ret;
> 	}
> 
> 	if (!mymtds[0] && !mymtds[1])
> 		return ret;
> 	
> 	for (i = 0; i < 2; i++) {

80 columns again...

> 		if (!mymtds[i]) {
> 			printk(KERN_WARNING "%s is absent. Skipping\n", mainstone_maps[i].name);
> 		} else if (nr_parsed_parts[i]) {
> 			add_mtd_partitions(mymtds[i], parsed_parts[i], nr_parsed_parts[i]);
> 		} else if (!i) {
> 			printk("Using static partitions on %s\n", mainstone_maps[i].name);
> 			add_mtd_partitions(mymtds[i], mainstone_partitions, ARRAY_SIZE(mainstone_partitions));
> 		} else {
> 			printk("Registering %s as whole device\n", mainstone_maps[i].name);
> 			add_mtd_device(mymtds[i]);
> 		}
> 	}
> 	return 0;
> }
> 
> static void __exit cleanup_mainstone(void)
> {
> 	int i;
> 	for (i = 0; i < 2; i++) {
> 		if (!mymtds[i])
> 			continue;
> 
> 		if (nr_parsed_parts[i] || !i)
> 			del_mtd_partitions(mymtds[i]);
> 		else
> 			del_mtd_device(mymtds[i]);			

Trailing whitespacs.

> 
> 		map_destroy(mymtds[i]);
> 		iounmap((void *)mainstone_maps[i].virt);
> 		if (mainstone_maps[i].cached)
> 			iounmap(mainstone_maps[i].cached);
> 
> 		if (parsed_parts[i])
> 			kfree(parsed_parts[i]);

kfree() can be called unconditionally.

> 	}
> }
> 
> module_init(init_mainstone);
> module_exit(cleanup_mainstone);
> 
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Nicolas Pitre <nico@cam.org>");
> MODULE_DESCRIPTION("MTD map driver for Intel Mainstone");
> 
> Index: Kconfig
> ===================================================================
> RCS file: /home/cvs/mtd/drivers/mtd/maps/Kconfig,v
> retrieving revision 1.53
> retrieving revision 1.54
> diff -u -r1.53 -r1.54
> --- Kconfig	29 Jun 2005 09:29:43 -0000	1.53
> +++ Kconfig	30 Jun 2005 22:41:36 -0000	1.54
> @@ -215,6 +215,14 @@
>  	  This provides a driver for the on-board flash of the Intel
>  	  'Lubbock' XScale evaluation board.
>  
> +config MTD_MAINSTONE
> +	tristate "CFI Flash device mapped on Intel Mainstone XScale eval board"
> +	depends on MACH_MAINSTONE && MTD_CFI_INTELEXT
> +	select MTD_PARTITIONS
> +	help
> +	  This provides a driver for the on-board flash of the Intel
> +	  'Mainstone PXA27x evaluation board.
> +
>  config MTD_OCTAGON
>  	tristate "JEDEC Flash device mapped on Octagon 5066 SBC"
>  	depends on X86 && MTD_JEDEC && MTD_COMPLEX_MAPPINGS
> 
> Index: Makefile.common
> ===================================================================
> RCS file: /home/cvs/mtd/drivers/mtd/maps/Makefile.common,v
> retrieving revision 1.28
> retrieving revision 1.29
> diff -u -r1.28 -r1.29
> --- Makefile.common	17 Mar 2005 14:16:13 -0000	1.28
> +++ Makefile.common	30 Jun 2005 22:41:36 -0000	1.29
> @@ -22,6 +22,7 @@
>  obj-$(CONFIG_MTD_ICHXROM)	+= ichxrom.o
>  obj-$(CONFIG_MTD_TSUNAMI)	+= tsunami_flash.o
>  obj-$(CONFIG_MTD_LUBBOCK)	+= lubbock-flash.o
> +obj-$(CONFIG_MTD_MAINSTONE)	+= mainstone-flash.o
>  obj-$(CONFIG_MTD_MBX860)	+= mbx860.o
>  obj-$(CONFIG_MTD_CEIVA)		+= ceiva.o
>  obj-$(CONFIG_MTD_OCTAGON)	+= octagon-5066.o
> 
> 
> __________________________________________________________
> Linux-MTD CVS commit list
> http://lists.infradead.org/mailman/listinfo/linux-mtd-cvs/

Jörn

-- 
Premature optimization is the root of all evil.
-- Donald Knuth

Re: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29

[Todd Poynor]

Jörn Engel wrote:
> On Thu, 30 June 2005 23:41:40 +0100, tpoynor@infradead.org wrote:
> 
>>--- NEW FILE mainstone-flash.c ---
> 
> 
> Since this one is new, would you mind some comments?

Not at all, thanks.

Will fix the 80 columns and trailing whitespace.

>>		mainstone_maps[i].virt = ioremap(mainstone_maps[i].phys, WINDOW_SIZE);
>>		if (!mainstone_maps[i].virt) {
>>			printk(KERN_WARNING "Failed to ioremap %s\n", mainstone_maps[i].name);
>>			if (!ret)
>>				ret = -ENOMEM;
>>			continue;
>>		}
>>		mainstone_maps[i].cached = ioremap_cached(mainstone_maps[i].phys, WINDOW_SIZE);
>>		if (!mainstone_maps[i].cached)
>>			printk(KERN_WARNING "Failed to ioremap cached %s\n", mainstone_maps[i].name);
> 
> 
> Shouldn't we error out instead of printing a message and continuing
> on?

The code still tries to handle the other flash bank, leaving the 
un-ioremapped bank unconfigured, and only errors out if neither bank can 
be probed+mapped.  In my limited experience with the mainstone board, 
the above will not fail if the flash is not present but instead the CFI 
probe will fail, but it does seem useful to still try to handle the 
other bank.

>>		mymtds[i] = do_map_probe("cfi_probe", &mainstone_maps[i]);
>>		
>>		if (!mymtds[i]) {
>>			iounmap((void *)mainstone_maps[i].virt);
>>			if (mainstone_maps[i].cached)
>>				iounmap(mainstone_maps[i].cached);
> 
> 
> This is broken.  After above code, mainstone_maps[i].virt can be NULL.
> So either you need a NULL check for both or for none.

Not sure I understand... if ioremap returns NULL for 
mainstone_maps[i].virt then we already skip to the next loop iteration. 
  Unless do_map_probe can modify the value which I didn't think it did. 
  mainstone_maps[i].cached on the other hand can be NULL here per the 
code above.

>>		if (parsed_parts[i])
>>			kfree(parsed_parts[i]);
> 
> 
> kfree() can be called unconditionally.

OK.  Something about passing a NULL pointer to anything gives me the 
heebie-jeebies ;)

-- 
Todd

Re: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29

[Jörn Engel]

On Fri, 1 July 2005 15:39:52 -0700, Todd Poynor wrote:
> Jörn Engel wrote:
> >On Thu, 30 June 2005 23:41:40 +0100, tpoynor@infradead.org wrote:
> 
> Will fix the 80 columns and trailing whitespace.

Thanks.  The same could be done to lubbock-flash.c, if you're getting
bored.  Both maps are practically identical, except for the renames.

> >Shouldn't we error out instead of printing a message and continuing
> >on?
> 
> The code still tries to handle the other flash bank, leaving the 
> un-ioremapped bank unconfigured, and only errors out if neither bank can 
> be probed+mapped.  In my limited experience with the mainstone board, 
> the above will not fail if the flash is not present but instead the CFI 
> probe will fail, but it does seem useful to still try to handle the 
> other bank.

I didn't really understand the chaching code last time.  Looks sane,
sorry for the noise.

> >This is broken.  After above code, mainstone_maps[i].virt can be NULL.
> >So either you need a NULL check for both or for none.
> 
> Not sure I understand... if ioremap returns NULL for 
> mainstone_maps[i].virt then we already skip to the next loop iteration. 
>  Unless do_map_probe can modify the value which I didn't think it did. 
>  mainstone_maps[i].cached on the other hand can be NULL here per the 
> code above.

Dito.

> >>		if (parsed_parts[i])
> >>			kfree(parsed_parts[i]);
> >
> >
> >kfree() can be called unconditionally.
> 
> OK.  Something about passing a NULL pointer to anything gives me the 
> heebie-jeebies ;)

Then you should stay away from running Linux.  This trick is used all
over the place - and it's a great trick.  Saves a bit of binary size
and execution speed for free.

Some sparse annotations, which function parameters actually accept
NULL as a valid parameter and which don't, would be a nice addition,
just as a remedy for your heebie-jeebies.  But I haven't found the
time for sparse hacking yet.

Jörn

-- 
"Security vulnerabilities are here to stay."
-- Scott Culp, Manager of the Microsoft Security Response Center, 2001