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 at 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