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;
Date:   Wed, 23 Dec 2020 10:04:43 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Dmitry Osipenko <digetx@gmail.com>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Peter Geis <pgwipeout@gmail.com>,
        Nicolas Chauvet <kwizart@gmail.com>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Kevin Hilman <khilman@kernel.org>,
        Peter De Schrijver <pdeschrijver@nvidia.com>,
        Viresh Kumar <vireshk@kernel.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-media@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-clk@vger.kernel.org
Subject: Re: [PATCH v2 19/48] opp: Fix adding OPP entries in a wrong order if
 rate is unavailable
Message-ID: <20201223043443.rklw5er6hck3gl4y@vireshk-i7>
References: <20201217180638.22748-1-digetx@gmail.com>
 <20201217180638.22748-20-digetx@gmail.com>
 <20201222091255.wentz5hyt726qezg@vireshk-i7>
 <db6273e6-8406-b7ae-b51d-48ceb6d21962@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <db6273e6-8406-b7ae-b51d-48ceb6d21962@gmail.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

On 22-12-20, 22:19, Dmitry Osipenko wrote:
> 22.12.2020 12:12, Viresh Kumar пишет:
> > On 17-12-20, 21:06, Dmitry Osipenko wrote:
> >> Fix adding OPP entries in a wrong (opposite) order if OPP rate is
> >> unavailable. The OPP comparison is erroneously skipped if OPP rate is
> >> missing, thus OPPs are left unsorted.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/opp/core.c | 23 ++++++++++++-----------
> >>  drivers/opp/opp.h  |  2 +-
> >>  2 files changed, 13 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> >> index 34f7e530d941..5c7f130a8de2 100644
> >> --- a/drivers/opp/core.c
> >> +++ b/drivers/opp/core.c
> >> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
> >>  	return true;
> >>  }
> >>  
> >> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
> >> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
> >> +		     bool rate_not_available)
> >>  {
> >> -	if (opp1->rate != opp2->rate)
> >> +	if (!rate_not_available && opp1->rate != opp2->rate)
> > 
> > rate will be 0 for both the OPPs here if rate_not_available is true and so this
> > change shouldn't be required.
> 
> The rate_not_available is negated in the condition. This change is
> required because both rates are 0 and then we should proceed to the
> levels comparison.

Won't that happen without this patch ?

> I guess it's not clear by looking at this patch, please see a full
> version of the function:
> 
> int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
>          bool rate_not_available)
> {
>   if (!rate_not_available && opp1->rate != opp2->rate)
>     return opp1->rate < opp2->rate ? -1 : 1;
>   if (opp1->bandwidth && opp2->bandwidth &&
>       opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
>     return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
>   if (opp1->level != opp2->level)
>     return opp1->level < opp2->level ? -1 : 1;
>   return 0;
> }
> 
> Perhaps we could check whether opp1->rate=0, like it's done for the
> opp1->bandwidth. I'll consider this variant for v3, thanks.

-- 
viresh