Thread (13 messages) 13 messages, 2 authors, 2013-07-04

[PATCH v2 1/6] misc: sram: fix error path in sram_probe

From: p.zabel@pengutronix.de (Philipp Zabel)
Date: 2013-07-04 15:42:35
Also in: linux-devicetree, lkml

Hi Heiko,

Am Donnerstag, den 04.07.2013, 16:34 +0200 schrieb Heiko St?bner:
Hi Philipp,

Am Dienstag, 25. Juni 2013, 11:04:34 schrieb Philipp Zabel:
quoted
Hi Heiko,

Am Dienstag, den 25.06.2013, 10:46 +0200 schrieb Heiko St?bner:
quoted
The pool is created thru devm_gen_pool_create, so the call to
gen_pool_destroy is not necessary.
Instead the sram-clock must be turned off again if it exists.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---

 drivers/misc/sram.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index d87cc91..afe66571 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -68,7 +68,8 @@ static int sram_probe(struct platform_device *pdev)

 	ret = gen_pool_add_virt(sram->pool, (unsigned long)virt_base,
 	
 				res->start, size, -1);
 	
 	if (ret < 0) {

-		gen_pool_destroy(sram->pool);
Right, thanks.
quoted
+		if (sram->clk)
+			clk_disable_unprepare(sram->clk);

 		return ret;
 	
 	}
In light of the following patch, I'd rather move the
clk_prepare_enable() call after gen_pool_add_virt() and its error path.
I'm not sure, but isn't moving the clock enablement below the pool allocation 
producing a race condition?
since this is a platform device, and the driver is not allowed to be
compiled as a module, it is probed at the time platform_driver_register
is called from postcore_initcall.
You'd have to go to quite some lengths to make this race by probing your
driver before mmio-sram and then asynchronously trying to allocate
memory from the pool some time later, without having made sure that the
pool exists during probe.

It would be nice to have callbacks from gen_pool_alloc/free to
enable/disable the clock.
For the time being, maybe it's the safest bet to just enable the clock
before, especially if you reorder the loop so there is only one
gen_pool_add_virt and corresponding error path.
I.e. can the case happen that some other part wants to allocate part of the 
newly generated pool already, while the subsequent gen_pool_add_virt calls 
from the following patch are still running? ... And what will happen in this 
case, when the sram clock is still disabled?
On i.MX6, the system will hang.

regards
Philipp
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help