Re: [PATCH 01/21] mtd: nand: pxa3xx: Allocate data buffer on detected flash size

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

On Thu, Sep 26, 2013 at 01:10:32PM -0700, Brian Norris wrote:
> On Thu, Sep 19, 2013 at 01:01:25PM -0300, Ezequiel Garcia wrote:
> > This commit replaces the currently hardcoded buffer size, by a
> > dynamic detection scheme. First a small 256 bytes buffer is allocated
> > so the device can be detected (using READID and friends commands).
> > 
> > After detection, this buffer is released and a new buffer is allocated
> > to acommodate the page size plus out-of-band size.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> This is the patch that breaks Daniel's DMA setup, right? It looks a bit
> off.

What do you mean by 'a bit off'?

> I'll wait to comment on it much until v2.
> 

Ok... let me re-work it then and prepare the v2.

> BTW, there is a similar issue with at least one other driver (denali.c,
> maybe others) where the driver uses some hard assumptions about the
> maximum page/OOB sizes (NAND_MAX_PAGESIZE and NAND_MAX_OOBSIZE). This is
> kind of ugly and not very sustainable/maintainable, since these
> dimensions keep increasing. I appreciate your effort to avoid simply
> kludging in a larger MAX_BUFF_SIZE :) I had similar plans for the other
> drivers, but I don't know if we'll have much testing opportunities for
> the ancient ones...
> 
> Also, it seems like your driver has a few potential leaks right now. If
> alloc_nand_resource() succeeds but pxa3xx_nand_probe() later fails
> (e.g., in pxa3xx_nand_scan()), you don't clean up after yourself. You
> should address this soon, even if not in this patch series.
> 

Hm.. are you sure about that? AFAICS, there's no leak at all.

If alloc_nand_resource() succeeds, the only leakable resources allocated
are the ones allocated at pxa3xx_nand_init_buff() and the NAND base
stuff.

If pxa3xx_nand_probe() later fails to complete, it calls pxa3xx_nand_remove()
in this part:

	if (!probe_success) {
		pxa3xx_nand_remove(pdev);
		return -ENODEV;
	}

Which takes care of cleaning both the buffers and the NAND base stuff.

Or am I missing something?

Feel free to ask more questions and thanks a lot for the feedback.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com