From 50b6b87c8484da8bb5dcce00f84ec80aee8fc2bd Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 3 Oct 2018 15:12:06 +0530 Subject: OPP: Improve error handling in dev_pm_opp_of_cpumask_add_table() The error handling wasn't appropriate in dev_pm_opp_of_cpumask_add_table(). For example it returns 0 on success and also for the case where cpumask is empty or cpu_device wasn't found for any of the CPUs. It should really return error on such cases, so that the callers can be aware of the outcome. Fix it. Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/opp/of.c b/drivers/opp/of.c index a71ff3acca0f..67a384c8ead2 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -614,16 +614,18 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_remove_table); int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask) { struct device *cpu_dev; - int cpu, ret = 0; + int cpu, ret; - WARN_ON(cpumask_empty(cpumask)); + if (WARN_ON(cpumask_empty(cpumask))) + return -ENODEV; for_each_cpu(cpu, cpumask) { cpu_dev = get_cpu_device(cpu); if (!cpu_dev) { pr_err("%s: failed to get cpu%d device\n", __func__, cpu); - continue; + ret = -ENODEV; + goto remove_table; } ret = dev_pm_opp_of_add_table(cpu_dev); @@ -635,12 +637,16 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask) pr_debug("%s: couldn't find opp table for cpu:%d, %d\n", __func__, cpu, ret); - /* Free all other OPPs */ - _dev_pm_opp_cpumask_remove_table(cpumask, cpu); - break; + goto remove_table; } } + return 0; + +remove_table: + /* Free all other OPPs */ + _dev_pm_opp_cpumask_remove_table(cpumask, cpu); + return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_add_table); -- cgit v1.2.3 From 09f662f95306f3e3d47ab6842bc4b0bb868a80ad Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 3 Oct 2018 15:22:03 +0530 Subject: OPP: Return error on error from dev_pm_opp_get_opp_count() Return error number instead of 0 on failures. Fixes: a1e8c13600bf ("PM / OPP: "opp-hz" is optional for power domains") Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/opp/core.c b/drivers/opp/core.c index cdf918aaac34..2c2df4e4fc14 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -318,7 +318,7 @@ int dev_pm_opp_get_opp_count(struct device *dev) count = PTR_ERR(opp_table); dev_dbg(dev, "%s: OPP table not found (%d)\n", __func__, count); - return 0; + return count; } count = _get_opp_count(opp_table); -- cgit v1.2.3 From 51c99dd2c06b234575661fa1e0a1dea6c3ef566f Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 3 Oct 2018 15:35:21 +0530 Subject: cpufreq: dt: Try freeing static OPPs only if we have added them We can not call dev_pm_opp_of_cpumask_remove_table() freely anymore since the latest OPP core updates as that uses reference counting to free resources. There are cases where no static OPPs are added (using DT) for a platform and trying to remove the OPP table may end up decrementing refcount which is already zero and hence generating warnings. Lets track if we were able to add static OPPs or not and then only remove the table based on that. Some reshuffling of code is also done to do that. Reported-by: Niklas Cassel Tested-by: Niklas Cassel Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq-dt.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) (limited to 'drivers') diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 0a9ebf00be46..e58bfcb1169e 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -32,6 +32,7 @@ struct private_data { struct device *cpu_dev; struct thermal_cooling_device *cdev; const char *reg_name; + bool have_static_opps; }; static struct freq_attr *cpufreq_dt_attr[] = { @@ -204,6 +205,15 @@ static int cpufreq_init(struct cpufreq_policy *policy) } } + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) { + ret = -ENOMEM; + goto out_put_regulator; + } + + priv->reg_name = name; + priv->opp_table = opp_table; + /* * Initialize OPP tables for all policy->cpus. They will be shared by * all CPUs which have marked their CPUs shared with OPP bindings. @@ -214,7 +224,8 @@ static int cpufreq_init(struct cpufreq_policy *policy) * * OPPs might be populated at runtime, don't check for error here */ - dev_pm_opp_of_cpumask_add_table(policy->cpus); + if (!dev_pm_opp_of_cpumask_add_table(policy->cpus)) + priv->have_static_opps = true; /* * But we need OPP table to function so if it is not there let's @@ -240,19 +251,10 @@ static int cpufreq_init(struct cpufreq_policy *policy) __func__, ret); } - priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (!priv) { - ret = -ENOMEM; - goto out_free_opp; - } - - priv->reg_name = name; - priv->opp_table = opp_table; - ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); if (ret) { dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); - goto out_free_priv; + goto out_free_opp; } priv->cpu_dev = cpu_dev; @@ -282,10 +284,11 @@ static int cpufreq_init(struct cpufreq_policy *policy) out_free_cpufreq_table: dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); -out_free_priv: - kfree(priv); out_free_opp: - dev_pm_opp_of_cpumask_remove_table(policy->cpus); + if (priv->have_static_opps) + dev_pm_opp_of_cpumask_remove_table(policy->cpus); + kfree(priv); +out_put_regulator: if (name) dev_pm_opp_put_regulators(opp_table); out_put_clk: @@ -300,7 +303,8 @@ static int cpufreq_exit(struct cpufreq_policy *policy) cpufreq_cooling_unregister(priv->cdev); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); - dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); + if (priv->have_static_opps) + dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); if (priv->reg_name) dev_pm_opp_put_regulators(priv->opp_table); -- cgit v1.2.3 From deac8703da5faafce96be3f91c668dcd5da931a5 Mon Sep 17 00:00:00 2001 From: Dave Gerlach Date: Wed, 3 Oct 2018 16:13:15 +0530 Subject: PM / OPP: _of_add_opp_table_v2(): increment count only if OPP is added 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 there are cases where _opp_add_static_v2() returns 0 but no new OPP is added to the list. This can happen while adding duplicate OPP or if the OPP isn't supported by hardware. 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 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. We need to clearly identify all the three cases (success, failure, unsupported/duplicate OPPs) and then increment count only on success case. Change return type of _opp_add_static_v2() to return the pointer to the newly added OPP instead of an integer. This routine now returns a valid pointer if the OPP is really added, NULL for unsupported or duplicate OPPs, and error value cased as a pointer on errors. Ideally the fixes tag in this commit should point back to the commit that introduced OPP v2 initially, as that's where we started incorrectly accounting for duplicate OPPs: commit 274659029c9d ("PM / OPP: Add support to parse "operating-points-v2" bindings") But it wasn't a real problem until recently as the count was only used to check if any OPPs are added or not. And so this commit points to a rather recent commit where we added more code that depends on the value of "count". Fixes: 3ba98324e81a ("PM / OPP: Get performance state using genpd helper") Reported-by: Dave Gerlach Reported-by: Niklas Cassel Tested-by: Niklas Cassel Signed-off-by: Dave Gerlach Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) (limited to 'drivers') diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 67a384c8ead2..5a4b47958073 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -297,15 +297,21 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table); * removed by dev_pm_opp_remove. * * Return: - * 0 On success OR + * Valid OPP pointer: + * On success + * NULL: * Duplicate OPPs (both freq and volt are same) and opp->available - * -EEXIST Freq are same and volt are different OR + * OR if the OPP is not supported by hardware. + * ERR_PTR(-EEXIST): + * Freq are same and volt are different OR * Duplicate OPPs (both freq and volt are same) and !opp->available - * -ENOMEM Memory allocation failure - * -EINVAL Failed parsing the OPP node + * ERR_PTR(-ENOMEM): + * Memory allocation failure + * ERR_PTR(-EINVAL): + * Failed parsing the OPP node */ -static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, - struct device_node *np) +static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, + struct device *dev, struct device_node *np) { struct dev_pm_opp *new_opp; u64 rate = 0; @@ -315,7 +321,7 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, new_opp = _opp_allocate(opp_table); if (!new_opp) - return -ENOMEM; + return ERR_PTR(-ENOMEM); ret = of_property_read_u64(np, "opp-hz", &rate); if (ret < 0) { @@ -390,12 +396,12 @@ static int _opp_add_static_v2(struct opp_table *opp_table, struct device *dev, * frequency/voltage list. */ blocking_notifier_call_chain(&opp_table->head, OPP_EVENT_ADD, new_opp); - return 0; + return new_opp; free_opp: _opp_free(new_opp); - return ret; + return ERR_PTR(ret); } /* Initializes OPP tables based on new bindings */ @@ -415,14 +421,15 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table) /* We have opp-table node now, iterate over it and add OPPs */ for_each_available_child_of_node(opp_table->np, np) { - count++; - - ret = _opp_add_static_v2(opp_table, dev, np); - if (ret) { + opp = _opp_add_static_v2(opp_table, dev, np); + if (IS_ERR(opp)) { + ret = PTR_ERR(opp); dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, ret); of_node_put(np); goto put_list_kref; + } else if (opp) { + count++; } } -- cgit v1.2.3