[patch 24/38] [media] imon: add conditional locking in change_protocol
From: Greg KH
Date: Thu May 05 2011 - 20:13:37 EST
2.6.38-stable review patch. If anyone has any objections, please let us know.
------------------
From: Jarod Wilson <jarod@xxxxxxxxxx>
commit 23ef710e1a6c4d6b9ef1c2fa19410f7f1479401e upstream.
The imon_ir_change_protocol function gets called two different ways, one
way is from rc_register_device, for initial protocol selection/setup,
and the other is via a userspace-initiated protocol change request,
either by direct sysfs prodding or by something like ir-keytable.
In the rc_register_device case, the imon context lock is already held,
but when initiated from userspace, it is not, so we must acquire it,
prior to calling send_packet, which requires that the lock is held.
Without this change, there's an easily reproduceable deadlock when
another function calls send_packet (such as either of the display write
fops) after a userspace-initiated change_protocol.
With a lock-debugging-enabled kernel, I was getting this:
[ 15.014153] =====================================
[ 15.015048] [ BUG: bad unlock balance detected! ]
[ 15.015048] -------------------------------------
[ 15.015048] ir-keytable/773 is trying to release lock (&ictx->lock) at:
[ 15.015048] [<ffffffff814c6297>] mutex_unlock+0xe/0x10
[ 15.015048] but there are no more locks to release!
[ 15.015048]
[ 15.015048] other info that might help us debug this:
[ 15.015048] 2 locks held by ir-keytable/773:
[ 15.015048] #0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8119d400>] sysfs_write_file+0x3c/0x144
[ 15.015048] #1: (s_active#87){.+.+.+}, at: [<ffffffff8119d4ab>] sysfs_write_file+0xe7/0x144
[ 15.015048]
[ 15.015048] stack backtrace:
[ 15.015048] Pid: 773, comm: ir-keytable Not tainted 2.6.38.4-20.fc15.x86_64.debug #1
[ 15.015048] Call Trace:
[ 15.015048] [<ffffffff81089715>] ? print_unlock_inbalance_bug+0xca/0xd5
[ 15.015048] [<ffffffff8108b35c>] ? lock_release_non_nested+0xc1/0x263
[ 15.015048] [<ffffffff814c6297>] ? mutex_unlock+0xe/0x10
[ 15.015048] [<ffffffff814c6297>] ? mutex_unlock+0xe/0x10
[ 15.015048] [<ffffffff8108b67b>] ? lock_release+0x17d/0x1a4
[ 15.015048] [<ffffffff814c6229>] ? __mutex_unlock_slowpath+0xc5/0x125
[ 15.015048] [<ffffffff814c6297>] ? mutex_unlock+0xe/0x10
[ 15.015048] [<ffffffffa02964b6>] ? send_packet+0x1c9/0x264 [imon]
[ 15.015048] [<ffffffff8108b376>] ? lock_release_non_nested+0xdb/0x263
[ 15.015048] [<ffffffffa0296731>] ? imon_ir_change_protocol+0x126/0x15e [imon]
[ 15.015048] [<ffffffffa024a334>] ? store_protocols+0x1c3/0x286 [rc_core]
[ 15.015048] [<ffffffff81326e4e>] ? dev_attr_store+0x20/0x22
[ 15.015048] [<ffffffff8119d4cc>] ? sysfs_write_file+0x108/0x144
...
The original report that led to the investigation was the following:
[ 1679.457305] INFO: task LCDd:8460 blocked for more than 120 seconds.
[ 1679.457307] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1679.457309] LCDd D ffff88010fcd89c8 0 8460 1 0x00000000
[ 1679.457312] ffff8800d5a03b48 0000000000000082 0000000000000000 ffff8800d5a03fd8
[ 1679.457314] 00000000012dcd30 fffffffffffffffd ffff8800d5a03fd8 ffff88010fcd86f0
[ 1679.457316] ffff8800d5a03fd8 ffff8800d5a03fd8 ffff88010fcd89d0 ffff8800d5a03fd8
[ 1679.457319] Call Trace:
[ 1679.457324] [<ffffffff810ff1a5>] ? zone_statistics+0x75/0x90
[ 1679.457327] [<ffffffff810ea907>] ? get_page_from_freelist+0x3c7/0x820
[ 1679.457330] [<ffffffff813b0a49>] __mutex_lock_slowpath+0x139/0x320
[ 1679.457335] [<ffffffff813b0c41>] mutex_lock+0x11/0x30
[ 1679.457338] [<ffffffffa0d54216>] display_open+0x66/0x130 [imon]
[ 1679.457345] [<ffffffffa01d06c0>] usb_open+0x180/0x310 [usbcore]
[ 1679.457349] [<ffffffff81143b3b>] chrdev_open+0x1bb/0x2d0
[ 1679.457350] [<ffffffff8113d93d>] __dentry_open+0x10d/0x370
[ 1679.457352] [<ffffffff81143980>] ? chrdev_open+0x0/0x2d0
...
Bump the driver version here so its easier to tell if people have this
locking fix or not, and also make locking during probe easier to follow.
Reported-by: Benjamin Hodgetts <ben@xxxxxxxxx>
Signed-off-by: Jarod Wilson <jarod@xxxxxxxxxx>
Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>
---
drivers/media/rc/imon.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -46,7 +46,7 @@
#define MOD_AUTHOR "Jarod Wilson <jarod@xxxxxxxxxxxx>"
#define MOD_DESC "Driver for SoundGraph iMON MultiMedia IR/Display"
#define MOD_NAME "imon"
-#define MOD_VERSION "0.9.2"
+#define MOD_VERSION "0.9.3"
#define DISPLAY_MINOR_BASE 144
#define DEVICE_NAME "lcd%d"
@@ -451,8 +451,9 @@ static int display_close(struct inode *i
}
/**
- * Sends a packet to the device -- this function must be called
- * with ictx->lock held.
+ * Sends a packet to the device -- this function must be called with
+ * ictx->lock held, or its unlock/lock sequence while waiting for tx
+ * to complete can/will lead to a deadlock.
*/
static int send_packet(struct imon_context *ictx)
{
@@ -982,12 +983,21 @@ static void imon_touch_display_timeout(u
* the iMON remotes, and those used by the Windows MCE remotes (which is
* really just RC-6), but only one or the other at a time, as the signals
* are decoded onboard the receiver.
+ *
+ * This function gets called two different ways, one way is from
+ * rc_register_device, for initial protocol selection/setup, and the other is
+ * via a userspace-initiated protocol change request, either by direct sysfs
+ * prodding or by something like ir-keytable. In the rc_register_device case,
+ * the imon context lock is already held, but when initiated from userspace,
+ * it is not, so we must acquire it prior to calling send_packet, which
+ * requires that the lock is held.
*/
static int imon_ir_change_protocol(struct rc_dev *rc, u64 rc_type)
{
int retval;
struct imon_context *ictx = rc->priv;
struct device *dev = ictx->dev;
+ bool unlock = false;
unsigned char ir_proto_packet[] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x86 };
@@ -1020,6 +1030,11 @@ static int imon_ir_change_protocol(struc
memcpy(ictx->usb_tx_buf, &ir_proto_packet, sizeof(ir_proto_packet));
+ if (!mutex_is_locked(&ictx->lock)) {
+ unlock = true;
+ mutex_lock(&ictx->lock);
+ }
+
retval = send_packet(ictx);
if (retval)
goto out;
@@ -1028,6 +1043,9 @@ static int imon_ir_change_protocol(struc
ictx->pad_mouse = false;
out:
+ if (unlock)
+ mutex_unlock(&ictx->lock);
+
return retval;
}
@@ -2125,6 +2143,7 @@ static struct imon_context *imon_init_in
goto rdev_setup_failed;
}
+ mutex_unlock(&ictx->lock);
return ictx;
rdev_setup_failed:
@@ -2196,6 +2215,7 @@ static struct imon_context *imon_init_in
goto urb_submit_failed;
}
+ mutex_unlock(&ictx->lock);
return ictx;
urb_submit_failed:
@@ -2290,6 +2310,8 @@ static int __devinit imon_probe(struct u
usb_set_intfdata(interface, ictx);
if (ifnum == 0) {
+ mutex_lock(&ictx->lock);
+
if (product == 0xffdc && ictx->rf_device) {
sysfs_err = sysfs_create_group(&interface->dev.kobj,
&imon_rf_attr_group);
@@ -2300,13 +2322,14 @@ static int __devinit imon_probe(struct u
if (ictx->display_supported)
imon_init_display(ictx, interface);
+
+ mutex_unlock(&ictx->lock);
}
dev_info(dev, "iMON device (%04x:%04x, intf%d) on "
"usb<%d:%d> initialized\n", vendor, product, ifnum,
usbdev->bus->busnum, usbdev->devnum);
- mutex_unlock(&ictx->lock);
mutex_unlock(&driver_lock);
return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/