Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <5437756c65c79f9520886bc54321d39c022c8638.1686739018.git.viresh.kumar@linaro.org>
In-Reply-To: <5437756c65c79f9520886bc54321d39c022c8638.1686739018.git.viresh.kumar@linaro.org>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 14 Jun 2023 14:33:38 +0200
Message-ID: <CAPDyKFoXW65mzNnuZD+Zcjg6dTdWWNqOO6TB+685EAG1x9P8iQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
        Stephen Boyd <sboyd@kernel.org>, linux-pm@vger.kernel.org,
        Vincent Guittot <vincent.guittot@linaro.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 14 Jun 2023 at 12:37, Viresh Kumar <viresh.kumar@linaro.org> 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@linaro.org>

Yes, this makes the code more clear!

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  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
> index 7290168ec806..bfb012f5383c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -227,16 +227,24 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_level);
>  unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
>                                             unsigned int index)
>  {
> +       struct opp_table *opp_table = opp->opp_table;
> +
>         if (IS_ERR_OR_NULL(opp) || !opp->available ||
> -           index >= opp->opp_table->required_opp_count) {
> +           index >= opp_table->required_opp_count) {
>                 pr_err("%s: Invalid parameters\n", __func__);
>                 return 0;
>         }
>
>         /* required-opps not fully initialized yet */
> -       if (lazy_linking_pending(opp->opp_table))
> +       if (lazy_linking_pending(opp_table))
>                 return 0;
>
> +       /* The required OPP table must belong to a genpd */
> +       if (unlikely(!opp_table->required_opp_tables[index]->is_genpd)) {
> +               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
> +               return 0;
> +       }
> +
>         return opp->required_opps[index]->pstate;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
> @@ -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)) {
> +               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
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 2c7fb683441e..0cc21e2b42ff 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -152,11 +152,13 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
>         debugfs_create_bool("dynamic", S_IRUGO, d, &opp->dynamic);
>         debugfs_create_bool("turbo", S_IRUGO, d, &opp->turbo);
>         debugfs_create_bool("suspend", S_IRUGO, d, &opp->suspend);
> -       debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
>         debugfs_create_u32("level", S_IRUGO, d, &opp->level);
>         debugfs_create_ulong("clock_latency_ns", S_IRUGO, d,
>                              &opp->clock_latency_ns);
>
> +       if (opp_table->is_genpd)
> +               debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
> +
>         opp->of_name = of_node_full_name(opp->np);
>         debugfs_create_str("of_name", S_IRUGO, d, (char **)&opp->of_name);
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 943c7fb7402b..e23ce6e78eb6 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1392,6 +1392,12 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
>                 goto put_required_np;
>         }
>
> +       /* The OPP tables must belong to a genpd */
> +       if (unlikely(!opp_table->is_genpd)) {
> +               pr_err("%s: Performance state is only valid for genpds.\n", __func__);
> +               goto put_required_np;
> +       }
> +
>         opp = _find_opp_of_np(opp_table, required_np);
>         if (opp) {
>                 pstate = opp->pstate;
> --
> 2.31.1.272.g89b43f80a514
>