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 <[email protected]>
---
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
> 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
On Wed, Jun 3, 2020 at 1:52 PM Markus Elfring <[email protected]> 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.
>> 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
Hi Markus,
Thanks for your comment!
On Wed, Jun 3, 2020 at 8:53 PM Markus Elfring <[email protected]> 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 -- [email protected]
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
>>> 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
> 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/[email protected]/
> 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
On 03-06-20, 14:17, Navid Emamdoost wrote:
> On Wed, Jun 3, 2020 at 1:52 PM Markus Elfring <[email protected]> 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
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 <[email protected]>
> ---
> 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