mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29
Todd Poynor
tpoynor at mvista.com
Fri Jul 1 18:39:52 EDT 2005
- Previous message: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29
- Next message: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
Jörn Engel wrote:
> On Thu, 30 June 2005 23:41:40 +0100, tpoynor at 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
- Previous message: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29
- Next message: mtd/drivers/mtd/maps mainstone-flash.c, NONE, 1.1 Kconfig, 1.53, 1.54 Makefile.common, 1.28, 1.29
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
More information about the linux-mtd
mailing list