Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <d48d824ab3abacb2356878780979d7ed42191eaf.1596080365.git.viresh.kumar@linaro.org>
 <CAHLCerP4YPHc4sKD_RTq=Gxfj+ex4F=J2is1Y-UzGXcOuEOrOQ@mail.gmail.com>
 <20200730061041.gyprgwfkzfb64t3m@vireshk-mac-ubuntu> <CAHLCerMD_spZFHER-y9dOzr7qo9xKXZdqy3cFt+W9QUW4Ng3jw@mail.gmail.com>
 <20200730064112.lvbwas7zzqruvprk@vireshk-mac-ubuntu>
In-Reply-To: <20200730064112.lvbwas7zzqruvprk@vireshk-mac-ubuntu>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 30 Jul 2020 18:25:59 +0200
Message-ID: <CAJZ5v0ia-kjAboeREyDESxp9f_E_SVdDNj0aU4Xgxjf4-=QGTw@mail.gmail.com>
Subject: Re: [PATCH] cpufreq: cached_resolved_idx can not be negative
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     Amit Kucheria <amitk@kernel.org>,
        Rafael Wysocki <rjw@rjwysocki.net>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Linux PM list <linux-pm@vger.kernel.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ionela Voinescu <ionela.voinescu@arm.com>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 30, 2020 at 8:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 30-07-20, 12:02, Amit Kucheria wrote:
> > Looking at this more closely, I found another call site for
> > cpufreq_frequency_table_target() in cpufreq.c that needs the index to
> > be unsigned int.
> >
> > But then cpufreq_frequency_table_target() returns -EINVAL, so we
>
> It returns -EINVAL only in the case where the relation is not valid,
> which will never happen. Maybe that should be marked with WARN or BUG
> and we should drop return value of -EINVAL.
>
> Rafael ?

Yeah, make it a WARN_ON_ONCE() IMO.

> > should be able to handle int values.
>
> And so no.
>
> > I think you will need to fix the unconditional assignment of
> >     policy->cached_resolved_idx = idx
> > in cpufreq_driver_resolve_freq(). It doesn't check for -EINVAL, so the
> > qcom driver is write in checking for a negative value.
>
> Right, I don't want it to have that check for the reason stated above.
>
> The point is I don't want code that verifies cached-idx at all, it is
> useless.
>
> --
> viresh