tjh.dev / kernel
Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

interconnect: avoid memory allocation when 'icc_bw_lock' is held

The 'icc_bw_lock' mutex is introduced in commit af42269c3523
("interconnect: Fix locking for runpm vs reclaim") in order to decouple
serialization of bw aggregation from codepaths that require memory
allocation.

However commit d30f83d278a9 ("interconnect: core: Add dynamic id
allocation support") added a devm_kasprintf() call into a path protected
by the 'icc_bw_lock' which causes the following lockdep warning on
machines like the Lenovo ThinkPad X13s:

======================================================
WARNING: possible circular locking dependency detected
6.16.0-rc3 #15 Not tainted
------------------------------------------------------
(udev-worker)/342 is trying to acquire lock:
ffffb973f7ec4638 (fs_reclaim){+.+.}-{0:0}, at: __kmalloc_node_track_caller_noprof+0xa0/0x3e0

but task is already holding lock:
ffffb973f7f7f0e8 (icc_bw_lock){+.+.}-{4:4}, at: icc_node_add+0x44/0x154

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (icc_bw_lock){+.+.}-{4:4}:
icc_init+0x48/0x108
do_one_initcall+0x64/0x30c
kernel_init_freeable+0x27c/0x500
kernel_init+0x20/0x1d8
ret_from_fork+0x10/0x20

-> #0 (fs_reclaim){+.+.}-{0:0}:
__lock_acquire+0x136c/0x2114
lock_acquire+0x1c8/0x354
fs_reclaim_acquire+0x74/0xa8
__kmalloc_node_track_caller_noprof+0xa0/0x3e0
devm_kmalloc+0x54/0x124
devm_kvasprintf+0x74/0xd4
devm_kasprintf+0x58/0x80
icc_node_add+0xb4/0x154
qcom_osm_l3_probe+0x20c/0x314 [icc_osm_l3]
platform_probe+0x68/0xd8
really_probe+0xc0/0x38c
__driver_probe_device+0x7c/0x160
driver_probe_device+0x40/0x110
__driver_attach+0xfc/0x208
bus_for_each_dev+0x74/0xd0
driver_attach+0x24/0x30
bus_add_driver+0x110/0x234
driver_register+0x60/0x128
__platform_driver_register+0x24/0x30
osm_l3_driver_init+0x20/0x1000 [icc_osm_l3]
do_one_initcall+0x64/0x30c
do_init_module+0x58/0x23c
load_module+0x1df8/0x1f70
init_module_from_file+0x88/0xc4
idempotent_init_module+0x188/0x280
__arm64_sys_finit_module+0x6c/0xd8
invoke_syscall+0x48/0x110
el0_svc_common.constprop.0+0xc0/0xe0
do_el0_svc+0x1c/0x28
el0_svc+0x4c/0x158
el0t_64_sync_handler+0xc8/0xcc
el0t_64_sync+0x198/0x19c

other info that might help us debug this:

Possible unsafe locking scenario:

CPU0 CPU1
---- ----
lock(icc_bw_lock);
lock(fs_reclaim);
lock(icc_bw_lock);
lock(fs_reclaim);

*** DEADLOCK ***

The icc_node_add() functions is not designed to fail, and as such it
should not do any memory allocation. In order to avoid this, add a new
helper function for the name generation to be called by drivers which
are using the new dynamic id feature.

Fixes: d30f83d278a9 ("interconnect: core: Add dynamic id allocation support")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
Link: https://lore.kernel.org/r/20250625-icc-bw-lockdep-v3-1-2b8f8b8987c4@gmail.com
Co-developed-by: Johan Hovold <johan+linaro@kernel.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
Link: https://lore.kernel.org/r/20250627075854.26943-1-johan+linaro@kernel.org
Signed-off-by: Georgi Djakov <djakov@kernel.org>

authored by

Gabor Juhos and committed by
Georgi Djakov c5b60592 618c810a

+44 -6
+25 -4
drivers/interconnect/core.c
··· 909 909 return; 910 910 911 911 kfree(node->links); 912 + if (node->id >= ICC_DYN_ID_START) 913 + kfree(node->name); 912 914 kfree(node); 913 915 } 914 916 EXPORT_SYMBOL_GPL(icc_node_destroy); 917 + 918 + /** 919 + * icc_node_set_name() - set node name 920 + * @node: node 921 + * @provider: node provider 922 + * @name: node name 923 + * 924 + * Return: 0 on success, or -ENOMEM on allocation failure 925 + */ 926 + int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider, const char *name) 927 + { 928 + if (node->id >= ICC_DYN_ID_START) { 929 + node->name = kasprintf(GFP_KERNEL, "%s@%s", name, 930 + dev_name(provider->dev)); 931 + if (!node->name) 932 + return -ENOMEM; 933 + } else { 934 + node->name = name; 935 + } 936 + 937 + return 0; 938 + } 939 + EXPORT_SYMBOL_GPL(icc_node_set_name); 915 940 916 941 /** 917 942 * icc_link_nodes() - create link between two nodes ··· 1065 1040 } 1066 1041 node->avg_bw = node->init_avg; 1067 1042 node->peak_bw = node->init_peak; 1068 - 1069 - if (node->id >= ICC_DYN_ID_START) 1070 - node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s", 1071 - node->name, dev_name(provider->dev)); 1072 1043 1073 1044 if (node->avg_bw || node->peak_bw) { 1074 1045 if (provider->pre_aggregate)
+6 -1
drivers/interconnect/qcom/icc-rpmh.c
··· 293 293 goto err_remove_nodes; 294 294 } 295 295 296 - node->name = qn->name; 296 + ret = icc_node_set_name(node, provider, qn->name); 297 + if (ret) { 298 + icc_node_destroy(node->id); 299 + goto err_remove_nodes; 300 + } 301 + 297 302 node->data = qn; 298 303 icc_node_add(node, provider); 299 304
+6 -1
drivers/interconnect/qcom/osm-l3.c
··· 236 236 goto err; 237 237 } 238 238 239 - node->name = qnodes[i]->name; 239 + ret = icc_node_set_name(node, provider, qnodes[i]->name); 240 + if (ret) { 241 + icc_node_destroy(node->id); 242 + goto err; 243 + } 244 + 240 245 /* Cast away const and add it back in qcom_osm_l3_set() */ 241 246 node->data = (void *)qnodes[i]; 242 247 icc_node_add(node, provider);
+7
include/linux/interconnect-provider.h
··· 119 119 struct icc_node *icc_node_create_dyn(void); 120 120 struct icc_node *icc_node_create(int id); 121 121 void icc_node_destroy(int id); 122 + int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider, const char *name); 122 123 int icc_link_nodes(struct icc_node *src_node, struct icc_node **dst_node); 123 124 int icc_link_create(struct icc_node *node, const int dst_id); 124 125 void icc_node_add(struct icc_node *node, struct icc_provider *provider); ··· 151 150 152 151 static inline void icc_node_destroy(int id) 153 152 { 153 + } 154 + 155 + static inline int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider, 156 + const char *name) 157 + { 158 + return -EOPNOTSUPP; 154 159 } 155 160 156 161 static inline int icc_link_nodes(struct icc_node *src_node, struct icc_node **dst_node)