2020-06-02 04:42:00

by Navid Emamdoost

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

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 <[email protected]>
---
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


2020-06-02 08:43:35

by Markus Elfring

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

> 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

2020-06-02 09:52:28

by Mark Brown

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

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.


Attachments:
(No filename) (638.00 B)
signature.asc (499.00 B)
Download all attachments

2020-06-02 09:56:27

by Mark Brown

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

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.


Attachments:
(No filename) (449.00 B)
signature.asc (499.00 B)
Download all attachments

2020-06-02 10:05:44

by Markus Elfring

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

> 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

2020-06-02 14:15:53

by Mark Brown

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

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.


Attachments:
(No filename) (527.00 B)
signature.asc (499.00 B)
Download all attachments

2020-06-02 15:10:27

by Markus Elfring

[permalink] [raw]
Subject: Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

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

2020-06-02 18:39:01

by Mark Brown

[permalink] [raw]
Subject: Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

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.


Attachments:
(No filename) (785.00 B)
signature.asc (499.00 B)
Download all attachments

2020-06-02 19:41:08

by Navid Emamdoost

[permalink] [raw]
Subject: Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

On Tue, Jun 2, 2020 at 1:36 PM Mark Brown <[email protected]> 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.

2020-06-02 19:58:23

by Markus Elfring

[permalink] [raw]
Subject: Re: spi: spi-ti-qspi: call pm_runtime_put on pm_runtime_get failure

> 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/[email protected]/
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/[email protected]/


> 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