mtd: nand: orion: fix clk handling
Subsystems (2)
PATCH v2 1/2 mtd: nand: orion: fix clk handling
The clk handling in orion_nand.c had two problems:
- In the probe function, clk_put() was called for an enabled clock, which violates the API (see documentation for clk_put() in include/linux/clk.h)
- In the error path of the probe function, clk_put() could be called twice for the same clock.
In order to clean this up, use the managed function devm_clk_get() and store the pointer to the clk in the driver data.
---
Changes in v2:
* changed whitespace alignment in orion_nand_info struct definition
drivers/mtd/nand/orion_nand.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
drivers/mtd/nand/orion_nand.c +21 -21
@@ -23,6 +23,11 @@#include <asm/sizes.h> #include <linux/platform_data/mtd-orion_nand.h> +struct orion_nand_info { +struct nand_chip chip; +struct clk *clk; +}; +static void orion_nand_cmd_ctrl (struct mtd_info *mtd,int cmd,unsigned int ctrl) {struct nand_chip *nc =mtd_to_nand (mtd);
@@ -75,20 +80,21 @@ static void orion_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)static int __init orion_nand_probe (struct platform_device *pdev) { +struct orion_nand_info *info;struct mtd_info *mtd;struct nand_chip *nc;struct orion_nand_data *board;struct resource *res; -struct clk *clk;void __iomem *io_base;int ret =0 ;u32 val =0 ; - nc =devm_kzalloc (&pdev->dev, -sizeof (struct nand_chip ), + info =devm_kzalloc (&pdev->dev, +sizeof (struct orion_nand_info ),GFP_KERNEL ); -if (!nc) +if (!info)return -ENOMEM ; + nc = &info->chip; mtd =nand_to_mtd (nc); res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );
@@ -145,15 +151,13 @@ static int __init orion_nand_probe(struct platform_device *pdev)if (board->dev_ready) nc->dev_ready = board->dev_ready; -platform_set_drvdata (pdev, mtd); +platform_set_drvdata (pdev, info);/* Not all platforms can gate the clock, so it is not an error if the clock does not exists. */ - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_prepare_enable (clk); -clk_put (clk); - } + info->clk =devm_clk_get (&pdev->dev,NULL ); +if (!IS_ERR (info->clk)) +clk_prepare_enable (info->clk); ret =nand_scan (mtd,1 );if (ret)
@@ -169,26 +173,22 @@ static int __init orion_nand_probe(struct platform_device *pdev)return 0 ; no_dev: -if (!IS_ERR (clk)) { -clk_disable_unprepare (clk); -clk_put (clk); - } +if (!IS_ERR (info->clk)) +clk_disable_unprepare (info->clk);return ret; }static int orion_nand_remove (struct platform_device *pdev) { -struct mtd_info *mtd =platform_get_drvdata (pdev); -struct clk *clk; +struct orion_nand_info *info =platform_get_drvdata (pdev); +struct nand_chip *chip = &info->chip; +struct mtd_info *mtd =nand_to_mtd (chip);nand_release (mtd); - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_disable_unprepare (clk); -clk_put (clk); - } +if (!IS_ERR (info->clk)) +clk_disable_unprepare (info->clk);return 0 ; }
Signature · 1 line
2.9.3
PATCH v2 2/2 mtd: nand: orion: improve handling of optional clock
The clock gate used by orion_nand is not available on all platforms. When getting this optional clock gate, the code masked all errors. Let's be more precise here and actually only allow ENOENT.
EPROBE_DEFER is handled like any other error code since probe deferral is not supported by drivers using module_platform_driver_probe().
---
Changes in v2:
* added patch
drivers/mtd/nand/orion_nand.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
drivers/mtd/nand/orion_nand.c +13 -7
@@ -156,8 +156,17 @@ static int __init orion_nand_probe(struct platform_device *pdev)/* Not all platforms can gate the clock, so it is not an error if the clock does not exists. */ info->clk =devm_clk_get (&pdev->dev,NULL ); -if (!IS_ERR (info->clk)) -clk_prepare_enable (info->clk); +if (IS_ERR (info->clk)) { + ret =PTR_ERR (info->clk); +if (ret == -ENOENT ) { + info->clk =NULL ; + }else { +dev_err (&pdev->dev,"failed to get clock!\n" ); +return ret; + } + } + +clk_prepare_enable (info->clk); ret =nand_scan (mtd,1 );if (ret)
@@ -173,9 +182,7 @@ static int __init orion_nand_probe(struct platform_device *pdev)return 0 ; no_dev: -if (!IS_ERR (info->clk)) -clk_disable_unprepare (info->clk); - +clk_disable_unprepare (info->clk);return ret; }
@@ -187,8 +194,7 @@ static int orion_nand_remove(struct platform_device *pdev)nand_release (mtd); -if (!IS_ERR (info->clk)) -clk_disable_unprepare (info->clk); +clk_disable_unprepare (info->clk);return 0 ; }
Signature · 1 line
2.9.3
PATCH v2 1/2 mtd: nand: orion: fix clk handling
In reply to Simon BaatzHello,
Quoted 7 lines
drivers/mtd/nand/orion_nand.c +1
-1@@ -145,15 +151,13 @@ static int __init orion_nand_probe(struct platform_device *pdev)if (board->dev_ready) nc->dev_ready = board->dev_ready; -platform_set_drvdata (pdev, mtd); +platform_set_drvdata (pdev, info);
drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 160
Quoted 10 lines
drivers/mtd/nand/orion_nand.c +3
-5/* Not all platforms can gate the clock, so it is not an error if the clock does not exists. */ - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_prepare_enable (clk); -clk_put (clk); - } + info->clk =devm_clk_get (&pdev->dev,NULL ); +if (!IS_ERR (info->clk)) +clk_prepare_enable (info->clk);
you could to the following here instead:
info->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(info->clk)) /* * We ignore all errors here, that's wrong but only for * one commit. */ info->clk = NULL; ret = clk_prepare_enable(info->clk); if (ret) ...
drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 177
Quoted 12 lines
drivers/mtd/nand/orion_nand.c +2
-4ret =nand_scan (mtd,1 );if (ret)@@ -169,26 +173,22 @@ static int __init orion_nand_probe(struct platform_device *pdev)return 0 ; no_dev: -if (!IS_ERR (clk)) { -clk_disable_unprepare (clk); -clk_put (clk); - } +if (!IS_ERR (info->clk)) +clk_disable_unprepare (info->clk);
this simplifies to
clk_disable_unprepare(info->clk);
then.
Quoted 11 lines
drivers/mtd/nand/orion_nand.c +3
-2return ret; }static int orion_nand_remove (struct platform_device *pdev) { -struct mtd_info *mtd =platform_get_drvdata (pdev); -struct clk *clk; +struct orion_nand_info *info =platform_get_drvdata (pdev); +struct nand_chip *chip = &info->chip; +struct mtd_info *mtd =nand_to_mtd (chip);
drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 191
Quoted 10 lines
drivers/mtd/nand/orion_nand.c +2
-5nand_release (mtd); - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_disable_unprepare (clk); -clk_put (clk); - } +if (!IS_ERR (info->clk)) +clk_disable_unprepare (info->clk);
ditto.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/2 mtd: nand: orion: fix clk handling
Hello Uwe,
Quoted 22 lines · Uwe Kleine-K?nig
Hello,
Quoted 7 lines
drivers/mtd/nand/orion_nand.c +1
-1@@ -145,15 +151,13 @@ static int __init orion_nand_probe(struct platform_device *pdev)if (board->dev_ready) nc->dev_ready = board->dev_ready; -platform_set_drvdata (pdev, mtd); +platform_set_drvdata (pdev, info);drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 160
Quoted 10 lines
drivers/mtd/nand/orion_nand.c +3
-5/* Not all platforms can gate the clock, so it is not an error if the clock does not exists. */ - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_prepare_enable (clk); -clk_put (clk); - } + info->clk =devm_clk_get (&pdev->dev,NULL ); +if (!IS_ERR (info->clk)) +clk_prepare_enable (info->clk);you could to the following here instead:
Quoted 11 lines
info->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(info->clk)) /* * We ignore all errors here, that's wrong but only for * one commit. */ info->clk = NULL; ret = clk_prepare_enable(info->clk); if (ret) ...
Makes sense. I don't think we should have such a comment, though. The patch description states what it fixes and the patch fixes exactly that. Moreover, I tagged this patch for stable (because of the double free it fixes) and there might be no other commit. (I think the error handling problem is not severe enough for stable. It has been this way a couple of years and nobody seems to have noticed.)
- Simon
PATCH v2 1/2 mtd: nand: orion: fix clk handling
Quoted 11 lines · Simon Baatz
Quoted 10 lines · Uwe Kleine-K?nig
you could to the following here instead:
info->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(info->clk)) /* * We ignore all errors here, that's wrong but only for * one commit. */ info->clk = NULL;
Quoted 5 lines
Quoted 3 lines
ret = clk_prepare_enable(info->clk); if (ret) ...Makes sense. I don't think we should have such a comment, though.
Yes I agree, I just wanted to prevent people thinking I suggest this code snipet for optional clock handling and so wanted to write a big No-No-No onto it.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH v2 1/2 mtd: nand: orion: fix clk handling
In reply to Simon BaatzQuoted 37 lines · On Wed, 29 Mar 2017 21:36:08 +0200 Simon Baatz <[email protected]> wrote:
Hello Uwe,
Quoted 22 lines · Uwe Kleine-K?nig
Hello,
Quoted 7 lines
drivers/mtd/nand/orion_nand.c +1
-1@@ -145,15 +151,13 @@ static int __init orion_nand_probe(struct platform_device *pdev)if (board->dev_ready) nc->dev_ready = board->dev_ready; -platform_set_drvdata (pdev, mtd); +platform_set_drvdata (pdev, info);drivers/mtd/nand/orion_nand.c static int __init orion_nand_probe(struct platform_device *pdev) · line 160
Quoted 10 lines
drivers/mtd/nand/orion_nand.c +3
-5/* Not all platforms can gate the clock, so it is not an error if the clock does not exists. */ - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_prepare_enable (clk); -clk_put (clk); - } + info->clk =devm_clk_get (&pdev->dev,NULL ); +if (!IS_ERR (info->clk)) +clk_prepare_enable (info->clk);you could to the following here instead:
Quoted 11 lines
info->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(info->clk)) /* * We ignore all errors here, that's wrong but only for * one commit. */ info->clk = NULL; ret = clk_prepare_enable(info->clk); if (ret) ...
Quoted 7 lines
Makes sense. I don't think we should have such a comment, though. The patch description states what it fixes and the patch fixes exactly that. Moreover, I tagged this patch for stable (because of the double free it fixes) and there might be no other commit. (I think the error handling problem is not severe enough for stable. It has been this way a couple of years and nobody seems to have noticed.)
No need to resend a new version, I'm fine with this one (actually I already applied it and am about to push it on nand/next).
PATCH v2 1/2 mtd: nand: orion: fix clk handling
In reply to Simon BaatzQuoted 9 lines · On Mon, 27 Mar 2017 20:02:07 +0200 Simon Baatz <[email protected]> wrote:
The clk handling in
orion_nand.chad two problems:- In the probe function,
clk_put()was called for an enabled clock, which violates the API (see documentation forclk_put()ininclude/linux/clk.h)- In the error path of the probe function,
clk_put()could be called twice for the same clock.
Quoted 6 lines
In order to clean this up, use the managed function
devm_clk_get()and store the pointer to the clk in the driver data.Fixes baffab28b13120694fa3ebab08d3e99667a851d2 ('ARM: Orion: fix driver probe error handling with respect to clk')Cc <[email protected]> # v4.5+Signed-off-by Simon Baatz <[email protected]>
Applied both.
Thanks,
Boris
Quoted 104 lines
---
Changes in v2:
* changed whitespace alignment inorion_nand_infostruct definitiondrivers/mtd/nand/orion_nand.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)drivers/mtd/nand/orion_nand.c +21
-21@@ -23,6 +23,11 @@#include <asm/sizes.h> #include <linux/platform_data/mtd-orion_nand.h> +struct orion_nand_info { +struct nand_chip chip; +struct clk *clk; +}; +static void orion_nand_cmd_ctrl (struct mtd_info *mtd,int cmd,unsigned int ctrl) {struct nand_chip *nc =mtd_to_nand (mtd);@@ -75,20 +80,21 @@ static void orion_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)static int __init orion_nand_probe (struct platform_device *pdev) { +struct orion_nand_info *info;struct mtd_info *mtd;struct nand_chip *nc;struct orion_nand_data *board;struct resource *res; -struct clk *clk;void __iomem *io_base;int ret =0 ;u32 val =0 ; - nc =devm_kzalloc (&pdev->dev, -sizeof (struct nand_chip ), + info =devm_kzalloc (&pdev->dev, +sizeof (struct orion_nand_info ),GFP_KERNEL ); -if (!nc) +if (!info)return -ENOMEM ; + nc = &info->chip; mtd =nand_to_mtd (nc); res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );@@ -145,15 +151,13 @@ static int __init orion_nand_probe(struct platform_device *pdev)if (board->dev_ready) nc->dev_ready = board->dev_ready; -platform_set_drvdata (pdev, mtd); +platform_set_drvdata (pdev, info);/* Not all platforms can gate the clock, so it is not an error if the clock does not exists. */ - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_prepare_enable (clk); -clk_put (clk); - } + info->clk =devm_clk_get (&pdev->dev,NULL ); +if (!IS_ERR (info->clk)) +clk_prepare_enable (info->clk); ret =nand_scan (mtd,1 );if (ret)@@ -169,26 +173,22 @@ static int __init orion_nand_probe(struct platform_device *pdev)return 0 ; no_dev: -if (!IS_ERR (clk)) { -clk_disable_unprepare (clk); -clk_put (clk); - } +if (!IS_ERR (info->clk)) +clk_disable_unprepare (info->clk);return ret; }static int orion_nand_remove (struct platform_device *pdev) { -struct mtd_info *mtd =platform_get_drvdata (pdev); -struct clk *clk; +struct orion_nand_info *info =platform_get_drvdata (pdev); +struct nand_chip *chip = &info->chip; +struct mtd_info *mtd =nand_to_mtd (chip);nand_release (mtd); - clk =clk_get (&pdev->dev,NULL ); -if (!IS_ERR (clk)) { -clk_disable_unprepare (clk); -clk_put (clk); - } +if (!IS_ERR (info->clk)) +clk_disable_unprepare (info->clk);return 0 ; }