All of lore.kernel.org
 help / color / mirror / Atom feed
* [Cocci] [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
@ 2015-06-12 17:09 ` Nicholas Mc Guire
  0 siblings, 0 replies; 10+ messages in thread
From: Nicholas Mc Guire @ 2015-06-12 17:09 UTC (permalink / raw)
  To: cocci

A number of functions takes a jiffies value as timeout. Passing in a 
constant makes the timeout HZ dependent which is wrong. Checking for
coccinelle constants only yields many false positives so they are
filtered for digits only.

A numeric value of 1 is though commonly in use for "shortest possible
delay" so those cases are treated as false positives as well and not
reported.

Link: https://systeme.lip6.fr/pipermail/cocci/2015-May/002085.html
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Note sure if a value of 2 should also be droped as false-positive
it seems to be in use in quite a few places - but I have not been able
to figure out a real rational for 2 jiffies...

V2: since sgen did not compile for me manually formated according to
    the tools/sgen/README - not really sure if I got it right.

The cases reported all look like they are actual API inconsistencies but
not reporting does not mean there is no bug with respect to jiffies. This
still can miss some values e.g. like in:
 drivers/staging/rts5208/rtsx_chip.h:66 #define POLLING_INTERVAL 30
 drivers/staging/rts5208/rtsx.c:540 schedule_timeout(POLLING_INTERVAL);

To be able to catch these cases an additional "strict" mode is added that
will list all C-constants except if HZ is used directly - this yields
some false positives so it is not on by default and will only print a
INFO message - not sure if this approach of adding a mode like this is 
actually ok.

Filtering by phython str rather than more precise rules is significantly 
faster in this case - see link above

 scripts/coccinelle/api/timeout_HZ_dependent.cocci |  123 +++++++++++++++++++++
 1 file changed, 123 insertions(+)
 create mode 100644 scripts/coccinelle/api/timeout_HZ_dependent.cocci

diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
new file mode 100644
index 0000000..9ec00cc
--- /dev/null
+++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
@@ -0,0 +1,121 @@
+/// Check for hardcoded numeric timeout values which are thus HZ dependent
+//# only report findings if the value is digits and != 1 as hardcoded
+//# 1 seems to be in use for short delays.
+//#
+//# No patch mode for this, as converting the value from C to
+//# msecs_to_jiffies(C) could be changing the effective timeout by more
+//# than a factor of 10 so this always needs manual inspection Most notably
+//# code that pre-dates variable HZ (prior to 2.4 kernel series) had HZ=100
+//# so a constant of 10 should be converted to 100ms for any old driver.
+//#
+//# In some cases C-constants are passed in that are not converted to
+//# jiffies, to locate these cases run in MODE=strict in which case these
+//# will also be reported except if HZ is passed. Note though many are
+//# likely to be false positives !
+//
+// Confidence: Medium
+// Copyright: (C) 2015 Nicholas Mc Guire <hofrat@osadl.org>, OSADL, GPL v2
+// URL: http://coccinelle.lip6.fr
+// Options: --no-includes --include-headers
+
+virtual context
+virtual patch
+virtual org
+virtual report
+virtual strict
+
+ at cc depends on !patch && (context || org || report || strict)@
+constant C;
+position p;
+@@
+
+(
+schedule_timeout at p(C)
+|
+schedule_timeout_interruptible at p(C)
+|
+schedule_timeout_killable at p(C)
+|
+schedule_timeout_uninterruptible at p(C)
+|
+mod_timer(...,C)
+|
+mod_timer_pinned(...,C)
+|
+mod_timer_pending(...,C)
+|
+apply_slack(...,C)
+|
+queue_delayed_work(...,C)
+|
+mod_delayed_work(...,C)
+|
+schedule_delayed_work_on(...,C)
+|
+schedule_delayed_work(...,C)
+|
+schedule_timeout(C)
+|
+schedule_timeout_interruptible(C)
+|
+schedule_timeout_killable(C)
+|
+schedule_timeout_uninterruptibl(C)
+|
+wait_event_timeout(...,C)
+|
+wait_event_interruptible_timeout(...,C)
+|
+wait_event_uninterruptible_timeout(...,C)
+|
+wait_event_interruptible_lock_irq_timeout(...,C)
+|
+wait_on_bit_timeout(...,C)
+|
+wait_for_completion_timeout(...,C)
+|
+wait_for_completion_io_timeout(...,C)
+|
+wait_for_completion_interruptible_timeout(...,C)
+|
+wait_for_completion_killable_timeout(...,C)
+)
+
+ at script:python depends on org@
+p << cc.p;
+timeout << cc.C;
+@@
+
+# schedule_timeout(1) for a "short" delay is not really HZ dependent
+# as it always would be converted to 1 by msecs_to_jiffies as well
+# so count this as false positive
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg = "WARNING: timeout is HZ dependent"
+      coccilib.org.print_safe_todo(p[0], msg)
+
+ at script:python depends on report@
+p << cc.p;
+timeout << cc.C;
+@@
+
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
+      coccilib.report.print_report(p[0], msg)
+
+@script:python depends on strict@
+p << cc.p;
+timeout << cc.C;
+@@
+
+# "strict" mode prints the cases that use C-constants != HZ
+# as well as the numeric constants != 1. This will deliver a false
+# positives if the C-constant is already in jiffies !
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg = "WARNING: timeout (%s) is HZ dependent" % (timeout)
+      coccilib.report.print_report(p[0], msg)
+elif (timeout != "HZ"):
+   msg = "INFO: timeout (%s) may be HZ dependent" % (timeout)
+   coccilib.report.print_report(p[0], msg)
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
@ 2015-06-12 17:09 ` Nicholas Mc Guire
  0 siblings, 0 replies; 10+ messages in thread
From: Nicholas Mc Guire @ 2015-06-12 17:09 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Gilles Muller, Nicolas Palix, Michal Marek, Joe Perches,
	Andy Whitcroft, John Stultz, cocci, linux-kernel,
	Nicholas Mc Guire

A number of functions takes a jiffies value as timeout. Passing in a 
constant makes the timeout HZ dependent which is wrong. Checking for
coccinelle constants only yields many false positives so they are
filtered for digits only.

A numeric value of 1 is though commonly in use for "shortest possible
delay" so those cases are treated as false positives as well and not
reported.

Link: https://systeme.lip6.fr/pipermail/cocci/2015-May/002085.html
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Note sure if a value of 2 should also be droped as false-positive
it seems to be in use in quite a few places - but I have not been able
to figure out a real rational for 2 jiffies...

V2: since sgen did not compile for me manually formated according to
    the tools/sgen/README - not really sure if I got it right.

The cases reported all look like they are actual API inconsistencies but
not reporting does not mean there is no bug with respect to jiffies. This
still can miss some values e.g. like in:
 drivers/staging/rts5208/rtsx_chip.h:66 #define POLLING_INTERVAL 30
 drivers/staging/rts5208/rtsx.c:540 schedule_timeout(POLLING_INTERVAL);

To be able to catch these cases an additional "strict" mode is added that
will list all C-constants except if HZ is used directly - this yields
some false positives so it is not on by default and will only print a
INFO message - not sure if this approach of adding a mode like this is 
actually ok.

Filtering by phython str rather than more precise rules is significantly 
faster in this case - see link above

 scripts/coccinelle/api/timeout_HZ_dependent.cocci |  123 +++++++++++++++++++++
 1 file changed, 123 insertions(+)
 create mode 100644 scripts/coccinelle/api/timeout_HZ_dependent.cocci

diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
new file mode 100644
index 0000000..9ec00cc
--- /dev/null
+++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
@@ -0,0 +1,121 @@
+/// Check for hardcoded numeric timeout values which are thus HZ dependent
+//# only report findings if the value is digits and != 1 as hardcoded
+//# 1 seems to be in use for short delays.
+//#
+//# No patch mode for this, as converting the value from C to
+//# msecs_to_jiffies(C) could be changing the effective timeout by more
+//# than a factor of 10 so this always needs manual inspection Most notably
+//# code that pre-dates variable HZ (prior to 2.4 kernel series) had HZ=100
+//# so a constant of 10 should be converted to 100ms for any old driver.
+//#
+//# In some cases C-constants are passed in that are not converted to
+//# jiffies, to locate these cases run in MODE=strict in which case these
+//# will also be reported except if HZ is passed. Note though many are
+//# likely to be false positives !
+//
+// Confidence: Medium
+// Copyright: (C) 2015 Nicholas Mc Guire <hofrat@osadl.org>, OSADL, GPL v2
+// URL: http://coccinelle.lip6.fr
+// Options: --no-includes --include-headers
+
+virtual context
+virtual patch
+virtual org
+virtual report
+virtual strict
+
+@cc depends on !patch && (context || org || report || strict)@
+constant C;
+position p;
+@@
+
+(
+schedule_timeout@p(C)
+|
+schedule_timeout_interruptible@p(C)
+|
+schedule_timeout_killable@p(C)
+|
+schedule_timeout_uninterruptible@p(C)
+|
+mod_timer(...,C)
+|
+mod_timer_pinned(...,C)
+|
+mod_timer_pending(...,C)
+|
+apply_slack(...,C)
+|
+queue_delayed_work(...,C)
+|
+mod_delayed_work(...,C)
+|
+schedule_delayed_work_on(...,C)
+|
+schedule_delayed_work(...,C)
+|
+schedule_timeout(C)
+|
+schedule_timeout_interruptible(C)
+|
+schedule_timeout_killable(C)
+|
+schedule_timeout_uninterruptibl(C)
+|
+wait_event_timeout(...,C)
+|
+wait_event_interruptible_timeout(...,C)
+|
+wait_event_uninterruptible_timeout(...,C)
+|
+wait_event_interruptible_lock_irq_timeout(...,C)
+|
+wait_on_bit_timeout(...,C)
+|
+wait_for_completion_timeout(...,C)
+|
+wait_for_completion_io_timeout(...,C)
+|
+wait_for_completion_interruptible_timeout(...,C)
+|
+wait_for_completion_killable_timeout(...,C)
+)
+
+@script:python depends on org@
+p << cc.p;
+timeout << cc.C;
+@@
+
+# schedule_timeout(1) for a "short" delay is not really HZ dependent
+# as it always would be converted to 1 by msecs_to_jiffies as well
+# so count this as false positive
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg = "WARNING: timeout is HZ dependent"
+      coccilib.org.print_safe_todo(p[0], msg)
+
+@script:python depends on report@
+p << cc.p;
+timeout << cc.C;
+@@
+
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
+      coccilib.report.print_report(p[0], msg)
+
+@script:python depends on strict@
+p << cc.p;
+timeout << cc.C;
+@@
+
+# "strict" mode prints the cases that use C-constants != HZ
+# as well as the numeric constants != 1. This will deliver a false
+# positives if the C-constant is already in jiffies !
+if str.isdigit(timeout):
+   if (int(timeout) != 1):
+      msg = "WARNING: timeout (%s) is HZ dependent" % (timeout)
+      coccilib.report.print_report(p[0], msg)
+elif (timeout != "HZ"):
+   msg = "INFO: timeout (%s) may be HZ dependent" % (timeout)
+   coccilib.report.print_report(p[0], msg)
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Cocci] [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
  2015-06-12 17:09 ` Nicholas Mc Guire
@ 2015-06-14  6:49   ` Julia Lawall
  -1 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2015-06-14  6:49 UTC (permalink / raw)
  To: cocci

> diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> new file mode 100644
> index 0000000..9ec00cc
> --- /dev/null
> +++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> @@ -0,0 +1,121 @@
> +/// Check for hardcoded numeric timeout values which are thus HZ dependent
> +//# only report findings if the value is digits and != 1 as hardcoded

only -> Only

> +virtual context
> +virtual patch

Does Coccincheck require that you put virtual rules for things you don't 
support?  You don't support patch, but you don't support context either.  
There are no *s in your rules.  And there can't be with the current 
organization, due to the filtering being done in the python code.

julia

> +virtual org
> +virtual report
> +virtual strict
> +
> + at cc depends on !patch && (context || org || report || strict)@
> +constant C;
> +position p;
> +@@
> +
> +(
> +schedule_timeout at p(C)
> +|
> +schedule_timeout_interruptible at p(C)
> +|
> +schedule_timeout_killable at p(C)
> +|
> +schedule_timeout_uninterruptible at p(C)
> +|
> +mod_timer(...,C)
> +|
> +mod_timer_pinned(...,C)
> +|
> +mod_timer_pending(...,C)
> +|
> +apply_slack(...,C)
> +|
> +queue_delayed_work(...,C)
> +|
> +mod_delayed_work(...,C)
> +|
> +schedule_delayed_work_on(...,C)
> +|
> +schedule_delayed_work(...,C)
> +|
> +schedule_timeout(C)
> +|
> +schedule_timeout_interruptible(C)
> +|
> +schedule_timeout_killable(C)
> +|
> +schedule_timeout_uninterruptibl(C)
> +|
> +wait_event_timeout(...,C)
> +|
> +wait_event_interruptible_timeout(...,C)
> +|
> +wait_event_uninterruptible_timeout(...,C)
> +|
> +wait_event_interruptible_lock_irq_timeout(...,C)
> +|
> +wait_on_bit_timeout(...,C)
> +|
> +wait_for_completion_timeout(...,C)
> +|
> +wait_for_completion_io_timeout(...,C)
> +|
> +wait_for_completion_interruptible_timeout(...,C)
> +|
> +wait_for_completion_killable_timeout(...,C)
> +)
> +
> + at script:python depends on org@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +# schedule_timeout(1) for a "short" delay is not really HZ dependent
> +# as it always would be converted to 1 by msecs_to_jiffies as well
> +# so count this as false positive
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout is HZ dependent"
> +      coccilib.org.print_safe_todo(p[0], msg)
> +
> + at script:python depends on report@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)
> +
> + at script:python depends on strict@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +# "strict" mode prints the cases that use C-constants != HZ
> +# as well as the numeric constants != 1. This will deliver a false
> +# positives if the C-constant is already in jiffies !
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) is HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)
> +elif (timeout != "HZ"):
> +   msg = "INFO: timeout (%s) may be HZ dependent" % (timeout)
> +   coccilib.report.print_report(p[0], msg)
> -- 
> 1.7.10.4
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
@ 2015-06-14  6:49   ` Julia Lawall
  0 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2015-06-14  6:49 UTC (permalink / raw)
  To: Nicholas Mc Guire
  Cc: Julia Lawall, Gilles Muller, Nicolas Palix, Michal Marek,
	Joe Perches, Andy Whitcroft, John Stultz, cocci, linux-kernel

> diff --git a/scripts/coccinelle/api/timeout_HZ_dependent.cocci b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> new file mode 100644
> index 0000000..9ec00cc
> --- /dev/null
> +++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> @@ -0,0 +1,121 @@
> +/// Check for hardcoded numeric timeout values which are thus HZ dependent
> +//# only report findings if the value is digits and != 1 as hardcoded

only -> Only

> +virtual context
> +virtual patch

Does Coccincheck require that you put virtual rules for things you don't 
support?  You don't support patch, but you don't support context either.  
There are no *s in your rules.  And there can't be with the current 
organization, due to the filtering being done in the python code.

julia

> +virtual org
> +virtual report
> +virtual strict
> +
> +@cc depends on !patch && (context || org || report || strict)@
> +constant C;
> +position p;
> +@@
> +
> +(
> +schedule_timeout@p(C)
> +|
> +schedule_timeout_interruptible@p(C)
> +|
> +schedule_timeout_killable@p(C)
> +|
> +schedule_timeout_uninterruptible@p(C)
> +|
> +mod_timer(...,C)
> +|
> +mod_timer_pinned(...,C)
> +|
> +mod_timer_pending(...,C)
> +|
> +apply_slack(...,C)
> +|
> +queue_delayed_work(...,C)
> +|
> +mod_delayed_work(...,C)
> +|
> +schedule_delayed_work_on(...,C)
> +|
> +schedule_delayed_work(...,C)
> +|
> +schedule_timeout(C)
> +|
> +schedule_timeout_interruptible(C)
> +|
> +schedule_timeout_killable(C)
> +|
> +schedule_timeout_uninterruptibl(C)
> +|
> +wait_event_timeout(...,C)
> +|
> +wait_event_interruptible_timeout(...,C)
> +|
> +wait_event_uninterruptible_timeout(...,C)
> +|
> +wait_event_interruptible_lock_irq_timeout(...,C)
> +|
> +wait_on_bit_timeout(...,C)
> +|
> +wait_for_completion_timeout(...,C)
> +|
> +wait_for_completion_io_timeout(...,C)
> +|
> +wait_for_completion_interruptible_timeout(...,C)
> +|
> +wait_for_completion_killable_timeout(...,C)
> +)
> +
> +@script:python depends on org@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +# schedule_timeout(1) for a "short" delay is not really HZ dependent
> +# as it always would be converted to 1 by msecs_to_jiffies as well
> +# so count this as false positive
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout is HZ dependent"
> +      coccilib.org.print_safe_todo(p[0], msg)
> +
> +@script:python depends on report@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)
> +
> +@script:python depends on strict@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +# "strict" mode prints the cases that use C-constants != HZ
> +# as well as the numeric constants != 1. This will deliver a false
> +# positives if the C-constant is already in jiffies !
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) is HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)
> +elif (timeout != "HZ"):
> +   msg = "INFO: timeout (%s) may be HZ dependent" % (timeout)
> +   coccilib.report.print_report(p[0], msg)
> -- 
> 1.7.10.4
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Cocci] [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
  2015-06-12 17:09 ` Nicholas Mc Guire
@ 2015-06-14  7:02   ` Julia Lawall
  -1 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2015-06-14  7:02 UTC (permalink / raw)
  To: cocci

> + at script:python depends on report@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)

The parentheses in the warning messages around the timeouts seem a little
strange to me.

Otherwise, as a semantic patch, it looks fine.  I can't judge the problem 
being solved though.

julia

> + at script:python depends on strict@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +# "strict" mode prints the cases that use C-constants != HZ
> +# as well as the numeric constants != 1. This will deliver a false
> +# positives if the C-constant is already in jiffies !
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) is HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)
> +elif (timeout != "HZ"):
> +   msg = "INFO: timeout (%s) may be HZ dependent" % (timeout)
> +   coccilib.report.print_report(p[0], msg)
> -- 
> 1.7.10.4
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
@ 2015-06-14  7:02   ` Julia Lawall
  0 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2015-06-14  7:02 UTC (permalink / raw)
  To: Nicholas Mc Guire
  Cc: Julia Lawall, Gilles Muller, Nicolas Palix, Michal Marek,
	Joe Perches, Andy Whitcroft, John Stultz, cocci, linux-kernel

> +@script:python depends on report@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)

The parentheses in the warning messages around the timeouts seem a little
strange to me.

Otherwise, as a semantic patch, it looks fine.  I can't judge the problem 
being solved though.

julia

> +@script:python depends on strict@
> +p << cc.p;
> +timeout << cc.C;
> +@@
> +
> +# "strict" mode prints the cases that use C-constants != HZ
> +# as well as the numeric constants != 1. This will deliver a false
> +# positives if the C-constant is already in jiffies !
> +if str.isdigit(timeout):
> +   if (int(timeout) != 1):
> +      msg = "WARNING: timeout (%s) is HZ dependent" % (timeout)
> +      coccilib.report.print_report(p[0], msg)
> +elif (timeout != "HZ"):
> +   msg = "INFO: timeout (%s) may be HZ dependent" % (timeout)
> +   coccilib.report.print_report(p[0], msg)
> -- 
> 1.7.10.4
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Cocci] [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
  2015-06-14  7:02   ` Julia Lawall
@ 2015-06-14  7:34     ` Nicholas Mc Guire
  -1 siblings, 0 replies; 10+ messages in thread
From: Nicholas Mc Guire @ 2015-06-14  7:34 UTC (permalink / raw)
  To: cocci

On Sun, 14 Jun 2015, Julia Lawall wrote:

> > + at script:python depends on report@
> > +p << cc.p;
> > +timeout << cc.C;
> > +@@
> > +
> > +if str.isdigit(timeout):
> > +   if (int(timeout) != 1):
> > +      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
> > +      coccilib.report.print_report(p[0], msg)
> 
> The parentheses in the warning messages around the timeouts seem a little
> strange to me.
>

should be \"%s\" I guess - will fix that and the other findings - just waiting
for feedback if it makes any sense at all to include something like this.
 
> Otherwise, as a semantic patch, it looks fine.  I can't judge the problem 
> being solved though.
>
it found aproximately 30 cases in the kernel some of which have been fixed
already and none seem to be false postitives - the hardcoded "2"
really being the only open issue if those should be counted as false-postitives
and filtered.

I'm not aware of any case in the kernel where the passing of a jiffies value
is intentionally HZ dependent - so I guess atleast warning on this
makes sense.

Anyway it might also make more sense to put something like this
into one of the build-bots rather than push it into mainline for general
use.

thx!
hofrat

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
@ 2015-06-14  7:34     ` Nicholas Mc Guire
  0 siblings, 0 replies; 10+ messages in thread
From: Nicholas Mc Guire @ 2015-06-14  7:34 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Nicholas Mc Guire, Gilles Muller, Nicolas Palix, Michal Marek,
	Joe Perches, Andy Whitcroft, John Stultz, cocci, linux-kernel

On Sun, 14 Jun 2015, Julia Lawall wrote:

> > +@script:python depends on report@
> > +p << cc.p;
> > +timeout << cc.C;
> > +@@
> > +
> > +if str.isdigit(timeout):
> > +   if (int(timeout) != 1):
> > +      msg = "WARNING: timeout (%s) seems HZ dependent" % (timeout)
> > +      coccilib.report.print_report(p[0], msg)
> 
> The parentheses in the warning messages around the timeouts seem a little
> strange to me.
>

should be \"%s\" I guess - will fix that and the other findings - just waiting
for feedback if it makes any sense at all to include something like this.
 
> Otherwise, as a semantic patch, it looks fine.  I can't judge the problem 
> being solved though.
>
it found aproximately 30 cases in the kernel some of which have been fixed
already and none seem to be false postitives - the hardcoded "2"
really being the only open issue if those should be counted as false-postitives
and filtered.

I'm not aware of any case in the kernel where the passing of a jiffies value
is intentionally HZ dependent - so I guess atleast warning on this
makes sense.

Anyway it might also make more sense to put something like this
into one of the build-bots rather than push it into mainline for general
use.

thx!
hofrat

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Cocci] [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
  2015-06-14  7:34     ` Nicholas Mc Guire
@ 2015-06-14  7:44       ` Julia Lawall
  -1 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2015-06-14  7:44 UTC (permalink / raw)
  To: cocci

> Anyway it might also make more sense to put something like this
> into one of the build-bots rather than push it into mainline for general
> use.

I think that the build-bots take from the mainline.

julia

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC V2] coccinelle: flag constants being passed for jiffies
@ 2015-06-14  7:44       ` Julia Lawall
  0 siblings, 0 replies; 10+ messages in thread
From: Julia Lawall @ 2015-06-14  7:44 UTC (permalink / raw)
  To: Nicholas Mc Guire
  Cc: Julia Lawall, Nicholas Mc Guire, Gilles Muller, Nicolas Palix,
	Michal Marek, Joe Perches, Andy Whitcroft, John Stultz, cocci,
	linux-kernel

> Anyway it might also make more sense to put something like this
> into one of the build-bots rather than push it into mainline for general
> use.

I think that the build-bots take from the mainline.

julia

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-06-14  7:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-12 17:09 [Cocci] [PATCH RFC V2] coccinelle: flag constants being passed for jiffies Nicholas Mc Guire
2015-06-12 17:09 ` Nicholas Mc Guire
2015-06-14  6:49 ` [Cocci] " Julia Lawall
2015-06-14  6:49   ` Julia Lawall
2015-06-14  7:02 ` [Cocci] " Julia Lawall
2015-06-14  7:02   ` Julia Lawall
2015-06-14  7:34   ` [Cocci] " Nicholas Mc Guire
2015-06-14  7:34     ` Nicholas Mc Guire
2015-06-14  7:44     ` [Cocci] " Julia Lawall
2015-06-14  7:44       ` Julia Lawall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.