From ef43f01ac06976b2aa2b17266d307bb1a4f7e6f9 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Thu, 22 Oct 2020 12:12:27 +0530 Subject: [PATCH 01/15] opp: Always add entries in dev_list with opp_table->lock held The readers of dev_list expect the updates to it to take place from within the opp_table->lock and this is missing in the case where the dev_list is updated for already managed OPPs. Fix that by calling _add_opp_dev() from there and remove the now unused _add_opp_dev_unlocked() callback. While at it, also reduce the length of the critical section in _add_opp_dev(). Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 0e0a5269dc82..84035ab8bb31 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1036,8 +1036,8 @@ static void _remove_opp_dev(struct opp_device *opp_dev, kfree(opp_dev); } -static struct opp_device *_add_opp_dev_unlocked(const struct device *dev, - struct opp_table *opp_table) +struct opp_device *_add_opp_dev(const struct device *dev, + struct opp_table *opp_table) { struct opp_device *opp_dev; @@ -1048,7 +1048,9 @@ static struct opp_device *_add_opp_dev_unlocked(const struct device *dev, /* Initialize opp-dev */ opp_dev->dev = dev; + mutex_lock(&opp_table->lock); list_add(&opp_dev->node, &opp_table->dev_list); + mutex_unlock(&opp_table->lock); /* Create debugfs entries for the opp_table */ opp_debug_register(opp_dev, opp_table); @@ -1056,18 +1058,6 @@ static struct opp_device *_add_opp_dev_unlocked(const struct device *dev, return opp_dev; } -struct opp_device *_add_opp_dev(const struct device *dev, - struct opp_table *opp_table) -{ - struct opp_device *opp_dev; - - mutex_lock(&opp_table->lock); - opp_dev = _add_opp_dev_unlocked(dev, opp_table); - mutex_unlock(&opp_table->lock); - - return opp_dev; -} - static struct opp_table *_allocate_opp_table(struct device *dev, int index) { struct opp_table *opp_table; @@ -1148,7 +1138,7 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index) opp_table = _managed_opp(dev, index); if (opp_table) { - if (!_add_opp_dev_unlocked(dev, opp_table)) { + if (!_add_opp_dev(dev, opp_table)) { dev_pm_opp_put_opp_table(opp_table); opp_table = ERR_PTR(-ENOMEM); } From 27c09484dd3d8fdb56eb3787877d6035d0e89669 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Tue, 27 Oct 2020 16:53:21 +0530 Subject: [PATCH 02/15] opp: Allocate the OPP table outside of opp_table_lock There is no critical section which needs protection with locks while allocating an OPP table, except while adding it to the opp_tables list. And taking the opp_table_lock for the entire duration causes circular dependency issues like the one mentioned below. This patch takes another approach to reduce the size of the critical section to avoid such issues, the details of that are present within the patch. ====================================================== WARNING: possible circular locking dependency detected 5.4.72 #14 Not tainted ------------------------------------------------------ chrome/1865 is trying to acquire lock: ffffffdd34921750 (opp_table_lock){+.+.}, at: _find_opp_table+0x34/0x74 but task is already holding lock: ffffff81f0fc71a8 (reservation_ww_class_mutex){+.+.}, at: submit_lock_objects+0x70/0x1ec fscrypt: AES-256-CTS-CBC using implementation "cts-cbc-aes-ce" which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #4 (reservation_ww_class_mutex){+.+.}: __mutex_lock_common+0xec/0xc0c ww_mutex_lock_interruptible+0x5c/0xc4 msm_gem_fault+0x2c/0x124 __do_fault+0x40/0x16c handle_mm_fault+0x7cc/0xd98 do_page_fault+0x230/0x3b4 do_translation_fault+0x5c/0x78 do_mem_abort+0x4c/0xb4 el0_da+0x1c/0x20 -> #3 (&mm->mmap_sem){++++}: __might_fault+0x70/0x98 compat_filldir+0xf8/0x48c dcache_readdir+0x70/0x1dc iterate_dir+0xd4/0x180 __arm64_compat_sys_getdents+0xa0/0x19c el0_svc_common+0xa8/0x178 el0_svc_compat_handler+0x2c/0x40 el0_svc_compat+0x8/0x10 -> #2 (&sb->s_type->i_mutex_key#3){++++}: down_write+0x54/0x16c start_creating+0x68/0x128 debugfs_create_dir+0x28/0x114 opp_debug_register+0x8c/0xc0 _add_opp_dev_unlocked+0x5c/0x70 _add_opp_dev+0x38/0x58 _opp_get_opp_table+0xdc/0x1ac dev_pm_opp_get_opp_table_indexed+0x24/0x30 dev_pm_opp_of_add_table_indexed+0x48/0x84 of_genpd_add_provider_onecell+0xc0/0x1b8 rpmhpd_probe+0x240/0x268 platform_drv_probe+0x90/0xb0 really_probe+0x134/0x2ec driver_probe_device+0x64/0xfc __device_attach_driver+0x8c/0xa4 bus_for_each_drv+0x90/0xd8 __device_attach+0xc0/0x148 device_initial_probe+0x20/0x2c bus_probe_device+0x34/0x94 device_add+0x1fc/0x3b0 of_device_add+0x3c/0x4c of_platform_device_create_pdata+0xb8/0xfc of_platform_bus_create+0x1e4/0x368 of_platform_populate+0x70/0xbc devm_of_platform_populate+0x58/0xa0 rpmh_rsc_probe+0x36c/0x3cc platform_drv_probe+0x90/0xb0 really_probe+0x134/0x2ec driver_probe_device+0x64/0xfc __device_attach_driver+0x8c/0xa4 bus_for_each_drv+0x90/0xd8 __device_attach+0xc0/0x148 device_initial_probe+0x20/0x2c bus_probe_device+0x34/0x94 device_add+0x1fc/0x3b0 of_device_add+0x3c/0x4c of_platform_device_create_pdata+0xb8/0xfc of_platform_bus_create+0x1e4/0x368 of_platform_bus_create+0x230/0x368 of_platform_populate+0x70/0xbc of_platform_default_populate_init+0xa8/0xc0 do_one_initcall+0x1c8/0x3fc do_initcall_level+0xb4/0x10c do_basic_setup+0x30/0x48 kernel_init_freeable+0x124/0x1a4 kernel_init+0x14/0x104 ret_from_fork+0x10/0x18 -> #1 (&opp_table->lock){+.+.}: __mutex_lock_common+0xec/0xc0c mutex_lock_nested+0x40/0x50 _add_opp_dev+0x2c/0x58 _opp_get_opp_table+0xdc/0x1ac dev_pm_opp_get_opp_table_indexed+0x24/0x30 dev_pm_opp_of_add_table_indexed+0x48/0x84 of_genpd_add_provider_onecell+0xc0/0x1b8 rpmhpd_probe+0x240/0x268 platform_drv_probe+0x90/0xb0 really_probe+0x134/0x2ec driver_probe_device+0x64/0xfc __device_attach_driver+0x8c/0xa4 bus_for_each_drv+0x90/0xd8 __device_attach+0xc0/0x148 device_initial_probe+0x20/0x2c bus_probe_device+0x34/0x94 device_add+0x1fc/0x3b0 of_device_add+0x3c/0x4c of_platform_device_create_pdata+0xb8/0xfc of_platform_bus_create+0x1e4/0x368 of_platform_populate+0x70/0xbc devm_of_platform_populate+0x58/0xa0 rpmh_rsc_probe+0x36c/0x3cc platform_drv_probe+0x90/0xb0 really_probe+0x134/0x2ec driver_probe_device+0x64/0xfc __device_attach_driver+0x8c/0xa4 bus_for_each_drv+0x90/0xd8 __device_attach+0xc0/0x148 device_initial_probe+0x20/0x2c bus_probe_device+0x34/0x94 device_add+0x1fc/0x3b0 of_device_add+0x3c/0x4c of_platform_device_create_pdata+0xb8/0xfc of_platform_bus_create+0x1e4/0x368 of_platform_populate+0x70/0xbc devm_of_platform_populate+0x58/0xa0 rpmh_rsc_probe+0x36c/0x3cc platform_drv_probe+0x90/0xb0 really_probe+0x134/0x2ec driver_probe_device+0x64/0xfc __device_attach_driver+0x8c/0xa4 bus_for_each_drv+0x90/0xd8 __device_attach+0xc0/0x148 device_initial_probe+0x20/0x2c bus_probe_device+0x34/0x94 device_add+0x1fc/0x3b0 of_device_add+0x3c/0x4c of_platform_device_create_pdata+0xb8/0xfc of_platform_bus_create+0x1e4/0x368 of_platform_bus_create+0x230/0x368 of_platform_populate+0x70/0xbc of_platform_default_populate_init+0xa8/0xc0 do_one_initcall+0x1c8/0x3fc do_initcall_level+0xb4/0x10c do_basic_setup+0x30/0x48 kernel_init_freeable+0x124/0x1a4 kernel_init+0x14/0x104 ret_from_fork+0x10/0x18 -> #0 (opp_table_lock){+.+.}: __lock_acquire+0xee4/0x2450 lock_acquire+0x1cc/0x210 __mutex_lock_common+0xec/0xc0c mutex_lock_nested+0x40/0x50 _find_opp_table+0x34/0x74 dev_pm_opp_find_freq_exact+0x2c/0xdc a6xx_gmu_resume+0xc8/0xecc a6xx_pm_resume+0x148/0x200 adreno_resume+0x28/0x34 pm_generic_runtime_resume+0x34/0x48 __rpm_callback+0x70/0x10c rpm_callback+0x34/0x8c rpm_resume+0x414/0x550 __pm_runtime_resume+0x7c/0xa0 msm_gpu_submit+0x60/0x1c0 msm_ioctl_gem_submit+0xadc/0xb60 drm_ioctl_kernel+0x9c/0x118 drm_ioctl+0x27c/0x408 drm_compat_ioctl+0xcc/0xdc __se_compat_sys_ioctl+0x100/0x206c __arm64_compat_sys_ioctl+0x20/0x2c el0_svc_common+0xa8/0x178 el0_svc_compat_handler+0x2c/0x40 el0_svc_compat+0x8/0x10 other info that might help us debug this: Chain exists of: opp_table_lock --> &mm->mmap_sem --> reservation_ww_class_mutex Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(reservation_ww_class_mutex); lock(&mm->mmap_sem); lock(reservation_ww_class_mutex); lock(opp_table_lock); *** DEADLOCK *** 3 locks held by chrome/1865: #0: ffffff81edecc0d8 (&dev->struct_mutex){+.+.}, at: msm_ioctl_gem_submit+0x264/0xb60 #1: ffffff81d0000870 (reservation_ww_class_acquire){+.+.}, at: msm_ioctl_gem_submit+0x8e8/0xb60 #2: ffffff81f0fc71a8 (reservation_ww_class_mutex){+.+.}, at: submit_lock_objects+0x70/0x1ec stack backtrace: CPU: 0 PID: 1865 Comm: chrome Not tainted 5.4.72 #14 Hardware name: Google Lazor (rev1+) with LTE (DT) Call trace: dump_backtrace+0x0/0x158 show_stack+0x20/0x2c dump_stack+0xc8/0x160 print_circular_bug+0x2c4/0x2c8 check_noncircular+0x1a8/0x1b0 __lock_acquire+0xee4/0x2450 lock_acquire+0x1cc/0x210 __mutex_lock_common+0xec/0xc0c mutex_lock_nested+0x40/0x50 _find_opp_table+0x34/0x74 dev_pm_opp_find_freq_exact+0x2c/0xdc a6xx_gmu_resume+0xc8/0xecc a6xx_pm_resume+0x148/0x200 adreno_resume+0x28/0x34 pm_generic_runtime_resume+0x34/0x48 __rpm_callback+0x70/0x10c rpm_callback+0x34/0x8c rpm_resume+0x414/0x550 __pm_runtime_resume+0x7c/0xa0 msm_gpu_submit+0x60/0x1c0 msm_ioctl_gem_submit+0xadc/0xb60 drm_ioctl_kernel+0x9c/0x118 drm_ioctl+0x27c/0x408 drm_compat_ioctl+0xcc/0xdc __se_compat_sys_ioctl+0x100/0x206c __arm64_compat_sys_ioctl+0x20/0x2c el0_svc_common+0xa8/0x178 el0_svc_compat_handler+0x2c/0x40 el0_svc_compat+0x8/0x10 Reported-by: Rob Clark Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 47 +++++++++++++++++++++++++++++++++++++++++----- drivers/opp/of.c | 5 +++-- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 84035ab8bb31..6f4a73a6391f 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -29,6 +29,8 @@ LIST_HEAD(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 */ +static bool opp_tables_busy; static struct opp_device *_find_opp_dev(const struct device *dev, struct opp_table *opp_table) @@ -1111,8 +1113,6 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index) INIT_LIST_HEAD(&opp_table->opp_list); kref_init(&opp_table->kref); - /* Secure the device table modification */ - list_add(&opp_table->node, &opp_tables); return opp_table; err: @@ -1125,27 +1125,64 @@ void _get_opp_table_kref(struct opp_table *opp_table) kref_get(&opp_table->kref); } +/* + * We need to make sure that the OPP table for a device doesn't get added twice, + * if this routine gets called in parallel with the same device pointer. + * + * The simplest way to enforce that is to perform everything (find existing + * table and if not found, create a new one) under the opp_table_lock, so only + * one creator gets access to the same. But that expands the critical section + * under the lock and may end up causing circular dependencies with frameworks + * like debugfs, interconnect or clock framework as they may be direct or + * indirect users of OPP core. + * + * And for that reason we have to go for a bit tricky implementation here, which + * uses the opp_tables_busy flag to indicate if another creator is in the middle + * of adding an OPP table and others should wait for it to finish. + */ static struct opp_table *_opp_get_opp_table(struct device *dev, int index) { struct opp_table *opp_table; - /* Hold our table modification lock here */ +again: mutex_lock(&opp_table_lock); opp_table = _find_opp_table_unlocked(dev); if (!IS_ERR(opp_table)) goto unlock; + /* + * The opp_tables list or an OPP table's dev_list is getting updated by + * another user, wait for it to finish. + */ + if (unlikely(opp_tables_busy)) { + mutex_unlock(&opp_table_lock); + cpu_relax(); + goto again; + } + + opp_tables_busy = true; opp_table = _managed_opp(dev, index); + + /* Drop the lock to reduce the size of critical section */ + mutex_unlock(&opp_table_lock); + if (opp_table) { if (!_add_opp_dev(dev, opp_table)) { dev_pm_opp_put_opp_table(opp_table); opp_table = ERR_PTR(-ENOMEM); } - goto unlock; + + mutex_lock(&opp_table_lock); + } else { + opp_table = _allocate_opp_table(dev, index); + + mutex_lock(&opp_table_lock); + if (!IS_ERR(opp_table)) + list_add(&opp_table->node, &opp_tables); } - opp_table = _allocate_opp_table(dev, index); + opp_tables_busy = false; unlock: mutex_unlock(&opp_table_lock); diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 9faeb83e4b32..aa0ac5d4e479 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -112,8 +112,6 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np) struct opp_table *opp_table; struct device_node *opp_table_np; - lockdep_assert_held(&opp_table_lock); - opp_table_np = of_get_parent(opp_np); if (!opp_table_np) goto err; @@ -121,12 +119,15 @@ static struct opp_table *_find_table_of_opp_np(struct device_node *opp_np) /* It is safe to put the node now as all we need now is its address */ of_node_put(opp_table_np); + mutex_lock(&opp_table_lock); list_for_each_entry(opp_table, &opp_tables, node) { if (opp_table_np == opp_table->np) { _get_opp_table_kref(opp_table); + mutex_unlock(&opp_table_lock); return opp_table; } } + mutex_unlock(&opp_table_lock); err: return ERR_PTR(-ENODEV); From 9e62edac519da71bbc981e4c984fe67729b0d1f3 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Tue, 27 Oct 2020 11:49:03 +0530 Subject: [PATCH 03/15] opp: Don't return opp_dev from _find_opp_dev() The caller of _find_opp_dev() only needs to know if the opp_dev is there in the list or not. Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 6f4a73a6391f..9915e8487f0b 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -32,31 +32,29 @@ DEFINE_MUTEX(opp_table_lock); /* Flag indicating that opp_tables list is being updated at the moment */ static bool opp_tables_busy; -static struct opp_device *_find_opp_dev(const struct device *dev, - struct opp_table *opp_table) +static bool _find_opp_dev(const struct device *dev, struct opp_table *opp_table) { struct opp_device *opp_dev; + bool found = false; + mutex_lock(&opp_table->lock); list_for_each_entry(opp_dev, &opp_table->dev_list, node) - if (opp_dev->dev == dev) - return opp_dev; + if (opp_dev->dev == dev) { + found = true; + break; + } - return NULL; + mutex_unlock(&opp_table->lock); + return found; } static struct opp_table *_find_opp_table_unlocked(struct device *dev) { struct opp_table *opp_table; - bool found; list_for_each_entry(opp_table, &opp_tables, node) { - mutex_lock(&opp_table->lock); - found = !!_find_opp_dev(dev, opp_table); - mutex_unlock(&opp_table->lock); - - if (found) { + if (_find_opp_dev(dev, opp_table)) { _get_opp_table_kref(opp_table); - return opp_table; } } From cf1fac943c6341dfed1db1293864c9fcad47bac3 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Thu, 19 Nov 2020 11:24:32 +0530 Subject: [PATCH 04/15] opp: Reduce the size of critical section in _opp_kref_release() There is a lot of stuff here which can be done outside of the opp_table->lock, do that. This helps avoiding a circular dependency lockdeps around debugfs. Reported-by: Rob Clark Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 94 +++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 9915e8487f0b..27a0e49b24ab 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1252,9 +1252,14 @@ void _opp_free(struct dev_pm_opp *opp) kfree(opp); } -static void _opp_kref_release(struct dev_pm_opp *opp, - struct opp_table *opp_table) +static void _opp_kref_release(struct kref *kref) { + struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); + struct opp_table *opp_table = opp->opp_table; + + list_del(&opp->node); + mutex_unlock(&opp_table->lock); + /* * Notify the changes in the availability of the operable * frequency/voltage list. @@ -1262,27 +1267,9 @@ static void _opp_kref_release(struct dev_pm_opp *opp, blocking_notifier_call_chain(&opp_table->head, OPP_EVENT_REMOVE, opp); _of_opp_free_required_opps(opp_table, opp); opp_debug_remove_one(opp); - list_del(&opp->node); kfree(opp); } -static void _opp_kref_release_unlocked(struct kref *kref) -{ - struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); - struct opp_table *opp_table = opp->opp_table; - - _opp_kref_release(opp, opp_table); -} - -static void _opp_kref_release_locked(struct kref *kref) -{ - struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref); - struct opp_table *opp_table = opp->opp_table; - - _opp_kref_release(opp, opp_table); - mutex_unlock(&opp_table->lock); -} - void dev_pm_opp_get(struct dev_pm_opp *opp) { kref_get(&opp->kref); @@ -1290,16 +1277,10 @@ void dev_pm_opp_get(struct dev_pm_opp *opp) void dev_pm_opp_put(struct dev_pm_opp *opp) { - kref_put_mutex(&opp->kref, _opp_kref_release_locked, - &opp->opp_table->lock); + kref_put_mutex(&opp->kref, _opp_kref_release, &opp->opp_table->lock); } EXPORT_SYMBOL_GPL(dev_pm_opp_put); -static void dev_pm_opp_put_unlocked(struct dev_pm_opp *opp) -{ - kref_put(&opp->kref, _opp_kref_release_unlocked); -} - /** * dev_pm_opp_remove() - Remove an OPP from OPP table * @dev: device for which we do this operation @@ -1343,30 +1324,49 @@ void dev_pm_opp_remove(struct device *dev, unsigned long freq) } EXPORT_SYMBOL_GPL(dev_pm_opp_remove); +static struct dev_pm_opp *_opp_get_next(struct opp_table *opp_table, + bool dynamic) +{ + struct dev_pm_opp *opp = NULL, *temp; + + mutex_lock(&opp_table->lock); + list_for_each_entry(temp, &opp_table->opp_list, node) { + if (dynamic == temp->dynamic) { + opp = temp; + break; + } + } + + mutex_unlock(&opp_table->lock); + return opp; +} + bool _opp_remove_all_static(struct opp_table *opp_table) { - struct dev_pm_opp *opp, *tmp; - bool ret = true; + struct dev_pm_opp *opp; mutex_lock(&opp_table->lock); if (!opp_table->parsed_static_opps) { - ret = false; - goto unlock; + mutex_unlock(&opp_table->lock); + return false; } - if (--opp_table->parsed_static_opps) - goto unlock; - - list_for_each_entry_safe(opp, tmp, &opp_table->opp_list, node) { - if (!opp->dynamic) - dev_pm_opp_put_unlocked(opp); + if (--opp_table->parsed_static_opps) { + mutex_unlock(&opp_table->lock); + return true; } -unlock: mutex_unlock(&opp_table->lock); - return ret; + /* + * Can't remove the OPP from under the lock, debugfs removal needs to + * happen lock less to avoid circular dependency issues. + */ + while ((opp = _opp_get_next(opp_table, false))) + dev_pm_opp_put(opp); + + return true; } /** @@ -1378,21 +1378,21 @@ bool _opp_remove_all_static(struct opp_table *opp_table) void dev_pm_opp_remove_all_dynamic(struct device *dev) { struct opp_table *opp_table; - struct dev_pm_opp *opp, *temp; + struct dev_pm_opp *opp; int count = 0; opp_table = _find_opp_table(dev); if (IS_ERR(opp_table)) return; - mutex_lock(&opp_table->lock); - list_for_each_entry_safe(opp, temp, &opp_table->opp_list, node) { - if (opp->dynamic) { - dev_pm_opp_put_unlocked(opp); - count++; - } + /* + * Can't remove the OPP from under the lock, debugfs removal needs to + * happen lock less to avoid circular dependency issues. + */ + while ((opp = _opp_get_next(opp_table, true))) { + dev_pm_opp_put(opp); + count++; } - mutex_unlock(&opp_table->lock); /* Drop the references taken by dev_pm_opp_add() */ while (count--) From 873c9851eb54b78c27a0d753f6dd7e377572a0aa Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 10:28:43 +0530 Subject: [PATCH 05/15] cpufreq: dt: Don't (ab)use dev_pm_opp_get_opp_table() to create OPP table Initially, the helper dev_pm_opp_get_opp_table() was supposed to be used only for the OPP core's internal use (it tries to find an existing OPP table and if it doesn't find one, then it allocates the OPP table). Sometime back, the cpufreq-dt driver started using it to make sure all the relevant resources required by the OPP core are available earlier during initialization process to properly propagate -EPROBE_DEFER. It worked but it also abused the API to create an OPP table, which should be created with the help of other helpers provided by the OPP core. The OPP core will be updated in a later commit to limit the scope of dev_pm_opp_get_opp_table() to only finding an existing OPP table and not create one. This commit updates the cpufreq-dt driver before that happens. Now the cpufreq-dt driver creates the OPP and cpufreq tables for all the CPUs from driver's init callback itself. Tested-by: Marek Szyprowski Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq-dt.c | 155 +++++++++++++++-------------------- 1 file changed, 67 insertions(+), 88 deletions(-) diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e363ae04aac6..5aa3d4e3140d 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -30,7 +30,7 @@ struct private_data { cpumask_var_t cpus; struct device *cpu_dev; struct opp_table *opp_table; - struct opp_table *reg_opp_table; + struct cpufreq_frequency_table *freq_table; bool have_static_opps; }; @@ -102,7 +102,6 @@ static const char *find_supply_name(struct device *dev) static int cpufreq_init(struct cpufreq_policy *policy) { - struct cpufreq_frequency_table *freq_table; struct private_data *priv; struct device *cpu_dev; struct clk *cpu_clk; @@ -114,9 +113,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) pr_err("failed to find data for cpu%d\n", policy->cpu); return -ENODEV; } - cpu_dev = priv->cpu_dev; - cpumask_copy(policy->cpus, priv->cpus); cpu_clk = clk_get(cpu_dev, NULL); if (IS_ERR(cpu_clk)) { @@ -125,67 +122,32 @@ static int cpufreq_init(struct cpufreq_policy *policy) return ret; } - /* - * Initialize OPP tables for all policy->cpus. They will be shared by - * all CPUs which have marked their CPUs shared with OPP bindings. - * - * For platforms not using operating-points-v2 bindings, we do this - * before updating policy->cpus. Otherwise, we will end up creating - * duplicate OPPs for policy->cpus. - * - * OPPs might be populated at runtime, don't check for error here - */ - 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 - * give platform code chance to provide it for us. - */ - ret = dev_pm_opp_get_opp_count(cpu_dev); - if (ret <= 0) { - dev_err(cpu_dev, "OPP table can't be empty\n"); - ret = -ENODEV; - goto out_free_opp; - } - - 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_opp; - } + transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev); + if (!transition_latency) + transition_latency = CPUFREQ_ETERNAL; + cpumask_copy(policy->cpus, priv->cpus); policy->driver_data = priv; policy->clk = cpu_clk; - policy->freq_table = freq_table; - + policy->freq_table = priv->freq_table; policy->suspend_freq = dev_pm_opp_get_suspend_opp_freq(cpu_dev) / 1000; + policy->cpuinfo.transition_latency = transition_latency; + policy->dvfs_possible_from_any_cpu = true; /* Support turbo/boost mode */ if (policy_has_boost_freq(policy)) { /* This gets disabled by core on driver unregister */ ret = cpufreq_enable_boost_support(); if (ret) - goto out_free_cpufreq_table; + goto out_clk_put; cpufreq_dt_attr[1] = &cpufreq_freq_attr_scaling_boost_freqs; } - transition_latency = dev_pm_opp_get_max_transition_latency(cpu_dev); - if (!transition_latency) - transition_latency = CPUFREQ_ETERNAL; - - policy->cpuinfo.transition_latency = transition_latency; - policy->dvfs_possible_from_any_cpu = true; - dev_pm_opp_of_register_em(cpu_dev, policy->cpus); return 0; -out_free_cpufreq_table: - dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); -out_free_opp: - if (priv->have_static_opps) - dev_pm_opp_of_cpumask_remove_table(policy->cpus); +out_clk_put: clk_put(cpu_clk); return ret; @@ -208,11 +170,6 @@ static int cpufreq_offline(struct cpufreq_policy *policy) static int cpufreq_exit(struct cpufreq_policy *policy) { - struct private_data *priv = policy->driver_data; - - dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); - if (priv->have_static_opps) - dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); clk_put(policy->clk); return 0; } @@ -236,6 +193,7 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu) { struct private_data *priv; struct device *cpu_dev; + bool fallback = false; const char *reg_name; int ret; @@ -254,68 +212,87 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu) if (!alloc_cpumask_var(&priv->cpus, GFP_KERNEL)) return -ENOMEM; + cpumask_set_cpu(cpu, priv->cpus); priv->cpu_dev = cpu_dev; - /* Try to get OPP table early to ensure resources are available */ - priv->opp_table = dev_pm_opp_get_opp_table(cpu_dev); - if (IS_ERR(priv->opp_table)) { - ret = PTR_ERR(priv->opp_table); - if (ret != -EPROBE_DEFER) - dev_err(cpu_dev, "failed to get OPP table: %d\n", ret); - goto free_cpumask; - } - /* * OPP layer will be taking care of regulators now, but it needs to know * the name of the regulator first. */ reg_name = find_supply_name(cpu_dev); if (reg_name) { - priv->reg_opp_table = dev_pm_opp_set_regulators(cpu_dev, - ®_name, 1); - if (IS_ERR(priv->reg_opp_table)) { - ret = PTR_ERR(priv->reg_opp_table); + priv->opp_table = dev_pm_opp_set_regulators(cpu_dev, ®_name, + 1); + if (IS_ERR(priv->opp_table)) { + ret = PTR_ERR(priv->opp_table); if (ret != -EPROBE_DEFER) dev_err(cpu_dev, "failed to set regulators: %d\n", ret); - goto put_table; + goto free_cpumask; } } - /* Find OPP sharing information so we can fill pri->cpus here */ /* Get OPP-sharing information from "operating-points-v2" bindings */ ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, priv->cpus); if (ret) { if (ret != -ENOENT) - goto put_reg; + goto out; /* * operating-points-v2 not supported, fallback to all CPUs share * OPP for backward compatibility if the platform hasn't set * sharing CPUs. */ - if (dev_pm_opp_get_sharing_cpus(cpu_dev, priv->cpus)) { - cpumask_setall(priv->cpus); + if (dev_pm_opp_get_sharing_cpus(cpu_dev, priv->cpus)) + fallback = true; + } - /* - * OPP tables are initialized only for cpu, do it for - * others as well. - */ - ret = dev_pm_opp_set_sharing_cpus(cpu_dev, priv->cpus); - if (ret) - dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", - __func__, ret); - } + /* + * Initialize OPP tables for all priv->cpus. They will be shared by + * all CPUs which have marked their CPUs shared with OPP bindings. + * + * For platforms not using operating-points-v2 bindings, we do this + * before updating priv->cpus. Otherwise, we will end up creating + * duplicate OPPs for the CPUs. + * + * OPPs might be populated at runtime, don't check for error here. + */ + if (!dev_pm_opp_of_cpumask_add_table(priv->cpus)) + priv->have_static_opps = true; + + /* + * The OPP table must be initialized, statically or dynamically, by this + * point. + */ + ret = dev_pm_opp_get_opp_count(cpu_dev); + if (ret <= 0) { + dev_err(cpu_dev, "OPP table can't be empty\n"); + ret = -ENODEV; + goto out; + } + + if (fallback) { + cpumask_setall(priv->cpus); + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, priv->cpus); + if (ret) + dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", + __func__, ret); + } + + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &priv->freq_table); + if (ret) { + dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); + goto out; } list_add(&priv->node, &priv_list); return 0; -put_reg: - if (priv->reg_opp_table) - dev_pm_opp_put_regulators(priv->reg_opp_table); -put_table: - dev_pm_opp_put_opp_table(priv->opp_table); +out: + if (priv->have_static_opps) + dev_pm_opp_of_cpumask_remove_table(priv->cpus); + if (priv->opp_table) + dev_pm_opp_put_regulators(priv->opp_table); free_cpumask: free_cpumask_var(priv->cpus); return ret; @@ -326,9 +303,11 @@ static void dt_cpufreq_release(void) struct private_data *priv, *tmp; list_for_each_entry_safe(priv, tmp, &priv_list, node) { - if (priv->reg_opp_table) - dev_pm_opp_put_regulators(priv->reg_opp_table); - dev_pm_opp_put_opp_table(priv->opp_table); + dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &priv->freq_table); + if (priv->have_static_opps) + dev_pm_opp_of_cpumask_remove_table(priv->cpus); + if (priv->opp_table) + dev_pm_opp_put_regulators(priv->opp_table); free_cpumask_var(priv->cpus); list_del(&priv->node); } From e77dcb0b732dd355ca594909f6c2085dfc46cde2 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 10:37:16 +0530 Subject: [PATCH 06/15] opp: Don't create an OPP table from dev_pm_opp_get_opp_table() It has been found that some users (like cpufreq-dt and others on LKML) have abused the helper dev_pm_opp_get_opp_table() to create the OPP table instead of just finding it, which is the wrong thing to do. This routine was meant for OPP core's internal working and exposed the whole functionality by mistake. Change the scope of dev_pm_opp_get_opp_table() to only finding the table. The internal helpers _opp_get_opp_table*() are thus renamed to _add_opp_table*(), dev_pm_opp_get_opp_table_indexed() is removed (as we don't need the index field for finding the OPP table) and so the only user, genpd, is updated. Note that the prototype of _add_opp_table() was already left in opp.h by mistake when it was removed earlier and so we weren't required to add it now. Acked-by: Ulf Hansson Signed-off-by: Viresh Kumar --- drivers/base/power/domain.c | 2 +- drivers/opp/core.c | 29 ++++++++++++++--------------- drivers/opp/of.c | 4 ++-- drivers/opp/opp.h | 1 + include/linux/pm_opp.h | 1 - 5 files changed, 18 insertions(+), 19 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 743268996336..92b750b865d5 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -2249,7 +2249,7 @@ int of_genpd_add_provider_onecell(struct device_node *np, * Save table for faster processing while setting * performance state. */ - genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i); + genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev); WARN_ON(IS_ERR(genpd->opp_table)); } diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 27a0e49b24ab..8f53c1b48911 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1138,7 +1138,7 @@ void _get_opp_table_kref(struct opp_table *opp_table) * uses the opp_tables_busy flag to indicate if another creator is in the middle * of adding an OPP table and others should wait for it to finish. */ -static struct opp_table *_opp_get_opp_table(struct device *dev, int index) +struct opp_table *_add_opp_table_indexed(struct device *dev, int index) { struct opp_table *opp_table; @@ -1188,18 +1188,17 @@ static struct opp_table *_opp_get_opp_table(struct device *dev, int index) return opp_table; } +struct opp_table *_add_opp_table(struct device *dev) +{ + return _add_opp_table_indexed(dev, 0); +} + struct opp_table *dev_pm_opp_get_opp_table(struct device *dev) { - return _opp_get_opp_table(dev, 0); + return _find_opp_table(dev); } EXPORT_SYMBOL_GPL(dev_pm_opp_get_opp_table); -struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev, - int index) -{ - return _opp_get_opp_table(dev, index); -} - static void _opp_table_kref_release(struct kref *kref) { struct opp_table *opp_table = container_of(kref, struct opp_table, kref); @@ -1627,7 +1626,7 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev, { struct opp_table *opp_table; - opp_table = dev_pm_opp_get_opp_table(dev); + opp_table = _add_opp_table(dev); if (IS_ERR(opp_table)) return opp_table; @@ -1686,7 +1685,7 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name) { struct opp_table *opp_table; - opp_table = dev_pm_opp_get_opp_table(dev); + opp_table = _add_opp_table(dev); if (IS_ERR(opp_table)) return opp_table; @@ -1779,7 +1778,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, struct regulator *reg; int ret, i; - opp_table = dev_pm_opp_get_opp_table(dev); + opp_table = _add_opp_table(dev); if (IS_ERR(opp_table)) return opp_table; @@ -1887,7 +1886,7 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name) struct opp_table *opp_table; int ret; - opp_table = dev_pm_opp_get_opp_table(dev); + opp_table = _add_opp_table(dev); if (IS_ERR(opp_table)) return opp_table; @@ -1955,7 +1954,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, if (!set_opp) return ERR_PTR(-EINVAL); - opp_table = dev_pm_opp_get_opp_table(dev); + opp_table = _add_opp_table(dev); if (IS_ERR(opp_table)) return opp_table; @@ -2039,7 +2038,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, int index = 0, ret = -EINVAL; const char **name = names; - opp_table = dev_pm_opp_get_opp_table(dev); + opp_table = _add_opp_table(dev); if (IS_ERR(opp_table)) return opp_table; @@ -2204,7 +2203,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) struct opp_table *opp_table; int ret; - opp_table = dev_pm_opp_get_opp_table(dev); + opp_table = _add_opp_table(dev); if (IS_ERR(opp_table)) return PTR_ERR(opp_table); diff --git a/drivers/opp/of.c b/drivers/opp/of.c index aa0ac5d4e479..6b7f0066942d 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -975,7 +975,7 @@ int dev_pm_opp_of_add_table(struct device *dev) struct opp_table *opp_table; int ret; - opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0); + opp_table = _add_opp_table_indexed(dev, 0); if (IS_ERR(opp_table)) return PTR_ERR(opp_table); @@ -1030,7 +1030,7 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index) index = 0; } - opp_table = dev_pm_opp_get_opp_table_indexed(dev, index); + opp_table = _add_opp_table_indexed(dev, index); if (IS_ERR(opp_table)) return PTR_ERR(opp_table); diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index ebd930e0b3ca..4ced7ffa8158 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -224,6 +224,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *o int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic); void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu); struct opp_table *_add_opp_table(struct device *dev); +struct opp_table *_add_opp_table_indexed(struct device *dev, int index); void _put_opp_list_kref(struct opp_table *opp_table); #ifdef CONFIG_OF diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index dbb484524f82..1435c054016a 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -90,7 +90,6 @@ struct dev_pm_set_opp_data { #if defined(CONFIG_PM_OPP) struct opp_table *dev_pm_opp_get_opp_table(struct device *dev); -struct opp_table *dev_pm_opp_get_opp_table_indexed(struct device *dev, int index); void dev_pm_opp_put_opp_table(struct opp_table *opp_table); unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp); From c7bf8758c955e6272c0f4b2411d7a85abce8fafe Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 12:16:52 +0530 Subject: [PATCH 07/15] opp: Allow dev_pm_opp_put_*() APIs to accept NULL opp_table This allows the callers to drop the unnecessary checks. Signed-off-by: Viresh Kumar --- drivers/opp/core.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 8f53c1b48911..4268eb359915 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1660,6 +1660,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_supported_hw); */ void dev_pm_opp_put_supported_hw(struct opp_table *opp_table) { + if (unlikely(!opp_table)) + return; + /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list)); @@ -1716,6 +1719,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_prop_name); */ void dev_pm_opp_put_prop_name(struct opp_table *opp_table) { + if (unlikely(!opp_table)) + return; + /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list)); @@ -1844,6 +1850,9 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table) { int i; + if (unlikely(!opp_table)) + return; + if (!opp_table->regulators) goto put_opp_table; @@ -1926,6 +1935,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_clkname); */ void dev_pm_opp_put_clkname(struct opp_table *opp_table) { + if (unlikely(!opp_table)) + return; + /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list)); @@ -1981,6 +1993,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_opp_helper); */ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table) { + if (unlikely(!opp_table)) + return; + /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list)); @@ -2109,6 +2124,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_attach_genpd); */ void dev_pm_opp_detach_genpd(struct opp_table *opp_table) { + if (unlikely(!opp_table)) + return; + /* * Acquire genpd_virt_dev_lock to make sure virt_dev isn't getting * used in parallel. From 5f6ffb8d8f8fdf672cbc4f27888ce075df13d49c Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 12:18:39 +0530 Subject: [PATCH 08/15] cpufreq: dt: dev_pm_opp_put_regulators() accepts NULL argument The dev_pm_opp_put_*() APIs now accepts a NULL opp_table pointer and so there is no need for us to carry the extra checks. Drop them. Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq-dt.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 5aa3d4e3140d..ad4234518ef6 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -291,8 +291,7 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu) out: if (priv->have_static_opps) dev_pm_opp_of_cpumask_remove_table(priv->cpus); - if (priv->opp_table) - dev_pm_opp_put_regulators(priv->opp_table); + dev_pm_opp_put_regulators(priv->opp_table); free_cpumask: free_cpumask_var(priv->cpus); return ret; @@ -306,8 +305,7 @@ static void dt_cpufreq_release(void) dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &priv->freq_table); if (priv->have_static_opps) dev_pm_opp_of_cpumask_remove_table(priv->cpus); - if (priv->opp_table) - dev_pm_opp_put_regulators(priv->opp_table); + dev_pm_opp_put_regulators(priv->opp_table); free_cpumask_var(priv->cpus); list_del(&priv->node); } From 2ff8fe13ac6da7a7c45d610cc3237c8556610f07 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 12:18:39 +0530 Subject: [PATCH 09/15] cpufreq: qcom-cpufreq-nvmem: dev_pm_opp_put_*() accepts NULL argument The dev_pm_opp_put_*() APIs now accepts a NULL opp_table pointer and so there is no need for us to carry the extra checks. Drop them. Reviewed-by: Ilia Lin Signed-off-by: Viresh Kumar --- drivers/cpufreq/qcom-cpufreq-nvmem.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c index d06b37822c3d..747d602f221e 100644 --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c @@ -397,19 +397,19 @@ static int qcom_cpufreq_probe(struct platform_device *pdev) free_genpd_opp: for_each_possible_cpu(cpu) { - if (IS_ERR_OR_NULL(drv->genpd_opp_tables[cpu])) + if (IS_ERR(drv->genpd_opp_tables[cpu])) break; dev_pm_opp_detach_genpd(drv->genpd_opp_tables[cpu]); } kfree(drv->genpd_opp_tables); free_opp: for_each_possible_cpu(cpu) { - if (IS_ERR_OR_NULL(drv->names_opp_tables[cpu])) + if (IS_ERR(drv->names_opp_tables[cpu])) break; dev_pm_opp_put_prop_name(drv->names_opp_tables[cpu]); } for_each_possible_cpu(cpu) { - if (IS_ERR_OR_NULL(drv->hw_opp_tables[cpu])) + if (IS_ERR(drv->hw_opp_tables[cpu])) break; dev_pm_opp_put_supported_hw(drv->hw_opp_tables[cpu]); } @@ -430,12 +430,9 @@ static int qcom_cpufreq_remove(struct platform_device *pdev) platform_device_unregister(cpufreq_dt_pdev); for_each_possible_cpu(cpu) { - if (drv->names_opp_tables[cpu]) - dev_pm_opp_put_supported_hw(drv->names_opp_tables[cpu]); - if (drv->hw_opp_tables[cpu]) - dev_pm_opp_put_supported_hw(drv->hw_opp_tables[cpu]); - if (drv->genpd_opp_tables[cpu]) - dev_pm_opp_detach_genpd(drv->genpd_opp_tables[cpu]); + dev_pm_opp_put_supported_hw(drv->names_opp_tables[cpu]); + dev_pm_opp_put_supported_hw(drv->hw_opp_tables[cpu]); + dev_pm_opp_detach_genpd(drv->genpd_opp_tables[cpu]); } kfree(drv->names_opp_tables); From 814568728373699907971f897b89d95736b0d880 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 12:18:39 +0530 Subject: [PATCH 10/15] PM / devfreq: exynos: dev_pm_opp_put_*() accepts NULL argument The dev_pm_opp_put_*() APIs now accepts a NULL opp_table pointer and so there is no need for us to carry the extra check. Drop them. Acked-by: Chanwoo Choi Signed-off-by: Viresh Kumar --- drivers/devfreq/exynos-bus.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 1e684a448c9e..143fd58ec3dc 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -158,10 +158,8 @@ static void exynos_bus_exit(struct device *dev) dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); - if (bus->opp_table) { - dev_pm_opp_put_regulators(bus->opp_table); - bus->opp_table = NULL; - } + dev_pm_opp_put_regulators(bus->opp_table); + bus->opp_table = NULL; } static void exynos_bus_passive_exit(struct device *dev) @@ -444,10 +442,8 @@ static int exynos_bus_probe(struct platform_device *pdev) dev_pm_opp_of_remove_table(dev); clk_disable_unprepare(bus->clk); err_reg: - if (!passive) { - dev_pm_opp_put_regulators(bus->opp_table); - bus->opp_table = NULL; - } + dev_pm_opp_put_regulators(bus->opp_table); + bus->opp_table = NULL; return ret; } From 72ba9e226fac8a9958b5201428a387c348515289 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 12:18:39 +0530 Subject: [PATCH 11/15] drm/lima: dev_pm_opp_put_*() accepts NULL argument The dev_pm_opp_put_*() APIs now accepts a NULL opp_table pointer and so there is no need for us to carry the extra check. Drop them. Reviewed-by: Qiang Yu Signed-off-by: Viresh Kumar --- drivers/gpu/drm/lima/lima_devfreq.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/lima/lima_devfreq.c b/drivers/gpu/drm/lima/lima_devfreq.c index bbe02817721b..e7b7b8dfd792 100644 --- a/drivers/gpu/drm/lima/lima_devfreq.c +++ b/drivers/gpu/drm/lima/lima_devfreq.c @@ -110,15 +110,10 @@ void lima_devfreq_fini(struct lima_device *ldev) devfreq->opp_of_table_added = false; } - if (devfreq->regulators_opp_table) { - dev_pm_opp_put_regulators(devfreq->regulators_opp_table); - devfreq->regulators_opp_table = NULL; - } - - if (devfreq->clkname_opp_table) { - dev_pm_opp_put_clkname(devfreq->clkname_opp_table); - devfreq->clkname_opp_table = NULL; - } + dev_pm_opp_put_regulators(devfreq->regulators_opp_table); + dev_pm_opp_put_clkname(devfreq->clkname_opp_table); + devfreq->regulators_opp_table = NULL; + devfreq->clkname_opp_table = NULL; } int lima_devfreq_init(struct lima_device *ldev) From b66ba5b5938f8a51d4cb97d1392065d09551bc75 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 12:18:39 +0530 Subject: [PATCH 12/15] drm/panfrost: dev_pm_opp_put_*() accepts NULL argument The dev_pm_opp_put_*() APIs now accepts a NULL opp_table pointer and so there is no need for us to carry the extra check. Drop them. Reviewed-by: Steven Price Signed-off-by: Viresh Kumar --- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 8ab025d0035f..97b5abc7c188 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -170,10 +170,8 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev) pfdevfreq->opp_of_table_added = false; } - if (pfdevfreq->regulators_opp_table) { - dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table); - pfdevfreq->regulators_opp_table = NULL; - } + dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table); + pfdevfreq->regulators_opp_table = NULL; } void panfrost_devfreq_resume(struct panfrost_device *pfdev) From e91e3d902b76c3f2a238873a17958080af018f08 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 6 Nov 2020 12:18:39 +0530 Subject: [PATCH 13/15] media: venus: dev_pm_opp_put_*() accepts NULL argument The dev_pm_opp_put_*() APIs now accepts a NULL opp_table pointer and so there is no need for us to carry the extra check. Drop them. Signed-off-by: Viresh Kumar --- drivers/media/platform/qcom/venus/pm_helpers.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c index 57877eacecf0..e1e9130288ad 100644 --- a/drivers/media/platform/qcom/venus/pm_helpers.c +++ b/drivers/media/platform/qcom/venus/pm_helpers.c @@ -898,8 +898,7 @@ static void core_put_v4(struct device *dev) if (core->has_opp_table) dev_pm_opp_of_remove_table(dev); - if (core->opp_table) - dev_pm_opp_put_clkname(core->opp_table); + dev_pm_opp_put_clkname(core->opp_table); } From 24b3c963c0108f3da6d978d74a745c824ab551dc Mon Sep 17 00:00:00 2001 From: Nicola Mazzucato Date: Tue, 8 Dec 2020 17:42:26 +0000 Subject: [PATCH 14/15] dt-bindings: opp: Allow empty OPP tables Currently the optional property opp-shared is used within an opp table to tell that a set of devices share their clock/voltage lines (and the OPP points). It is therefore possible to use an empty OPP table to convey only that information, useful in situations where the opp points are provided via other means (hardware. firmware, etc). Update the documentation to remark this additional case and provide an example. Signed-off-by: Nicola Mazzucato Signed-off-by: Viresh Kumar --- Documentation/devicetree/bindings/opp/opp.txt | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt index 9847dfeeffcb..08b3da4736cf 100644 --- a/Documentation/devicetree/bindings/opp/opp.txt +++ b/Documentation/devicetree/bindings/opp/opp.txt @@ -65,7 +65,9 @@ Required properties: - OPP nodes: One or more OPP nodes describing voltage-current-frequency combinations. Their name isn't significant but their phandle can be used to - reference an OPP. + reference an OPP. These are mandatory except for the case where the OPP table + is present only to indicate dependency between devices using the opp-shared + property. Optional properties: - opp-shared: Indicates that device nodes using this OPP Table Node's phandle @@ -568,3 +570,53 @@ Example 6: opp-microvolt-, opp-microamp-: }; }; }; + +Example 7: Single cluster Quad-core ARM cortex A53, OPP points from firmware, +distinct clock controls but two sets of clock/voltage/current lines. + +/ { + cpus { + #address-cells = <2>; + #size-cells = <0>; + + cpu@0 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x100>; + next-level-cache = <&A53_L2>; + clocks = <&dvfs_controller 0>; + operating-points-v2 = <&cpu_opp0_table>; + }; + cpu@1 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x101>; + next-level-cache = <&A53_L2>; + clocks = <&dvfs_controller 1>; + operating-points-v2 = <&cpu_opp0_table>; + }; + cpu@2 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x102>; + next-level-cache = <&A53_L2>; + clocks = <&dvfs_controller 2>; + operating-points-v2 = <&cpu_opp1_table>; + }; + cpu@3 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x103>; + next-level-cache = <&A53_L2>; + clocks = <&dvfs_controller 3>; + operating-points-v2 = <&cpu_opp1_table>; + }; + + }; + + cpu_opp0_table: opp0_table { + compatible = "operating-points-v2"; + opp-shared; + }; + + cpu_opp1_table: opp1_table { + compatible = "operating-points-v2"; + opp-shared; + }; +}; From 6ee70e8c34e37a34f4dc2c8bc06febffd375fac4 Mon Sep 17 00:00:00 2001 From: Nicola Mazzucato Date: Tue, 8 Dec 2020 17:42:27 +0000 Subject: [PATCH 15/15] opp: of: Allow empty opp-table with opp-shared The opp binding now allows to have an empty opp table and shared-opp to still describe that devices share v/f lines. When initialising an empty opp table, allow such case by: - treating such conditions with warnings in place of errors - don't fail on empty table Signed-off-by: Nicola Mazzucato Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 874b58756220..96113fc0e18c 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -169,7 +169,8 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, /* Traversing the first OPP node is all we need */ np = of_get_next_available_child(opp_np, NULL); if (!np) { - dev_err(dev, "Empty OPP table\n"); + dev_warn(dev, "Empty OPP table\n"); + return; } @@ -377,7 +378,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, struct icc_path **paths; ret = _bandwidth_supported(dev, opp_table); - if (ret <= 0) + if (ret == -EINVAL) + return 0; /* Empty OPP table is a valid corner-case, let's not fail */ + else if (ret <= 0) return ret; ret = 0;