| Message ID | 5437756c65c79f9520886bc54321d39c022c8638.1686739018.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [1/2] OPP: pstate is only valid for genpd OPP tables | expand |
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 >
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@linaro.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. Regards, Bjorn
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@linaro.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.
On Fri, Jun 16, 2023 at 11:48:30AM +0530, Viresh Kumar wrote: > 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@linaro.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. > Looks good, thanks for the quick response. Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com> Tested-by: Bjorn Andersson <quic_bjorande@quicinc.com> Regards, Bjorn
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;
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> --- drivers/opp/core.c | 18 ++++++++++++++++-- drivers/opp/debugfs.c | 4 +++- drivers/opp/of.c | 6 ++++++ 3 files changed, 25 insertions(+), 3 deletions(-)