[Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
pm_runtime_get_sync keeps a reference count on failure, which can lead
to leaks. pm_runtime_resume_and_get drops the reference count in the
failure case. This rule very conservatively follows the definition of
pm_runtime_resume_and_get to address the cases where the reference
count is unlikely to be needed in the failure case. Specifically, the
change is only done when pm_runtime_get_sync is followed immediately
by an if and when the branch of the if is immediately a call to
pm_runtime_put_noidle (like in the definition of
pm_runtime_resume_and_get) or something that is likely a print
statement followed by a pm_runtime_put_noidle call. The patch
case appears somewhat more complicated, because it also deals with the
cases where {}s need to be removed.
pm_runtime_resume_and_get was introduced in
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
deal with usage counter")
Signed-off-by: Julia Lawall
Acked-by: Rafael J. Wysocki
---
v5: print a message with the new function name, as suggested by Markus Elfring
v4: s/pm_runtime_resume_and_get/pm_runtime_put_noidle/ as noted by John Hovold
v3: add the people who signed off on commit dd8088d5a896, expand the log message
v2: better keyword
scripts/coccinelle/api/pm_runtime_resume_and_get.cocci | 153 +
1 file changed, 153 insertions(+)
diff --git a/scripts/coccinelle/api/pm_runtime_resume_and_get.cocci
b/scripts/coccinelle/api/pm_runtime_resume_and_get.cocci
new file mode 100644
index ..3387cb606f9b
--- /dev/null
+++ b/scripts/coccinelle/api/pm_runtime_resume_and_get.cocci
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Use pm_runtime_resume_and_get.
+/// pm_runtime_get_sync keeps a reference count on failure,
+/// which can lead to leaks. pm_runtime_resume_and_get
+/// drops the reference count in the failure case.
+/// This rule addresses the cases where the reference count
+/// is unlikely to be needed in the failure case.
+///
+// Confidence: High
+// Copyright: (C) 2021 Julia Lawall, Inria
+// URL: https://coccinelle.gitlabpages.inria.fr/website
+// Options: --include-headers --no-includes
+// Keywords: pm_runtime_get_sync
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@r0 depends on patch && !context && !org && !report@
+expression ret,e;
+@@
+
+- ret = pm_runtime_get_sync(e);
++ ret = pm_runtime_resume_and_get(e);
+- if (ret < 0)
+- pm_runtime_put_noidle(e);
+
+@r1 depends on patch && !context && !org && !report@
+expression ret,e;
+statement S1,S2;
+@@
+
+- ret = pm_runtime_get_sync(e);
++ ret = pm_runtime_resume_and_get(e);
+ if (ret < 0)
+- {
+- pm_runtime_put_noidle(e);
+ S1
+- }
+ else S2
+
+@r2 depends on patch && !context && !org && !report@
+expression ret,e;
+statement S;
+@@
+
+- ret = pm_runtime_get_sync(e);
++ ret = pm_runtime_resume_and_get(e);
+ if (ret < 0) {
+- pm_runtime_put_noidle(e);
+ ...
+ } else S
+
+@r3 depends on patch && !context && !org && !report@
+expression ret,e;
+identifier f;
+constant char[] c;
+statement S;
+@@
+
+- ret = pm_runtime_get_sync(e);
++ ret = pm_runtime_resume_and_get(e);
+ if (ret < 0)
+- {
+ f(...,c,...);
+- pm_runtime_put_noidle(e);
+- }
+ else S
+
+@r4 depends on patch && !context && !org && !report@
+expression ret,e;
+identifier f;
+constant char[] c;
+statement S;
+@@
+
+- ret = pm_runtime_get_sync(e);
++ ret = pm_runtime_resume_and_get(e);
+ if (ret < 0) {
+ f(...,c,...);
+- pm_runtime_put_noidle(e);
+ ...
+ } else S
+
+//
+
+@r2_context depends on !patch && (context || org || report)@
+statement S;
+expression e, ret;
+position j0, j1;
+@@
+
+* ret@j0 = pm_runtime_get_sync(e);
+ if (ret < 0) {
+* pm_runtime_put_noidle@j1(e);
+ ...
+ } else S
+
+@r3_context depends on !patch && (context || org || report)@
+identifier f;
+statement S;
+constant char []c;
+expression e, ret;
+position j0, j1;
+@@
+
+* ret@j0 = pm_runtime_get_sync(e);
+ if (ret < 0) {
+ f(...,c,...);
+* pm_runtime_put_noidle@j1(e);
+ ...
+ } else S
+
+//
+
+@script:python r2_org depends on org@
+j0 << r2_context.j0;
+j1 << r2_context.j1;
+@@
+
+msg = "WARNING: opportunity for pm_runtime_resume_and_get"
+coccilib.org.print_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+@script:python r3_org depends on org@
+j0 << r3_context.j0;
+j1 << r3_context.j1;
+@@
+
+msg = "WARNING: opportunity for pm_runtime_resume_and_get"
+coccilib.org.print_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+//
+
+@script:
Re: [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
… > +msg = "WARNING: opportunity for pm_runtime_get_sync" > +coccilib.org.print_todo(j0[0], msg) … Do you find the following message variant more helpful? +coccilib.org.print_todo(j0[0], +"WARNING: opportunity for replacing pm_runtime_get_sync() by pm_runtime_resume_and_get()") Will the Python code be accordingly adjusted for the SmPL report rule? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
>… keeps a reference count on failure, …
Would you get into the mood to perform a systematic source code search
for similar function implementations according to resource clean-up?
> v2: better keyword
How do you think about to add the information “wrapper functions” here?
…
> +@r0 depends on patch && !context && !org && !report@
> +expression ret,e;
> +@@
> +
> +- ret = pm_runtime_get_sync(e);
> ++ ret = pm_runtime_resume_and_get(e);
…
I suggest once more to concentrate the specification of such a change
to the desired replacement of the mentioned function name.
+ ret =
+- pm_runtime_get_sync
++ pm_runtime_resume_and_get
+ (e);
…
> + if (ret < 0)
> +- {
> +- pm_runtime_put_noidle(e);
> + S1
> +- }
> + else S2
…
I propose to put this adjustment into a disjunction for the semantic patch
language.
How do you think about to combine five SmPL rules into one?
…
> +* ret@j0 = pm_runtime_get_sync(e);
> + if (ret < 0) {
> + f(...,c,...);
> +* pm_runtime_put_noidle@j1(e);
> + ...
> + } else S
…
Would you like to express in the SmPL context rules that a macro or function
call
is optional here?
Are there any opportunities to consider for the avoidance of duplicate SmPL
code?
…
> +msg = "WARNING: opportunity for pm_runtime_get_sync"
> +coccilib.org.print_todo(j0[0], msg)
…
Can the following Python code be more appropriate?
+coccilib.org.print_todo(j0[0], "WARNING: opportunity for
pm_runtime_resume_and_get")
Would you like to reconsider the message also for the SmPL report rule
accordingly?
Regards,
Markus
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH v3] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
On Tue, Apr 27, 2021 at 02:58:34PM +0200, Julia Lawall wrote:
> pm_runtime_get_sync keeps a reference count on failure, which can lead
> to leaks. pm_runtime_resume_and_get drops the reference count in the
> failure case. This rule very conservatively follows the definition of
> pm_runtime_resume_and_get to address the cases where the reference
> count is unlikely to be needed in the failure case. Specifically, the
> change is only done when pm_runtime_get_sync is followed immediately
> by an if and when the branch of the if is immediately a call to
> pm_runtime_put_noidle (like in the definition of
> pm_runtime_resume_and_get) or something that is likely a print
> statement followed by a pm_runtime_resume_and_get call. The patch
s/pm_runtime_resume_and_get/pm_runtime_put_noidle/
> case appears somewhat more complicated, because it also deals with the
> cases where {}s need to be removed.
>
> pm_runtime_resume_and_get was introduced in
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
> deal with usage counter")
>
> Signed-off-by: Julia Lawall
>
> ---
> v3: add the people who signed off on commit dd8088d5a896, expand the log
> message
> v2: better keyword
Johan
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH v2] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
On Tue, Apr 27, 2021 at 03:44:25PM +0200, Julia Lawall wrote:
> On Tue, 27 Apr 2021, Johan Hovold wrote:
>
> > On Mon, Apr 26, 2021 at 08:54:04PM +0200, Julia Lawall wrote:
> > > pm_runtime_get_sync keeps a reference count on failure, which can lead
> > > to leaks. pm_runtime_resume_and_get drops the reference count in the
> > > failure case. This rule very conservatively follows the definition of
> > > pm_runtime_resume_and_get to address the cases where the reference
> > > count is unlikely to be needed in the failure case.
> > >
> > > pm_runtime_resume_and_get was introduced in
> > > commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
> > > deal with usage counter")
> > >
> > > Signed-off-by: Julia Lawall
> >
> > As I've said elsewhere, not sure trying to do a mass conversion of this
> > is a good idea. People may not be used to the interface, but it is
> > consistent and has its use. The recent flurry of conversions show that
> > those also risk introducing new bugs in code that is currently tested
> > and correct.
>
> I looked some of the patches you commented on, and this rule would not
> have transformed those cases. This rule is very restricted to ensure that
> the transformed code follows the behavior of the new function.
Ah, ok. I didn't look too closely at the semantic patch itself and
wrongly associated it with the all-or-nothing media subsystem
conversions.
Thanks for clarifying further in v3 too.
Still a bit worried that this will push the cleanup crew to send more
broken patches since it sends a signal that pm_runtime_get_sync() is
always wrong. But guess there's not much to do about that now after
having added pm_runtime_resume_and_get() in the first place.
Johan
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
