In its current form, the omap_mcbsp_request() function can return after irq_request() failure without any cleanups, effectively locking out the port forever with clocks left running. Fix it.
Signed-off-by: Janusz Krzysztofik <[email protected]> --- Wednesday 16 December 2009 09:12:55 Jarkko Nikula napisał(a): > On Tue, 15 Dec 2009 01:36:47 +0100 > Janusz Krzysztofik <[email protected]> wrote: > > 3. In omap_mcbsp_free(), marking the port as free before deallocating the > > cache, could result in memory leak as well. > > > > I hope all are addressed correctly. Since releasing the port with > > > > mcbsp->free = 1; > > > > after kzalloc() failure should be atomic, I have assumed this doesn't > > require locking. > > Good catch. One comment below. > > > @@ -471,6 +491,9 @@ void omap_mcbsp_free(unsigned int id) > > free_irq(mcbsp->tx_irq, (void *)mcbsp); > > } > > > > + kfree(mcbsp->reg_cache); > > + mcbsp->reg_cache = NULL; > > + > > spin_lock(&mcbsp->lock); > > if (mcbsp->free) { > > dev_err(mcbsp->dev, "McBSP%d was not reserved\n", > > This is fine in current form but to play safe, lets do the kfree > inside the spin_lock section where the mcbsp->free is set. Then there > is no risk if some future code would use the mcbsp->free as a guard for > cache etc. access. Jarkko, More and more looking at this, I think that omap_mcbsp_request() should be cleaned up frist in respect of freeing resources on error before we put any new functionality there. This patch could be either joined with patch 1/5 or introduced as extra patch 1.5/5 into the McBSP caching set if not accepted as a separate fix. I'm not sure if it is really required to revert any mcbsp->pdata->ops->request() or omap34xx_mcbsp_request() applied changes by calling mcbsp->pdata->ops->free() or omap34xx_mcbsp_free() from here. Thanks, Janusz diff -upr git.orig/arch/arm/plat-omap/mcbsp.c git/arch/arm/plat-omap/mcbsp.c --- git.orig/arch/arm/plat-omap/mcbsp.c 2009-12-16 16:33:16.000000000 +0100 +++ git/arch/arm/plat-omap/mcbsp.c 2009-12-16 16:32:50.000000000 +0100 @@ -436,7 +436,7 @@ int omap_mcbsp_request(unsigned int id) dev_err(mcbsp->dev, "Unable to request TX IRQ %d " "for McBSP%d\n", mcbsp->tx_irq, mcbsp->id); - return err; + goto error; } init_completion(&mcbsp->rx_irq_completion); @@ -446,12 +446,26 @@ int omap_mcbsp_request(unsigned int id) dev_err(mcbsp->dev, "Unable to request RX IRQ %d " "for McBSP%d\n", mcbsp->rx_irq, mcbsp->id); - free_irq(mcbsp->tx_irq, (void *)mcbsp); - return err; + goto tx_irq; } } return 0; +tx_irq: + free_irq(mcbsp->tx_irq, (void *)mcbsp); +error: + if (mcbsp->pdata && mcbsp->pdata->ops && mcbsp->pdata->ops->free) + mcbsp->pdata->ops->free(id); + + /* Do procedure specific to omap34xx arch, if applicable */ + omap34xx_mcbsp_free(mcbsp); + + clk_disable(mcbsp->fclk); + clk_disable(mcbsp->iclk); + + mcbsp->free = 1; + + return err; } EXPORT_SYMBOL(omap_mcbsp_request); -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
