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 <jkrzyszt@xxxxxxxxxxxx>
---
Wednesday 16 December 2009 09:12:55 Jarkko Nikula napisał(a):
> On Tue, 15 Dec 2009 01:36:47 +0100
> Janusz Krzysztofik <jkrzyszt@xxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
