| Message ID | 1522811151-18853-1-git-send-email-wangxidong_97@163.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] mtd:nand:fix memory leak | expand |
Hi Xidong,
As part of a reorganization in the NAND subsystem, you should now
prefix your commit title this way:
mtd: rawnand: tango: fix memory leak
Not sure if this patch is candidate to cc:stable?
On Wed, 4 Apr 2018 11:05:51 +0800, Xidong Wang
<wangxidong_97@163.com> wrote:
> In function tango_nand_probe(), the memory allocated by
> clk_get() is not released on the normal path and
> the error path that IS_ERR(nfc->chan) returns true.
The fact that the error path returns true looks out of topic, can you
remove it? Just saying that you fix a memory leak is enough I guess.
> This will result in a memory leak bug.
>
> Signed-off-by: Xidong Wang <wangxidong_97@163.com>
> ---
> drivers/mtd/nand/tango_nand.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
> index c5bee00b..8083459 100644
> --- a/drivers/mtd/nand/tango_nand.c
> +++ b/drivers/mtd/nand/tango_nand.c
> @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev)
> return PTR_ERR(clk);
>
> nfc->chan = dma_request_chan(&pdev->dev, "rxtx");
> - if (IS_ERR(nfc->chan))
> + if (IS_ERR(nfc->chan)) {
> + clk_put(clk);
> return PTR_ERR(nfc->chan);
> + }
>
> platform_set_drvdata(pdev, nfc);
> nand_hw_control_init(&nfc->hw);
> nfc->freq_kHz = clk_get_rate(clk) / 1000;
> + clk_put(clk);
If the clock is used only here, better do the frequency derivation
right after the clock_get(), and follow with a clk_put()? This way you
don't have to change the error path and 'related' actions remain
grouped.
Thanks for fixing this,
Miquèl
On Wed, 4 Apr 2018 08:28:07 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Xidong, > > As part of a reorganization in the NAND subsystem, you should now > prefix your commit title this way: > > mtd: rawnand: tango: fix memory leak > > Not sure if this patch is candidate to cc:stable? > > On Wed, 4 Apr 2018 11:05:51 +0800, Xidong Wang > <wangxidong_97@163.com> wrote: > > > In function tango_nand_probe(), the memory allocated by > > clk_get() is not released on the normal path and > > the error path that IS_ERR(nfc->chan) returns true. > > The fact that the error path returns true looks out of topic, can you > remove it? Just saying that you fix a memory leak is enough I guess. > > > This will result in a memory leak bug. > > > > Signed-off-by: Xidong Wang <wangxidong_97@163.com> > > --- > > drivers/mtd/nand/tango_nand.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > > index c5bee00b..8083459 100644 > > --- a/drivers/mtd/nand/tango_nand.c > > +++ b/drivers/mtd/nand/tango_nand.c > > @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev) > > return PTR_ERR(clk); > > > > nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); > > - if (IS_ERR(nfc->chan)) > > + if (IS_ERR(nfc->chan)) { > > + clk_put(clk); > > return PTR_ERR(nfc->chan); > > + } > > > > platform_set_drvdata(pdev, nfc); > > nand_hw_control_init(&nfc->hw); > > nfc->freq_kHz = clk_get_rate(clk) / 1000; > > + clk_put(clk); > > If the clock is used only here, better do the frequency derivation > right after the clock_get(), and follow with a clk_put()? This way you > don't have to change the error path and 'related' actions remain > grouped. Hm, definitely not a good idea to release the reference you have on the clk if the driver depends on it. I recommend using devm_clk_get() to solve this leak.
On Wed, 4 Apr 2018 09:07:10 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Wed, 4 Apr 2018 08:28:07 +0200 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Hi Xidong, > > > > As part of a reorganization in the NAND subsystem, you should now > > prefix your commit title this way: > > > > mtd: rawnand: tango: fix memory leak > > > > Not sure if this patch is candidate to cc:stable? > > > > On Wed, 4 Apr 2018 11:05:51 +0800, Xidong Wang > > <wangxidong_97@163.com> wrote: > > > > > In function tango_nand_probe(), the memory allocated by > > > clk_get() is not released on the normal path and > > > the error path that IS_ERR(nfc->chan) returns true. > > > > The fact that the error path returns true looks out of topic, can you > > remove it? Just saying that you fix a memory leak is enough I guess. > > > > > This will result in a memory leak bug. > > > > > > Signed-off-by: Xidong Wang <wangxidong_97@163.com> > > > --- > > > drivers/mtd/nand/tango_nand.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > > > index c5bee00b..8083459 100644 > > > --- a/drivers/mtd/nand/tango_nand.c > > > +++ b/drivers/mtd/nand/tango_nand.c > > > @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev) > > > return PTR_ERR(clk); > > > > > > nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); > > > - if (IS_ERR(nfc->chan)) > > > + if (IS_ERR(nfc->chan)) { > > > + clk_put(clk); > > > return PTR_ERR(nfc->chan); > > > + } > > > > > > platform_set_drvdata(pdev, nfc); > > > nand_hw_control_init(&nfc->hw); > > > nfc->freq_kHz = clk_get_rate(clk) / 1000; > > > + clk_put(clk); > > > > If the clock is used only here, better do the frequency derivation > > right after the clock_get(), and follow with a clk_put()? This way you > > don't have to change the error path and 'related' actions remain > > grouped. > > Hm, definitely not a good idea to release the reference you have on the > clk if the driver depends on it. I recommend using devm_clk_get() to > solve this leak. > BTW, it's also weird that the driver does not prepare_enable the clk. Mark, any comments?
On 04/04/2018 09:08, Boris Brezillon wrote: > On Wed, 4 Apr 2018 09:07:10 +0200 > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > >> On Wed, 4 Apr 2018 08:28:07 +0200 >> Miquel Raynal <miquel.raynal@bootlin.com> wrote: >> >>> Hi Xidong, >>> >>> As part of a reorganization in the NAND subsystem, you should now >>> prefix your commit title this way: >>> >>> mtd: rawnand: tango: fix memory leak >>> >>> Not sure if this patch is candidate to cc:stable? >>> >>> On Wed, 4 Apr 2018 11:05:51 +0800, Xidong Wang >>> <wangxidong_97@163.com> wrote: >>> >>>> In function tango_nand_probe(), the memory allocated by >>>> clk_get() is not released on the normal path and >>>> the error path that IS_ERR(nfc->chan) returns true. >>> >>> The fact that the error path returns true looks out of topic, can you >>> remove it? Just saying that you fix a memory leak is enough I guess. >>> >>>> This will result in a memory leak bug. >>>> >>>> Signed-off-by: Xidong Wang <wangxidong_97@163.com> >>>> --- >>>> drivers/mtd/nand/tango_nand.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c >>>> index c5bee00b..8083459 100644 >>>> --- a/drivers/mtd/nand/tango_nand.c >>>> +++ b/drivers/mtd/nand/tango_nand.c >>>> @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev) >>>> return PTR_ERR(clk); >>>> >>>> nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); >>>> - if (IS_ERR(nfc->chan)) >>>> + if (IS_ERR(nfc->chan)) { >>>> + clk_put(clk); >>>> return PTR_ERR(nfc->chan); >>>> + } >>>> >>>> platform_set_drvdata(pdev, nfc); >>>> nand_hw_control_init(&nfc->hw); >>>> nfc->freq_kHz = clk_get_rate(clk) / 1000; >>>> + clk_put(clk); >>> >>> If the clock is used only here, better do the frequency derivation >>> right after the clock_get(), and follow with a clk_put()? This way you >>> don't have to change the error path and 'related' actions remain >>> grouped. >> >> Hm, definitely not a good idea to release the reference you have on the >> clk if the driver depends on it. I recommend using devm_clk_get() to >> solve this leak. > > BTW, it's also weird that the driver does not prepare_enable the clk. > Marc, any comments? I was not aware that clk_get() allocated memory, and required clk_put() for cleanup. IIRC, I looked at Documentation/clk.txt On tango, clocks are configured by the boot loader. The existing clk driver provides only read access to various clocks -- except the CPU clock, which can be changed by tweaking a post-divider. Tweaking the PLLs requires much more complex code. The boot loader enables every clock, and Linux has no way to gate any of them. In the nfc driver, all I needed was the system frequency, since the NFC is driven by the system clock (which can never be disabled). Thus, I wrote the naive (and apparently incorrect) clk = clk_get(&pdev->dev, NULL); nfc->freq_kHz = clk_get_rate(clk) / 1000; I suppose the following patch would fix the memory leak, and matches what Miquèl suggested. Regards. diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c index c5bee00b7f5e..fba162af333f 100644 --- a/drivers/mtd/nand/tango_nand.c +++ b/drivers/mtd/nand/tango_nand.c @@ -646,6 +646,8 @@ static int tango_nand_probe(struct platform_device *pdev) clk = clk_get(&pdev->dev, NULL); if (IS_ERR(clk)) return PTR_ERR(clk); + nfc->freq_kHz = clk_get_rate(clk) / 1000; + clk_put(clk); nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); if (IS_ERR(nfc->chan)) @@ -653,7 +655,6 @@ static int tango_nand_probe(struct platform_device *pdev) platform_set_drvdata(pdev, nfc); nand_hw_control_init(&nfc->hw); - nfc->freq_kHz = clk_get_rate(clk) / 1000; for_each_child_of_node(pdev->dev.of_node, np) { err = chip_init(&pdev->dev, np);
Hi Marc, On Thu, 5 Apr 2018 11:12:11 +0200, Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > On 04/04/2018 09:08, Boris Brezillon wrote: > > > On Wed, 4 Apr 2018 09:07:10 +0200 > > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > >> On Wed, 4 Apr 2018 08:28:07 +0200 > >> Miquel Raynal <miquel.raynal@bootlin.com> wrote: > >> > >>> Hi Xidong, > >>> > >>> As part of a reorganization in the NAND subsystem, you should now > >>> prefix your commit title this way: > >>> > >>> mtd: rawnand: tango: fix memory leak > >>> > >>> Not sure if this patch is candidate to cc:stable? > >>> > >>> On Wed, 4 Apr 2018 11:05:51 +0800, Xidong Wang > >>> <wangxidong_97@163.com> wrote: > >>> > >>>> In function tango_nand_probe(), the memory allocated by > >>>> clk_get() is not released on the normal path and > >>>> the error path that IS_ERR(nfc->chan) returns true. > >>> > >>> The fact that the error path returns true looks out of topic, can you > >>> remove it? Just saying that you fix a memory leak is enough I guess. > >>> > >>>> This will result in a memory leak bug. > >>>> > >>>> Signed-off-by: Xidong Wang <wangxidong_97@163.com> > >>>> --- > >>>> drivers/mtd/nand/tango_nand.c | 5 ++++- > >>>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > >>>> index c5bee00b..8083459 100644 > >>>> --- a/drivers/mtd/nand/tango_nand.c > >>>> +++ b/drivers/mtd/nand/tango_nand.c > >>>> @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev) > >>>> return PTR_ERR(clk); > >>>> > >>>> nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); > >>>> - if (IS_ERR(nfc->chan)) > >>>> + if (IS_ERR(nfc->chan)) { > >>>> + clk_put(clk); > >>>> return PTR_ERR(nfc->chan); > >>>> + } > >>>> > >>>> platform_set_drvdata(pdev, nfc); > >>>> nand_hw_control_init(&nfc->hw); > >>>> nfc->freq_kHz = clk_get_rate(clk) / 1000; > >>>> + clk_put(clk); > >>> > >>> If the clock is used only here, better do the frequency derivation > >>> right after the clock_get(), and follow with a clk_put()? This way you > >>> don't have to change the error path and 'related' actions remain > >>> grouped. > >> > >> Hm, definitely not a good idea to release the reference you have on the > >> clk if the driver depends on it. I recommend using devm_clk_get() to > >> solve this leak. > > > > BTW, it's also weird that the driver does not prepare_enable the clk. > > Marc, any comments? > > I was not aware that clk_get() allocated memory, and required clk_put() > for cleanup. IIRC, I looked at Documentation/clk.txt I ignored there was an actual leak too, but the 'struct clk' seems to be allocated here [1] (cascaded calls from clk_get()) and freed here [2]. [1] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L3044 [2] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L3472 > > On tango, clocks are configured by the boot loader. The existing clk driver > provides only read access to various clocks -- except the CPU clock, which > can be changed by tweaking a post-divider. Tweaking the PLLs requires much > more complex code. The boot loader enables every clock, and Linux has no > way to gate any of them. > > In the nfc driver, all I needed was the system frequency, since the NFC is > driven by the system clock (which can never be disabled). > > Thus, I wrote the naive (and apparently incorrect) > > clk = clk_get(&pdev->dev, NULL); > nfc->freq_kHz = clk_get_rate(clk) / 1000; > > > I suppose the following patch would fix the memory leak, and > matches what Miquèl suggested. Boris can you confirm: 1/ there is no need to enable the clock from this driver (from the API point of view) before the clk_get_rate()? 2/ there is no risk to do the clkd_put() right after instead of keeping it until a potential __exit? Thanks, Miquèl
On Thu, 5 Apr 2018 11:12:11 +0200 Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > On 04/04/2018 09:08, Boris Brezillon wrote: > > > On Wed, 4 Apr 2018 09:07:10 +0200 > > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > >> On Wed, 4 Apr 2018 08:28:07 +0200 > >> Miquel Raynal <miquel.raynal@bootlin.com> wrote: > >> > >>> Hi Xidong, > >>> > >>> As part of a reorganization in the NAND subsystem, you should now > >>> prefix your commit title this way: > >>> > >>> mtd: rawnand: tango: fix memory leak > >>> > >>> Not sure if this patch is candidate to cc:stable? > >>> > >>> On Wed, 4 Apr 2018 11:05:51 +0800, Xidong Wang > >>> <wangxidong_97@163.com> wrote: > >>> > >>>> In function tango_nand_probe(), the memory allocated by > >>>> clk_get() is not released on the normal path and > >>>> the error path that IS_ERR(nfc->chan) returns true. > >>> > >>> The fact that the error path returns true looks out of topic, can you > >>> remove it? Just saying that you fix a memory leak is enough I guess. > >>> > >>>> This will result in a memory leak bug. > >>>> > >>>> Signed-off-by: Xidong Wang <wangxidong_97@163.com> > >>>> --- > >>>> drivers/mtd/nand/tango_nand.c | 5 ++++- > >>>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > >>>> index c5bee00b..8083459 100644 > >>>> --- a/drivers/mtd/nand/tango_nand.c > >>>> +++ b/drivers/mtd/nand/tango_nand.c > >>>> @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev) > >>>> return PTR_ERR(clk); > >>>> > >>>> nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); > >>>> - if (IS_ERR(nfc->chan)) > >>>> + if (IS_ERR(nfc->chan)) { > >>>> + clk_put(clk); > >>>> return PTR_ERR(nfc->chan); > >>>> + } > >>>> > >>>> platform_set_drvdata(pdev, nfc); > >>>> nand_hw_control_init(&nfc->hw); > >>>> nfc->freq_kHz = clk_get_rate(clk) / 1000; > >>>> + clk_put(clk); > >>> > >>> If the clock is used only here, better do the frequency derivation > >>> right after the clock_get(), and follow with a clk_put()? This way you > >>> don't have to change the error path and 'related' actions remain > >>> grouped. > >> > >> Hm, definitely not a good idea to release the reference you have on the > >> clk if the driver depends on it. I recommend using devm_clk_get() to > >> solve this leak. > > > > BTW, it's also weird that the driver does not prepare_enable the clk. > > Marc, any comments? > > I was not aware that clk_get() allocated memory, and required clk_put() > for cleanup. IIRC, I looked at Documentation/clk.txt > > On tango, clocks are configured by the boot loader. The existing clk driver > provides only read access to various clocks -- except the CPU clock, which > can be changed by tweaking a post-divider. Tweaking the PLLs requires much > more complex code. The boot loader enables every clock, and Linux has no > way to gate any of them. Well, even if that's not supported today, it's always a good practice to retain reference and prepare/enable clks your HW depends on. This change should be harmless and when/if you someday decide to provide a way to gate clks, it will work out of the box. > > In the nfc driver, all I needed was the system frequency, since the NFC is > driven by the system clock (which can never be disabled). > > Thus, I wrote the naive (and apparently incorrect) > > clk = clk_get(&pdev->dev, NULL); > nfc->freq_kHz = clk_get_rate(clk) / 1000; > > > I suppose the following patch would fix the memory leak, and > matches what Miquèl suggested. > > Regards. > > > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > index c5bee00b7f5e..fba162af333f 100644 > --- a/drivers/mtd/nand/tango_nand.c > +++ b/drivers/mtd/nand/tango_nand.c > @@ -646,6 +646,8 @@ static int tango_nand_probe(struct platform_device *pdev) > clk = clk_get(&pdev->dev, NULL); Why not using devm_clk_get() and be done with it? > if (IS_ERR(clk)) > return PTR_ERR(clk); > + nfc->freq_kHz = clk_get_rate(clk) / 1000; > + clk_put(clk); And that's where I disagree. Clearly, you're not following one of the clk consumer's rule: "when you need a clk, keep a reference to it and enable it before you start using the HW". > > nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); > if (IS_ERR(nfc->chan)) > @@ -653,7 +655,6 @@ static int tango_nand_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, nfc); > nand_hw_control_init(&nfc->hw); > - nfc->freq_kHz = clk_get_rate(clk) / 1000; > > for_each_child_of_node(pdev->dev.of_node, np) { > err = chip_init(&pdev->dev, np);
On Thu, 5 Apr 2018 11:44:10 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Marc, > > On Thu, 5 Apr 2018 11:12:11 +0200, Marc Gonzalez > <marc.w.gonzalez@free.fr> wrote: > > > On 04/04/2018 09:08, Boris Brezillon wrote: > > > > > On Wed, 4 Apr 2018 09:07:10 +0200 > > > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > > > >> On Wed, 4 Apr 2018 08:28:07 +0200 > > >> Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > >> > > >>> Hi Xidong, > > >>> > > >>> As part of a reorganization in the NAND subsystem, you should now > > >>> prefix your commit title this way: > > >>> > > >>> mtd: rawnand: tango: fix memory leak > > >>> > > >>> Not sure if this patch is candidate to cc:stable? > > >>> > > >>> On Wed, 4 Apr 2018 11:05:51 +0800, Xidong Wang > > >>> <wangxidong_97@163.com> wrote: > > >>> > > >>>> In function tango_nand_probe(), the memory allocated by > > >>>> clk_get() is not released on the normal path and > > >>>> the error path that IS_ERR(nfc->chan) returns true. > > >>> > > >>> The fact that the error path returns true looks out of topic, can you > > >>> remove it? Just saying that you fix a memory leak is enough I guess. > > >>> > > >>>> This will result in a memory leak bug. > > >>>> > > >>>> Signed-off-by: Xidong Wang <wangxidong_97@163.com> > > >>>> --- > > >>>> drivers/mtd/nand/tango_nand.c | 5 ++++- > > >>>> 1 file changed, 4 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > > >>>> index c5bee00b..8083459 100644 > > >>>> --- a/drivers/mtd/nand/tango_nand.c > > >>>> +++ b/drivers/mtd/nand/tango_nand.c > > >>>> @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev) > > >>>> return PTR_ERR(clk); > > >>>> > > >>>> nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); > > >>>> - if (IS_ERR(nfc->chan)) > > >>>> + if (IS_ERR(nfc->chan)) { > > >>>> + clk_put(clk); > > >>>> return PTR_ERR(nfc->chan); > > >>>> + } > > >>>> > > >>>> platform_set_drvdata(pdev, nfc); > > >>>> nand_hw_control_init(&nfc->hw); > > >>>> nfc->freq_kHz = clk_get_rate(clk) / 1000; > > >>>> + clk_put(clk); > > >>> > > >>> If the clock is used only here, better do the frequency derivation > > >>> right after the clock_get(), and follow with a clk_put()? This way you > > >>> don't have to change the error path and 'related' actions remain > > >>> grouped. > > >> > > >> Hm, definitely not a good idea to release the reference you have on the > > >> clk if the driver depends on it. I recommend using devm_clk_get() to > > >> solve this leak. > > > > > > BTW, it's also weird that the driver does not prepare_enable the clk. > > > Marc, any comments? > > > > I was not aware that clk_get() allocated memory, and required clk_put() > > for cleanup. IIRC, I looked at Documentation/clk.txt > > I ignored there was an actual leak too, but the 'struct clk' seems to > be allocated here [1] (cascaded calls from clk_get()) and freed here > [2]. > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L3044 > [2] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L3472 > > > > > On tango, clocks are configured by the boot loader. The existing clk driver > > provides only read access to various clocks -- except the CPU clock, which > > can be changed by tweaking a post-divider. Tweaking the PLLs requires much > > more complex code. The boot loader enables every clock, and Linux has no > > way to gate any of them. > > > > In the nfc driver, all I needed was the system frequency, since the NFC is > > driven by the system clock (which can never be disabled). > > > > Thus, I wrote the naive (and apparently incorrect) > > > > clk = clk_get(&pdev->dev, NULL); > > nfc->freq_kHz = clk_get_rate(clk) / 1000; > > > > > > I suppose the following patch would fix the memory leak, and > > matches what Miquèl suggested. > > Boris can you confirm: > 1/ there is no need to enable the clock from this driver (from the API > point of view) before the clk_get_rate()? It's not strictly required, but I'd recommend doing it. Not necessarily before enabling the clk though. > 2/ there is no risk to do the clkd_put() right after instead of keeping > it until a potential __exit? It's not a good idea to do that, especially since devm_clk_get() can release the clk for you when the device is destroyed.
On 05/04/2018 11:54, Boris Brezillon wrote: > On Thu, 5 Apr 2018 11:12:11 +0200, Marc Gonzalez wrote: > >> I was not aware that clk_get() allocated memory, and required clk_put() >> for cleanup. IIRC, I looked at Documentation/clk.txt >> >> On tango, clocks are configured by the boot loader. The existing clk driver >> provides only read access to various clocks -- except the CPU clock, which >> can be changed by tweaking a post-divider. Tweaking the PLLs requires much >> more complex code. The boot loader enables every clock, and Linux has no >> way to gate any of them. > > Well, even if that's not supported today, it's always a good practice > to retain reference and prepare/enable clks your HW depends on. This > change should be harmless and when/if you someday decide to provide a > way to gate clks, it will work out of the box. IIUC, you're saying: 1) use devm_clk_get() instead of clk_get() to solve the memory leak 2) call clk_prepare_enable() before clk_get_rate() even if the former is a no-op today The following patch implements these suggestions. (Only compile-tested) diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c index c5bee00b7f5e..39d190b7521f 100644 --- a/drivers/mtd/nand/tango_nand.c +++ b/drivers/mtd/nand/tango_nand.c @@ -643,7 +643,7 @@ static int tango_nand_probe(struct platform_device *pdev) writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE); - clk = clk_get(&pdev->dev, NULL); + clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(clk)) return PTR_ERR(clk); @@ -651,6 +651,10 @@ static int tango_nand_probe(struct platform_device *pdev) if (IS_ERR(nfc->chan)) return PTR_ERR(nfc->chan); + err = clk_prepare_enable(clk); + if (err) + return err; + platform_set_drvdata(pdev, nfc); nand_hw_control_init(&nfc->hw); nfc->freq_kHz = clk_get_rate(clk) / 1000;
On Thu, 5 Apr 2018 13:26:31 +0200 Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > On 05/04/2018 11:54, Boris Brezillon wrote: > > > On Thu, 5 Apr 2018 11:12:11 +0200, Marc Gonzalez wrote: > > > >> I was not aware that clk_get() allocated memory, and required clk_put() > >> for cleanup. IIRC, I looked at Documentation/clk.txt > >> > >> On tango, clocks are configured by the boot loader. The existing clk driver > >> provides only read access to various clocks -- except the CPU clock, which > >> can be changed by tweaking a post-divider. Tweaking the PLLs requires much > >> more complex code. The boot loader enables every clock, and Linux has no > >> way to gate any of them. > > > > Well, even if that's not supported today, it's always a good practice > > to retain reference and prepare/enable clks your HW depends on. This > > change should be harmless and when/if you someday decide to provide a > > way to gate clks, it will work out of the box. > > IIUC, you're saying: > 1) use devm_clk_get() instead of clk_get() to solve the memory leak > 2) call clk_prepare_enable() before clk_get_rate() even if the former is a no-op today + a disable_unprepare() in the remove path and a another one in the error path (in case something fails after prepare_enable() has been called). > > The following patch implements these suggestions. > (Only compile-tested) > > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > index c5bee00b7f5e..39d190b7521f 100644 > --- a/drivers/mtd/nand/tango_nand.c > +++ b/drivers/mtd/nand/tango_nand.c > @@ -643,7 +643,7 @@ static int tango_nand_probe(struct platform_device *pdev) > > writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE); > > - clk = clk_get(&pdev->dev, NULL); > + clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(clk)) > return PTR_ERR(clk); > > @@ -651,6 +651,10 @@ static int tango_nand_probe(struct platform_device *pdev) > if (IS_ERR(nfc->chan)) > return PTR_ERR(nfc->chan); > > + err = clk_prepare_enable(clk); > + if (err) > + return err; > + > platform_set_drvdata(pdev, nfc); > nand_hw_control_init(&nfc->hw); > nfc->freq_kHz = clk_get_rate(clk) / 1000;
On 05/04/2018 13:47, Boris Brezillon wrote: > On Thu, 5 Apr 2018 13:26:31 +0200, Marc Gonzalez wrote: > >> IIUC, you're saying: >> 1) use devm_clk_get() instead of clk_get() to solve the memory leak >> 2) call clk_prepare_enable() before clk_get_rate() even if the former is a no-op today > > + a disable_unprepare() in the remove path and a another one in the > error path (in case something fails after prepare_enable() has been > called). I hate C error-handling with all my heart... Why is it taking so long for Linux APIs to be turned into devm managed functions? It makes life so much easier...
diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c index c5bee00b..8083459 100644 --- a/drivers/mtd/nand/tango_nand.c +++ b/drivers/mtd/nand/tango_nand.c @@ -648,12 +648,15 @@ static int tango_nand_probe(struct platform_device *pdev) return PTR_ERR(clk); nfc->chan = dma_request_chan(&pdev->dev, "rxtx"); - if (IS_ERR(nfc->chan)) + if (IS_ERR(nfc->chan)) { + clk_put(clk); return PTR_ERR(nfc->chan); + } platform_set_drvdata(pdev, nfc); nand_hw_control_init(&nfc->hw); nfc->freq_kHz = clk_get_rate(clk) / 1000; + clk_put(clk); for_each_child_of_node(pdev->dev.of_node, np) { err = chip_init(&pdev->dev, np);
In function tango_nand_probe(), the memory allocated by clk_get() is not released on the normal path and the error path that IS_ERR(nfc->chan) returns true. This will result in a memory leak bug. Signed-off-by: Xidong Wang <wangxidong_97@163.com> --- drivers/mtd/nand/tango_nand.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)