Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a given OPP

[Saravana Kannan]

On Wed, Jul 24, 2019 at 10:38 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 24-07-19, 20:46, Saravana Kannan wrote:
> > On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > > On 23-07-19, 17:23, Saravana Kannan wrote:
>
> > > > I almost said "not sure. Let me just compare pointers".
> > > > I think (not sure) it has to do with the same OPP table being used to
> > > > create multiple OPP table copies if the "shared OPP table" flag isn't
> > > > set?
> > > > Can you confirm if this makes sense? If so, I can add a comment patch
> > > > that adds comments to the existing code and then copies it into this
> > > > function in this patch.
> > >
> > > Right, that was the reason but we also need to fix ...
> >
> > I know I gave that explanation but I'm still a bit confused by the
> > existing logic. If the same DT OPP table is used to create multiple in
> > memory OPP tables, how do you device which in memory OPP table is the
> > right one to point to?
>
> This is a bit broken actually, we don't see any problems right now but
> may eventually have to fix it someday.
>
> We pick the first in-memory OPP table that was created using the DT
> OPP table. This is done because the DT doesn't provide any explicit
> linking to the required-opp device right now.
>
> Right now the required-opps is only used for power domains and so it
> is working fine. It may work fine for your case as well. But once we
> have a case we want to use required-opps in a single OPP table for
> both power-domains and master/slave thing you are proposing, we may
> see more problems.
>
> > > > > > +                     break;
> > > > > > +     }
> > > > > > +
> > > > > > +     if (unlikely(i == src_table->required_opp_count)) {
> > > > > > +             pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
> > > > > > +                    __func__, src_table, dst_table);
> > > > > > +             return NULL;
> > > > > > +     }
> > > > > > +
> > > > > > +     mutex_lock(&src_table->lock);
> > > > > > +
> > > > > > +     list_for_each_entry(opp, &src_table->opp_list, node) {
> > > > > > +             if (opp == src_opp) {
> > >
> > > ... this as well. We must be comparing node pointers here as well.
> >
> > Not really, if an in memory OPP entry is not part of an in memory OPP
> > table list, I don't think it should be considered part of the OPP
> > table just because the node pointer is the same. I think that's
> > explicitly wrong and the above code is correct as is.
>
> I understand what you are saying, but because we match the very first
> OPP table that was there in the list we need to match the DT node here
> as well.
>
> Or somehow we make sure to have the correct in-memory OPP table being
> pointed by the required-opp-table array. Then we don't need the node
> pointer anywhere here.

Ah, right. I'll fix this.

-Saravana