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

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 v2] dmaengine: stm32-dma: call pm_runtime_put if pm_runtime_get_sync fails

[Markus Elfring]

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

Re: [PATCH] dmaengine: stm32-dmamux: Fix pm_runtime_get_sync() failure cases

[Markus Elfring]

> 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

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

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