dmaengine Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails
@ 2020-06-03 18:52 Markus Elfring
  2020-06-03 19:17 ` Navid Emamdoost
  2020-06-03 22:01 ` Geert Uytterhoeven
  0 siblings, 2 replies; 9+ messages in thread
From: Markus Elfring @ 2020-06-03 18:52 UTC (permalink / raw)
  To: Navid Emamdoost, dmaengine, linux-arm-kernel, linux-stm32
  Cc: Alexandre Torgue, Dan Williams, Navid Emamdoost, Kangjie Lu,
	Stephen McCamant, Qiushi Wu, Vinod Koul, Maxime Coquelin,
	linux-kernel, kernel-janitors

> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails
  2020-06-03 18:52 [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails Markus Elfring
@ 2020-06-03 19:17 ` Navid Emamdoost
  2020-06-03 19:40   ` [PATCH v2] dmaengine: stm32-dma: " Markus Elfring
                     ` (2 more replies)
  2020-06-03 22:01 ` Geert Uytterhoeven
  1 sibling, 3 replies; 9+ messages in thread
From: Navid Emamdoost @ 2020-06-03 19:17 UTC (permalink / raw)
  To: Markus Elfring
  Cc: dmaengine, linux-arm-kernel, linux-stm32, Alexandre Torgue,
	Dan Williams, Navid Emamdoost, Kangjie Lu, Stephen McCamant,
	Qiushi Wu, Vinod Koul, Maxime Coquelin, LKML, kernel-janitors

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] dmaengine: stm32-dma: call pm_runtime_put if pm_runtime_get_sync fails
  2020-06-03 19:17 ` Navid Emamdoost
@ 2020-06-03 19:40   ` Markus Elfring
  2020-06-04  9:36   ` [PATCH] dmaengine: stm32-dmamux: Fix pm_runtime_get_sync() failure cases Markus Elfring
  2020-06-17 13:59   ` [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails Vinod Koul
  2 siblings, 0 replies; 9+ messages in thread
From: Markus Elfring @ 2020-06-03 19:40 UTC (permalink / raw)
  To: Navid Emamdoost, dmaengine, linux-arm-kernel, linux-stm32
  Cc: Alexandre Torgue, Dan Williams, Navid Emamdoost, Kangjie Lu,
	Stephen McCamant, Qiushi Wu, Vinod Koul, Maxime Coquelin, LKML,
	kernel-janitors

>> How do you think about a wording variant like the following?
> Please stop proposing rewording on my patches!

I am trying to remind you on open issues according to patch review concerns.


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

* I hope that more contributors would like to improve the software quality
  also for commit messages.

* Would the adjusted patch prefix need a different version indication?

Regards,
Markus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails
  2020-06-03 18:52 [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails Markus Elfring
  2020-06-03 19:17 ` Navid Emamdoost
@ 2020-06-03 22:01 ` Geert Uytterhoeven
  2020-06-04  5:43   ` Markus Elfring
  1 sibling, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-06-03 22:01 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Navid Emamdoost, dmaengine, Linux ARM, linux-stm32,
	Alexandre Torgue, Dan Williams, Navid Emamdoost, Kangjie Lu,
	Stephen McCamant, Qiushi Wu, Vinod Koul, Maxime Coquelin,
	Linux Kernel Mailing List, kernel-janitors

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails
  2020-06-03 22:01 ` Geert Uytterhoeven
@ 2020-06-04  5:43   ` Markus Elfring
  0 siblings, 0 replies; 9+ messages in thread
From: Markus Elfring @ 2020-06-04  5:43 UTC (permalink / raw)
  To: Geert Uytterhoeven, Navid Emamdoost, dmaengine, linux-arm-kernel,
	linux-stm32
  Cc: Alexandre Torgue, Dan Williams, Navid Emamdoost, Kangjie Lu,
	Stephen McCamant, Qiushi Wu, Vinod Koul, Maxime Coquelin,
	linux-kernel, kernel-janitors

>>> 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.

Would you like to comment any more of the presented patch review concerns?

Can it make sense to combine any adjustments into a single patch
according to the discussed software transformation pattern?
https://lore.kernel.org/patchwork/project/lkml/list/?submitter=26544&state=*&q=engine%3A+stm32&archive=both

Regards,
Markus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dmaengine: stm32-dmamux: Fix pm_runtime_get_sync() failure cases
  2020-06-03 19:17 ` Navid Emamdoost
  2020-06-03 19:40   ` [PATCH v2] dmaengine: stm32-dma: " Markus Elfring
@ 2020-06-04  9:36   ` Markus Elfring
  2020-06-17 13:59   ` [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails Vinod Koul
  2 siblings, 0 replies; 9+ messages in thread
From: Markus Elfring @ 2020-06-04  9:36 UTC (permalink / raw)
  To: Navid Emamdoost, dmaengine, linux-arm-kernel, linux-stm32
  Cc: Navid Emamdoost, Kangjie Lu, Stephen McCamant, Qiushi Wu,
	Alexandre Torgue, Dan Williams, Maxime Coquelin, Vinod Koul,
	LKML, kernel-janitors

> Please stop proposing rewording on my patches!

I find it interesting that you got into the mood to choose
another patch subject variant.
https://lore.kernel.org/patchwork/patch/1252131/
https://lore.kernel.org/dmaengine/20200603193648.19190-1-navid.emamdoost@gmail.com/


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

Now I am curious if you would like take my patch review request into account
to avoid a typo there.
How will the quality evolve for such commit messages?

Regards,
Markus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails
  2020-06-03 19:17 ` Navid Emamdoost
  2020-06-03 19:40   ` [PATCH v2] dmaengine: stm32-dma: " Markus Elfring
  2020-06-04  9:36   ` [PATCH] dmaengine: stm32-dmamux: Fix pm_runtime_get_sync() failure cases Markus Elfring
@ 2020-06-17 13:59   ` Vinod Koul
  2 siblings, 0 replies; 9+ messages in thread
From: Vinod Koul @ 2020-06-17 13:59 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Markus Elfring, dmaengine, linux-arm-kernel, linux-stm32,
	Alexandre Torgue, Dan Williams, Navid Emamdoost, Kangjie Lu,
	Stephen McCamant, Qiushi Wu, Maxime Coquelin, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails
  2020-06-03 18:28 [PATCH] " Navid Emamdoost
@ 2020-06-24  7:46 ` Vinod Koul
  0 siblings, 0 replies; 9+ messages in thread
From: Vinod Koul @ 2020-06-24  7:46 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Dan Williams, Maxime Coquelin, Alexandre Torgue, dmaengine,
	linux-stm32, linux-arm-kernel, linux-kernel, emamd001, wu000273,
	kjlu, smccaman

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails
@ 2020-06-03 18:28 Navid Emamdoost
  2020-06-24  7:46 ` Vinod Koul
  0 siblings, 1 reply; 9+ messages in thread
From: Navid Emamdoost @ 2020-06-03 18:28 UTC (permalink / raw)
  To: Vinod Koul, Dan Williams, Maxime Coquelin, Alexandre Torgue,
	dmaengine, linux-stm32, linux-arm-kernel, linux-kernel
  Cc: emamd001, wu000273, kjlu, smccaman, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 18:52 [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails Markus Elfring
2020-06-03 19:17 ` Navid Emamdoost
2020-06-03 19:40   ` [PATCH v2] dmaengine: stm32-dma: " Markus Elfring
2020-06-04  9:36   ` [PATCH] dmaengine: stm32-dmamux: Fix pm_runtime_get_sync() failure cases Markus Elfring
2020-06-17 13:59   ` [PATCH] dmaengine: stm32-mdma: call pm_runtime_put if pm_runtime_get_sync fails Vinod Koul
2020-06-03 22:01 ` Geert Uytterhoeven
2020-06-04  5:43   ` Markus Elfring
  -- strict thread matches above, loose matches on Subject: below --
2020-06-03 18:28 [PATCH] " Navid Emamdoost
2020-06-24  7:46 ` Vinod Koul

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git