Message-ID: <20171009025039.77oaplaovnvpy6nv@wfg-t540p.sh.intel.com>
Date:   Mon, 9 Oct 2017 10:50:39 +0800
From:   Fengguang Wu <fengguang.wu@...el.com>
To:     Julia Lawall <julia.lawall@...6.fr>
Cc:     Liam Breck <liam@...workimprov.net>,
        Sebastian Reichel <sebastian.reichel@...labora.co.uk>,
        Pali Rohár <pali.rohar@...il.com>,
        Linux PM mailing list <linux-pm@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>, cocci@...teme.lip6.fr,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
Subject: Re: [PATCH] coccinelle: api: detect duplicate chip data arrays

On Thu, Oct 05, 2017 at 09:40:51PM +0200, Julia Lawall wrote:
>
>
>On Thu, 5 Oct 2017, Liam Breck wrote:
>
>> Hi Julia,
>>
>> On Thu, Oct 5, 2017 at 12:25 PM, Julia Lawall <julia.lawall@...6.fr> wrote:
>> >
>> >
>> > On Thu, 5 Oct 2017, Liam Breck wrote:
>> >
>> >> Hi, sorry for slow reply...
>> >>
>> >> Can we patch something to make this script run by default on
>> >> bq7xxxx_battery_i2c build? If so let's do that.
>> >
>> > I don't think anything is set up for that.  But any changes to the file
>> > should be checked by the 0-day bot.
>>
>> How do we ask the 0-day bot team to configure their build to run this
>> script? Will they just pick it up?
>
>I put kbuild in CC.  I don't know what is the criterion for including
>semantic patches.

We may have 3 options:

1) push this patch to upstream, so that all users can run the check
   (with obvious cost)

2) try to optimize away the extra overheads by improving
   script/coccicheck to support conditional run of some cocci scripts

3) let 0-day maintain and run a collection of 3rd-party scripts if
   (1,2) turns out to be hard or not always applicable for some scripts

What do you think? At least it'd be easy and quick for us to implement (3).

Thanks,
Fengguang

>> >> Also maybe the name of the script should include "bq27xxx_data"?
>> >
>> > OK
>> >
>> >> Few more comments below...
>> >>
>> >> On Sun, Oct 1, 2017 at 5:42 AM, Julia Lawall <Julia.Lawall@...6.fr> wrote:
>> >> > This semantic patch detects duplicate arrays declared using BQ27XXX_DATA
>> >> > within a single structure.  It is currently specific to the file
>> >> > drivers/power/supply/bq27xxx_battery.c.
>> >> >
>> >> > Signed-off-by: Julia Lawall <Julia.Lawall@...6.fr>
>> >> >
>> >> > ---
>> >> >  scripts/coccinelle/api/battery.cocci |  161 +++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 161 insertions(+)
>> >> >
>> >> > diff --git a/scripts/coccinelle/api/battery.cocci b/scripts/coccinelle/api/battery.cocci
>> >> > new file mode 100644
>> >> > index 0000000..77c145a
>> >> > --- /dev/null
>> >> > +++ b/scripts/coccinelle/api/battery.cocci
>> >> > @@ -0,0 +1,161 @@
>> >> > +/// Detect BQ27XXX_DATA structures with identical registers, dm registers or
>> >> > +/// properties.
>> >> > +//# Doesn't unfold macros used in register or property fields.
>> >> > +//# Requires OCaml scripting
>> >> > +///
>> >> > +// Confidence: High
>> >> > +// Copyright: (C) 2017 Julia Lawall, Inria/LIP6, GPLv2.
>> >> > +// URL: http://coccinelle.lip6.fr/
>> >> > +// Requires: 1.0.7
>> >> > +// Keywords: BQ27XXX_DATA
>> >> > +
>> >> > +virtual report
>> >> > +
>> >> > +@...tialize:ocaml@
>> >> > +@@
>> >> > +
>> >> > +let print_report p msg =
>> >> > +  let p = List.hd p in
>> >> > +  Printf.printf "%s:%d:%d-%d: %s" p.file p.line p.col p.col_end msg
>> >> > +
>> >> > +@str depends on report@
>> >> > +type t;
>> >> > +identifier i,i1,i2;
>> >> > +expression e1,e2;
>> >> > +@@
>> >> > +
>> >> > +t i[] = {
>> >> > +  ...,
>> >> > +  [e1] = BQ27XXX_DATA(i1,...),
>> >> > +  ...,
>> >> > +  [e2] = BQ27XXX_DATA(i2,...),
>> >> > +  ...,
>> >> > +};
>> >> > +
>> >> > +@...ipt:ocaml tocheck@
>> >> > +i1 << str.i1;
>> >> > +i2 << str.i2;
>> >> > +i1regs; i2regs;
>> >> > +i1dmregs; i2dmregs;
>> >> > +i1props; i2props;
>> >> > +@@
>> >> > +
>> >> > +if not(i1 = i2)
>> >> > +then
>> >> > +  begin
>> >> > +    i1regs := make_ident (i1 ^ "_regs");
>> >> > +    i2regs := make_ident (i2 ^ "_regs");
>> >> > +    i1dmregs := make_ident (i1 ^ "_dm_regs");
>> >> > +    i2dmregs := make_ident (i2 ^ "_dm_regs");
>> >> > +    i1props := make_ident (i1 ^ "_props");
>> >> > +    i2props := make_ident (i2 ^ "_props")
>> >> > +  end
>> >> > +
>> >> > +(* ---------------------------------------------------------------- *)
>> >> > +
>> >> > +@...regs1@
>> >> > +typedef u8;
>> >> > +identifier tocheck.i1regs;
>> >> > +initializer list i1regs_vals;
>> >> > +position p1;
>> >> > +@@
>> >> > +
>> >> > +u8 i1regs@p1[...] = { i1regs_vals, };
>> >> > +
>> >> > +@...regs2@
>> >> > +identifier tocheck.i2regs;
>> >> > +initializer list i2regs_vals;
>> >> > +position p2;
>> >> > +@@
>> >> > +
>> >> > +u8 i2regs@p2[...] = { i2regs_vals, };
>> >> > +
>> >> > +@...ipt:ocaml@
>> >> > +(_,i1regs_vals) << getregs1.i1regs_vals;
>> >> > +(_,i2regs_vals) << getregs2.i2regs_vals;
>> >> > +i1regs << tocheck.i1regs;
>> >> > +i2regs << tocheck.i2regs;
>> >> > +p1 << getregs1.p1;
>> >> > +p2 << getregs2.p2;
>> >> > +@@
>> >> > +
>> >> > +if i1regs < i2regs &&
>> >> > +   List.sort compare i1regs_vals = List.sort compare i2regs_vals
>> >> > +then
>> >> > +  let msg =
>> >> > +    Printf.sprintf
>> >> > +      "WARNING %s and %s (line %d) have the same registers\n"
>> >>
>> >> "are identical" vs "have the same..."
>> >
>> > OK, I guess identical would be appropriate for regsand dm_regs, but
>> > perhaps not for properties because there the same values might be in a
>> > different order.
>>
>> The order of the properties has no impact, so a duplicate is
>> functionally identical.
>>
>> >
>> > julia
>> >
>> >> > +      i1regs i2regs (List.hd p2).line in
>> >> > +  print_report p1 msg
>> >> > +
>> >> > +(* ---------------------------------------------------------------- *)
>> >> > +
>> >> > +@...dmregs1@
>> >> > +identifier tocheck.i1dmregs;
>> >> > +initializer list i1dmregs_vals;
>> >> > +position p1;
>> >> > +@@
>> >> > +
>> >> > +struct bq27xxx_dm_reg i1dmregs@p1[] = { i1dmregs_vals, };
>> >> > +
>> >> > +@...dmregs2@
>> >> > +identifier tocheck.i2dmregs;
>> >> > +initializer list i2dmregs_vals;
>> >> > +position p2;
>> >> > +@@
>> >> > +
>> >> > +struct bq27xxx_dm_reg i2dmregs@p2[] = { i2dmregs_vals, };
>> >> > +
>> >> > +@...ipt:ocaml@
>> >> > +(_,i1dmregs_vals) << getdmregs1.i1dmregs_vals;
>> >> > +(_,i2dmregs_vals) << getdmregs2.i2dmregs_vals;
>> >> > +i1dmregs << tocheck.i1dmregs;
>> >> > +i2dmregs << tocheck.i2dmregs;
>> >> > +p1 << getdmregs1.p1;
>> >> > +p2 << getdmregs2.p2;
>> >> > +@@
>> >> > +
>> >> > +if i1dmregs < i2dmregs &&
>> >> > +   List.sort compare i1dmregs_vals = List.sort compare i2dmregs_vals
>> >> > +then
>> >> > +  let msg =
>> >> > +    Printf.sprintf
>> >> > +      "WARNING %s and %s (line %d) have the same dm registers\n"
>> >>
>> >> "are identical" vs "have the same..."
>> >>
>> >> > +      i1dmregs i2dmregs (List.hd p2).line in
>> >> > +  print_report p1 msg
>> >> > +
>> >> > +(* ---------------------------------------------------------------- *)
>> >> > +
>> >> > +@...props1@
>> >> > +identifier tocheck.i1props;
>> >> > +initializer list[n1] i1props_vals;
>> >> > +position p1;
>> >> > +@@
>> >> > +
>> >> > +enum power_supply_property i1props@p1[] = { i1props_vals, };
>> >> > +
>> >> > +@...props2@
>> >> > +identifier tocheck.i2props;
>> >> > +initializer list[n2] i2props_vals;
>> >> > +position p2;
>> >> > +@@
>> >> > +
>> >> > +enum power_supply_property i2props@p2[] = { i2props_vals, };
>> >> > +
>> >> > +@...ipt:ocaml@
>> >> > +(_,i1props_vals) << getprops1.i1props_vals;
>> >> > +(_,i2props_vals) << getprops2.i2props_vals;
>> >> > +i1props << tocheck.i1props;
>> >> > +i2props << tocheck.i2props;
>> >> > +p1 << getprops1.p1;
>> >> > +p2 << getprops2.p2;
>> >> > +@@
>> >> > +
>> >> > +if i1props < i2props &&
>> >> > +   List.sort compare i1props_vals = List.sort compare i2props_vals
>> >> > +then
>> >> > +  let msg =
>> >> > +    Printf.sprintf
>> >> > +      "WARNING %s and %s (line %d) have the same properties\n"
>> >>
>> >> "are identical" vs "have the same..."
>> >>
>> >>
>> >> > +      i1props i2props (List.hd p2).line in
>> >> > +  print_report p1 msg
>> >> >
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> >> the body of a message to majordomo@...r.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>
>>

Powered by blists - more mailing lists