Re: [PATCH] mtd: nand: pxa3xx: Use info->use_dma to release DMA resources

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

> Brian,
> 
> On Wed, Nov 27, 2013 at 08:34:05AM -0300, Ezequiel Garcia wrote:
> > On Tue, Nov 26, 2013 at 11:58:19PM -0800, Brian Norris wrote:
> > > On Tue, Nov 26, 2013 at 09:52:24AM -0300, Ezequiel Garcia wrote:
> > > > After the driver allocates all DMA resources, it sets "info->use_dma".
> > > > Therefore, we need to check that variable to decide which resources
> > > > needs to be freed, instead of the global use_dma variable.
> > > > 
> > > > Without this change, when the device probe fails, the driver will try
> > > > to release unallocated DMA resources, with nasty results.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > > ---
> > > > This is minor fix, but a fix anyway, so it should be queued for stable.
> > > > 
> > > >  drivers/mtd/nand/pxa3xx_nand.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > > > index 97c3eb5..8f2104c 100644
> > > > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > > > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > > > @@ -1288,7 +1288,7 @@ static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
> > > >  static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
> > > >  {
> > > >  	struct platform_device *pdev = info->pdev;
> > > > -	if (use_dma) {
> > > > +	if (info->use_dma) {
> > > 
> > > With this change, then the 'else' case will be executed and will kfree()
> > > the data buffer that we didn't allocate?
> > > 
> > >   kfree(info->data_buff);
> > > 
> > > Fortunately kfree()'ing a NULL is a no-op.
> > > 
> > 
> > No. The buffer is allocated at a very early stage in DMA and non-DMA modes.
> > It's later reallocated, but in any case when pxa3xx_nand_free_buff()
> > is called, the buffer can never be NULL.
> > 
> > The situation this commit fixes is that when the cleanup is called from
> > the probe(), the DMA resources haven't been allocated yet and so the
> > release must be kfree().
> > 
> > Remember we've recently changed this driver to work in non-DMA mode
> > until the page size is correctly detected. Until then, info->use_dma is
> > false.
> > 
> > > But this highlights some of the strangeness of the error handling in
> > > this driver. It has an atypical style, in which the probe routine calls
> > > the remove routine if it fails. Normally, the probe routine has teardown
> > > carefully planned in stages, whereas the remove routine just does it all
> > > in one go (since it can assume the device was fully initialized). (And
> > > of course, the devm_* functions can help with simplifying some of
> > > this...)
> > > 
> > 
> > Yes, I know. The driver is very very old, and some parts of it need some
> > love. However, the error handling is not so bad. When
> > pxa3xx_nand_remove() is called, all resources are guaranteed to be
> > allocated so it works like a full-fledged release.
> > 
> > I'm actually more annoyed by the fact there's two variables with the
> > same name: use_dma, and info->use_dma.
> > 
> > > But I suppose this patch is correct as-is, and this issue could be
> > > revisited later.
> > > 
> > > For my reference, have you actually seen this bug in practice? I'm not
> > > sure how well this fits in the -stable rules, if it hasn't been observed
> > > and tested appropriately.
> > > 
> > 
> > Yes, I've seen this bug in practice, or otherwise wouldn't notice such small
> > typo :-) The bug was triggered in PXA platforms, when the device cannot
> > be identified (nand_scan fails).
> > 
> > It's a very rare condition, but it's a real bug. Is that enough for -stable?
> 
> Just a gentle ping, in case this one felt through the cracks.
> 

I'm sorry to bother by pinging *again*, but I think this is a fix for v3.13.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com