* [PATCH 1/1] mtd:nand:fix memory leak @ 2018-04-04 3:05 ` Xidong Wang 0 siblings, 0 replies; 15+ messages in thread From: Xidong Wang @ 2018-04-04 3:05 UTC (permalink / raw) To: Boris Brezillon, Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, Marc Gonzalez, Mans Rullgard Cc: Xidong Wang, linux-mtd, linux-arm-kernel, linux-kernel 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(-) 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 1/1] mtd:nand:fix memory leak @ 2018-04-04 3:05 ` Xidong Wang 0 siblings, 0 replies; 15+ messages in thread From: Xidong Wang @ 2018-04-04 3:05 UTC (permalink / raw) To: linux-arm-kernel 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(-) 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-04 3:05 ` Xidong Wang (?) @ 2018-04-04 6:28 ` Miquel Raynal 2018-04-04 7:07 ` Boris Brezillon -1 siblings, 1 reply; 15+ messages in thread From: Miquel Raynal @ 2018-04-04 6:28 UTC (permalink / raw) To: Xidong Wang Cc: Boris Brezillon, Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, Marc Gonzalez, Mans Rullgard, linux-mtd 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 -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-04 6:28 ` Miquel Raynal @ 2018-04-04 7:07 ` Boris Brezillon 2018-04-04 7:08 ` Boris Brezillon 2018-04-05 12:57 ` [PATCH v2] mtd: rawnand: tango: Fix struct clk " Marc Gonzalez 0 siblings, 2 replies; 15+ messages in thread From: Boris Brezillon @ 2018-04-04 7:07 UTC (permalink / raw) To: Miquel Raynal Cc: Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Marc Gonzalez, linux-mtd, Cyrille Pitchen, Brian Norris, David Woodhouse 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-04 7:07 ` Boris Brezillon @ 2018-04-04 7:08 ` Boris Brezillon 2018-04-05 9:12 ` Marc Gonzalez 2018-04-05 12:57 ` [PATCH v2] mtd: rawnand: tango: Fix struct clk " Marc Gonzalez 1 sibling, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-04-04 7:08 UTC (permalink / raw) To: Miquel Raynal, Marc Gonzalez Cc: Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, linux-mtd, Cyrille Pitchen, Brian Norris, David Woodhouse 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-04 7:08 ` Boris Brezillon @ 2018-04-05 9:12 ` Marc Gonzalez 2018-04-05 9:44 ` Miquel Raynal 2018-04-05 9:54 ` Boris Brezillon 0 siblings, 2 replies; 15+ messages in thread From: Marc Gonzalez @ 2018-04-05 9:12 UTC (permalink / raw) To: Boris Brezillon, Miquel Raynal Cc: Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, linux-mtd 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); ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-05 9:12 ` Marc Gonzalez @ 2018-04-05 9:44 ` Miquel Raynal 2018-04-05 11:04 ` Boris Brezillon 2018-04-05 9:54 ` Boris Brezillon 1 sibling, 1 reply; 15+ messages in thread From: Miquel Raynal @ 2018-04-05 9:44 UTC (permalink / raw) To: Marc Gonzalez Cc: Boris Brezillon, Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, linux-mtd 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 -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-05 9:44 ` Miquel Raynal @ 2018-04-05 11:04 ` Boris Brezillon 0 siblings, 0 replies; 15+ messages in thread From: Boris Brezillon @ 2018-04-05 11:04 UTC (permalink / raw) To: Miquel Raynal Cc: Marc Gonzalez, Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, linux-mtd 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-05 9:12 ` Marc Gonzalez 2018-04-05 9:44 ` Miquel Raynal @ 2018-04-05 9:54 ` Boris Brezillon 2018-04-05 11:26 ` Marc Gonzalez 1 sibling, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-04-05 9:54 UTC (permalink / raw) To: Marc Gonzalez Cc: Miquel Raynal, Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, linux-mtd 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); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-05 9:54 ` Boris Brezillon @ 2018-04-05 11:26 ` Marc Gonzalez 2018-04-05 11:47 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: Marc Gonzalez @ 2018-04-05 11:26 UTC (permalink / raw) To: Boris Brezillon, Miquel Raynal Cc: Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, linux-mtd 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; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-05 11:26 ` Marc Gonzalez @ 2018-04-05 11:47 ` Boris Brezillon 2018-04-05 12:00 ` Marc Gonzalez 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-04-05 11:47 UTC (permalink / raw) To: Marc Gonzalez Cc: Miquel Raynal, Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, linux-mtd 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; ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] mtd:nand:fix memory leak 2018-04-05 11:47 ` Boris Brezillon @ 2018-04-05 12:00 ` Marc Gonzalez 0 siblings, 0 replies; 15+ messages in thread From: Marc Gonzalez @ 2018-04-05 12:00 UTC (permalink / raw) To: Boris Brezillon Cc: Miquel Raynal, Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, linux-mtd 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... ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] mtd: rawnand: tango: Fix struct clk memory leak 2018-04-04 7:07 ` Boris Brezillon 2018-04-04 7:08 ` Boris Brezillon @ 2018-04-05 12:57 ` Marc Gonzalez 2018-04-05 14:22 ` Miquel Raynal 2018-04-24 15:50 ` Boris Brezillon 1 sibling, 2 replies; 15+ messages in thread From: Marc Gonzalez @ 2018-04-05 12:57 UTC (permalink / raw) To: Boris Brezillon, Miquel Raynal Cc: Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, Marc Gonzalez, linux-mtd Use devm_clk_get() to let Linux manage struct clk memory. Fixes: 6956e2385a16 ("add tango NAND flash controller support") Reported-by: Xidong Wang <wangxidong_97@163.com> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> --- Missing clk_prepare_enable() would be handled in a separate patch. --- drivers/mtd/nand/tango_nand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c index c5bee00b7f5e..76761b841f1f 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); -- 2.16.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] mtd: rawnand: tango: Fix struct clk memory leak 2018-04-05 12:57 ` [PATCH v2] mtd: rawnand: tango: Fix struct clk " Marc Gonzalez @ 2018-04-05 14:22 ` Miquel Raynal 2018-04-24 15:50 ` Boris Brezillon 1 sibling, 0 replies; 15+ messages in thread From: Miquel Raynal @ 2018-04-05 14:22 UTC (permalink / raw) To: Marc Gonzalez Cc: Boris Brezillon, Xidong Wang, Mans Rullgard, Marek Vasut, Richard Weinberger, Cyrille Pitchen, Brian Norris, David Woodhouse, Marc Gonzalez, linux-mtd Hi Marc, On Thu, 5 Apr 2018 14:57:59 +0200, Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote: > Use devm_clk_get() to let Linux manage struct clk memory. > > Fixes: 6956e2385a16 ("add tango NAND flash controller support") > Reported-by: Xidong Wang <wangxidong_97@163.com> > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> > --- > Missing clk_prepare_enable() would be handled in a separate patch. > --- > drivers/mtd/nand/tango_nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > index c5bee00b7f5e..76761b841f1f 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); > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] mtd: rawnand: tango: Fix struct clk memory leak 2018-04-05 12:57 ` [PATCH v2] mtd: rawnand: tango: Fix struct clk " Marc Gonzalez 2018-04-05 14:22 ` Miquel Raynal @ 2018-04-24 15:50 ` Boris Brezillon 1 sibling, 0 replies; 15+ messages in thread From: Boris Brezillon @ 2018-04-24 15:50 UTC (permalink / raw) To: Marc Gonzalez Cc: Miquel Raynal, Mans Rullgard, Marek Vasut, Richard Weinberger, Marc Gonzalez, Xidong Wang, linux-mtd, Cyrille Pitchen, Brian Norris, David Woodhouse On Thu, 5 Apr 2018 14:57:59 +0200 Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote: > Use devm_clk_get() to let Linux manage struct clk memory. > > Fixes: 6956e2385a16 ("add tango NAND flash controller support") > Reported-by: Xidong Wang <wangxidong_97@163.com> > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> Applied. Thanks, Boris > --- > Missing clk_prepare_enable() would be handled in a separate patch. > --- > drivers/mtd/nand/tango_nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > index c5bee00b7f5e..76761b841f1f 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); > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-04-24 15:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-04 3:05 [PATCH 1/1] mtd:nand:fix memory leak Xidong Wang 2018-04-04 3:05 ` Xidong Wang 2018-04-04 6:28 ` Miquel Raynal 2018-04-04 7:07 ` Boris Brezillon 2018-04-04 7:08 ` Boris Brezillon 2018-04-05 9:12 ` Marc Gonzalez 2018-04-05 9:44 ` Miquel Raynal 2018-04-05 11:04 ` Boris Brezillon 2018-04-05 9:54 ` Boris Brezillon 2018-04-05 11:26 ` Marc Gonzalez 2018-04-05 11:47 ` Boris Brezillon 2018-04-05 12:00 ` Marc Gonzalez 2018-04-05 12:57 ` [PATCH v2] mtd: rawnand: tango: Fix struct clk " Marc Gonzalez 2018-04-05 14:22 ` Miquel Raynal 2018-04-24 15:50 ` Boris Brezillon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.