Skip to content

Commit c1f70b0

Browse files
rafaeljwgregtwallace
rafaeljw
authored and
gregtwallace
committed
PM / QoS: Avoid possible deadlock related to sysfs access
Commit b81ea1b (PM / QoS: Fix concurrency issues and memory leaks in device PM QoS) put calls to pm_qos_sysfs_add_latency(), pm_qos_sysfs_add_flags(), pm_qos_sysfs_remove_latency(), and pm_qos_sysfs_remove_flags() under dev_pm_qos_mtx, which was a mistake, because it may lead to deadlocks in some situations. For example, if pm_qos_remote_wakeup_store() is run in parallel with dev_pm_qos_constraints_destroy(), they may deadlock in the following way: ====================================================== [ INFO: possible circular locking dependency detected ] 3.9.0-rc4-next-20130328-sasha-00014-g91a3267 #319 Tainted: G W ------------------------------------------------------- trinity-child6/12371 is trying to acquire lock: (s_active#54){++++.+}, at: [<ffffffff81301631>] sysfs_addrm_finish+0x31/0x60 but task is already holding lock: (dev_pm_qos_mtx){+.+.+.}, at: [<ffffffff81f07cc3>] dev_pm_qos_constraints_destroy+0x23/0x250 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> lg-devs#1 (dev_pm_qos_mtx){+.+.+.}: [<ffffffff811811da>] lock_acquire+0x1aa/0x240 [<ffffffff83dab809>] __mutex_lock_common+0x59/0x5e0 [<ffffffff83dabebf>] mutex_lock_nested+0x3f/0x50 [<ffffffff81f07f2f>] dev_pm_qos_update_flags+0x3f/0xc0 [<ffffffff81f05f4f>] pm_qos_remote_wakeup_store+0x3f/0x70 [<ffffffff81efbb43>] dev_attr_store+0x13/0x20 [<ffffffff812ffdaa>] sysfs_write_file+0xfa/0x150 [<ffffffff8127f2c1>] __kernel_write+0x81/0x150 [<ffffffff812afc2d>] write_pipe_buf+0x4d/0x80 [<ffffffff812af57c>] splice_from_pipe_feed+0x7c/0x120 [<ffffffff812afa25>] __splice_from_pipe+0x45/0x80 [<ffffffff812b14fc>] splice_from_pipe+0x4c/0x70 [<ffffffff812b1538>] default_file_splice_write+0x18/0x30 [<ffffffff812afae3>] do_splice_from+0x83/0xb0 [<ffffffff812afb2e>] direct_splice_actor+0x1e/0x20 [<ffffffff812b0277>] splice_direct_to_actor+0xe7/0x200 [<ffffffff812b15bc>] do_splice_direct+0x4c/0x70 [<ffffffff8127eda9>] do_sendfile+0x169/0x300 [<ffffffff8127ff94>] SyS_sendfile64+0x64/0xb0 [<ffffffff83db7d18>] tracesys+0xe1/0xe6 -> #0 (s_active#54){++++.+}: [<ffffffff811800cf>] __lock_acquire+0x15bf/0x1e50 [<ffffffff811811da>] lock_acquire+0x1aa/0x240 [<ffffffff81300aa2>] sysfs_deactivate+0x122/0x1a0 [<ffffffff81301631>] sysfs_addrm_finish+0x31/0x60 [<ffffffff812ff77f>] sysfs_hash_and_remove+0x7f/0xb0 [<ffffffff813035a1>] sysfs_unmerge_group+0x51/0x70 [<ffffffff81f068f4>] pm_qos_sysfs_remove_flags+0x14/0x20 [<ffffffff81f07490>] __dev_pm_qos_hide_flags+0x30/0x70 [<ffffffff81f07cd5>] dev_pm_qos_constraints_destroy+0x35/0x250 [<ffffffff81f06931>] dpm_sysfs_remove+0x11/0x50 [<ffffffff81efcf6f>] device_del+0x3f/0x1b0 [<ffffffff81efd128>] device_unregister+0x48/0x60 [<ffffffff82d4083c>] usb_hub_remove_port_device+0x1c/0x20 [<ffffffff82d2a9cd>] hub_disconnect+0xdd/0x160 [<ffffffff82d36ab7>] usb_unbind_interface+0x67/0x170 [<ffffffff81f001a7>] __device_release_driver+0x87/0xe0 [<ffffffff81f00559>] device_release_driver+0x29/0x40 [<ffffffff81effc58>] bus_remove_device+0x148/0x160 [<ffffffff81efd07f>] device_del+0x14f/0x1b0 [<ffffffff82d344f9>] usb_disable_device+0xf9/0x280 [<ffffffff82d34ff8>] usb_set_configuration+0x268/0x840 [<ffffffff82d3a7fc>] usb_remove_store+0x4c/0x80 [<ffffffff81efbb43>] dev_attr_store+0x13/0x20 [<ffffffff812ffdaa>] sysfs_write_file+0xfa/0x150 [<ffffffff8127f71d>] do_loop_readv_writev+0x4d/0x90 [<ffffffff8127f999>] do_readv_writev+0xf9/0x1e0 [<ffffffff8127faba>] vfs_writev+0x3a/0x60 [<ffffffff8127fc60>] SyS_writev+0x50/0xd0 [<ffffffff83db7d18>] tracesys+0xe1/0xe6 other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(dev_pm_qos_mtx); lock(s_active#54); lock(dev_pm_qos_mtx); lock(s_active#54); *** DEADLOCK *** To avoid that, remove the calls to functions mentioned above from under dev_pm_qos_mtx and introduce a separate lock to prevent races between functions that add or remove device PM QoS sysfs attributes from happening. Reported-by: Sasha Levin <[email protected]> Signed-off-by: Rafael J. Wysocki <[email protected]>
1 parent 803922f commit c1f70b0

File tree

1 file changed

+47
-13
lines changed

1 file changed

+47
-13
lines changed

‎drivers/base/power/qos.c‎

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
#include "power.h"
4747

4848
static DEFINE_MUTEX(dev_pm_qos_mtx);
49+
static DEFINE_MUTEX(dev_pm_qos_sysfs_mtx);
4950

5051
static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
5152

@@ -216,12 +217,17 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
216217
struct pm_qos_constraints *c;
217218
struct pm_qos_flags *f;
218219

219-
mutex_lock(&dev_pm_qos_mtx);
220+
mutex_lock(&dev_pm_qos_sysfs_mtx);
220221

221222
/*
222223
* If the device's PM QoS resume latency limit or PM QoS flags have been
223224
* exposed to user space, they have to be hidden at this point.
224225
*/
226+
pm_qos_sysfs_remove_latency(dev);
227+
pm_qos_sysfs_remove_flags(dev);
228+
229+
mutex_lock(&dev_pm_qos_mtx);
230+
225231
__dev_pm_qos_hide_latency_limit(dev);
226232
__dev_pm_qos_hide_flags(dev);
227233

@@ -254,6 +260,8 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
254260

255261
out:
256262
mutex_unlock(&dev_pm_qos_mtx);
263+
264+
mutex_unlock(&dev_pm_qos_sysfs_mtx);
257265
}
258266

259267
/**
@@ -558,6 +566,14 @@ static void __dev_pm_qos_drop_user_request(struct device *dev,
558566
kfree(req);
559567
}
560568

569+
static void dev_pm_qos_drop_user_request(struct device *dev,
570+
enum dev_pm_qos_req_type type)
571+
{
572+
mutex_lock(&dev_pm_qos_mtx);
573+
__dev_pm_qos_drop_user_request(dev, type);
574+
mutex_unlock(&dev_pm_qos_mtx);
575+
}
576+
561577
/**
562578
* dev_pm_qos_expose_latency_limit - Expose PM QoS latency limit to user space.
563579
* @dev: Device whose PM QoS latency limit is to be exposed to user space.
@@ -581,6 +597,8 @@ int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value)
581597
return ret;
582598
}
583599

600+
mutex_lock(&dev_pm_qos_sysfs_mtx);
601+
584602
mutex_lock(&dev_pm_qos_mtx);
585603

586604
if (IS_ERR_OR_NULL(dev->power.qos))
@@ -591,26 +609,27 @@ int dev_pm_qos_expose_latency_limit(struct device *dev, s32 value)
591609
if (ret < 0) {
592610
__dev_pm_qos_remove_request(req);
593611
kfree(req);
612+
mutex_unlock(&dev_pm_qos_mtx);
594613
goto out;
595614
}
596-
597615
dev->power.qos->latency_req = req;
616+
617+
mutex_unlock(&dev_pm_qos_mtx);
618+
598619
ret = pm_qos_sysfs_add_latency(dev);
599620
if (ret)
600-
__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
621+
dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
601622

602623
out:
603-
mutex_unlock(&dev_pm_qos_mtx);
624+
mutex_unlock(&dev_pm_qos_sysfs_mtx);
604625
return ret;
605626
}
606627
EXPORT_SYMBOL_GPL(dev_pm_qos_expose_latency_limit);
607628

608629
static void __dev_pm_qos_hide_latency_limit(struct device *dev)
609630
{
610-
if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->latency_req) {
611-
pm_qos_sysfs_remove_latency(dev);
631+
if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->latency_req)
612632
__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY);
613-
}
614633
}
615634

616635
/**
@@ -619,9 +638,15 @@ static void __dev_pm_qos_hide_latency_limit(struct device *dev)
619638
*/
620639
void dev_pm_qos_hide_latency_limit(struct device *dev)
621640
{
641+
mutex_lock(&dev_pm_qos_sysfs_mtx);
642+
643+
pm_qos_sysfs_remove_latency(dev);
644+
622645
mutex_lock(&dev_pm_qos_mtx);
623646
__dev_pm_qos_hide_latency_limit(dev);
624647
mutex_unlock(&dev_pm_qos_mtx);
648+
649+
mutex_unlock(&dev_pm_qos_sysfs_mtx);
625650
}
626651
EXPORT_SYMBOL_GPL(dev_pm_qos_hide_latency_limit);
627652

@@ -649,6 +674,8 @@ int dev_pm_qos_expose_flags(struct device *dev, s32 val)
649674
}
650675

651676
pm_runtime_get_sync(dev);
677+
mutex_lock(&dev_pm_qos_sysfs_mtx);
678+
652679
mutex_lock(&dev_pm_qos_mtx);
653680

654681
if (IS_ERR_OR_NULL(dev->power.qos))
@@ -659,27 +686,28 @@ int dev_pm_qos_expose_flags(struct device *dev, s32 val)
659686
if (ret < 0) {
660687
__dev_pm_qos_remove_request(req);
661688
kfree(req);
689+
mutex_unlock(&dev_pm_qos_mtx);
662690
goto out;
663691
}
664-
665692
dev->power.qos->flags_req = req;
693+
694+
mutex_unlock(&dev_pm_qos_mtx);
695+
666696
ret = pm_qos_sysfs_add_flags(dev);
667697
if (ret)
668-
__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
698+
dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
669699

670700
out:
671-
mutex_unlock(&dev_pm_qos_mtx);
701+
mutex_unlock(&dev_pm_qos_sysfs_mtx);
672702
pm_runtime_put(dev);
673703
return ret;
674704
}
675705
EXPORT_SYMBOL_GPL(dev_pm_qos_expose_flags);
676706

677707
static void __dev_pm_qos_hide_flags(struct device *dev)
678708
{
679-
if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->flags_req) {
680-
pm_qos_sysfs_remove_flags(dev);
709+
if (!IS_ERR_OR_NULL(dev->power.qos) && dev->power.qos->flags_req)
681710
__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_FLAGS);
682-
}
683711
}
684712

685713
/**
@@ -689,9 +717,15 @@ static void __dev_pm_qos_hide_flags(struct device *dev)
689717
void dev_pm_qos_hide_flags(struct device *dev)
690718
{
691719
pm_runtime_get_sync(dev);
720+
mutex_lock(&dev_pm_qos_sysfs_mtx);
721+
722+
pm_qos_sysfs_remove_flags(dev);
723+
692724
mutex_lock(&dev_pm_qos_mtx);
693725
__dev_pm_qos_hide_flags(dev);
694726
mutex_unlock(&dev_pm_qos_mtx);
727+
728+
mutex_unlock(&dev_pm_qos_sysfs_mtx);
695729
pm_runtime_put(dev);
696730
}
697731
EXPORT_SYMBOL_GPL(dev_pm_qos_hide_flags);

0 commit comments

Comments
 (0)