Skip to content

Commit c5b6059

Browse files
juhosgGeorgi Djakov
authored andcommitted
interconnect: avoid memory allocation when 'icc_bw_lock' is held
The 'icc_bw_lock' mutex is introduced in commit af42269 ("interconnect: Fix locking for runpm vs reclaim") in order to decouple serialization of bw aggregation from codepaths that require memory allocation. However commit d30f83d ("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: d30f83d ("interconnect: core: Add dynamic id allocation support") Signed-off-by: Gabor Juhos <[email protected]> Link: https://lore.kernel.org/r/[email protected] Co-developed-by: Johan Hovold <[email protected]> Signed-off-by: Johan Hovold <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Georgi Djakov <[email protected]>
1 parent 618c810 commit c5b6059

File tree

4 files changed

+44
-6
lines changed

4 files changed

+44
-6
lines changed

drivers/interconnect/core.c

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -909,10 +909,35 @@ void icc_node_destroy(int id)
909909
return;
910910

911911
kfree(node->links);
912+
if (node->id >= ICC_DYN_ID_START)
913+
kfree(node->name);
912914
kfree(node);
913915
}
914916
EXPORT_SYMBOL_GPL(icc_node_destroy);
915917

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);
940+
916941
/**
917942
* icc_link_nodes() - create link between two nodes
918943
* @src_node: source node
@@ -1041,10 +1066,6 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
10411066
node->avg_bw = node->init_avg;
10421067
node->peak_bw = node->init_peak;
10431068

1044-
if (node->id >= ICC_DYN_ID_START)
1045-
node->name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
1046-
node->name, dev_name(provider->dev));
1047-
10481069
if (node->avg_bw || node->peak_bw) {
10491070
if (provider->pre_aggregate)
10501071
provider->pre_aggregate(node);

drivers/interconnect/qcom/icc-rpmh.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,12 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
293293
goto err_remove_nodes;
294294
}
295295

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+
297302
node->data = qn;
298303
icc_node_add(node, provider);
299304

drivers/interconnect/qcom/osm-l3.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,12 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
236236
goto err;
237237
}
238238

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+
240245
/* Cast away const and add it back in qcom_osm_l3_set() */
241246
node->data = (void *)qnodes[i];
242247
icc_node_add(node, provider);

include/linux/interconnect-provider.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
119119
struct icc_node *icc_node_create_dyn(void);
120120
struct icc_node *icc_node_create(int id);
121121
void icc_node_destroy(int id);
122+
int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider, const char *name);
122123
int icc_link_nodes(struct icc_node *src_node, struct icc_node **dst_node);
123124
int icc_link_create(struct icc_node *node, const int dst_id);
124125
void icc_node_add(struct icc_node *node, struct icc_provider *provider);
@@ -152,6 +153,12 @@ static inline void icc_node_destroy(int id)
152153
{
153154
}
154155

156+
static inline int icc_node_set_name(struct icc_node *node, const struct icc_provider *provider,
157+
const char *name)
158+
{
159+
return -EOPNOTSUPP;
160+
}
161+
155162
static inline int icc_link_nodes(struct icc_node *src_node, struct icc_node **dst_node)
156163
{
157164
return -EOPNOTSUPP;

0 commit comments

Comments
 (0)