Re: [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor

["Łukasz Bartosik" <lb@semihalf.com>]

>
> > > >
> > > > How about using list_empty()?
> > > >
> > > >         if (list_empty() || cn->clk != clk)
> > > >
> > > > Then the diff is very small.
> > >
> > > Good point. I will use list_empty(). Thanks
> >
> > Although your comment was appealing to make the diff neat and small I
> > realized it won't work after making the changes.
> > Looking at the original code:
> >     /* search the list of notifiers for this clk */
> >     list_for_each_entry(cn, &clk_notifier_list, node)
> >         if (cn->clk == clk)
> >         break;
> >
> >     /* if clk wasn't in the notifier list, allocate new clk_notifier */
> >     if (cn->clk != clk) {
> >
> > The list cursor (cn) in the comparison above will not be pointing to a
> > valid entry not only when the clk_notifier_list is empty but also in
> > the case when clk_notifier_list list was completely traversed without
> > breaking from the list_for_each_entry loop on one of the entries.
> > Therefore making the comparison
> >
> >     if (list_empty() || cn->clk != clk) {
> >
> > will not help because if the list is not empty and there was no match
> > cn will not point to a valid entry. I have not noticed that before.
> > Based on that I propose to stay with my original fix.
>
> Ok, so then use 'goto found' approach?
>

I will use goto approach and send v3 patch.

> ----8<----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5052541a0986..16634d5912be 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4357,20 +4357,19 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
>         /* search the list of notifiers for this clk */
>         list_for_each_entry(cn, &clk_notifier_list, node)
>                 if (cn->clk == clk)
> -                       break;
> +                       goto found;
>
>         /* if clk wasn't in the notifier list, allocate new clk_notifier */
> -       if (cn->clk != clk) {
> -               cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> -               if (!cn)
> -                       goto out;
> +       cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> +       if (!cn)
> +               goto out;
>
> -               cn->clk = clk;
> -               srcu_init_notifier_head(&cn->notifier_head);
> +       cn->clk = clk;
> +       srcu_init_notifier_head(&cn->notifier_head);
>
> -               list_add(&cn->node, &clk_notifier_list);
> -       }
> +       list_add(&cn->node, &clk_notifier_list);
>
> +found:
>         ret = srcu_notifier_chain_register(&cn->notifier_head, nb);
>
>         clk->core->notifier_count++;