[PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails

[PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails

[Navid Emamdoost]

Calling pm_runtime_get_sync increments the counter even in case of
failure, causing incorrect ref count. Call pm_runtime_put if
pm_runtime_get_sync fails.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/dma/stm32-mdma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/stm32-mdma.c b/drivers/dma/stm32-mdma.c
index 5469563703d1..79bee1bb73f6 100644
--- a/drivers/dma/stm32-mdma.c
+++ b/drivers/dma/stm32-mdma.c
@@ -1449,8 +1449,10 @@ static int stm32_mdma_alloc_chan_resources(struct dma_chan *c)
 	}
 
 	ret = pm_runtime_get_sync(dmadev->ddev.dev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put(dmadev->ddev.dev);
 		return ret;
+	}
 
 	ret = stm32_mdma_disable_chan(chan);
 	if (ret < 0)
@@ -1718,8 +1720,10 @@ static int stm32_mdma_pm_suspend(struct device *dev)
 	int ret;
 
 	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_sync(dev);
 		return ret;
+	}
 
 	for (id = 0; id < dmadev->nr_channels; id++) {
 		ccr = stm32_mdma_read(dmadev, STM32_MDMA_CCR(id));
-- 
2.17.1

Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails

[Vinod Koul]

On 03-06-20, 13:28, Navid Emamdoost wrote:
> Calling pm_runtime_get_sync increments the counter even in case of
> failure, causing incorrect ref count. Call pm_runtime_put if
> pm_runtime_get_sync fails.
> 
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
>  drivers/dma/stm32-mdma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/stm32-mdma.c b/drivers/dma/stm32-mdma.c
> index 5469563703d1..79bee1bb73f6 100644
> --- a/drivers/dma/stm32-mdma.c
> +++ b/drivers/dma/stm32-mdma.c
> @@ -1449,8 +1449,10 @@ static int stm32_mdma_alloc_chan_resources(struct dma_chan *c)
>  	}
>  
>  	ret = pm_runtime_get_sync(dmadev->ddev.dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put(dmadev->ddev.dev);
>  		return ret;
> +	}
>  
>  	ret = stm32_mdma_disable_chan(chan);
>  	if (ret < 0)
> @@ -1718,8 +1720,10 @@ static int stm32_mdma_pm_suspend(struct device *dev)
>  	int ret;
>  
>  	ret = pm_runtime_get_sync(dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put_sync(dev);

Not put_sync()...

>  		return ret;
> +	}
>  
>  	for (id = 0; id < dmadev->nr_channels; id++) {
>  		ccr = stm32_mdma_read(dmadev, STM32_MDMA_CCR(id));
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails

[Markus Elfring]

> Calling pm_runtime_get_sync increments the counter even in case of
> failure, causing incorrect ref count. Call pm_runtime_put if
> pm_runtime_get_sync fails.

Is it appropriate to copy a sentence from the change description
into the patch subject?

How do you think about a wording variant like the following?

   The PM runtime reference counter is generally incremented by a call of
   the function “pm_runtime_get_sync”.
   Thus call the function “pm_runtime_put” also in two error cases
   to keep the reference counting consistent.


Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails

[Navid Emamdoost]

On Wed, Jun 3, 2020 at 1:52 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Calling pm_runtime_get_sync increments the counter even in case of
> > failure, causing incorrect ref count. Call pm_runtime_put if
> > pm_runtime_get_sync fails.
>
> Is it appropriate to copy a sentence from the change description
> into the patch subject?
>
> How do you think about a wording variant like the following?
Please stop proposing rewording on my patches!

I will consider updating my patches only if a maintainer asks for it.

>
>    The PM runtime reference counter is generally incremented by a call of
>    the function “pm_runtime_get_sync”.
>    Thus call the function “pm_runtime_put” also in two error cases
>    to keep the reference counting consistent.
>
>
> Would you like to add the tag “Fixes” to the commit message?
>
> Regards,
> Markus



-- 
Navid.

Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails

[Vinod Koul]

On 03-06-20, 14:17, Navid Emamdoost wrote:
> On Wed, Jun 3, 2020 at 1:52 PM Markus Elfring <Markus.Elfring@web.de> wrote:
> >
> > > Calling pm_runtime_get_sync increments the counter even in case of
> > > failure, causing incorrect ref count. Call pm_runtime_put if
> > > pm_runtime_get_sync fails.
> >
> > Is it appropriate to copy a sentence from the change description
> > into the patch subject?
> >
> > How do you think about a wording variant like the following?
> Please stop proposing rewording on my patches!
> 
> I will consider updating my patches only if a maintainer asks for it.

Yeah ignore these :) no one takes this 'bot' seriously, it is annoying
yes :(

> >
> >    The PM runtime reference counter is generally incremented by a call of
> >    the function “pm_runtime_get_sync”.
> >    Thus call the function “pm_runtime_put” also in two error cases
> >    to keep the reference counting consistent.
> >
> >
> > Would you like to add the tag “Fixes” to the commit message?
> >
> > Regards,
> > Markus
> 
> 
> 
> -- 
> Navid.

-- 
~Vinod

Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails

[Geert Uytterhoeven]

Hi Markus,

Thanks for your comment!

On Wed, Jun 3, 2020 at 8:53 PM Markus Elfring <Markus.Elfring@web.de> wrote:
> > Calling pm_runtime_get_sync increments the counter even in case of
> > failure, causing incorrect ref count. Call pm_runtime_put if
> > pm_runtime_get_sync fails.
>
> Is it appropriate to copy a sentence from the change description
> into the patch subject?
>
> How do you think about a wording variant like the following?
>
>    The PM runtime reference counter is generally incremented by a call of
>    the function “pm_runtime_get_sync”.
>    Thus call the function “pm_runtime_put” also in two error cases
>    to keep the reference counting consistent.

IMHO the important part is "even in case of failure", which you dropped.
Missing that point was the root cause of the issue being fixed.
Hence I prefer the original description, FWIW.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds