* [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
@ 2021-04-29 17:43 Julia Lawall
2021-04-30 16:25 ` [Cocci] [PATCH v5] " Markus Elfring
2021-05-05 8:44 ` Mauro Carvalho Chehab
0 siblings, 2 replies; 7+ messages in thread
From: Julia Lawall @ 2021-04-29 17:43 UTC (permalink / raw)
To: Julia Lawall, Krzysztof Kozlowski, Mauro Carvalho Chehab
Cc: Michal Marek, Rafael J . Wysocki, kernel-janitors, Zhang Qilong,
Rafael J . Wysocki, Nicolas Palix, linux-kernel, Johan Hovold,
Jakub Kicinski, cocci
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 <Julia.Lawall@inria.fr>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
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 000000000000..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:python r2_report depends on report@
+j0 << r2_context.j0;
+j1 << r2_context.j1;
+@@
+
+msg = "WARNING: opportunity for pm_runtime_resume_and_get on line %s." % (j0[0].line)
+coccilib.report.print_report(j0[0], msg)
+
+@script:python r3_report depends on report@
+j0 << r3_context.j0;
+j1 << r3_context.j1;
+@@
+
+msg = "WARNING: opportunity for pm_runtime_resume_and_get on %s." % (j0[0].line)
+coccilib.report.print_report(j0[0], msg)
+
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Cocci] [PATCH v5] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
2021-04-29 17:43 [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get Julia Lawall
@ 2021-04-30 16:25 ` Markus Elfring
2021-05-05 8:44 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2021-04-30 16:25 UTC (permalink / raw)
To: Julia Lawall, Johan Hovold, Jakub Kicinski, Krzysztof Kozlowski,
Mauro Carvalho Chehab, Michal Marek, Nicolas Palix,
Rafael J. Wysocki, Zhang Qilong
Cc: kernel-janitors, cocci, linux-kernel
> v5: print a message with the new function name, as suggested by …
Thanks for another small adjustment.
Thus I find an other version number consistent in the patch subject.
Do you care to influence the run time characteristics a bit more
for such a script variant by reducing the number of SmPL rules
according to software design possibilities with SmPL disjunctions?
Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Cocci] [PATCH v5] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
2021-04-29 17:43 [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get Julia Lawall
2021-04-30 16:25 ` [Cocci] [PATCH v5] " Markus Elfring
@ 2021-05-05 8:44 ` Mauro Carvalho Chehab
2021-05-16 16:27 ` Julia Lawall
1 sibling, 1 reply; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-05 8:44 UTC (permalink / raw)
To: Julia Lawall
Cc: Michal Marek, Rafael J . Wysocki, Krzysztof Kozlowski,
kernel-janitors, Zhang Qilong, Rafael J . Wysocki, Nicolas Palix,
linux-kernel, Johan Hovold, Jonathan Cameron, Sakari Ailus,
Jakub Kicinski, cocci, Dan Carpenter
Hi Julia,
Em Thu, 29 Apr 2021 19:43:43 +0200
Julia Lawall <Julia.Lawall@inria.fr> escreveu:
> 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 <Julia.Lawall@inria.fr>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
First of all, thanks for doing that! It sounds a lot better to have
a script doing the check than newbies trying to address it manually,
as there are several aspects to be considered on such replacement.
>
> ---
> 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 000000000000..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
Long story short, I got a corner case where the script is doing
the wrong thing.
---
A detailed explanation follows:
As you know, I'm doing some manual work to address issues related
to pm_runtime_get() on media.
There, I found a corner case: There is a functional difference
between:
ret = pm_runtime_get_sync(&client->dev);
if (ret < 0) {
pm_runtime_put_noidle(&client->dev);
return ret;
}
and:
ret = pm_runtime_resume_and_get(&client->dev);
if (ret < 0)
return ret;
On success, pm_runtime_get_sync() can return either 0 or 1.
When 1 is returned, it means that the driver was already resumed.
pm_runtime_resume_and_get(), on the other hand, don't have the same
behavior. On success, it always return zero.
IMO, this is actually a good thing, as it helps to address a common
mistake:
ret = pm_runtime_get_sync(&client->dev);
/*
* or, even worse:
* ret = some_function_that_calls_pm_runtime_get_sync();
*/
if (ret) {
pm_runtime_put_noidle(&client->dev);
return ret;
}
FYI, Dan pointed one media driver to me those days with the above
issue at the imx334 driver, which I'm fixing on my patch series.
-
Anyway, after revisiting my patches, I found several cases that were
doing things like:
int ret;
ret = pm_runtime_get_sync(dev);
pm_runtime_put_noidle(dev); /* Or without it, on drivers with unbalanced get/put */
return ret > 0 ? 0 : ret;
Which can be replaced by just:
return pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
Yet, I found a single corner case on media where a driver is actually
using the positive return: the ccs-core camera sensor driver.
There, the driver checks the past state of RPM. If the
device was indeed suspended, the driver restores the hardware
controls (on V4L2, a control is something like brightness,
contrast, etc) to the last used value set.
This is the right thing to be done there, as setting values
to such hardware can be a slow operation, as it is done via I2C.
So, this particular driver checks if the RPM returned 0 or 1,
in order to check the previous RPM state before get.
In this particular case, replacing:
pm_runtime_get_sync()
with
pm_runtime_resume_and_get()
Will make part of the code unreachable.
While it won't break this specific driver, It could have
cause troubles if the logic there were different.
In any case, I tested the coccinelle script, and it produces
this change:
static int ccs_pm_get_init(struct ccs_sensor *sensor)
{
struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
int rval;
- rval = pm_runtime_get_sync(&client->dev);
- if (rval < 0) {
- pm_runtime_put_noidle(&client->dev);
+ rval = pm_runtime_resume_and_get(&client->dev);
+ if (rval < 0)
return rval;
- } else if (!rval) {
+ else if (!rval) {
rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
ctrl_handler);
if (rval)
return rval;
return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
}
return 0;
which will make v4l2_ctrl_handler_setup() to always being called,
even if the device was already resumed.
Thanks,
Mauro
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Cocci] [PATCH v5] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
2021-05-05 8:44 ` Mauro Carvalho Chehab
@ 2021-05-16 16:27 ` Julia Lawall
0 siblings, 0 replies; 7+ messages in thread
From: Julia Lawall @ 2021-05-16 16:27 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Michal Marek, Rafael J . Wysocki, Krzysztof Kozlowski,
kernel-janitors, Zhang Qilong, Rafael J . Wysocki, Nicolas Palix,
linux-kernel, Johan Hovold, Jonathan Cameron, Sakari Ailus,
Jakub Kicinski, cocci, Dan Carpenter
On Wed, 5 May 2021, Mauro Carvalho Chehab wrote:
> Hi Julia,
>
> Em Thu, 29 Apr 2021 19:43:43 +0200
> Julia Lawall <Julia.Lawall@inria.fr> escreveu:
>
> > 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 <Julia.Lawall@inria.fr>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> First of all, thanks for doing that! It sounds a lot better to have
> a script doing the check than newbies trying to address it manually,
> as there are several aspects to be considered on such replacement.
>
> >
> > ---
> > 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 000000000000..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
>
> Long story short, I got a corner case where the script is doing
> the wrong thing.
>
> ---
>
> A detailed explanation follows:
>
> As you know, I'm doing some manual work to address issues related
> to pm_runtime_get() on media.
>
> There, I found a corner case: There is a functional difference
> between:
>
> ret = pm_runtime_get_sync(&client->dev);
> if (ret < 0) {
> pm_runtime_put_noidle(&client->dev);
> return ret;
> }
>
> and:
> ret = pm_runtime_resume_and_get(&client->dev);
> if (ret < 0)
> return ret;
>
> On success, pm_runtime_get_sync() can return either 0 or 1.
> When 1 is returned, it means that the driver was already resumed.
>
> pm_runtime_resume_and_get(), on the other hand, don't have the same
> behavior. On success, it always return zero.
>
> IMO, this is actually a good thing, as it helps to address a common
> mistake:
>
> ret = pm_runtime_get_sync(&client->dev);
> /*
> * or, even worse:
> * ret = some_function_that_calls_pm_runtime_get_sync();
> */
>
> if (ret) {
> pm_runtime_put_noidle(&client->dev);
> return ret;
> }
>
> FYI, Dan pointed one media driver to me those days with the above
> issue at the imx334 driver, which I'm fixing on my patch series.
>
> -
>
> Anyway, after revisiting my patches, I found several cases that were
> doing things like:
>
> int ret;
>
> ret = pm_runtime_get_sync(dev);
> pm_runtime_put_noidle(dev); /* Or without it, on drivers with unbalanced get/put */
>
> return ret > 0 ? 0 : ret;
>
> Which can be replaced by just:
>
> return pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
>
> Yet, I found a single corner case on media where a driver is actually
> using the positive return: the ccs-core camera sensor driver.
>
> There, the driver checks the past state of RPM. If the
> device was indeed suspended, the driver restores the hardware
> controls (on V4L2, a control is something like brightness,
> contrast, etc) to the last used value set.
>
> This is the right thing to be done there, as setting values
> to such hardware can be a slow operation, as it is done via I2C.
>
> So, this particular driver checks if the RPM returned 0 or 1,
> in order to check the previous RPM state before get.
>
> In this particular case, replacing:
> pm_runtime_get_sync()
> with
> pm_runtime_resume_and_get()
>
> Will make part of the code unreachable.
>
> While it won't break this specific driver, It could have
> cause troubles if the logic there were different.
>
> In any case, I tested the coccinelle script, and it produces
> this change:
>
> static int ccs_pm_get_init(struct ccs_sensor *sensor)
> {
> struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> int rval;
>
> - rval = pm_runtime_get_sync(&client->dev);
> - if (rval < 0) {
> - pm_runtime_put_noidle(&client->dev);
> + rval = pm_runtime_resume_and_get(&client->dev);
> + if (rval < 0)
>
> return rval;
> - } else if (!rval) {
> + else if (!rval) {
> rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->
> ctrl_handler);
> if (rval)
> return rval;
>
> return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler);
> }
>
> return 0;
>
> which will make v4l2_ctrl_handler_setup() to always being called,
> even if the device was already resumed.
Thanks for the feedback. It looks like what you are saying that the
script should ensure that the return value of pm_runtime_get_sync is not
used for anything else. That can be added to the script.
julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
2021-04-27 14:19 [Cocci] [PATCH v4] " Julia Lawall
2021-04-27 19:30 ` Markus Elfring
@ 2021-04-29 18:00 ` Markus Elfring
1 sibling, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2021-04-29 18:00 UTC (permalink / raw)
To: Julia Lawall, Johan Hovold, Jakub Kicinski, Krzysztof Kozlowski,
Mauro Carvalho Chehab, Michal Marek, Nicolas Palix,
Rafael J. Wysocki, Zhang Qilong
Cc: kernel-janitors, cocci, linux-kernel
…
> +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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
2021-04-27 14:19 [Cocci] [PATCH v4] " Julia Lawall
@ 2021-04-27 19:30 ` Markus Elfring
2021-04-29 18:00 ` Markus Elfring
1 sibling, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2021-04-27 19:30 UTC (permalink / raw)
To: Julia Lawall, Krzysztof Kozlowski, Mauro Carvalho Chehab, cocci,
kernel-janitors
Cc: Michal Marek, Rafael J. Wysocki, Zhang Qilong, Nicolas Palix,
linux-kernel, Johan Hovold, Jakub Kicinski
>… 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get
@ 2021-04-27 14:19 Julia Lawall
2021-04-27 19:30 ` Markus Elfring
2021-04-29 18:00 ` Markus Elfring
0 siblings, 2 replies; 7+ messages in thread
From: Julia Lawall @ 2021-04-27 14:19 UTC (permalink / raw)
To: Julia Lawall, Krzysztof Kozlowski, Mauro Carvalho Chehab
Cc: Michal Marek, Rafael J . Wysocki, kernel-janitors, Zhang Qilong,
Rafael J . Wysocki, Nicolas Palix, linux-kernel, Johan Hovold,
Jakub Kicinski, cocci
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 <Julia.Lawall@inria.fr>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
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 000000000000..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_get_sync"
+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_get_sync"
+coccilib.org.print_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+// ----------------------------------------------------------------------------
+
+@script:python r2_report depends on report@
+j0 << r2_context.j0;
+j1 << r2_context.j1;
+@@
+
+msg = "WARNING: opportunity for pm_runtime_get_sync on line %s." % (j0[0].line)
+coccilib.report.print_report(j0[0], msg)
+
+@script:python r3_report depends on report@
+j0 << r3_context.j0;
+j1 << r3_context.j1;
+@@
+
+msg = "WARNING: opportunity for pm_runtime_get_sync on %s." % (j0[0].line)
+coccilib.report.print_report(j0[0], msg)
+
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-16 16:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 17:43 [Cocci] [PATCH v4] coccinelle: api: semantic patch to use pm_runtime_resume_and_get Julia Lawall
2021-04-30 16:25 ` [Cocci] [PATCH v5] " Markus Elfring
2021-05-05 8:44 ` Mauro Carvalho Chehab
2021-05-16 16:27 ` Julia Lawall
-- strict thread matches above, loose matches on Subject: below --
2021-04-27 14:19 [Cocci] [PATCH v4] " Julia Lawall
2021-04-27 19:30 ` Markus Elfring
2021-04-29 18:00 ` Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).