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:   Fri, 4 Jun 2021 09:23:12 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        Dmitry Osipenko <digetx@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Stephan Gerhold <stephan@gerhold.net>,
        Roja Rani Yarubandi <rojay@codeaurora.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/4] PM: domains: Drop/restore performance state votes
 for devices at runtime PM
Message-ID: <20210604035312.jp2gshfigsodwvcg@vireshk-i7>
References: <20210603093438.138705-1-ulf.hansson@linaro.org>
 <20210603093438.138705-4-ulf.hansson@linaro.org>
 <20210603095538.b2t3cq25tq7v7kih@vireshk-i7>
 <CAPDyKFqNNeeMo6+gKgaPtPvgC_NuMxxYKkr+TzyP3vjYoHoDOw@mail.gmail.com>
 <CAPDyKFofsuY_RAMGsRLtKo=JxJ11DgGqOijZEEf1HEANCvomzg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPDyKFofsuY_RAMGsRLtKo=JxJ11DgGqOijZEEf1HEANCvomzg@mail.gmail.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

On 03-06-21, 13:17, Ulf Hansson wrote:
> On Thu, 3 Jun 2021 at 12:31, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > +static int genpd_drop_performance_state(struct device *dev)
> > > > +{
> > > > +     unsigned int prev_state = dev_gpd_data(dev)->performance_state;
> > > > +
> > > > +     if (!genpd_set_performance_state(dev, 0))
> > > > +             return prev_state;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void genpd_restore_performance_state(struct device *dev,
> > > > +                                         unsigned int state)
> > > > +{
> > > > +     if (state)
> > >
> > > I will skip this check, as we are checking it in
> > > genpd_set_performance_state() anyway ?
> >
> > I don't want us to override OPP votes made by the subsystem/driver
> > level runtime PM callbacks. For example, if the drivers manage this
> > thing themselves, that should be preserved.
> >
> > That said, by the check above I want to avoid setting the state to
> > zero internally by genpd, if the driver level ->runtime_resume()
> > callback has already restored the state.
> 
> Ehh, forget about what I said about the ->runtime_resume() callback.
> 
> I am mostly trying to avoid restoring a state that is zero, just to be
> sure nobody else on some different level outside gendp, have decided
> to set a new OPP in-between our calls to
> genpd_drop|restore_performance state.

What stops the core to call genpd_drop_performance_state() in the
first place here, if the driver was doing its own thing ? If that gets
called, then restore should be without any checks IMO. The state
should already be 0 at this point of time, I don't know why this will
get called again with state 0, but it will have no effect.

Can you give some sort of flow sequence where I can see the problem a
bit more clearly ?

-- 
viresh