* [PATCH 2/2] OPP: Simplify the over-designed pstate <-> level dance
2023-06-14 10:37 [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables Viresh Kumar
@ 2023-06-14 10:37 ` Viresh Kumar
2023-06-14 12:37 ` Ulf Hansson
2023-06-14 12:33 ` [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables Ulf Hansson
2023-06-15 23:35 ` Bjorn Andersson
2 siblings, 1 reply; 8+ messages in thread
From: Viresh Kumar @ 2023-06-14 10:37 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael J. Wysocki,
Ulf Hansson, linux-kernel
While adding support for "performance states" in the OPP and genpd core,
it was decided to set the `pstate` field via genpd's
pm_genpd_opp_to_performance_state() helper, to allow platforms to set
`pstate` even if they don't have a corresponding `level` field in the DT
OPP tables (More details are present in commit 6e41766a6a50 ("PM /
Domain: Implement of_genpd_opp_to_performance_state()")).
Revisiting that five years later clearly suggests that it was
over-designed as all current users are eventually using the `level`
value only.
The previous commit already added necessary checks to make sure pstate
is only used for genpd tables. Lets now simplify this a little, and use
`level` directly and remove `pstate` field altogether.
Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 8 ++++----
drivers/opp/debugfs.c | 2 +-
drivers/opp/of.c | 5 +----
drivers/opp/opp.h | 2 --
4 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index bfb012f5383c..48ddd72d2c71 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -245,7 +245,7 @@ unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
return 0;
}
- return opp->required_opps[index]->pstate;
+ return opp->required_opps[index]->level;
}
EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
@@ -943,7 +943,7 @@ static int _set_opp_bw(const struct opp_table *opp_table,
static int _set_performance_state(struct device *dev, struct device *pd_dev,
struct dev_pm_opp *opp, int i)
{
- unsigned int pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
+ unsigned int pstate = likely(opp) ? opp->required_opps[i]->level: 0;
int ret;
if (!pd_dev)
@@ -2728,8 +2728,8 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
mutex_lock(&src_table->lock);
list_for_each_entry(opp, &src_table->opp_list, node) {
- if (opp->pstate == pstate) {
- dest_pstate = opp->required_opps[i]->pstate;
+ if (opp->level == pstate) {
+ dest_pstate = opp->required_opps[i]->level;
goto unlock;
}
}
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 0cc21e2b42ff..954ea31a2ff3 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -157,7 +157,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
&opp->clock_latency_ns);
if (opp_table->is_genpd)
- debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
+ debugfs_create_u32("performance_state", S_IRUGO, d, &opp->level);
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 e23ce6e78eb6..e6d1155d0990 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -945,9 +945,6 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
if (ret)
goto free_required_opps;
- if (opp_table->is_genpd)
- new_opp->pstate = pm_genpd_opp_to_performance_state(dev, new_opp);
-
ret = _opp_add(dev, new_opp, opp_table);
if (ret) {
/* Don't return error for duplicate OPPs */
@@ -1400,7 +1397,7 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
opp = _find_opp_of_np(opp_table, required_np);
if (opp) {
- pstate = opp->pstate;
+ pstate = opp->level;
dev_pm_opp_put(opp);
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 3805b92a6100..8a5ea38f3a3d 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -78,7 +78,6 @@ struct opp_config_data {
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
* @removed: flag indicating that OPP's reference is dropped by OPP core.
- * @pstate: Device's power domain's performance state.
* @rates: Frequencies in hertz
* @level: Performance level
* @supplies: Power supplies voltage/current values
@@ -101,7 +100,6 @@ struct dev_pm_opp {
bool turbo;
bool suspend;
bool removed;
- unsigned int pstate;
unsigned long *rates;
unsigned int level;
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] OPP: Simplify the over-designed pstate <-> level dance
2023-06-14 10:37 ` [PATCH 2/2] OPP: Simplify the over-designed pstate <-> level dance Viresh Kumar
@ 2023-06-14 12:37 ` Ulf Hansson
2023-06-16 6:20 ` Viresh Kumar
0 siblings, 1 reply; 8+ messages in thread
From: Ulf Hansson @ 2023-06-14 12:37 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On Wed, 14 Jun 2023 at 12:37, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> While adding support for "performance states" in the OPP and genpd core,
> it was decided to set the `pstate` field via genpd's
> pm_genpd_opp_to_performance_state() helper, to allow platforms to set
> `pstate` even if they don't have a corresponding `level` field in the DT
> OPP tables (More details are present in commit 6e41766a6a50 ("PM /
> Domain: Implement of_genpd_opp_to_performance_state()")).
>
> Revisiting that five years later clearly suggests that it was
> over-designed as all current users are eventually using the `level`
> value only.
>
> The previous commit already added necessary checks to make sure pstate
> is only used for genpd tables. Lets now simplify this a little, and use
> `level` directly and remove `pstate` field altogether.
>
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Again, thanks for improving this code!
Only a minor thing below and after that, feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/opp/core.c | 8 ++++----
> drivers/opp/debugfs.c | 2 +-
> drivers/opp/of.c | 5 +----
> drivers/opp/opp.h | 2 --
> 4 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index bfb012f5383c..48ddd72d2c71 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -245,7 +245,7 @@ unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
> return 0;
> }
>
> - return opp->required_opps[index]->pstate;
> + return opp->required_opps[index]->level;
> }
> EXPORT_SYMBOL_GPL(dev_pm_opp_get_required_pstate);
>
> @@ -943,7 +943,7 @@ static int _set_opp_bw(const struct opp_table *opp_table,
> static int _set_performance_state(struct device *dev, struct device *pd_dev,
> struct dev_pm_opp *opp, int i)
> {
> - unsigned int pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
> + unsigned int pstate = likely(opp) ? opp->required_opps[i]->level: 0;
> int ret;
>
> if (!pd_dev)
> @@ -2728,8 +2728,8 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
> mutex_lock(&src_table->lock);
>
> list_for_each_entry(opp, &src_table->opp_list, node) {
> - if (opp->pstate == pstate) {
> - dest_pstate = opp->required_opps[i]->pstate;
> + if (opp->level == pstate) {
> + dest_pstate = opp->required_opps[i]->level;
> goto unlock;
> }
> }
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 0cc21e2b42ff..954ea31a2ff3 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -157,7 +157,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
> &opp->clock_latency_ns);
>
> if (opp_table->is_genpd)
> - debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate);
> + debugfs_create_u32("performance_state", S_IRUGO, d, &opp->level);
I think this should be dropped altogether. The "performance_state"
node is just confusing - and we already have a node for "level" a few
lines above.
>
> 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 e23ce6e78eb6..e6d1155d0990 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -945,9 +945,6 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> if (ret)
> goto free_required_opps;
>
> - if (opp_table->is_genpd)
> - new_opp->pstate = pm_genpd_opp_to_performance_state(dev, new_opp);
> -
> ret = _opp_add(dev, new_opp, opp_table);
> if (ret) {
> /* Don't return error for duplicate OPPs */
> @@ -1400,7 +1397,7 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
>
> opp = _find_opp_of_np(opp_table, required_np);
> if (opp) {
> - pstate = opp->pstate;
> + pstate = opp->level;
> dev_pm_opp_put(opp);
> }
>
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 3805b92a6100..8a5ea38f3a3d 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -78,7 +78,6 @@ struct opp_config_data {
> * @turbo: true if turbo (boost) OPP
> * @suspend: true if suspend OPP
> * @removed: flag indicating that OPP's reference is dropped by OPP core.
> - * @pstate: Device's power domain's performance state.
> * @rates: Frequencies in hertz
> * @level: Performance level
> * @supplies: Power supplies voltage/current values
> @@ -101,7 +100,6 @@ struct dev_pm_opp {
> bool turbo;
> bool suspend;
> bool removed;
> - unsigned int pstate;
> unsigned long *rates;
> unsigned int level;
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] OPP: Simplify the over-designed pstate <-> level dance
2023-06-14 12:37 ` Ulf Hansson
@ 2023-06-16 6:20 ` Viresh Kumar
0 siblings, 0 replies; 8+ messages in thread
From: Viresh Kumar @ 2023-06-16 6:20 UTC (permalink / raw)
To: Ulf Hansson
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On 14-06-23, 14:37, Ulf Hansson wrote:
> On Wed, 14 Jun 2023 at 12:37, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > While adding support for "performance states" in the OPP and genpd core,
> > it was decided to set the `pstate` field via genpd's
> > pm_genpd_opp_to_performance_state() helper, to allow platforms to set
> > `pstate` even if they don't have a corresponding `level` field in the DT
> > OPP tables (More details are present in commit 6e41766a6a50 ("PM /
> > Domain: Implement of_genpd_opp_to_performance_state()")).
> >
> > Revisiting that five years later clearly suggests that it was
> > over-designed as all current users are eventually using the `level`
> > value only.
> >
> > The previous commit already added necessary checks to make sure pstate
> > is only used for genpd tables. Lets now simplify this a little, and use
> > `level` directly and remove `pstate` field altogether.
> >
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Again, thanks for improving this code!
>
> Only a minor thing below and after that, feel free to add:
Done.
--
viresh
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables
2023-06-14 10:37 [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables Viresh Kumar
2023-06-14 10:37 ` [PATCH 2/2] OPP: Simplify the over-designed pstate <-> level dance Viresh Kumar
@ 2023-06-14 12:33 ` Ulf Hansson
2023-06-15 23:35 ` Bjorn Andersson
2 siblings, 0 replies; 8+ messages in thread
From: Ulf Hansson @ 2023-06-14 12:33 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables
2023-06-14 10:37 [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables Viresh Kumar
2023-06-14 10:37 ` [PATCH 2/2] OPP: Simplify the over-designed pstate <-> level dance Viresh Kumar
2023-06-14 12:33 ` [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables Ulf Hansson
@ 2023-06-15 23:35 ` Bjorn Andersson
2023-06-16 6:18 ` Viresh Kumar
2 siblings, 1 reply; 8+ messages in thread
From: Bjorn Andersson @ 2023-06-15 23:35 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, Ulf Hansson, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables
2023-06-15 23:35 ` Bjorn Andersson
@ 2023-06-16 6:18 ` Viresh Kumar
2023-06-16 14:31 ` Bjorn Andersson
0 siblings, 1 reply; 8+ messages in thread
From: Viresh Kumar @ 2023-06-16 6:18 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, Ulf Hansson, linux-kernel
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.
--
viresh
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] OPP: pstate is only valid for genpd OPP tables
2023-06-16 6:18 ` Viresh Kumar
@ 2023-06-16 14:31 ` Bjorn Andersson
0 siblings, 0 replies; 8+ messages in thread
From: Bjorn Andersson @ 2023-06-16 14:31 UTC (permalink / raw)
To: Viresh Kumar
Cc: Bjorn Andersson, Viresh Kumar, Nishanth Menon, Stephen Boyd,
linux-pm, Vincent Guittot, Rafael J. Wysocki, Ulf Hansson,
linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread