* [PATCH 1/5] OPP: Fix formatting of if/else block
2023-10-13 8:48 [PATCH 0/5] OPP: Minor cleanups Viresh Kumar
@ 2023-10-13 8:48 ` Viresh Kumar
2023-10-16 10:11 ` Ulf Hansson
2023-10-13 8:48 ` [PATCH 2/5] OPP: Add _link_required_opps() to avoid code duplication Viresh Kumar
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-10-13 8:48 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael J. Wysocki,
Ulf Hansson, linux-kernel
Add {} to both if else blocks or none.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/of.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index ae5c405bbf9a..85e2af3d6a49 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -208,9 +208,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
mutex_lock(&opp_table_lock);
list_add(&opp_table->lazy, &lazy_opp_tables);
mutex_unlock(&opp_table_lock);
- }
- else
+ } else {
_update_set_required_opps(opp_table);
+ }
goto put_np;
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] OPP: Fix formatting of if/else block
2023-10-13 8:48 ` [PATCH 1/5] OPP: Fix formatting of if/else block Viresh Kumar
@ 2023-10-16 10:11 ` Ulf Hansson
0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2023-10-16 10:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On Fri, 13 Oct 2023 at 10:48, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Add {} to both if else blocks or none.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/opp/of.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index ae5c405bbf9a..85e2af3d6a49 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -208,9 +208,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
> mutex_lock(&opp_table_lock);
> list_add(&opp_table->lazy, &lazy_opp_tables);
> mutex_unlock(&opp_table_lock);
> - }
> - else
> + } else {
> _update_set_required_opps(opp_table);
> + }
>
> goto put_np;
>
> --
> 2.31.1.272.g89b43f80a514
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] OPP: Add _link_required_opps() to avoid code duplication
2023-10-13 8:48 [PATCH 0/5] OPP: Minor cleanups Viresh Kumar
2023-10-13 8:48 ` [PATCH 1/5] OPP: Fix formatting of if/else block Viresh Kumar
@ 2023-10-13 8:48 ` Viresh Kumar
2023-10-16 10:11 ` Ulf Hansson
2023-10-13 8:48 ` [PATCH 3/5] OPP: Reorder code in _opp_set_required_opps_genpd() Viresh Kumar
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-10-13 8:48 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael J. Wysocki,
Ulf Hansson, linux-kernel
Factor out _link_required_opps() to remove duplicate code. No functional
change.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/of.c | 62 ++++++++++++++++++++++--------------------------
1 file changed, 29 insertions(+), 33 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 85e2af3d6a49..81fa27599d58 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -296,24 +296,41 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp)
of_node_put(opp->np);
}
+static int _link_required_opps(struct dev_pm_opp *opp,
+ struct opp_table *required_table, int index)
+{
+ struct device_node *np;
+
+ np = of_parse_required_opp(opp->np, index);
+ if (unlikely(!np))
+ return -ENODEV;
+
+ opp->required_opps[index] = _find_opp_of_np(required_table, np);
+ of_node_put(np);
+
+ if (!opp->required_opps[index]) {
+ pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
+ __func__, opp->np, index);
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
/* Populate all required OPPs which are part of "required-opps" list */
static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
struct dev_pm_opp *opp)
{
- struct dev_pm_opp **required_opps;
struct opp_table *required_table;
- struct device_node *np;
int i, ret, count = opp_table->required_opp_count;
if (!count)
return 0;
- required_opps = kcalloc(count, sizeof(*required_opps), GFP_KERNEL);
- if (!required_opps)
+ opp->required_opps = kcalloc(count, sizeof(*opp->required_opps), GFP_KERNEL);
+ if (!opp->required_opps)
return -ENOMEM;
- opp->required_opps = required_opps;
-
for (i = 0; i < count; i++) {
required_table = opp_table->required_opp_tables[i];
@@ -321,21 +338,9 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
if (IS_ERR_OR_NULL(required_table))
continue;
- np = of_parse_required_opp(opp->np, i);
- if (unlikely(!np)) {
- ret = -ENODEV;
- goto free_required_opps;
- }
-
- required_opps[i] = _find_opp_of_np(required_table, np);
- of_node_put(np);
-
- if (!required_opps[i]) {
- pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
- __func__, opp->np, i);
- ret = -ENODEV;
+ ret = _link_required_opps(opp, required_table, i);
+ if (ret)
goto free_required_opps;
- }
}
return 0;
@@ -350,22 +355,13 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
static int lazy_link_required_opps(struct opp_table *opp_table,
struct opp_table *new_table, int index)
{
- struct device_node *required_np;
struct dev_pm_opp *opp;
+ int ret;
list_for_each_entry(opp, &opp_table->opp_list, node) {
- required_np = of_parse_required_opp(opp->np, index);
- if (unlikely(!required_np))
- return -ENODEV;
-
- opp->required_opps[index] = _find_opp_of_np(new_table, required_np);
- of_node_put(required_np);
-
- if (!opp->required_opps[index]) {
- pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
- __func__, opp->np, index);
- return -ENODEV;
- }
+ ret = _link_required_opps(opp, new_table, index);
+ if (ret)
+ return ret;
}
return 0;
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/5] OPP: Add _link_required_opps() to avoid code duplication
2023-10-13 8:48 ` [PATCH 2/5] OPP: Add _link_required_opps() to avoid code duplication Viresh Kumar
@ 2023-10-16 10:11 ` Ulf Hansson
0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2023-10-16 10:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On Fri, 13 Oct 2023 at 10:48, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Factor out _link_required_opps() to remove duplicate code. No functional
> change.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/opp/of.c | 62 ++++++++++++++++++++++--------------------------
> 1 file changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 85e2af3d6a49..81fa27599d58 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -296,24 +296,41 @@ void _of_clear_opp(struct opp_table *opp_table, struct dev_pm_opp *opp)
> of_node_put(opp->np);
> }
>
> +static int _link_required_opps(struct dev_pm_opp *opp,
> + struct opp_table *required_table, int index)
> +{
> + struct device_node *np;
> +
> + np = of_parse_required_opp(opp->np, index);
> + if (unlikely(!np))
> + return -ENODEV;
> +
> + opp->required_opps[index] = _find_opp_of_np(required_table, np);
> + of_node_put(np);
> +
> + if (!opp->required_opps[index]) {
> + pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
> + __func__, opp->np, index);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> /* Populate all required OPPs which are part of "required-opps" list */
> static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
> struct dev_pm_opp *opp)
> {
> - struct dev_pm_opp **required_opps;
> struct opp_table *required_table;
> - struct device_node *np;
> int i, ret, count = opp_table->required_opp_count;
>
> if (!count)
> return 0;
>
> - required_opps = kcalloc(count, sizeof(*required_opps), GFP_KERNEL);
> - if (!required_opps)
> + opp->required_opps = kcalloc(count, sizeof(*opp->required_opps), GFP_KERNEL);
> + if (!opp->required_opps)
> return -ENOMEM;
>
> - opp->required_opps = required_opps;
> -
> for (i = 0; i < count; i++) {
> required_table = opp_table->required_opp_tables[i];
>
> @@ -321,21 +338,9 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
> if (IS_ERR_OR_NULL(required_table))
> continue;
>
> - np = of_parse_required_opp(opp->np, i);
> - if (unlikely(!np)) {
> - ret = -ENODEV;
> - goto free_required_opps;
> - }
> -
> - required_opps[i] = _find_opp_of_np(required_table, np);
> - of_node_put(np);
> -
> - if (!required_opps[i]) {
> - pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
> - __func__, opp->np, i);
> - ret = -ENODEV;
> + ret = _link_required_opps(opp, required_table, i);
> + if (ret)
> goto free_required_opps;
> - }
> }
>
> return 0;
> @@ -350,22 +355,13 @@ static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
> static int lazy_link_required_opps(struct opp_table *opp_table,
> struct opp_table *new_table, int index)
> {
> - struct device_node *required_np;
> struct dev_pm_opp *opp;
> + int ret;
>
> list_for_each_entry(opp, &opp_table->opp_list, node) {
> - required_np = of_parse_required_opp(opp->np, index);
> - if (unlikely(!required_np))
> - return -ENODEV;
> -
> - opp->required_opps[index] = _find_opp_of_np(new_table, required_np);
> - of_node_put(required_np);
> -
> - if (!opp->required_opps[index]) {
> - pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
> - __func__, opp->np, index);
> - return -ENODEV;
> - }
> + ret = _link_required_opps(opp, new_table, index);
> + if (ret)
> + return ret;
> }
>
> return 0;
> --
> 2.31.1.272.g89b43f80a514
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/5] OPP: Reorder code in _opp_set_required_opps_genpd()
2023-10-13 8:48 [PATCH 0/5] OPP: Minor cleanups Viresh Kumar
2023-10-13 8:48 ` [PATCH 1/5] OPP: Fix formatting of if/else block Viresh Kumar
2023-10-13 8:48 ` [PATCH 2/5] OPP: Add _link_required_opps() to avoid code duplication Viresh Kumar
@ 2023-10-13 8:48 ` Viresh Kumar
2023-10-16 10:11 ` Ulf Hansson
2023-10-13 8:48 ` [PATCH 4/5] OPP: Remove genpd_virt_dev_lock Viresh Kumar
2023-10-13 8:48 ` [PATCH 5/5] OPP: No need to defer probe from _opp_attach_genpd() Viresh Kumar
4 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-10-13 8:48 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael J. Wysocki,
Ulf Hansson, linux-kernel
Reorder code in _opp_set_required_opps_genpd() to reduce duplicate code.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index f42b663a4d8b..3516e79cf743 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1076,7 +1076,18 @@ static int _opp_set_required_opps_genpd(struct device *dev,
{
struct device **genpd_virt_devs =
opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
- int i, ret = 0;
+ int index, target, delta, ret;
+
+ /* Scaling up? Set required OPPs in normal order, else reverse */
+ if (!scaling_down) {
+ index = 0;
+ target = opp_table->required_opp_count;
+ delta = 1;
+ } else {
+ index = opp_table->required_opp_count - 1;
+ target = -1;
+ delta = -1;
+ }
/*
* Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
@@ -1084,24 +1095,17 @@ static int _opp_set_required_opps_genpd(struct device *dev,
*/
mutex_lock(&opp_table->genpd_virt_dev_lock);
- /* Scaling up? Set required OPPs in normal order, else reverse */
- if (!scaling_down) {
- for (i = 0; i < opp_table->required_opp_count; i++) {
- ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
- if (ret)
- break;
- }
- } else {
- for (i = opp_table->required_opp_count - 1; i >= 0; i--) {
- ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
- if (ret)
- break;
- }
+ while (index != target) {
+ ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index);
+ if (ret)
+ break;
+
+ index += delta;
}
mutex_unlock(&opp_table->genpd_virt_dev_lock);
- return ret;
+ return 0;
}
/* This is only called for PM domain for now */
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] OPP: Reorder code in _opp_set_required_opps_genpd()
2023-10-13 8:48 ` [PATCH 3/5] OPP: Reorder code in _opp_set_required_opps_genpd() Viresh Kumar
@ 2023-10-16 10:11 ` Ulf Hansson
2023-10-16 10:38 ` Viresh Kumar
0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2023-10-16 10:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On Fri, 13 Oct 2023 at 10:48, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Reorder code in _opp_set_required_opps_genpd() to reduce duplicate code.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/opp/core.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index f42b663a4d8b..3516e79cf743 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1076,7 +1076,18 @@ static int _opp_set_required_opps_genpd(struct device *dev,
> {
> struct device **genpd_virt_devs =
> opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
> - int i, ret = 0;
> + int index, target, delta, ret;
> +
> + /* Scaling up? Set required OPPs in normal order, else reverse */
> + if (!scaling_down) {
> + index = 0;
> + target = opp_table->required_opp_count;
> + delta = 1;
> + } else {
> + index = opp_table->required_opp_count - 1;
> + target = -1;
> + delta = -1;
> + }
>
> /*
> * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
> @@ -1084,24 +1095,17 @@ static int _opp_set_required_opps_genpd(struct device *dev,
> */
> mutex_lock(&opp_table->genpd_virt_dev_lock);
>
> - /* Scaling up? Set required OPPs in normal order, else reverse */
> - if (!scaling_down) {
> - for (i = 0; i < opp_table->required_opp_count; i++) {
> - ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
> - if (ret)
> - break;
> - }
> - } else {
> - for (i = opp_table->required_opp_count - 1; i >= 0; i--) {
> - ret = _set_performance_state(dev, genpd_virt_devs[i], opp, i);
> - if (ret)
> - break;
> - }
> + while (index != target) {
> + ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index);
> + if (ret)
> + break;
> +
> + index += delta;
> }
>
> mutex_unlock(&opp_table->genpd_virt_dev_lock);
>
> - return ret;
> + return 0;
Why always return 0 and not the error code anymore?
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] OPP: Reorder code in _opp_set_required_opps_genpd()
2023-10-16 10:11 ` Ulf Hansson
@ 2023-10-16 10:38 ` Viresh Kumar
2023-10-16 14:50 ` Ulf Hansson
0 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-10-16 10:38 UTC (permalink / raw)
To: Ulf Hansson
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On 16-10-23, 12:11, Ulf Hansson wrote:
> Why always return 0 and not the error code anymore?
Oops, fixed with:
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3516e79cf743..42ca52fbe210 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1076,7 +1076,7 @@ static int _opp_set_required_opps_genpd(struct device *dev,
{
struct device **genpd_virt_devs =
opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
- int index, target, delta, ret;
+ int index, target, delta, ret = 0;
/* Scaling up? Set required OPPs in normal order, else reverse */
if (!scaling_down) {
@@ -1105,7 +1105,7 @@ static int _opp_set_required_opps_genpd(struct device *dev,
mutex_unlock(&opp_table->genpd_virt_dev_lock);
- return 0;
+ return ret;
}
/* This is only called for PM domain for now */
--
viresh
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] OPP: Reorder code in _opp_set_required_opps_genpd()
2023-10-16 10:38 ` Viresh Kumar
@ 2023-10-16 14:50 ` Ulf Hansson
0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2023-10-16 14:50 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On Mon, 16 Oct 2023 at 12:38, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-10-23, 12:11, Ulf Hansson wrote:
> > Why always return 0 and not the error code anymore?
>
> Oops, fixed with:
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 3516e79cf743..42ca52fbe210 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1076,7 +1076,7 @@ static int _opp_set_required_opps_genpd(struct device *dev,
> {
> struct device **genpd_virt_devs =
> opp_table->genpd_virt_devs ? opp_table->genpd_virt_devs : &dev;
> - int index, target, delta, ret;
> + int index, target, delta, ret = 0;
>
> /* Scaling up? Set required OPPs in normal order, else reverse */
> if (!scaling_down) {
> @@ -1105,7 +1105,7 @@ static int _opp_set_required_opps_genpd(struct device *dev,
>
> mutex_unlock(&opp_table->genpd_virt_dev_lock);
>
> - return 0;
> + return ret;
> }
>
> /* This is only called for PM domain for now */
>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] OPP: Remove genpd_virt_dev_lock
2023-10-13 8:48 [PATCH 0/5] OPP: Minor cleanups Viresh Kumar
` (2 preceding siblings ...)
2023-10-13 8:48 ` [PATCH 3/5] OPP: Reorder code in _opp_set_required_opps_genpd() Viresh Kumar
@ 2023-10-13 8:48 ` Viresh Kumar
2023-10-16 10:11 ` Ulf Hansson
2023-10-13 8:48 ` [PATCH 5/5] OPP: No need to defer probe from _opp_attach_genpd() Viresh Kumar
4 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-10-13 8:48 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael J. Wysocki,
Ulf Hansson, linux-kernel
All the config operations for OPP tables share common code paths now and
none of the other ones have such protection in place. Either all should
have it or none.
The understanding here is that user won't clear the OPP configs while
still using them and so such a case won't happen. We can always come
back and use a wider lock for all resource types if required.
Also fix the error on failing to allocate memory.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 33 +++------------------------------
drivers/opp/opp.h | 2 --
2 files changed, 3 insertions(+), 32 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3516e79cf743..befe46036ad5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1089,12 +1089,6 @@ static int _opp_set_required_opps_genpd(struct device *dev,
delta = -1;
}
- /*
- * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
- * after it is freed from another thread.
- */
- mutex_lock(&opp_table->genpd_virt_dev_lock);
-
while (index != target) {
ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index);
if (ret)
@@ -1103,8 +1097,6 @@ static int _opp_set_required_opps_genpd(struct device *dev,
index += delta;
}
- mutex_unlock(&opp_table->genpd_virt_dev_lock);
-
return 0;
}
@@ -1474,7 +1466,6 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
return ERR_PTR(-ENOMEM);
mutex_init(&opp_table->lock);
- mutex_init(&opp_table->genpd_virt_dev_lock);
INIT_LIST_HEAD(&opp_table->dev_list);
INIT_LIST_HEAD(&opp_table->lazy);
@@ -1510,7 +1501,6 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
remove_opp_dev:
_of_clear_opp_table(opp_table);
_remove_opp_dev(opp_dev, opp_table);
- mutex_destroy(&opp_table->genpd_virt_dev_lock);
mutex_destroy(&opp_table->lock);
err:
kfree(opp_table);
@@ -1678,7 +1668,6 @@ static void _opp_table_kref_release(struct kref *kref)
list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node)
_remove_opp_dev(opp_dev, opp_table);
- mutex_destroy(&opp_table->genpd_virt_dev_lock);
mutex_destroy(&opp_table->lock);
kfree(opp_table);
}
@@ -2395,7 +2384,7 @@ static void _opp_put_config_regulators_helper(struct opp_table *opp_table)
opp_table->config_regulators = NULL;
}
-static void _detach_genpd(struct opp_table *opp_table)
+static void _opp_detach_genpd(struct opp_table *opp_table)
{
int index;
@@ -2449,13 +2438,11 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
if (!opp_table->required_opp_count)
return -EPROBE_DEFER;
- mutex_lock(&opp_table->genpd_virt_dev_lock);
-
opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count,
sizeof(*opp_table->genpd_virt_devs),
GFP_KERNEL);
if (!opp_table->genpd_virt_devs)
- goto unlock;
+ return -ENOMEM;
while (*name) {
if (index >= opp_table->required_opp_count) {
@@ -2478,29 +2465,15 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
if (virt_devs)
*virt_devs = opp_table->genpd_virt_devs;
- mutex_unlock(&opp_table->genpd_virt_dev_lock);
return 0;
err:
- _detach_genpd(opp_table);
-unlock:
- mutex_unlock(&opp_table->genpd_virt_dev_lock);
+ _opp_detach_genpd(opp_table);
return ret;
}
-static void _opp_detach_genpd(struct opp_table *opp_table)
-{
- /*
- * Acquire genpd_virt_dev_lock to make sure virt_dev isn't getting
- * used in parallel.
- */
- mutex_lock(&opp_table->genpd_virt_dev_lock);
- _detach_genpd(opp_table);
- mutex_unlock(&opp_table->genpd_virt_dev_lock);
-}
-
static void _opp_clear_config(struct opp_config_data *data)
{
if (data->flags & OPP_CONFIG_GENPD)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index fefdf9845692..08366f90f16b 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -160,7 +160,6 @@ enum opp_table_access {
* @rate_clk_single: Currently configured frequency for single clk.
* @current_opp: Currently configured OPP for the table.
* @suspend_opp: Pointer to OPP to be used during device suspend.
- * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
* @genpd_virt_devs: List of virtual devices for multiple genpd support.
* @required_opp_tables: List of device OPP tables that are required by OPPs in
* this table.
@@ -212,7 +211,6 @@ struct opp_table {
struct dev_pm_opp *current_opp;
struct dev_pm_opp *suspend_opp;
- struct mutex genpd_virt_dev_lock;
struct device **genpd_virt_devs;
struct opp_table **required_opp_tables;
unsigned int required_opp_count;
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/5] OPP: Remove genpd_virt_dev_lock
2023-10-13 8:48 ` [PATCH 4/5] OPP: Remove genpd_virt_dev_lock Viresh Kumar
@ 2023-10-16 10:11 ` Ulf Hansson
0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2023-10-16 10:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On Fri, 13 Oct 2023 at 10:49, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> All the config operations for OPP tables share common code paths now and
> none of the other ones have such protection in place. Either all should
> have it or none.
>
> The understanding here is that user won't clear the OPP configs while
> still using them and so such a case won't happen. We can always come
> back and use a wider lock for all resource types if required.
>
> Also fix the error on failing to allocate memory.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/opp/core.c | 33 +++------------------------------
> drivers/opp/opp.h | 2 --
> 2 files changed, 3 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 3516e79cf743..befe46036ad5 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1089,12 +1089,6 @@ static int _opp_set_required_opps_genpd(struct device *dev,
> delta = -1;
> }
>
> - /*
> - * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
> - * after it is freed from another thread.
> - */
> - mutex_lock(&opp_table->genpd_virt_dev_lock);
> -
> while (index != target) {
> ret = _set_performance_state(dev, genpd_virt_devs[index], opp, index);
> if (ret)
> @@ -1103,8 +1097,6 @@ static int _opp_set_required_opps_genpd(struct device *dev,
> index += delta;
> }
>
> - mutex_unlock(&opp_table->genpd_virt_dev_lock);
> -
> return 0;
> }
>
> @@ -1474,7 +1466,6 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> return ERR_PTR(-ENOMEM);
>
> mutex_init(&opp_table->lock);
> - mutex_init(&opp_table->genpd_virt_dev_lock);
> INIT_LIST_HEAD(&opp_table->dev_list);
> INIT_LIST_HEAD(&opp_table->lazy);
>
> @@ -1510,7 +1501,6 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
> remove_opp_dev:
> _of_clear_opp_table(opp_table);
> _remove_opp_dev(opp_dev, opp_table);
> - mutex_destroy(&opp_table->genpd_virt_dev_lock);
> mutex_destroy(&opp_table->lock);
> err:
> kfree(opp_table);
> @@ -1678,7 +1668,6 @@ static void _opp_table_kref_release(struct kref *kref)
> list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node)
> _remove_opp_dev(opp_dev, opp_table);
>
> - mutex_destroy(&opp_table->genpd_virt_dev_lock);
> mutex_destroy(&opp_table->lock);
> kfree(opp_table);
> }
> @@ -2395,7 +2384,7 @@ static void _opp_put_config_regulators_helper(struct opp_table *opp_table)
> opp_table->config_regulators = NULL;
> }
>
> -static void _detach_genpd(struct opp_table *opp_table)
> +static void _opp_detach_genpd(struct opp_table *opp_table)
> {
> int index;
>
> @@ -2449,13 +2438,11 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
> if (!opp_table->required_opp_count)
> return -EPROBE_DEFER;
>
> - mutex_lock(&opp_table->genpd_virt_dev_lock);
> -
> opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count,
> sizeof(*opp_table->genpd_virt_devs),
> GFP_KERNEL);
> if (!opp_table->genpd_virt_devs)
> - goto unlock;
> + return -ENOMEM;
>
> while (*name) {
> if (index >= opp_table->required_opp_count) {
> @@ -2478,29 +2465,15 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
>
> if (virt_devs)
> *virt_devs = opp_table->genpd_virt_devs;
> - mutex_unlock(&opp_table->genpd_virt_dev_lock);
>
> return 0;
>
> err:
> - _detach_genpd(opp_table);
> -unlock:
> - mutex_unlock(&opp_table->genpd_virt_dev_lock);
> + _opp_detach_genpd(opp_table);
> return ret;
>
> }
>
> -static void _opp_detach_genpd(struct opp_table *opp_table)
> -{
> - /*
> - * Acquire genpd_virt_dev_lock to make sure virt_dev isn't getting
> - * used in parallel.
> - */
> - mutex_lock(&opp_table->genpd_virt_dev_lock);
> - _detach_genpd(opp_table);
> - mutex_unlock(&opp_table->genpd_virt_dev_lock);
> -}
> -
> static void _opp_clear_config(struct opp_config_data *data)
> {
> if (data->flags & OPP_CONFIG_GENPD)
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index fefdf9845692..08366f90f16b 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -160,7 +160,6 @@ enum opp_table_access {
> * @rate_clk_single: Currently configured frequency for single clk.
> * @current_opp: Currently configured OPP for the table.
> * @suspend_opp: Pointer to OPP to be used during device suspend.
> - * @genpd_virt_dev_lock: Mutex protecting the genpd virtual device pointers.
> * @genpd_virt_devs: List of virtual devices for multiple genpd support.
> * @required_opp_tables: List of device OPP tables that are required by OPPs in
> * this table.
> @@ -212,7 +211,6 @@ struct opp_table {
> struct dev_pm_opp *current_opp;
> struct dev_pm_opp *suspend_opp;
>
> - struct mutex genpd_virt_dev_lock;
> struct device **genpd_virt_devs;
> struct opp_table **required_opp_tables;
> unsigned int required_opp_count;
> --
> 2.31.1.272.g89b43f80a514
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/5] OPP: No need to defer probe from _opp_attach_genpd()
2023-10-13 8:48 [PATCH 0/5] OPP: Minor cleanups Viresh Kumar
` (3 preceding siblings ...)
2023-10-13 8:48 ` [PATCH 4/5] OPP: Remove genpd_virt_dev_lock Viresh Kumar
@ 2023-10-13 8:48 ` Viresh Kumar
2023-10-16 10:11 ` Ulf Hansson
4 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-10-13 8:48 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael J. Wysocki,
Ulf Hansson, linux-kernel
When the new interface for attaching genpd's via the OPP core was added,
it was possible for required_opp_count to be zero, but not anymore.
Remove the unused check.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index befe46036ad5..c069aabefa00 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2430,14 +2430,6 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
if (opp_table->genpd_virt_devs)
return 0;
- /*
- * If the genpd's OPP table isn't already initialized, parsing of the
- * required-opps fail for dev. We should retry this after genpd's OPP
- * table is added.
- */
- if (!opp_table->required_opp_count)
- return -EPROBE_DEFER;
-
opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count,
sizeof(*opp_table->genpd_virt_devs),
GFP_KERNEL);
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] OPP: No need to defer probe from _opp_attach_genpd()
2023-10-13 8:48 ` [PATCH 5/5] OPP: No need to defer probe from _opp_attach_genpd() Viresh Kumar
@ 2023-10-16 10:11 ` Ulf Hansson
0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2023-10-16 10:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael J. Wysocki, linux-kernel
On Fri, 13 Oct 2023 at 10:49, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> When the new interface for attaching genpd's via the OPP core was added,
> it was possible for required_opp_count to be zero, but not anymore.
>
> Remove the unused check.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/opp/core.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index befe46036ad5..c069aabefa00 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2430,14 +2430,6 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
> if (opp_table->genpd_virt_devs)
> return 0;
>
> - /*
> - * If the genpd's OPP table isn't already initialized, parsing of the
> - * required-opps fail for dev. We should retry this after genpd's OPP
> - * table is added.
> - */
> - if (!opp_table->required_opp_count)
> - return -EPROBE_DEFER;
> -
> opp_table->genpd_virt_devs = kcalloc(opp_table->required_opp_count,
> sizeof(*opp_table->genpd_virt_devs),
> GFP_KERNEL);
> --
> 2.31.1.272.g89b43f80a514
>
^ permalink raw reply [flat|nested] 13+ messages in thread