[PATCH] PM / OPP: Refactor counting of added OPPs for v2 to avoid unsupported OPPs

[PATCH] PM / OPP: Refactor counting of added OPPs for v2 to avoid unsupported OPPs

[Dave Gerlach]

Currently the _of_add_opp_table_v2 call loops through the OPP nodes in
the operating-points-v2 table in the device tree and calls
_opp_add_static_v2 for each to add them to the table. It counts each
iteration through this loop as an added OPP, however on platforms making
use of the opp-supported-hw property, _opp_add_static_v2 does not add
OPPs that are not seen as supported on the platform but still returns
success, as this is valid. Because of this the count variable will
contain the number of OPP nodes in the table in device tree but not
necessarily the ones that are supported and actually added.

As this count value is what is checked to determine if there are any
valid OPPs, if a platform has an operating-points-v2 table with all OPP
nodes containing opp-supported-hw values that are not currently
supported then _of_add_opp_table_v2 will fail to abort as it should due
to an empty table.

Additionally, since commit 3ba98324e81a ("PM / OPP: Get
performance state using genpd helper"), the same count variable is
compared against the number of OPPs containing performance states and
requires that either all or none have pstates set, however in the case
of any opp table that has any entries that do not get added by
_opp_add_static_v2 due to incompatible opp-supported-hw fields, these
numbers will not match and _of_add_opp_table_v2 will incorrectly fail.

In order to ensure the count variable reflects the number of OPPs
actually in the table, increment it during the existing loop which walks
the opp table to check if pstate is set and then use that for the
aforementioned checks.

Fixes: 3ba98324e81a ("PM / OPP: Get performance state using genpd helper")
Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/opp/of.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 7af0ddec936b..f288f83a2e62 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -399,8 +399,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 
 	/* We have opp-table node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
-		count++;
-
 		ret = _opp_add_static_v2(opp_table, dev, np);
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
@@ -411,15 +409,22 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 		}
 	}
 
+	/*
+	 * Iterate over the list of OPPs that were actually added, as
+	 * OPPs not supported by the hardware will be ignored by
+	 * _opp_add_static_v2 above.
+	 */
+	list_for_each_entry(opp, &opp_table->opp_list, node) {
+		count++;
+		pstate_count += !!opp->pstate;
+	}
+
 	/* There should be one of more OPP defined */
 	if (WARN_ON(!count)) {
 		ret = -ENOENT;
 		goto put_opp_table;
 	}
 
-	list_for_each_entry(opp, &opp_table->opp_list, node)
-		pstate_count += !!opp->pstate;
-
 	/* Either all or none of the nodes shall have performance state set */
 	if (pstate_count && pstate_count != count) {
 		dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
-- 
2.16.1

[PATCH] PM / OPP: Refactor counting of added OPPs for v2 to avoid unsupported OPPs

[Dave Gerlach]

Currently the _of_add_opp_table_v2 call loops through the OPP nodes in
the operating-points-v2 table in the device tree and calls
_opp_add_static_v2 for each to add them to the table. It counts each
iteration through this loop as an added OPP, however on platforms making
use of the opp-supported-hw property, _opp_add_static_v2 does not add
OPPs that are not seen as supported on the platform but still returns
success, as this is valid. Because of this the count variable will
contain the number of OPP nodes in the table in device tree but not
necessarily the ones that are supported and actually added.

As this count value is what is checked to determine if there are any
valid OPPs, if a platform has an operating-points-v2 table with all OPP
nodes containing opp-supported-hw values that are not currently
supported then _of_add_opp_table_v2 will fail to abort as it should due
to an empty table.

Additionally, since commit 3ba98324e81a ("PM / OPP: Get
performance state using genpd helper"), the same count variable is
compared against the number of OPPs containing performance states and
requires that either all or none have pstates set, however in the case
of any opp table that has any entries that do not get added by
_opp_add_static_v2 due to incompatible opp-supported-hw fields, these
numbers will not match and _of_add_opp_table_v2 will incorrectly fail.

In order to ensure the count variable reflects the number of OPPs
actually in the table, increment it during the existing loop which walks
the opp table to check if pstate is set and then use that for the
aforementioned checks.

Fixes: 3ba98324e81a ("PM / OPP: Get performance state using genpd helper")
Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
---
 drivers/opp/of.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 7af0ddec936b..f288f83a2e62 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -399,8 +399,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 
 	/* We have opp-table node now, iterate over it and add OPPs */
 	for_each_available_child_of_node(opp_np, np) {
-		count++;
-
 		ret = _opp_add_static_v2(opp_table, dev, np);
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
@@ -411,15 +409,22 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 		}
 	}
 
+	/*
+	 * Iterate over the list of OPPs that were actually added, as
+	 * OPPs not supported by the hardware will be ignored by
+	 * _opp_add_static_v2 above.
+	 */
+	list_for_each_entry(opp, &opp_table->opp_list, node) {
+		count++;
+		pstate_count += !!opp->pstate;
+	}
+
 	/* There should be one of more OPP defined */
 	if (WARN_ON(!count)) {
 		ret = -ENOENT;
 		goto put_opp_table;
 	}
 
-	list_for_each_entry(opp, &opp_table->opp_list, node)
-		pstate_count += !!opp->pstate;
-
 	/* Either all or none of the nodes shall have performance state set */
 	if (pstate_count && pstate_count != count) {
 		dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
-- 
2.16.1

Re: [PATCH] PM / OPP: Refactor counting of added OPPs for v2 to avoid unsupported OPPs

[Dave Gerlach]

Hi,
On 08/21/2018 10:10 PM, Dave Gerlach wrote:
> Currently the _of_add_opp_table_v2 call loops through the OPP nodes in
> the operating-points-v2 table in the device tree and calls
> _opp_add_static_v2 for each to add them to the table. It counts each
> iteration through this loop as an added OPP, however on platforms making
> use of the opp-supported-hw property, _opp_add_static_v2 does not add
> OPPs that are not seen as supported on the platform but still returns
> success, as this is valid. Because of this the count variable will
> contain the number of OPP nodes in the table in device tree but not
> necessarily the ones that are supported and actually added.
> 
> As this count value is what is checked to determine if there are any
> valid OPPs, if a platform has an operating-points-v2 table with all OPP
> nodes containing opp-supported-hw values that are not currently
> supported then _of_add_opp_table_v2 will fail to abort as it should due
> to an empty table.
> 
> Additionally, since commit 3ba98324e81a ("PM / OPP: Get
> performance state using genpd helper"), the same count variable is
> compared against the number of OPPs containing performance states and
> requires that either all or none have pstates set, however in the case
> of any opp table that has any entries that do not get added by
> _opp_add_static_v2 due to incompatible opp-supported-hw fields, these
> numbers will not match and _of_add_opp_table_v2 will incorrectly fail.
> 
> In order to ensure the count variable reflects the number of OPPs
> actually in the table, increment it during the existing loop which walks
> the opp table to check if pstate is set and then use that for the
> aforementioned checks.
> 
> Fixes: 3ba98324e81a ("PM / OPP: Get performance state using genpd helper")
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---

This is needed to fix cpufreq probe on TI am335x platforms. Seems that
arch/arm/boot/dts/am33xx.dtsi is the only operating-points-v2 table with
multiple opp-supported-hw entries which may not all be supported at once, and
this caused the count differences that led me here.

Regards,
Dave

>  drivers/opp/of.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 7af0ddec936b..f288f83a2e62 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -399,8 +399,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>  
>  	/* We have opp-table node now, iterate over it and add OPPs */
>  	for_each_available_child_of_node(opp_np, np) {
> -		count++;
> -
>  		ret = _opp_add_static_v2(opp_table, dev, np);
>  		if (ret) {
>  			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
> @@ -411,15 +409,22 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>  		}
>  	}
>  
> +	/*
> +	 * Iterate over the list of OPPs that were actually added, as
> +	 * OPPs not supported by the hardware will be ignored by
> +	 * _opp_add_static_v2 above.
> +	 */
> +	list_for_each_entry(opp, &opp_table->opp_list, node) {
> +		count++;
> +		pstate_count += !!opp->pstate;
> +	}
> +
>  	/* There should be one of more OPP defined */
>  	if (WARN_ON(!count)) {
>  		ret = -ENOENT;
>  		goto put_opp_table;
>  	}
>  
> -	list_for_each_entry(opp, &opp_table->opp_list, node)
> -		pstate_count += !!opp->pstate;
> -
>  	/* Either all or none of the nodes shall have performance state set */
>  	if (pstate_count && pstate_count != count) {
>  		dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
> 

Re: [PATCH] PM / OPP: Refactor counting of added OPPs for v2 to avoid unsupported OPPs

[Dave Gerlach]

Hi,
On 08/21/2018 10:10 PM, Dave Gerlach wrote:
> Currently the _of_add_opp_table_v2 call loops through the OPP nodes in
> the operating-points-v2 table in the device tree and calls
> _opp_add_static_v2 for each to add them to the table. It counts each
> iteration through this loop as an added OPP, however on platforms making
> use of the opp-supported-hw property, _opp_add_static_v2 does not add
> OPPs that are not seen as supported on the platform but still returns
> success, as this is valid. Because of this the count variable will
> contain the number of OPP nodes in the table in device tree but not
> necessarily the ones that are supported and actually added.
> 
> As this count value is what is checked to determine if there are any
> valid OPPs, if a platform has an operating-points-v2 table with all OPP
> nodes containing opp-supported-hw values that are not currently
> supported then _of_add_opp_table_v2 will fail to abort as it should due
> to an empty table.
> 
> Additionally, since commit 3ba98324e81a ("PM / OPP: Get
> performance state using genpd helper"), the same count variable is
> compared against the number of OPPs containing performance states and
> requires that either all or none have pstates set, however in the case
> of any opp table that has any entries that do not get added by
> _opp_add_static_v2 due to incompatible opp-supported-hw fields, these
> numbers will not match and _of_add_opp_table_v2 will incorrectly fail.
> 
> In order to ensure the count variable reflects the number of OPPs
> actually in the table, increment it during the existing loop which walks
> the opp table to check if pstate is set and then use that for the
> aforementioned checks.
> 
> Fixes: 3ba98324e81a ("PM / OPP: Get performance state using genpd helper")
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---

This is needed to fix cpufreq probe on TI am335x platforms. Seems that
arch/arm/boot/dts/am33xx.dtsi is the only operating-points-v2 table with
multiple opp-supported-hw entries which may not all be supported at once, and
this caused the count differences that led me here.

Regards,
Dave

>  drivers/opp/of.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 7af0ddec936b..f288f83a2e62 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -399,8 +399,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>  
>  	/* We have opp-table node now, iterate over it and add OPPs */
>  	for_each_available_child_of_node(opp_np, np) {
> -		count++;
> -
>  		ret = _opp_add_static_v2(opp_table, dev, np);
>  		if (ret) {
>  			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
> @@ -411,15 +409,22 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>  		}
>  	}
>  
> +	/*
> +	 * Iterate over the list of OPPs that were actually added, as
> +	 * OPPs not supported by the hardware will be ignored by
> +	 * _opp_add_static_v2 above.
> +	 */
> +	list_for_each_entry(opp, &opp_table->opp_list, node) {
> +		count++;
> +		pstate_count += !!opp->pstate;
> +	}
> +
>  	/* There should be one of more OPP defined */
>  	if (WARN_ON(!count)) {
>  		ret = -ENOENT;
>  		goto put_opp_table;
>  	}
>  
> -	list_for_each_entry(opp, &opp_table->opp_list, node)
> -		pstate_count += !!opp->pstate;
> -
>  	/* Either all or none of the nodes shall have performance state set */
>  	if (pstate_count && pstate_count != count) {
>  		dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
> 

Re: [PATCH] PM / OPP: Refactor counting of added OPPs for v2 to avoid unsupported OPPs

[Viresh Kumar]

On 21-08-18, 22:10, Dave Gerlach wrote:

Please ping people back if you haven't received a response for too long. Sorry
that I missed getting to this at an earlier point of time.

> Currently the _of_add_opp_table_v2 call loops through the OPP nodes in
> the operating-points-v2 table in the device tree and calls
> _opp_add_static_v2 for each to add them to the table. It counts each
> iteration through this loop as an added OPP, however on platforms making
> use of the opp-supported-hw property, _opp_add_static_v2 does not add
> OPPs that are not seen as supported on the platform but still returns
> success, as this is valid. Because of this the count variable will
> contain the number of OPP nodes in the table in device tree but not
> necessarily the ones that are supported and actually added.
> 
> As this count value is what is checked to determine if there are any
> valid OPPs, if a platform has an operating-points-v2 table with all OPP
> nodes containing opp-supported-hw values that are not currently
> supported then _of_add_opp_table_v2 will fail to abort as it should due
> to an empty table.
> 
> Additionally, since commit 3ba98324e81a ("PM / OPP: Get
> performance state using genpd helper"), the same count variable is
> compared against the number of OPPs containing performance states and
> requires that either all or none have pstates set, however in the case
> of any opp table that has any entries that do not get added by
> _opp_add_static_v2 due to incompatible opp-supported-hw fields, these
> numbers will not match and _of_add_opp_table_v2 will incorrectly fail.
> 
> In order to ensure the count variable reflects the number of OPPs
> actually in the table, increment it during the existing loop which walks
> the opp table to check if pstate is set and then use that for the
> aforementioned checks.
> 
> Fixes: 3ba98324e81a ("PM / OPP: Get performance state using genpd helper")
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---
>  drivers/opp/of.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 7af0ddec936b..f288f83a2e62 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -399,8 +399,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>  
>  	/* We have opp-table node now, iterate over it and add OPPs */
>  	for_each_available_child_of_node(opp_np, np) {
> -		count++;
> -
>  		ret = _opp_add_static_v2(opp_table, dev, np);
>  		if (ret) {
>  			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
> @@ -411,15 +409,22 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
>  		}
>  	}
>  
> +	/*
> +	 * Iterate over the list of OPPs that were actually added, as
> +	 * OPPs not supported by the hardware will be ignored by
> +	 * _opp_add_static_v2 above.
> +	 */
> +	list_for_each_entry(opp, &opp_table->opp_list, node) {
> +		count++;
> +		pstate_count += !!opp->pstate;
> +	}

So the problem is genuine and is still true with the latest code in linux-next,
but this patch wouldn't fix all cases. What if some OPPs were dynamically added
using dev_pm_opp_add() and everything failed in this routine? We will still have
the problem you are trying to fix here.

What needs to be done is to identify the case where the OPP wasn't supported.
Maybe return the new OPPs pointer from _opp_add_static_v2(), return -ve ERR_PTR
values on error and NULL for the case where OPP isn't added but that's not an
error. Apart from your example another case is of duplicate OPPs where we return
0 without adding the OPP.

Please post patch against linux-next as some updates are already pushed for OPP
core there.

-- 
viresh