From 9e28f7a74581204807f20ae46568939038e327aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Larumbe?= Date: Sat, 20 May 2023 18:00:35 +0100 Subject: [PATCH 1/9] OPP: rate-limit debug messages when no change in OPP is required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise, when enabling a debug build and dynamic debug in the kernel, it quickly floods the kernel ring buffer and makes debugging of other subsystems almost impossible, unless manually disabled. Signed-off-by: Adrián Larumbe Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 954c94865cf56..85cbc8de407c6 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1091,7 +1091,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, /* Return early if nothing to do */ if (!forced && old_opp == opp && opp_table->enabled) { - dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__); + dev_dbg_ratelimited(dev, "%s: OPPs are same, nothing to do\n", __func__); return 0; } From b2a2ab039bd58f51355e33d7d3fc64605d7f870d Mon Sep 17 00:00:00 2001 From: Stephan Gerhold Date: Tue, 30 May 2023 17:54:46 +0200 Subject: [PATCH 2/9] opp: Fix use-after-free in lazy_opp_tables after probe deferral When dev_pm_opp_of_find_icc_paths() in _allocate_opp_table() returns -EPROBE_DEFER, the opp_table is freed again, to wait until all the interconnect paths are available. However, if the OPP table is using required-opps then it may already have been added to the global lazy_opp_tables list. The error path does not remove the opp_table from the list again. This can cause crashes later when the provider of the required-opps is added, since we will iterate over OPP tables that have already been freed. E.g.: Unable to handle kernel NULL pointer dereference when read CPU: 0 PID: 7 Comm: kworker/0:0 Not tainted 6.4.0-rc3 PC is at _of_add_opp_table_v2 (include/linux/of.h:949 drivers/opp/of.c:98 drivers/opp/of.c:344 drivers/opp/of.c:404 drivers/opp/of.c:1032) -> lazy_link_required_opp_table() Fix this by calling _of_clear_opp_table() to remove the opp_table from the list and clear other allocated resources. While at it, also add the missing mutex_destroy() calls in the error path. Cc: stable@vger.kernel.org Suggested-by: Viresh Kumar Fixes: 7eba0c7641b0 ("opp: Allow lazy-linking of required-opps") Signed-off-by: Stephan Gerhold Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 85cbc8de407c6..7046487dc6f43 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1358,7 +1358,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index) return opp_table; 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); return ERR_PTR(ret); From 167eb2bd947d9c04b0f6f1a5495ce4a99eeab598 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Thu, 8 Jun 2023 12:55:07 +0530 Subject: [PATCH 3/9] OPP: Staticize `lazy_opp_tables` in of.c `lazy_opp_tables` is only used in of.c, move it there and mark it `static`. Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 3 --- drivers/opp/of.c | 3 +++ drivers/opp/opp.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 7046487dc6f43..9f918077cd629 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -29,9 +29,6 @@ */ LIST_HEAD(opp_tables); -/* OPP tables with uninitialized required OPPs */ -LIST_HEAD(lazy_opp_tables); - /* Lock to allow exclusive modification to the device and opp lists */ DEFINE_MUTEX(opp_table_lock); /* Flag indicating that opp_tables list is being updated at the moment */ diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 8246e9b7afe7f..c740a907ef767 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -21,6 +21,9 @@ #include "opp.h" +/* OPP tables with uninitialized required OPPs */ +static LIST_HEAD(lazy_opp_tables); + /* * Returns opp descriptor node for a device node, caller must * do of_node_put(). diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index 2a057c42ddf4d..eb71385d96c1a 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -26,7 +26,7 @@ struct regulator; /* Lock to allow exclusive modification to the device and opp lists */ extern struct mutex opp_table_lock; -extern struct list_head opp_tables, lazy_opp_tables; +extern struct list_head opp_tables; /* OPP Config flags */ #define OPP_CONFIG_CLK BIT(0) From 64aaeb708245acdcbe51cf30c84418668c044d80 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Thu, 8 Jun 2023 13:10:14 +0530 Subject: [PATCH 4/9] OPP: Protect `lazy_opp_tables` list with `opp_table_lock` The `opp_table_lock` lock is already used to protect the list elsewhere, use it while adding or removing entries from it. Reported-by: Stephan Gerhold Signed-off-by: Viresh Kumar Tested-by: Stephan Gerhold --- drivers/opp/of.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index c740a907ef767..ac2179d5da4cf 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -21,7 +21,7 @@ #include "opp.h" -/* OPP tables with uninitialized required OPPs */ +/* OPP tables with uninitialized required OPPs, protected by opp_table_lock */ static LIST_HEAD(lazy_opp_tables); /* @@ -148,7 +148,10 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table) opp_table->required_opp_count = 0; opp_table->required_opp_tables = NULL; + + mutex_lock(&opp_table_lock); list_del(&opp_table->lazy); + mutex_unlock(&opp_table_lock); } /* @@ -197,8 +200,15 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, } /* Let's do the linking later on */ - if (lazy) + if (lazy) { + /* + * The OPP table is not held while allocating the table, take it + * now to avoid corruption to the lazy_opp_tables list. + */ + mutex_lock(&opp_table_lock); list_add(&opp_table->lazy, &lazy_opp_tables); + mutex_unlock(&opp_table_lock); + } else _update_set_required_opps(opp_table); From 04bd2eafee153b9cc4b411d4a24d32b1ec2ce41c Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 14 Jun 2023 12:50:20 +0530 Subject: [PATCH 5/9] OPP: don't drop performance constraint on OPP table removal This code was added (long back) by commit 009acd196fc8 ("PM / OPP: Support updating performance state of device's power domain") and at that time the `opp->pstate` field was used to store the performance state required by a device's OPP. Over time that changed and the `->pstate` field is now used only for genpd devices and consumer devices access that via the required-opps instead. Because of all these changes, _opp_table_kref_release() now drops the constraint only when the genpd's OPP table gets freed and not the device's. Which is definitely not what we wanted. And dropping the constraint doesn't have much meaning as the genpd itself is going away. Moreover, if we want to drop constraints here, then just dropping the performance constraint alone isn't sufficient as there are other resource constraints like clk, regulator, etc. too, which must be handled. Probably the right thing to do here is to leave this decision to the consumers, which can call `dev_pm_opp_set_rate(dev, 0)` or similar APIs to drop all constraints properly. Which many of the consumers already do. Remove the special code, which is broken anyway. Signed-off-by: Viresh Kumar Reviewed-by: Ulf Hansson --- drivers/opp/core.c | 10 +--------- drivers/opp/of.c | 8 -------- drivers/opp/opp.h | 2 -- 3 files changed, 1 insertion(+), 19 deletions(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 9f918077cd629..7290168ec8067 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1522,16 +1522,8 @@ static void _opp_table_kref_release(struct kref *kref) WARN_ON(!list_empty(&opp_table->opp_list)); - list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node) { - /* - * The OPP table is getting removed, drop the performance state - * constraints. - */ - if (opp_table->genpd_performance_state) - dev_pm_genpd_set_performance_state((struct device *)(opp_dev->dev), 0); - + 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); diff --git a/drivers/opp/of.c b/drivers/opp/of.c index ac2179d5da4cf..943c7fb7402b3 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -1034,14 +1034,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table) goto remove_static_opp; } - list_for_each_entry(opp, &opp_table->opp_list, node) { - /* Any non-zero performance state would enable the feature */ - if (opp->pstate) { - opp_table->genpd_performance_state = true; - break; - } - } - lazy_link_required_opp_table(opp_table); return 0; diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index eb71385d96c1a..3805b92a6100f 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -182,7 +182,6 @@ enum opp_table_access { * @paths: Interconnect path handles * @path_count: Number of interconnect paths * @enabled: Set to true if the device's resources are enabled/configured. - * @genpd_performance_state: Device's power domain support performance state. * @is_genpd: Marks if the OPP table belongs to a genpd. * @set_required_opps: Helper responsible to set required OPPs. * @dentry: debugfs dentry pointer of the real device directory (not links). @@ -233,7 +232,6 @@ struct opp_table { struct icc_path **paths; unsigned int path_count; bool enabled; - bool genpd_performance_state; bool is_genpd; int (*set_required_opps)(struct device *dev, struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down); From 84cb7ff35fcf7c0b552f553a3f2db9c3e92fc707 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 14 Jun 2023 15:57:43 +0530 Subject: [PATCH 6/9] OPP: pstate is only valid for genpd OPP tables 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 Reviewed-by: Ulf Hansson Reviewed-by: Bjorn Andersson Tested-by: Bjorn Andersson --- 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 7290168ec8067..4c82ee20ceb78 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); @@ -2696,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; diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c index 2c7fb683441ef..0cc21e2b42ffe 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 943c7fb7402b3..e23ce6e78eb69 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; From 7c41cdcd3bbee5d49de9d4821b15e49d155ff22b Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 14 Jun 2023 15:29:32 +0530 Subject: [PATCH 7/9] OPP: Simplify the over-designed pstate <-> level dance 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 Signed-off-by: Viresh Kumar Reviewed-by: Ulf Hansson --- drivers/opp/core.c | 8 ++++---- drivers/opp/debugfs.c | 3 --- drivers/opp/of.c | 5 +---- drivers/opp/opp.h | 2 -- 4 files changed, 5 insertions(+), 13 deletions(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 4c82ee20ceb78..3f46e499d615f 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 0cc21e2b42ffe..17543c0aa5b68 100644 --- a/drivers/opp/debugfs.c +++ b/drivers/opp/debugfs.c @@ -156,9 +156,6 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table) 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 e23ce6e78eb69..e6d1155d0990d 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 3805b92a6100f..8a5ea38f3a3d0 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; From fa155f4f834882a79788218aea4914568b41dd0f Mon Sep 17 00:00:00 2001 From: Andrew Halaney Date: Fri, 23 Jun 2023 10:53:38 -0500 Subject: [PATCH 8/9] OPP: Use dev_err_probe() when failing to get icc_path This, in tandem with dynamic debug, can print useful information about -EPROBE_DEFFER like below, and keeps similar behavior for other errors: [ 16.561072] cpu cpu0: error -EPROBE_DEFER: dev_pm_opp_of_find_icc_paths: Unable to get path0 [ 16.575777] platform 18591000.cpufreq: deferred probe pending Signed-off-by: Andrew Halaney Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index e6d1155d0990d..1f0923cc1cd93 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -513,11 +513,7 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, for (i = 0; i < num_paths; i++) { paths[i] = of_icc_get_by_index(dev, i); if (IS_ERR(paths[i])) { - ret = PTR_ERR(paths[i]); - if (ret != -EPROBE_DEFER) { - dev_err(dev, "%s: Unable to get path%d: %d\n", - __func__, i, ret); - } + ret = dev_err_probe(dev, ret, "%s: Unable to get path%d\n", __func__, i); goto err; } } From 5fb2864cbd50a84a73af4fdd900b31f2daddea34 Mon Sep 17 00:00:00 2001 From: Andrew Halaney Date: Mon, 26 Jun 2023 08:46:46 -0500 Subject: [PATCH 9/9] OPP: Properly propagate error along when failing to get icc_path fa155f4f8348 ("OPP: Use dev_err_probe() when failing to get icc_path") failed to actually use the error it was trying to log: smatch warnings: drivers/opp/of.c:516 dev_pm_opp_of_find_icc_paths() warn: passing zero to 'dev_err_probe' Make sure to use the right error and pass it along. Fixes: fa155f4f8348 ("OPP: Use dev_err_probe() when failing to get icc_path") Reported-by: kernel test robot Reported-by: Dan Carpenter Closes: https://lore.kernel.org/r/202306262008.guNLgjt6-lkp@intel.com/ Signed-off-by: Andrew Halaney Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 1f0923cc1cd93..ada4963c7cfae 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -513,7 +513,7 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, for (i = 0; i < num_paths; i++) { paths[i] = of_icc_get_by_index(dev, i); if (IS_ERR(paths[i])) { - ret = dev_err_probe(dev, ret, "%s: Unable to get path%d\n", __func__, i); + ret = dev_err_probe(dev, PTR_ERR(paths[i]), "%s: Unable to get path%d\n", __func__, i); goto err; } }