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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Jul 2019 18:41:44 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     MyungJoo Ham <myungjoo.ham@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Sibi Sankar <sibis@...eaurora.org>,
        Android Kernel Team <kernel-team@...roid.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/5] OPP: Add function to look up required OPP's for a
 given OPP

On Wed, Jul 24, 2019 at 10:38 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 24-07-19, 20:46, Saravana Kannan wrote:
> > On Wed, Jul 24, 2019 at 7:58 PM Viresh Kumar <viresh.kumar@...aro.org> 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