| Message ID | 20170327180208.13414-1-gmbnomis@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, > @@ -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); 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) ... > 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); this simplifies to clk_disable_unprepare(info->clk); then. > > 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); ditto. Best regards Uwe
Hello Uwe, On Mon, Mar 27, 2017 at 08:19:49PM +0200, Uwe Kleine-K?nig wrote: > Hello, > > > @@ -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); > > 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) ... > 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
On Wed, Mar 29, 2017 at 09:36:08PM +0200, Simon Baatz wrote: > On Mon, Mar 27, 2017 at 08:19:49PM +0200, Uwe Kleine-König wrote: > > 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) ... > > > > 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
On Wed, 29 Mar 2017 21:36:08 +0200 Simon Baatz <gmbnomis@gmail.com> wrote: > Hello Uwe, > > On Mon, Mar 27, 2017 at 08:19:49PM +0200, Uwe Kleine-K?nig wrote: > > Hello, > > > > > @@ -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); > > > > 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) ... > > > > 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).
On Mon, 27 Mar 2017 20:02:07 +0200 Simon Baatz <gmbnomis@gmail.com> wrote: > 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. > > Fixes: baffab28b13120694fa3ebab08d3e99667a851d2 ('ARM: Orion: fix driver probe error handling with respect to clk') > Cc: <stable@vger.kernel.org> # v4.5+ > Signed-off-by: Simon Baatz <gmbnomis@gmail.com> Applied both. Thanks, Boris > --- > > 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(-) > > diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c > index 4a91c5d000be..3acdc20485f1 100644 > --- a/drivers/mtd/nand/orion_nand.c > +++ b/drivers/mtd/nand/orion_nand.c > @@ -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; > }
diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c index 4a91c5d000be..3acdc20485f1 100644 --- a/drivers/mtd/nand/orion_nand.c +++ b/drivers/mtd/nand/orion_nand.c @@ -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; }
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. Fixes: baffab28b13120694fa3ebab08d3e99667a851d2 ('ARM: Orion: fix driver probe error handling with respect to clk') Cc: <stable@vger.kernel.org> # v4.5+ Signed-off-by: Simon Baatz <gmbnomis@gmail.com> --- 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(-)