[PATCH] OMAP: McBSP: Fix possible port lockout

[PATCH] OMAP: McBSP: Fix possible port lockout

[Janusz Krzysztofik]

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@tis.icnet.pl>

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

Re: [PATCH] OMAP: McBSP: Fix possible port lockout

[Jarkko Nikula]

On Wed, 16 Dec 2009 17:16:28 +0100
Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote:

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

> 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.
> 
It is allways worth/must to keep request and free calls in balance as
your patch does. In this case some clocks may left running
(omap1_mcbsp_request/_free) or wakeup behavior (34xx) otherwise.

Acked-by: Jarkko Nikula <jhnikula@gmail.com>

Re: [PATCH] OMAP: McBSP: Fix possible port lockout

[Peter Ujfalusi]

Hello,

On Wednesday 16 December 2009 18:16:28 ext Janusz Krzysztofik wrote:
> 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@tis.icnet.pl>

Good catch,

Acked-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>