Re: [PATCH] spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

Re: [PATCH] spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Markus Elfring]

> The counter is incremented via pm_runtime_get even in failure case.
> To correct the counter call pm_runtime_put in case of failure, too.

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

   Change description:
   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_autosuspend” also in one error case
   to keep the reference counting consistent.


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

Regards,
Markus

Re: [PATCH] spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Tue, Jun 02, 2020 at 10:40:33AM +0200, Markus Elfring wrote:
> > The counter is incremented via pm_runtime_get even in failure case.
> > To correct the counter call pm_runtime_put in case of failure, too.
> 
> How do you think about a wording variant like the following?
> 
>    Change description:
>    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_autosuspend” also in one error case
>    to keep the reference counting consistent.

The original changelog is perfectly fine, please stop sending these.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Markus Elfring]

> The original changelog is perfectly fine, please stop sending these.

I find this commit message improvable also according to Linux software
development documentation.

Regards,
Markus

Re: [PATCH] spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Tue, Jun 02, 2020 at 12:02:11PM +0200, Markus Elfring wrote:

> > The original changelog is perfectly fine, please stop sending these.

> I find this commit message improvable also according to Linux software
> development documentation.

Causing people to send out new versions of things for tweaks to the
commit log consumes time for them and everyone they're sending changes
to.  Pushing people to make trivial rewordings of their commit logs to
match your particular taste is not a good use of people's time.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Markus Elfring]

>> I find this commit message improvable also according to Linux software
>> development documentation.
>
> Causing people to send out new versions of things for tweaks to the
> commit log consumes time for them and everyone they're sending changes to.

Improving patches (besides source code adjustments) is an usual software
development activity, isn't it?


> Pushing people to make trivial rewordings of their commit logs to
> match your particular taste is not a good use of people's time.

Corresponding tweaks can be combined with recommended tags.
It can be that only “trivial” items were left over for another bit
of fine-tuning. Subsequent description variants can reduce
the probability for additional patch review iterations, can't they?

Regards,
Markus

Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

On Tue, Jun 02, 2020 at 05:05:18PM +0200, Markus Elfring wrote:
> >> I find this commit message improvable also according to Linux software
> >> development documentation.

> > Causing people to send out new versions of things for tweaks to the
> > commit log consumes time for them and everyone they're sending changes to.

> Improving patches (besides source code adjustments) is an usual software
> development activity, isn't it?

Your updates were not improvements.  The formatting was worse and to my
native speaker eyes the grammar was worse.  With this sort of stylistic
thing it's especially important that any review aligns with the needs
and practices of the subsystem, there is opinion in there and multiple
opinions just makes things harder for submitters.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Navid Emamdoost]

On Tue, Jun 2, 2020 at 1:36 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 02, 2020 at 05:05:18PM +0200, Markus Elfring wrote:
> > >> I find this commit message improvable also according to Linux software
> > >> development documentation.
>
> > > Causing people to send out new versions of things for tweaks to the
> > > commit log consumes time for them and everyone they're sending changes to.
>
> > Improving patches (besides source code adjustments) is an usual software
> > development activity, isn't it?
>
> Your updates were not improvements.  The formatting was worse and to my
> native speaker eyes the grammar was worse.  With this sort of stylistic
> thing it's especially important that any review aligns with the needs
> and practices of the subsystem, there is opinion in there and multiple
> opinions just makes things harder for submitters.

Thanks Mark for your constructive opinion,
In most cases, such stylistic comments become confusing and
discouraging to those who are trying to chip in. Personally I think as
long as the patch does not contain typo and is not ambiguous from the
maintainer's perspective, it should be fine to let it go forward.



-- 
Navid.

Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Markus Elfring]

> Your updates were not improvements.

I find your view interesting.

Do you refer to a specific wording suggestion here?
https://lore.kernel.org/linux-spi/26028f50-3fb8-eb08-3c9f-08ada018bf9e@web.de/
https://lkml.org/lkml/2020/6/2/210

You pointed another programming alternative out.
https://lore.kernel.org/patchwork/comment/1447149/
https://lore.kernel.org/linux-spi/20200602095411.GB5684@sirena.org.uk/


> The formatting was worse

Do you prefer an other quotation style for function names?


> and to my native speaker eyes the grammar was worse.

I am curious if a more pleasing wording variant will be found.


> With this sort of stylistic thing it's especially important
> that any review aligns with the needs and practices of the subsystem,

Such an expectation is reasonable to some degree.


> there is opinion in there and multiple opinions just makes things harder
> for submitters.

Do any of such views deviate from the Linux development documentation?

Regards,
Markus

[PATCH] spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Navid Emamdoost]

The counter is incremented via pm_runtime_get even in failure case.
To correct the counter call pm_runtime_put in case of failure, too.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/spi/spi-ti-qspi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 366a3e5cca6b..dfb811c5ef76 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -174,6 +174,7 @@ static int ti_qspi_setup(struct spi_device *spi)
 
 	ret = pm_runtime_get_sync(qspi->dev);
 	if (ret < 0) {
+		pm_runtime_put_autosuspend(qspi->dev);
 		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
 		return ret;
 	}
-- 
2.17.1

Re: [PATCH] spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On Mon, Jun 01, 2020 at 11:36:37PM -0500, Navid Emamdoost wrote:
> The counter is incremented via pm_runtime_get even in failure case.
> To correct the counter call pm_runtime_put in case of failure, too.

Someone already sent a fix for this but in any case this isn't the
correct fix - pm_runtime_put() will also undo the pm_runtime_resume()
that pm_runtime_get() failed to do which is wrong.  pm_runtime_idle() is
what you're looking for.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]