[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230616061830.fysc7l7jxymmhb3m@vireshk-i7>
Date: Fri, 16 Jun 2023 11:48:30 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
Stephen Boyd <sboyd@...nel.org>, linux-pm@...r.kernel.org,
Vincent Guittot <vincent.guittot@...aro.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Ulf Hansson <ulf.hansson@...aro.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables
On 15-06-23, 18:35, Bjorn Andersson wrote:
> On Wed, Jun 14, 2023 at 04:07:25PM +0530, Viresh Kumar wrote:
> > It is not very clear right now that the `pstate` field is only valid for
> > genpd OPP tables and not consumer tables. And there is no checking for
> > the same at various places.
> >
> > Add checks in place to verify that and make it clear to the reader.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> > ---
> > drivers/opp/core.c | 18 ++++++++++++++++--
> > drivers/opp/debugfs.c | 4 +++-
> > drivers/opp/of.c | 6 ++++++
> > 3 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> [..]
> > @@ -2686,6 +2694,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
> > int dest_pstate = -EINVAL;
> > int i;
> >
> > + /* Both OPP tables must belong to genpds */
> > + if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
>
> I have a platform_device, with a required-opps and with an associated
> genpd which does not implement set_performance_state(), but its parent
> genpd does.
>
> This results in me arriving here with src_table of NULL, and boom...
>
>
> Looking at the very next statement in this function, it seems arriving
> here without src_table was valid up until this change.
Fixed as:
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 48ddd72d2c71..3f46e499d615 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2694,12 +2694,6 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
int dest_pstate = -EINVAL;
int i;
- /* Both OPP tables must belong to genpds */
- if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
- pr_err("%s: Performance state is only valid for genpds.\n", __func__);
- return -EINVAL;
- }
-
/*
* Normally the src_table will have the "required_opps" property set to
* point to one of the OPPs in the dst_table, but in some cases the
@@ -2710,6 +2704,12 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
if (!src_table || !src_table->required_opp_count)
return pstate;
+ /* Both OPP tables must belong to genpds */
+ if (unlikely(!src_table->is_genpd || !dst_table->is_genpd)) {
+ pr_err("%s: Performance state is only valid for genpds.\n", __func__);
+ return -EINVAL;
+ }
+
/* required-opps not fully initialized yet */
if (lazy_linking_pending(src_table))
return -EBUSY;
Thanks.
--
viresh
Powered by blists - more mailing lists