misc: sram: fix resource leaks in probe error path
Subsystems (2)
PATCH 1/2 misc: sram: fix resource leaks in probe error path
Make sure to disable clocks and deregister any exported partitions before returning on late probe errors.
Note that since commit ee895ccdf776 ("misc: sram: fix enabled clock leak on error path"), partitions are deliberately exported before enabling the clock so we stick to that logic here. A follow up patch will address this.
--- drivers/misc/sram.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/misc/sram.c +8 -1
@@ -407,13 +407,20 @@ static int sram_probe(struct platform_device *pdev)if (init_func) { ret =init_func ();if (ret) -return ret; + goto err_disable_clk; }dev_dbg (sram->dev,"SRAM pool: %zu KiB @ 0x%p\n" ,gen_pool_size (sram->pool) /1024 , sram->virt_base);return 0 ; + +err_disable_clk: +if (sram->clk) +clk_disable_unprepare (sram->clk); +sram_free_partitions (sram); + +return ret; }static int sram_remove (struct platform_device *pdev)
Signature · 1 line
2.18.0
PATCH 2/2 misc: sram: enable clock before registering regions
Make sure to enable the clock before registering regions and exporting partitions to user space at which point we must be prepared for I/O.
--- drivers/misc/sram.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/misc/sram.c +7 -6
@@ -391,23 +391,23 @@ static int sram_probe(struct platform_device *pdev)if (IS_ERR (sram->pool))return PTR_ERR (sram->pool); - ret =sram_reserve_regions (sram, res); -if (ret) -return ret; - sram->clk =devm_clk_get (sram->dev,NULL );if (IS_ERR (sram->clk)) sram->clk =NULL ;else clk_prepare_enable (sram->clk); + ret =sram_reserve_regions (sram, res); +if (ret) + goto err_disable_clk; +platform_set_drvdata (pdev, sram); init_func =of_device_get_match_data (&pdev->dev);if (init_func) { ret =init_func ();if (ret) - goto err_disable_clk; + goto err_free_partitions; } dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
@@ -415,10 +415,11 @@ static int sram_probe(struct platform_device *pdev)return 0 ; +err_free_partitions: +sram_free_partitions (sram); err_disable_clk:if (sram->clk)clk_disable_unprepare (sram->clk); -sram_free_partitions (sram);return ret; }
Signature · 1 line
2.18.0
Re: [PATCH 2/2] misc: sram: enable clock before registering regions
Hi Johan,
Quoted 6 lines · Johan Hovold
Make sure to enable the clock before registering regions and exporting partitions to user space at which point we must be prepared for I/O.
Fixes ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")Cc Vladimir Zapolskiy <[email protected]>Signed-off-by Johan Hovold <[email protected]>
thank you for the change, however please note that the identified commit for the fix is incorrect one apparently.
In my opinion the proper tag contents would be
I hope you agree to it, also I would suggest to swap the changes in the series.
--
Best wishes,
Vladimir
Re: [PATCH 2/2] misc: sram: enable clock before registering regions
Quoted 13 lines · Vladimir Zapolskiy
Hi Johan,
Quoted 6 lines · Johan Hovold
Make sure to enable the clock before registering regions and exporting partitions to user space at which point we must be prepared for I/O.
Fixes ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")Cc Vladimir Zapolskiy <[email protected]>Signed-off-by Johan Hovold <[email protected]>thank you for the change, however please note that the identified commit for the fix is incorrect one apparently.
Quoted 6 lines
In my opinion the proper tag contents would be
Fixes b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")I hope you agree to it, also I would suggest to swap the changes in the series.
No, I think I used the right commit in the Fixes tag as that was the commit which moved the clock enable to after the memory-region registration (at which point the memory could potentially be accessed).
Johan
Vladimir Zapolskiy
Reviewed-byRe: [PATCH 2/2] misc: sram: enable clock before registering regions
Quoted 16 lines · Johan Hovold
Quoted 15 lines · Vladimir Zapolskiy
Hi Johan,
Quoted 6 lines · Johan Hovold
Make sure to enable the clock before registering regions and exporting partitions to user space at which point we must be prepared for I/O.
Fixes ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")Cc Vladimir Zapolskiy <[email protected]>Signed-off-by Johan Hovold <[email protected]>thank you for the change, however please note that the identified commit for the fix is incorrect one apparently.
In my opinion the proper tag contents would be
Quoted 8 lines
Quoted 4 lines
Fixes b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")I hope you agree to it, also I would suggest to swap the changes in the series.
No, I think I used the right commit in the Fixes tag as that was the commit which moved the clock enable to after the memory-region registration (at which point the memory could potentially be accessed).
I was confused by the moved sram_reserve_regions() call, which was added way later.
Allright, if it is assumed that gen_pool_get() interface requires only a registered memory pool provider device, and it does, then there is another kind of a problem, a SRAM/genpool consumer may not get access to a valid region in SRAM before the latter is added to the SRAM pool in sram_probe().
Instantly I don't know how to solve the issue above, it may require a change to lib/genalloc.c to request a registration of genpool device driver, but then such a change solves the problem identified by you as well.
For your change as a proper (partial?) fix:
--
Best wishes,
Vladimir
Re: [PATCH 2/2] misc: sram: enable clock before registering regions
Quoted 26 lines · Vladimir Zapolskiy
Quoted 16 lines · Johan Hovold
Quoted 15 lines · Vladimir Zapolskiy
Hi Johan,
Quoted 6 lines · Johan Hovold
Make sure to enable the clock before registering regions and exporting partitions to user space at which point we must be prepared for I/O.
Fixes ee895ccdf776 ("misc: sram: fix enabled clock leak on error path")Cc Vladimir Zapolskiy <[email protected]>Signed-off-by Johan Hovold <[email protected]>thank you for the change, however please note that the identified commit for the fix is incorrect one apparently.
In my opinion the proper tag contents would be
Quoted 8 lines
Quoted 4 lines
Fixes b4c3fcb3c71f ("misc: sram: extend usage of reserved partitions")I hope you agree to it, also I would suggest to swap the changes in the series.
No, I think I used the right commit in the Fixes tag as that was the commit which moved the clock enable to after the memory-region registration (at which point the memory could potentially be accessed).
Quoted 8 lines
I was confused by the moved
sram_reserve_regions()call, which was added way later.Allright, if it is assumed that
gen_pool_get()interface requires only a registered memory pool provider device, and it does, then there is another kind of a problem, a SRAM/genpool consumer may not get access to a valid region in SRAM before the latter is added to the SRAM pool insram_probe().
Right, this whole genpool interface is fragile, but that's a different story.
Quoted 4 lines
Instantly I don't know how to solve the issue above, it may require a change to
lib/genalloc.cto request a registration of genpool device driver, but then such a change solves the problem identified by you as well.
The resource (genpool) should not be registered before it's been fully initialised, while any prior attempts to look it up could cause the consumer driver to defer their probes, for example. But again, that's beyond the scope here.
Quoted 3 lines
For your change as a proper (partial?) fix:
Reviewed-by Vladimir Zapolskiy <[email protected]>
Thanks for the review!
Johan