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 <[email protected]>
---
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, 14 Jun 2023 at 12:37, Viresh Kumar <[email protected]> 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 <[email protected]>
Yes, this makes the code more clear!
Reviewed-by: Ulf Hansson <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
Tested-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn