linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2] Coccinelle: check usleep_range() usage
@ 2016-12-15  2:41 Nicholas Mc Guire
  2016-12-15  5:52 ` Julia Lawall
  0 siblings, 1 reply; 4+ messages in thread
From: Nicholas Mc Guire @ 2016-12-15  2:41 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Gilles Muller, Nicolas Palix, Michal Marek, Thomas Gleixner,
	Joe Perches, cocci, linux-kernel, Nicholas Mc Guire

Documentation/timers/timers-howto.txt outlines the intended usage of 
usleep_range(), this spatch tries to locate missuse/out-of-spec cases.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
V2: added context mode as suggested by Julia Lawall <julia.lawall@lip6.fr>
    added min<max case sugested by Joe Perches <joe@perches.com>
    added in the range checks as they are resonably reliable based on 
    a review of all 1648 call sites of usleep_range() 

1648 calls total                                                                
1488 pass numeric values only (90.29%)
  27 min below 10us (1.81%)
  40 min above 10ms (2.68%)
     min out of spec 4.50%
  76 preprocessor constants (4.61%)
   1 min below 10us (1.31%)
   8 min above 10ms (10.52%)
     min out of spec 11.84%
  85 expressions (5.15%)
1(0) min below 10us (1.50%)*
6(2) min above 10ms (7.50%)*
     min out of spec 9.0%
Errors:
  23 where min==max  (1.39%)
   0 where max < min (0.00%)

Total:
  Bugs: 6.48%-10.70%*
  Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
  Detectable by coccinelle:
  Bugs: 74/103 (71.8%)
  Crit: 50/52 (96.1%)
* numbers estimated based on code review

Patch is againts 4.9.0 (localversion-next is next-20161214)

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

diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
new file mode 100644
index 0000000..003e9ef
--- /dev/null
+++ b/scripts/coccinelle/api/bad_usleep_range.cocci
@@ -0,0 +1,88 @@
+/// report bad/problematic usleep_range usage
+//
+// This is a checker for the documented intended use of usleep_range
+// see: Documentation/timers/timers-howto.txt and
+// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
+//       when mdelay might not be a suitable replacement
+//
+// Limitations:
+//  * The numeric limits are only checked when numeric constants are in
+//    use (as of 4.9.0 thats 90.29% of the calls) no constant folding
+//    is done - so this can miss some out-of-range cases - but in 4.9.0
+//    it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
+//    (96.1%) of the critical cases (min < 10 and min==max - there
+//  * There may be RT use-cases where both min < 10 and min==max)
+//    justified (e.g. high-throughput drivers on a shielded core)
+//
+// 1) warn if min == max
+//
+//  The problem is that usleep_range is calculating the delay by
+//      exp = ktime_add_us(ktime_get(), min)
+//      delta = (u64)(max - min) * NSEC_PER_USEC
+//  so delta is set to 0 if min==max
+//  and then calls
+//      schedule_hrtimeout_range(exp, 0,...)
+//  effectively this means that the clock subsystem has no room to
+//  optimize. usleep_range() is in non-atomic context so a 0 range
+//  makes very little sense as the task can be preempted anyway so
+//  there is no guarantee that the 0 range would be adding much
+//  precision - it just removes optimization potential, so it probably
+//  never really makes sense.
+//
+// 2) warn if min < 10 or min > 20ms
+//
+//  it makes little sense to use a non-atomic call for very short
+//  delays because the scheduling jitter will most likely exceed
+//  this limit - udelay() makes more sense in that case. For very
+//  large delays using hrtimers is useless as preemption becomes
+//  quite likely resulting in high inaccuracy anyway - so use
+//  jiffies based msleep and don't burden the hrtimer subsystem.
+//
+// 3) warn if max < min
+//
+//  Joe Perches <joe@perches.com> added a check for this case
+//  that is definitely wrong.
+//
+// Confidence: Moderate
+// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual org
+virtual report
+virtual context
+
+@nullrangectx depends on context@
+expression E1,E2;
+position p;
+@@
+
+* usleep_range@p(E1,E2)
+
+
+@nullrange@
+expression E1,E2;
+position p;
+@@
+
+  usleep_range@p(E1,E2)
+
+@script:python depends on !context@
+p << nullrange.p;
+min << nullrange.E1;
+max << nullrange.E2;
+@@
+
+if(min == max):
+   msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
+   coccilib.report.print_report(p[0], msg)
+if str.isdigit(min):
+   if(int(min) < 10):
+      msg = "ERROR: usleep_range min (%s) less than 10us - consider using udelay()" % (min)
+      coccilib.report.print_report(p[0], msg)
+   if(20000 < int(min)):
+      msg = "ERROR: usleep_range min (%s) exceed 20m - consider using mslee()" % (min)
+      coccilib.report.print_report(p[0], msg)
+   if(int(max) < int(min)):
+      msg = "ERROR: usleep_range max (%s) less than min (%s)" % (max,min)
+      coccilib.report.print_report(p[0], msg)
-- 
2.1.4

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

* Re: [PATCH V2] Coccinelle: check usleep_range() usage
  2016-12-15  2:41 [PATCH V2] Coccinelle: check usleep_range() usage Nicholas Mc Guire
@ 2016-12-15  5:52 ` Julia Lawall
  2016-12-15  6:16   ` Nicholas Mc Guire
  0 siblings, 1 reply; 4+ messages in thread
From: Julia Lawall @ 2016-12-15  5:52 UTC (permalink / raw)
  To: Nicholas Mc Guire
  Cc: Gilles Muller, Nicolas Palix, Michal Marek, Thomas Gleixner,
	Joe Perches, cocci, linux-kernel



On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:

> Documentation/timers/timers-howto.txt outlines the intended usage of
> usleep_range(), this spatch tries to locate missuse/out-of-spec cases.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> V2: added context mode as suggested by Julia Lawall <julia.lawall@lip6.fr>
>     added min<max case sugested by Joe Perches <joe@perches.com>
>     added in the range checks as they are resonably reliable based on
>     a review of all 1648 call sites of usleep_range()
>
> 1648 calls total
> 1488 pass numeric values only (90.29%)
>   27 min below 10us (1.81%)
>   40 min above 10ms (2.68%)
>      min out of spec 4.50%
>   76 preprocessor constants (4.61%)
>    1 min below 10us (1.31%)
>    8 min above 10ms (10.52%)
>      min out of spec 11.84%
>   85 expressions (5.15%)
> 1(0) min below 10us (1.50%)*
> 6(2) min above 10ms (7.50%)*
>      min out of spec 9.0%
> Errors:
>   23 where min==max  (1.39%)
>    0 where max < min (0.00%)
>
> Total:
>   Bugs: 6.48%-10.70%*
>   Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
>   Detectable by coccinelle:
>   Bugs: 74/103 (71.8%)
>   Crit: 50/52 (96.1%)
> * numbers estimated based on code review
>
> Patch is againts 4.9.0 (localversion-next is next-20161214)
>
>  scripts/coccinelle/api/bad_usleep_range.cocci | 88 +++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 scripts/coccinelle/api/bad_usleep_range.cocci
>
> diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
> new file mode 100644
> index 0000000..003e9ef
> --- /dev/null
> +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> @@ -0,0 +1,88 @@
> +/// report bad/problematic usleep_range usage
> +//
> +// This is a checker for the documented intended use of usleep_range
> +// see: Documentation/timers/timers-howto.txt and
> +// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
> +//       when mdelay might not be a suitable replacement
> +//
> +// Limitations:
> +//  * The numeric limits are only checked when numeric constants are in
> +//    use (as of 4.9.0 thats 90.29% of the calls) no constant folding
> +//    is done - so this can miss some out-of-range cases - but in 4.9.0
> +//    it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
> +//    (96.1%) of the critical cases (min < 10 and min==max - there
> +//  * There may be RT use-cases where both min < 10 and min==max)
> +//    justified (e.g. high-throughput drivers on a shielded core)
> +//
> +// 1) warn if min == max
> +//
> +//  The problem is that usleep_range is calculating the delay by
> +//      exp = ktime_add_us(ktime_get(), min)
> +//      delta = (u64)(max - min) * NSEC_PER_USEC
> +//  so delta is set to 0 if min==max
> +//  and then calls
> +//      schedule_hrtimeout_range(exp, 0,...)
> +//  effectively this means that the clock subsystem has no room to
> +//  optimize. usleep_range() is in non-atomic context so a 0 range
> +//  makes very little sense as the task can be preempted anyway so
> +//  there is no guarantee that the 0 range would be adding much
> +//  precision - it just removes optimization potential, so it probably
> +//  never really makes sense.
> +//
> +// 2) warn if min < 10 or min > 20ms
> +//
> +//  it makes little sense to use a non-atomic call for very short
> +//  delays because the scheduling jitter will most likely exceed
> +//  this limit - udelay() makes more sense in that case. For very
> +//  large delays using hrtimers is useless as preemption becomes
> +//  quite likely resulting in high inaccuracy anyway - so use
> +//  jiffies based msleep and don't burden the hrtimer subsystem.
> +//
> +// 3) warn if max < min
> +//
> +//  Joe Perches <joe@perches.com> added a check for this case
> +//  that is definitely wrong.
> +//
> +// Confidence: Moderate
> +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual org
> +virtual report
> +virtual context
> +
> +@nullrangectx depends on context@
> +expression E1,E2;
> +position p;
> +@@
> +
> +* usleep_range@p(E1,E2)

This is going to give a context warning on every call to usleep_range.
Why not E1,E1?

> +
> +
> +@nullrange@
> +expression E1,E2;
> +position p;
> +@@
> +
> +  usleep_range@p(E1,E2)
> +
> +@script:python depends on !context@
> +p << nullrange.p;
> +min << nullrange.E1;
> +max << nullrange.E2;
> +@@
> +
> +if(min == max):
> +   msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
> +   coccilib.report.print_report(p[0], msg)
> +if str.isdigit(min):

I guess this checks if min is a constant, but doesn't the last case also
need to check if max is a constant?

julia


> +   if(int(min) < 10):
> +      msg = "ERROR: usleep_range min (%s) less than 10us - consider using udelay()" % (min)
> +      coccilib.report.print_report(p[0], msg)
> +   if(20000 < int(min)):
> +      msg = "ERROR: usleep_range min (%s) exceed 20m - consider using mslee()" % (min)
> +      coccilib.report.print_report(p[0], msg)
> +   if(int(max) < int(min)):
> +      msg = "ERROR: usleep_range max (%s) less than min (%s)" % (max,min)
> +      coccilib.report.print_report(p[0], msg)
> --
> 2.1.4
>
>

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

* Re: [PATCH V2] Coccinelle: check usleep_range() usage
  2016-12-15  5:52 ` Julia Lawall
@ 2016-12-15  6:16   ` Nicholas Mc Guire
  2016-12-15  7:44     ` Julia Lawall
  0 siblings, 1 reply; 4+ messages in thread
From: Nicholas Mc Guire @ 2016-12-15  6:16 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Nicholas Mc Guire, Gilles Muller, Nicolas Palix, Michal Marek,
	Thomas Gleixner, Joe Perches, cocci, linux-kernel

On Thu, Dec 15, 2016 at 06:52:28AM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
> 
> > Documentation/timers/timers-howto.txt outlines the intended usage of
> > usleep_range(), this spatch tries to locate missuse/out-of-spec cases.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > V2: added context mode as suggested by Julia Lawall <julia.lawall@lip6.fr>
> >     added min<max case sugested by Joe Perches <joe@perches.com>
> >     added in the range checks as they are resonably reliable based on
> >     a review of all 1648 call sites of usleep_range()
> >
> > 1648 calls total
> > 1488 pass numeric values only (90.29%)
> >   27 min below 10us (1.81%)
> >   40 min above 10ms (2.68%)
> >      min out of spec 4.50%
> >   76 preprocessor constants (4.61%)
> >    1 min below 10us (1.31%)
> >    8 min above 10ms (10.52%)
> >      min out of spec 11.84%
> >   85 expressions (5.15%)
> > 1(0) min below 10us (1.50%)*
> > 6(2) min above 10ms (7.50%)*
> >      min out of spec 9.0%
> > Errors:
> >   23 where min==max  (1.39%)
> >    0 where max < min (0.00%)
> >
> > Total:
> >   Bugs: 6.48%-10.70%*
> >   Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
> >   Detectable by coccinelle:
> >   Bugs: 74/103 (71.8%)
> >   Crit: 50/52 (96.1%)
> > * numbers estimated based on code review
> >
> > Patch is againts 4.9.0 (localversion-next is next-20161214)
> >
> >  scripts/coccinelle/api/bad_usleep_range.cocci | 88 +++++++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> >  create mode 100644 scripts/coccinelle/api/bad_usleep_range.cocci
> >
> > diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
> > new file mode 100644
> > index 0000000..003e9ef
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> > @@ -0,0 +1,88 @@
> > +/// report bad/problematic usleep_range usage
> > +//
> > +// This is a checker for the documented intended use of usleep_range
> > +// see: Documentation/timers/timers-howto.txt and
> > +// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
> > +//       when mdelay might not be a suitable replacement
> > +//
> > +// Limitations:
> > +//  * The numeric limits are only checked when numeric constants are in
> > +//    use (as of 4.9.0 thats 90.29% of the calls) no constant folding
> > +//    is done - so this can miss some out-of-range cases - but in 4.9.0
> > +//    it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
> > +//    (96.1%) of the critical cases (min < 10 and min==max - there
> > +//  * There may be RT use-cases where both min < 10 and min==max)
> > +//    justified (e.g. high-throughput drivers on a shielded core)
> > +//
> > +// 1) warn if min == max
> > +//
> > +//  The problem is that usleep_range is calculating the delay by
> > +//      exp = ktime_add_us(ktime_get(), min)
> > +//      delta = (u64)(max - min) * NSEC_PER_USEC
> > +//  so delta is set to 0 if min==max
> > +//  and then calls
> > +//      schedule_hrtimeout_range(exp, 0,...)
> > +//  effectively this means that the clock subsystem has no room to
> > +//  optimize. usleep_range() is in non-atomic context so a 0 range
> > +//  makes very little sense as the task can be preempted anyway so
> > +//  there is no guarantee that the 0 range would be adding much
> > +//  precision - it just removes optimization potential, so it probably
> > +//  never really makes sense.
> > +//
> > +// 2) warn if min < 10 or min > 20ms
> > +//
> > +//  it makes little sense to use a non-atomic call for very short
> > +//  delays because the scheduling jitter will most likely exceed
> > +//  this limit - udelay() makes more sense in that case. For very
> > +//  large delays using hrtimers is useless as preemption becomes
> > +//  quite likely resulting in high inaccuracy anyway - so use
> > +//  jiffies based msleep and don't burden the hrtimer subsystem.
> > +//
> > +// 3) warn if max < min
> > +//
> > +//  Joe Perches <joe@perches.com> added a check for this case
> > +//  that is definitely wrong.
> > +//
> > +// Confidence: Moderate
> > +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
> > +// Comments:
> > +// Options: --no-includes --include-headers
> > +
> > +virtual org
> > +virtual report
> > +virtual context
> > +
> > +@nullrangectx depends on context@
> > +expression E1,E2;
> > +position p;
> > +@@
> > +
> > +* usleep_range@p(E1,E2)
> 
> This is going to give a context warning on every call to usleep_range.
> Why not E1,E1?
yes this triggers on all use of usleep_ranges - as the report mode is
checking for more than just min==max I thought its resonable to simply
report all cases - maybe not.
Not sure if it makes sense to add in the filter from below,

Thre actually are quite a few bad use patters beyond these basic ones
like
                unsigned long timeout = jiffies + msecs_to_jiffies(250);

-               while (time_before(jiffies, timeout)) {
                        value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
                        if (value == MBOX_OWNER_NONE)
                                break;

-                       usleep_range(10, 20);
                }

> 
> > +
> > +
> > +@nullrange@
> > +expression E1,E2;
> > +position p;
> > +@@
> > +
> > +  usleep_range@p(E1,E2)
> > +
> > +@script:python depends on !context@
> > +p << nullrange.p;
> > +min << nullrange.E1;
> > +max << nullrange.E2;
> > +@@
> > +
> > +if(min == max):
> > +   msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
> > +   coccilib.report.print_report(p[0], msg)
> > +if str.isdigit(min):
> 
> I guess this checks if min is a constant, but doesn't the last case also
> need to check if max is a constant?
>
yes it does - seems that there simply was no
such case so it went unnoticed.

also just noticed that the org mode would also be using 
coccilib.report rather than coccilib.org...

thx!
hofrat
 

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

* Re: [PATCH V2] Coccinelle: check usleep_range() usage
  2016-12-15  6:16   ` Nicholas Mc Guire
@ 2016-12-15  7:44     ` Julia Lawall
  0 siblings, 0 replies; 4+ messages in thread
From: Julia Lawall @ 2016-12-15  7:44 UTC (permalink / raw)
  To: Nicholas Mc Guire
  Cc: Julia Lawall, Nicholas Mc Guire, Gilles Muller, Nicolas Palix,
	Michal Marek, Thomas Gleixner, Joe Perches, cocci, linux-kernel



On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:

> On Thu, Dec 15, 2016 at 06:52:28AM +0100, Julia Lawall wrote:
> >
> >
> > On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
> >
> > > Documentation/timers/timers-howto.txt outlines the intended usage of
> > > usleep_range(), this spatch tries to locate missuse/out-of-spec cases.
> > >
> > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > ---
> > > V2: added context mode as suggested by Julia Lawall <julia.lawall@lip6.fr>
> > >     added min<max case sugested by Joe Perches <joe@perches.com>
> > >     added in the range checks as they are resonably reliable based on
> > >     a review of all 1648 call sites of usleep_range()
> > >
> > > 1648 calls total
> > > 1488 pass numeric values only (90.29%)
> > >   27 min below 10us (1.81%)
> > >   40 min above 10ms (2.68%)
> > >      min out of spec 4.50%
> > >   76 preprocessor constants (4.61%)
> > >    1 min below 10us (1.31%)
> > >    8 min above 10ms (10.52%)
> > >      min out of spec 11.84%
> > >   85 expressions (5.15%)
> > > 1(0) min below 10us (1.50%)*
> > > 6(2) min above 10ms (7.50%)*
> > >      min out of spec 9.0%
> > > Errors:
> > >   23 where min==max  (1.39%)
> > >    0 where max < min (0.00%)
> > >
> > > Total:
> > >   Bugs: 6.48%-10.70%*
> > >   Crit: 3.09%-3.15%* (min < 10, min==max, max < min)
> > >   Detectable by coccinelle:
> > >   Bugs: 74/103 (71.8%)
> > >   Crit: 50/52 (96.1%)
> > > * numbers estimated based on code review
> > >
> > > Patch is againts 4.9.0 (localversion-next is next-20161214)
> > >
> > >  scripts/coccinelle/api/bad_usleep_range.cocci | 88 +++++++++++++++++++++++++++
> > >  1 file changed, 88 insertions(+)
> > >  create mode 100644 scripts/coccinelle/api/bad_usleep_range.cocci
> > >
> > > diff --git a/scripts/coccinelle/api/bad_usleep_range.cocci b/scripts/coccinelle/api/bad_usleep_range.cocci
> > > new file mode 100644
> > > index 0000000..003e9ef
> > > --- /dev/null
> > > +++ b/scripts/coccinelle/api/bad_usleep_range.cocci
> > > @@ -0,0 +1,88 @@
> > > +/// report bad/problematic usleep_range usage
> > > +//
> > > +// This is a checker for the documented intended use of usleep_range
> > > +// see: Documentation/timers/timers-howto.txt and
> > > +// Link: http://lkml.org/lkml/2016/11/29/54 for some notes on
> > > +//       when mdelay might not be a suitable replacement
> > > +//
> > > +// Limitations:
> > > +//  * The numeric limits are only checked when numeric constants are in
> > > +//    use (as of 4.9.0 thats 90.29% of the calls) no constant folding
> > > +//    is done - so this can miss some out-of-range cases - but in 4.9.0
> > > +//    it was catching 74 of the 103 bad cases (71.8%) and 50 of 52
> > > +//    (96.1%) of the critical cases (min < 10 and min==max - there
> > > +//  * There may be RT use-cases where both min < 10 and min==max)
> > > +//    justified (e.g. high-throughput drivers on a shielded core)
> > > +//
> > > +// 1) warn if min == max
> > > +//
> > > +//  The problem is that usleep_range is calculating the delay by
> > > +//      exp = ktime_add_us(ktime_get(), min)
> > > +//      delta = (u64)(max - min) * NSEC_PER_USEC
> > > +//  so delta is set to 0 if min==max
> > > +//  and then calls
> > > +//      schedule_hrtimeout_range(exp, 0,...)
> > > +//  effectively this means that the clock subsystem has no room to
> > > +//  optimize. usleep_range() is in non-atomic context so a 0 range
> > > +//  makes very little sense as the task can be preempted anyway so
> > > +//  there is no guarantee that the 0 range would be adding much
> > > +//  precision - it just removes optimization potential, so it probably
> > > +//  never really makes sense.
> > > +//
> > > +// 2) warn if min < 10 or min > 20ms
> > > +//
> > > +//  it makes little sense to use a non-atomic call for very short
> > > +//  delays because the scheduling jitter will most likely exceed
> > > +//  this limit - udelay() makes more sense in that case. For very
> > > +//  large delays using hrtimers is useless as preemption becomes
> > > +//  quite likely resulting in high inaccuracy anyway - so use
> > > +//  jiffies based msleep and don't burden the hrtimer subsystem.
> > > +//
> > > +// 3) warn if max < min
> > > +//
> > > +//  Joe Perches <joe@perches.com> added a check for this case
> > > +//  that is definitely wrong.
> > > +//
> > > +// Confidence: Moderate
> > > +// Copyright: (C) 2016 Nicholas Mc Guire, OSADL.  GPLv2.
> > > +// Comments:
> > > +// Options: --no-includes --include-headers
> > > +
> > > +virtual org
> > > +virtual report
> > > +virtual context
> > > +
> > > +@nullrangectx depends on context@
> > > +expression E1,E2;
> > > +position p;
> > > +@@
> > > +
> > > +* usleep_range@p(E1,E2)
> >
> > This is going to give a context warning on every call to usleep_range.
> > Why not E1,E1?
> yes this triggers on all use of usleep_ranges - as the report mode is
> checking for more than just min==max I thought its resonable to simply
> report all cases - maybe not.
> Not sure if it makes sense to add in the filter from below,

I think that simply flagging all the calls for context mode is not a good
idea.  Especilly since there is no explanation in this case.  If it is too
awkward to support context mode, just drop it.

julia

>
> Thre actually are quite a few bad use patters beyond these basic ones
> like
>                 unsigned long timeout = jiffies + msecs_to_jiffies(250);
>
> -               while (time_before(jiffies, timeout)) {
>                         value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER);
>                         if (value == MBOX_OWNER_NONE)
>                                 break;
>
> -                       usleep_range(10, 20);
>                 }
>
> >
> > > +
> > > +
> > > +@nullrange@
> > > +expression E1,E2;
> > > +position p;
> > > +@@
> > > +
> > > +  usleep_range@p(E1,E2)
> > > +
> > > +@script:python depends on !context@
> > > +p << nullrange.p;
> > > +min << nullrange.E1;
> > > +max << nullrange.E2;
> > > +@@
> > > +
> > > +if(min == max):
> > > +   msg = "WARNING: usleep_range min == max (%s) - consider delta " % (min)
> > > +   coccilib.report.print_report(p[0], msg)
> > > +if str.isdigit(min):
> >
> > I guess this checks if min is a constant, but doesn't the last case also
> > need to check if max is a constant?
> >
> yes it does - seems that there simply was no
> such case so it went unnoticed.
>
> also just noticed that the org mode would also be using
> coccilib.report rather than coccilib.org...
>
> thx!
> hofrat
>
>

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

end of thread, other threads:[~2016-12-15  7:44 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-15  2:41 [PATCH V2] Coccinelle: check usleep_range() usage Nicholas Mc Guire
2016-12-15  5:52 ` Julia Lawall
2016-12-15  6:16   ` Nicholas Mc Guire
2016-12-15  7:44     ` Julia Lawall

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