On Thu, 28 May 2015, Julia Lawall wrote:
>
>
> On Wed, 27 May 2015, Nicholas Mc Guire wrote:
>
> > schedule_timeout_* takes a jiffies value as timeout - passing in a constant
> > makes the timeout HZ dependent which is wrong. Checking for contstants 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.
> >
> > Signed-off-by: Nicholas Mc Guire <hof...@osadl.org>
> > ---
> >
> > 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);
> >
> > For schedule_timeout_*() it reports 23 cases - all of which look like
> > they are correct findings.
> >
> > Further the list of functions taking jiffies as arguments is much longer
> > but they can be added once the basic script is sound - which it probably
> > is not at this point.
> >
> > scripts/coccinelle/api/timeout_HZ_dependent.cocci | 62
> > +++++++++++++++++++++
> > 1 file changed, 62 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..6e98da1
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/timeout_HZ_dependent.cocci
> > @@ -0,0 +1,62 @@
> > +/* check for hardcoded 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
> > + *
> > + * Options: --no-includes --include-headers
> > + */
> > +virtual context
> > +virtual patch
> > +virtual org
> > +virtual report
> > +
> > +@ep depends on !patch && (org || report)@
> > +identifier f;
> > +constant C;
> > +position p1,p2;
> > +@@
> > +
> > +f@p1(...) {
>
> Do you need the function for anything? This would be more efficient with
> just the calls.
>
nop - just forgot to remove it - thanks !
>
> > + <+...
> > +(
> > +* schedule_timeout@p2(C)
>
> If you put in the argument list \(i\|1\|C@p2\) then you can get rid of the
> python code (i would be an identifier metavariable).
*schedule_timeout(\(i\|1\|C@p\))
works but it is a lot slower than handling it with those 2 lines of
python script - about 75min vs. 1min 30sec ?
>
> What kind of false positives do you get for the macro case? Would it be
> sufficient to do:
>
> @r@
> expression e != 1;
> identifier i,
> @@
>
> #define i e
>
> @@
> identifier r.i,
> @@
>
> *schedule_timeout(i)
>
> (and all the other cases?)
>
this will work but has the same problem as (C@p) it will report
false positives like
./arch/x86/kernel/apm_32.c:1440:19-36: WARNING: timeout (APM_CHECK_TIMEOUT)
is HZ dependent
with
#define APM_CHECK_TIMEOUT (HZ)
this is not a bug.
and its also a lot slower than the brute force version with str.isdigit
thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
