Re: [Outreachy kernel] [PATCH] gpu: drm: Use list_{next/prev}_entry instead of list_entry

[Julia Lawall]

On Sun, 25 Mar 2018, Arushi Singhal wrote:

>
>
> On Mon, Mar 19, 2018 at 12:44 PM, Julia Lawall <julia.lawall@xxxxxxx> wrote:
>
>
>       On Mon, 19 Mar 2018, Arushi Singhal wrote:
>
>       > This patch replace list_entry with list_{next/prev}_entry as
>       it makes
>       > the code more clear to read.
>       > Done using coccinelle:
>       >
>       > @@
>       > expression e1;
>       > identifier e3;
>       > type t;
>       > @@
>       > (
>       > - list_entry(e1->e3.next,t,e3)
>       > + list_next_entry(e1,e3)
>       > |
>       > - list_entry(e1->e3.prev,t,e3)
>       > + list_prev_entry(e1,e3)
>       > )
>
>       This looks like a rule that could be nice for the Linux kernel
>       in general,
>       because the code really is much simpler.
>
>       I would suggest to write the rule in a more robust way, as
>       follows:
>
>       @@
>       identifier e3;
>       type t;
>       t *e1;
>       @@
>
>       (
>       - list_entry(e1->e3.next,t,e3)
>       + list_next_entry(e1,e3)
>       |
>       - list_entry(e1->e3.prev,t,e3)
>       + list_prev_entry(e1,e3)
>       )
>
>       @@
>       expression e1;
>       identifier e3;
>       @@
>
>       (
>       - list_entry(e1->e3.next,typeof(*e1),e3)
>       + list_next_entry(e1,e3)
>       |
>       - list_entry(e1->e3.prev,typeof(*e1),e3)
>       + list_prev_entry(e1,e3)
>
>       This checks that the type that is specified corresponds to the
>       one on e1.
>       It could actually be that the call is getting the first element
>       of a list,
>       from some different type, and coincidentally the two types have
>       the same
>       field name for the list element.
>
>       Unfortunately, the second rule, with the typeof call, doesn't
>       currently
>       work in Coccinelle, because the semantic patch language doesn't
>       actually
>       support typeof, and thinks that it is a function call. I will
>       fix this.
>
>       To make a semantic patch for the kernel, you can try running
>       spgen on the
>       above file and answer the questions that it asks. You can find
>       examples
>       in the coccinelle/scripts directory. Just run
>
>       spgen foo.cocci
>
>       Then answer the questions. Then run
>
>       spgen foo.cocci > foo_for_kernel.cocci
>
>       The second run will use the results of the first run to print
>       the semantic
>       patch. Let me know if you have any questions. You can always
>       adjust the
>       semantic patch that is generated by hand afterwards if needed.
>
>
> Hi Julia,
>
> I tried spgen and found that second rule is still not working. It's not able
> to detect the second rule.
> Is it working for you?

If you get the latest version of Coccinelle from github, it shoudl be
working now.

julia

>
> Thanks,
> Arushi
>
>
>       julia
>
>
>       >
>       > Signed-off-by: Arushi Singhal
>       <arushisinghal19971997@xxxxxxxxx>
>       > ---
>       > drivers/gpu/drm/drm_lease.c          | 2 +-
>       >Â drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 2 +-
>       >Â 2 files changed, 2 insertions(+), 2 deletions(-)
>       >
>       > diff --git a/drivers/gpu/drm/drm_lease.c
>       b/drivers/gpu/drm/drm_lease.c
>       > index 1402c0e..4dcfb5f 100644
>       > --- a/drivers/gpu/drm/drm_lease.c
>       > +++ b/drivers/gpu/drm/drm_lease.c
>       > @@ -340,7 +340,7 @@ static void _drm_lease_revoke(struct
>       drm_master *top)
>       >Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âbreak;
>       >
>       >Â Â Â Â Â Â Â Â Â Â Â Â/* Over */
>       > -Â Â Â Â Â Â Â Â Â Â Âmaster =
>       list_entry(master->lessee_list.next, struct drm_master,
>       lessee_list);
>       > +Â Â Â Â Â Â Â Â Â Â Âmaster = list_next_entry(master,
>       lessee_list);
>       >Â Â Â Â Â Â Â Â}
>       >Â Â Â Â}
>       >Â }
>       > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
>       b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
>       > index e4c8d31..81c3567 100644
>       > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
>       > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
>       > @@ -134,7 +134,7 @@ nvkm_cstate_find_best(struct nvkm_clk
>       *clk, struct nvkm_pstate *pstate,
>       >Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â nvkm_volt_map(volt,
>       volt->max2_id, clk->temp));
>       >
>       >Â Â Â Âfor (cstate = start; &cstate->head != &pstate->list;
>       > -Â Â Â Â Â cstate = list_entry(cstate->head.prev,
>       typeof(*cstate), head)) {
>       > +Â Â Â Â Â cstate = list_prev_entry(cstate, head)) {
>       >Â Â Â Â Â Â Â Âif (nvkm_cstate_valid(clk, cstate, max_volt,
>       clk->temp))
>       >Â Â Â Â Â Â Â Â Â Â Â Âbreak;
>       >Â Â Â Â}
>       > --
>       > 2.7.4
>       >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it,
> send an email to outreachy-kernel+unsubscribe@xxxxxxxxxxxxxxxxx
> > To post to this group, send email to
> outreachy-kernel@xxxxxxxxxxxxxxxxx
> > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/20180319050530.GA25589%4
> 0seema-Inspiron-15-3567.
> > For more options, visit https://groups.google.com/d/optout.
> >
>
>
>
>