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

[Brian Norris]

On Mon, Sep 30, 2013 at 09:37:57AM -0300, Ezequiel Garcia wrote:
> 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 at 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'?

The existing code allocates data_buff with kmalloc() or
dma_alloc_coherent() depending on ARCH_HAS_DMA, but your patch does the
initial fixed-size allocation only with kmalloc(). It's just what you
and Daniel already identified: that this causes problems for the DMA
case.

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

No, I'm not sure :)

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

Nope, you were correct. I just missed this point when reading.

Brian