On Sat, Mar 06, 2010 at 08:52:24AM -0800, Dmitry Torokhov wrote:
> On Sat, Mar 06, 2010 at 11:27:01AM -0500, Alan Stern wrote:
> > On Sat, 6 Mar 2010, Oliver Neukum wrote:
> >
> > > Am Samstag, 6. März 2010 16:34:31 schrieb Alan Stern:
> > > > On Sat, 6 Mar 2010, Oliver Neukum wrote:
> > >
> > > > > The caller of usbfs_conn_disc_event() already holds usbfs_mutex.
> > > > > Don't take it again.
> > > >
> > > > That's not true. The caller holds usbfs_mutex only when the action is
> > > > USB_DEVICE_REMOVE. Not when the action is USB_DEVICE_ADD or USB_BUS_*.
> > >
> > > Right. That's what you get for wanting a quick fix. What do you
> > > propose?
> >
> > Guard conndiscevcnt and file->private_data with a separate static
> > mutex instead of usbfs_mutex.
> >
>
> If you think it is important use atomic here, however I do not think it
> is realistically needed. If we have 2 devices appearing and disappearing
> at the same time we may "skip" version but still anyone issuing select()
> before that will get woken up and will see both in /proc.
>
> > Incidentally, what's supposed to happen if more than one thread tries
> > to poll the same file descriptor simultaneously? Do you really want
> > the first thread to wait only until the first event while the second
> > thread is forced to wait for a second event?
>
> I think it is like with read and write - if userspace does not do proper
> locking while accessing the same fd it gets to hold both pieces. So
> again, no reason for using mutex there either.
>
> Also, if you use file->f_version you would not need kallocing that
> single integer and then having to free it. See how it is done in input.
>
OK, the patch below unwedges vmware-usb-arbitrator that tries to access
/proc/bus/input/devices (it doers what input core does now), however
khubd is still wedged with the following lockdep output:
[ 222.934310] usb 5-1: new full speed USB device using uhci_hcd and address 2
[ 223.099666] usb 5-1: New USB device found, idVendor=04b3, idProduct=3016
[ 223.099670] usb 5-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 223.099674] usb 5-1: Product: USB 1.1 2 port downstream low-power hub
[ 223.099676] usb 5-1: Manufacturer: Lite-On Tech
[ 223.103137] hub 5-1:1.0: USB hub found
[ 223.104336] hub 5-1:1.0: 4 ports detected
[ 223.383357] usb 5-1.3: new low speed USB device using uhci_hcd and address 3
[ 223.517352] usb 5-1.3: New USB device found, idVendor=04b3, idProduct=3018
[ 223.517356] usb 5-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 223.517360] usb 5-1.3: Product: IBM USB Keyboard with UltraNav
[ 223.517362] usb 5-1.3: Manufacturer: Lite-On Tech
[ 223.536770] input: Lite-On Tech IBM USB Keyboard with UltraNav as /devices/pci0000:00/0000:00:1d.0/usb5/5-1/5-1.3/5-1.3:1.0/input/input12
[ 223.540879] generic-usb 0003:04B3:3018.0001: input,hidraw0: USB HID v1.10 Keyboard [Lite-On Tech IBM USB Keyboard with UltraNav] on usb-0000:00:1d.0-1.3/input0
[ 223.569291] input: Lite-On Tech IBM USB Keyboard with UltraNav as /devices/pci0000:00/0000:00:1d.0/usb5/5-1/5-1.3/5-1.3:1.1/input/input13
[ 223.569535] generic-usb 0003:04B3:3018.0002: input,hidraw1: USB HID v1.10 Device [Lite-On Tech IBM USB Keyboard with UltraNav] on usb-0000:00:1d.0-1.3/input1
[ 223.750369] usb 5-1.4: new low speed USB device using uhci_hcd and address 4
[ 223.897343] usb 5-1.4: New USB device found, idVendor=06cb, idProduct=0009
[ 223.897347] usb 5-1.4: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 223.897350] usb 5-1.4: Product: Composite TouchPad / TrackPoint
[ 223.897353] usb 5-1.4: Manufacturer: Synaptics Inc.
[ 223.919655] input: Synaptics Inc. Composite TouchPad / TrackPoint as /devices/pci0000:00/0000:00:1d.0/usb5/5-1/5-1.4/5-1.4:1.0/input/input14
[ 223.919836] generic-usb 0003:06CB:0009.0003: input,hidraw2: USB HID v1.00 Mouse [Synaptics Inc. Composite TouchPad / TrackPoint] on usb-0000:00:1d.0-1.4/input0
[ 223.940807] input: Synaptics Inc. Composite TouchPad / TrackPoint as /devices/pci0000:00/0000:00:1d.0/usb5/5-1/5-1.4/5-1.4:1.1/input/input15
[ 223.941089] generic-usb 0003:06CB:0009.0004: input,hidraw3: USB HID v1.00 Mouse [Synaptics Inc. Composite TouchPad / TrackPoint] on usb-0000:00:1d.0-1.4/input1
[ 248.704305] usb 5-1: USB disconnect, address 2
[ 248.704317] usb 5-1.3: USB disconnect, address 3
[ 248.727933]
[ 248.727935] =======================================================
[ 248.727939] [ INFO: possible circular locking dependency detected ]
[ 248.727942] 2.6.33 #233
[ 248.727943] -------------------------------------------------------
[ 248.727946] khubd/27 is trying to acquire lock:
[ 248.727948] ((usb_notifier_list).rwsem){.+.+.+}, at: [<ffffffff8106f480>] __blocking_notifier_call_chain+0x40/0x80
[ 248.727959]
[ 248.727959] but task is already holding lock:
[ 248.727961] (usbfs_mutex){+.+...}, at: [<ffffffff8136e83a>] usb_notify_remove_device+0x1a/0x50
[ 248.727969]
[ 248.727970] which lock already depends on the new lock.
[ 248.727971]
[ 248.727973]
[ 248.727974] the existing dependency chain (in reverse order) is:
[ 248.727977]
[ 248.727977] -> #1 (usbfs_mutex){+.+...}:
[ 248.727982] [<ffffffff8107f222>] check_prev_add+0x242/0x570
[ 248.727988] [<ffffffff8107fba4>] validate_chain+0x654/0x750
[ 248.727992] [<ffffffff8107ffc7>] __lock_acquire+0x327/0x4c0
[ 248.727996] [<ffffffff810801f9>] lock_acquire+0x99/0x140
[ 248.728001] [<ffffffff81478375>] __mutex_lock_common+0x65/0x4a0
[ 248.728006] [<ffffffff8147888e>] mutex_lock_nested+0x3e/0x50
[ 248.728008] [<ffffffff8136f9b2>] usbfs_conn_disc_event+0x12/0x40
[ 248.728008] [<ffffffff813710a4>] usbfs_notify+0xa4/0x240
[ 248.728008] [<ffffffff8147df47>] notifier_call_chain+0x47/0x90
[ 248.728008] [<ffffffff8106f495>] __blocking_notifier_call_chain+0x55/0x80
[ 248.728008] [<ffffffff8106f4d1>] blocking_notifier_call_chain+0x11/0x20
[ 248.728008] [<ffffffff8136e7f8>] usb_notify_add_bus+0x18/0x20
[ 248.728008] [<ffffffff8135f089>] usb_register_bus+0x69/0xd0
[ 248.728008] [<ffffffff81360fd8>] usb_add_hcd+0xa8/0x420
[ 248.728008] [<ffffffff8136fcfd>] usb_hcd_pci_probe+0x15d/0x310
[ 248.728008] [<ffffffff8124d1b2>] local_pci_probe+0x12/0x20
[ 248.728008] [<ffffffff8124e9e0>] pci_device_probe+0x80/0xb0
[ 248.728008] [<ffffffff81303bd7>] really_probe+0x67/0x170
[ 248.728008] [<ffffffff81303d20>] driver_probe_device+0x40/0x70
[ 248.728008] [<ffffffff81303deb>] __driver_attach+0x9b/0xa0
[ 248.728008] [<ffffffff81303054>] bus_for_each_dev+0x64/0x90
[ 248.728008] [<ffffffff81303a59>] driver_attach+0x19/0x20
[ 248.728008] [<ffffffff8130331c>] bus_add_driver+0xdc/0x280
[ 248.728008] [<ffffffff813040ff>] driver_register+0x6f/0x130
[ 248.728008] [<ffffffff8124e351>] __pci_register_driver+0x61/0xe0
[ 248.728008] [<ffffffff818bb25e>] ehci_hcd_init+0x68/0x76
[ 248.728008] [<ffffffff810001d7>] do_one_initcall+0x37/0x190
[ 248.728008] [<ffffffff8189274e>] do_basic_setup+0x54/0x66
[ 248.728008] [<ffffffff818927f5>] kernel_init+0x95/0x10d
[ 248.728008] [<ffffffff81003dd4>] kernel_thread_helper+0x4/0x10
[ 248.728008]
[ 248.728008] -> #0 ((usb_notifier_list).rwsem){.+.+.+}:
[ 248.728008] [<ffffffff8107f51e>] check_prev_add+0x53e/0x570
[ 248.728008] [<ffffffff8107fba4>] validate_chain+0x654/0x750
[ 248.728008] [<ffffffff8107ffc7>] __lock_acquire+0x327/0x4c0
[ 248.728008] [<ffffffff810801f9>] lock_acquire+0x99/0x140
[ 248.728008] [<ffffffff81478be2>] down_read+0x42/0x60
[ 248.728008] [<ffffffff8106f480>] __blocking_notifier_call_chain+0x40/0x80
[ 248.728008] [<ffffffff8106f4d1>] blocking_notifier_call_chain+0x11/0x20
[ 248.728008] [<ffffffff8136e84e>] usb_notify_remove_device+0x2e/0x50
[ 248.728008] [<ffffffff8136e931>] generic_disconnect+0x11/0x30
[ 248.728008] [<ffffffff81365e49>] usb_unbind_device+0x29/0x50
[ 248.728008] [<ffffffff813038c0>] __device_release_driver+0x70/0xe0
[ 248.728008] [<ffffffff81303a28>] device_release_driver+0x28/0x40
[ 248.728008] [<ffffffff81302a8c>] bus_remove_device+0xac/0xe0
[ 248.728008] [<ffffffff81300df7>] device_del+0x127/0x1a0
[ 248.728008] [<ffffffff8135bfbc>] usb_disconnect+0xec/0x160
[ 248.728008] [<ffffffff8135bf87>] usb_disconnect+0xb7/0x160
[ 248.728008] [<ffffffff8135c77e>] hub_port_connect_change+0x8e/0x9f0
[ 248.728008] [<ffffffff8135e2f3>] hub_events+0x3c3/0x580
[ 248.728008] [<ffffffff8135e505>] hub_thread+0x55/0x190
[ 248.728008] [<ffffffff8106908e>] kthread+0x8e/0xa0
[ 248.728008] [<ffffffff81003dd4>] kernel_thread_helper+0x4/0x10
[ 248.728008]
[ 248.728008] other info that might help us debug this:
[ 248.728008]
[ 248.728008] 1 lock held by khubd/27:
[ 248.728216] #0: (usbfs_mutex){+.+...}, at: [<ffffffff8136e83a>] usb_notify_remove_device+0x1a/0x50
[ 248.728218]
[ 248.728218] stack backtrace:
[ 248.728218] Pid: 27, comm: khubd Tainted: P 2.6.33 #233
[ 248.728218] Call Trace:
[ 248.728218] [<ffffffff8107db69>] print_circular_bug+0xe9/0xf0
[ 248.728218] [<ffffffff8107f51e>] check_prev_add+0x53e/0x570
[ 248.728218] [<ffffffff8107ffc7>] ? __lock_acquire+0x327/0x4c0
[ 248.728218] [<ffffffff8107fba4>] validate_chain+0x654/0x750
[ 248.728218] [<ffffffff8107ffc7>] ? __lock_acquire+0x327/0x4c0
[ 248.728218] [<ffffffff8107ffc7>] __lock_acquire+0x327/0x4c0
[ 248.728218] [<ffffffff810801f9>] lock_acquire+0x99/0x140
[ 248.728218] [<ffffffff8106f480>] ? __blocking_notifier_call_chain+0x40/0x80
[ 248.728218] [<ffffffff81478680>] ? __mutex_lock_common+0x370/0x4a0
[ 248.728218] [<ffffffff8136e83a>] ? usb_notify_remove_device+0x1a/0x50
[ 248.728218] [<ffffffff81478be2>] down_read+0x42/0x60
[ 248.728218] [<ffffffff8106f480>] ? __blocking_notifier_call_chain+0x40/0x80
[ 248.728218] [<ffffffff8106f480>] __blocking_notifier_call_chain+0x40/0x80
[ 248.728218] [<ffffffff8106f4d1>] blocking_notifier_call_chain+0x11/0x20
[ 248.728218] [<ffffffff8136e84e>] usb_notify_remove_device+0x2e/0x50
[ 248.728218] [<ffffffff8136e931>] generic_disconnect+0x11/0x30
[ 248.728218] [<ffffffff81365e49>] usb_unbind_device+0x29/0x50
[ 248.728218] [<ffffffff813038c0>] __device_release_driver+0x70/0xe0
[ 248.728218] [<ffffffff81303a28>] device_release_driver+0x28/0x40
[ 248.728218] [<ffffffff81302a8c>] bus_remove_device+0xac/0xe0
[ 248.728218] [<ffffffff81300df7>] device_del+0x127/0x1a0
[ 248.728218] [<ffffffff8135bfbc>] usb_disconnect+0xec/0x160
[ 248.728218] [<ffffffff8135bf87>] usb_disconnect+0xb7/0x160
[ 248.728218] [<ffffffff8135c77e>] hub_port_connect_change+0x8e/0x9f0
[ 248.728218] [<ffffffff81478309>] ? mutex_unlock+0x9/0x10
[ 248.728218] [<ffffffff8135e2f3>] hub_events+0x3c3/0x580
[ 248.728218] [<ffffffff8147a2ed>] ? _raw_spin_unlock_irqrestore+0x5d/0x70
[ 248.728218] [<ffffffff8135e505>] hub_thread+0x55/0x190
[ 248.728218] [<ffffffff810695a0>] ? autoremove_wake_function+0x0/0x40
[ 248.728218] [<ffffffff8135e4b0>] ? hub_thread+0x0/0x190
[ 248.728218] [<ffffffff8106908e>] kthread+0x8e/0xa0
[ 248.728218] [<ffffffff81003dd4>] kernel_thread_helper+0x4/0x10
[ 248.728218] [<ffffffff8147aa80>] ? restore_args+0x0/0x30
[ 248.728218] [<ffffffff81069000>] ? kthread+0x0/0xa0
[ 248.728218] [<ffffffff81003dd0>] ? kernel_thread_helper+0x0/0x10
[ 480.551213] INFO: task khubd:27 blocked for more than 120 seconds.
[ 480.551223] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 480.551231] khubd D 0000000000000202 0 27 2 0x00000000
[ 480.551247] ffff88011ba29960 0000000000000046 ffff88000260eec0 ffff88011ba20000
[ 480.551290] ffff88011ba28000 ffff88011ba29fd8 ffff88011ba29fd8 ffff88011ba20000
[ 480.551306] ffff88011ba29fd8 0000000000013d80 0000000000013d80 0000000000013d80
[ 480.551328] Call Trace:
[ 480.551349] [<ffffffff814784b1>] __mutex_lock_common+0x1a1/0x4a0
[ 480.551365] [<ffffffff8136f9b2>] ? usbfs_conn_disc_event+0x12/0x40
[ 480.551380] [<ffffffff8107eb7d>] ? trace_hardirqs_on+0xd/0x10
[ 480.551394] [<ffffffff8136f9b2>] ? usbfs_conn_disc_event+0x12/0x40
[ 480.551410] [<ffffffff8147888e>] mutex_lock_nested+0x3e/0x50
[ 480.551421] [<ffffffff8136f9b2>] usbfs_conn_disc_event+0x12/0x40
[ 480.551434] [<ffffffff813710a4>] usbfs_notify+0xa4/0x240
[ 480.551447] [<ffffffff8147df47>] notifier_call_chain+0x47/0x90
[ 480.551460] [<ffffffff8106f495>] __blocking_notifier_call_chain+0x55/0x80
[ 480.551473] [<ffffffff8106f4d1>] blocking_notifier_call_chain+0x11/0x20
[ 480.551484] [<ffffffff8136e84e>] usb_notify_remove_device+0x2e/0x50
[ 480.551495] [<ffffffff8136e931>] generic_disconnect+0x11/0x30
[ 480.551509] [<ffffffff81365e49>] usb_unbind_device+0x29/0x50
[ 480.551523] [<ffffffff813038c0>] __device_release_driver+0x70/0xe0
[ 480.551536] [<ffffffff81303a28>] device_release_driver+0x28/0x40
[ 480.551550] [<ffffffff81302a8c>] bus_remove_device+0xac/0xe0
[ 480.551562] [<ffffffff81300df7>] device_del+0x127/0x1a0
[ 480.551574] [<ffffffff8135bfbc>] usb_disconnect+0xec/0x160
[ 480.551586] [<ffffffff8135bf87>] usb_disconnect+0xb7/0x160
[ 480.551598] [<ffffffff8135c77e>] hub_port_connect_change+0x8e/0x9f0
[ 480.551610] [<ffffffff81478309>] ? mutex_unlock+0x9/0x10
[ 480.551625] [<ffffffff8135e2f3>] hub_events+0x3c3/0x580
[ 480.551638] [<ffffffff8147a2ed>] ? _raw_spin_unlock_irqrestore+0x5d/0x70
[ 480.551651] [<ffffffff8135e505>] hub_thread+0x55/0x190
[ 480.551666] [<ffffffff810695a0>] ? autoremove_wake_function+0x0/0x40
[ 480.551678] [<ffffffff8135e4b0>] ? hub_thread+0x0/0x190
[ 480.551689] [<ffffffff8106908e>] kthread+0x8e/0xa0
[ 480.551707] [<ffffffff81003dd4>] kernel_thread_helper+0x4/0x10
[ 480.551721] [<ffffffff8147aa80>] ? restore_args+0x0/0x30
[ 480.551735] [<ffffffff81069000>] ? kthread+0x0/0xa0
[ 480.551746] [<ffffffff81003dd0>] ? kernel_thread_helper+0x0/0x10
--
Dmitry
USB: simplify /proc/bus/usb/devices handling
From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
There is no need to allocate a single integer for usbfs state counter
in file structure, just use f_version. Also, there is not need for
locking, if program decides to poll from 2 threads on the same descriptor
it gets to hold both pieces, same as with read and write.
Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
---
drivers/usb/core/devices.c | 41 ++++-------------------------------------
1 files changed, 4 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index c83c975..b776bef 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -628,46 +628,15 @@ static ssize_t usb_device_read(struct file *file, char __user *buf,
return total_written;
}
-/* Kernel lock for "lastev" protection */
static unsigned int usb_device_poll(struct file *file,
struct poll_table_struct *wait)
{
- struct usb_device_status *st;
- unsigned int mask = 0;
-
- mutex_lock(&usbfs_mutex);
- st = file->private_data;
- if (!st) {
- st = kmalloc(sizeof(struct usb_device_status), GFP_KERNEL);
- if (!st) {
- mutex_unlock(&usbfs_mutex);
- return POLLIN;
- }
-
- st->lastev = conndiscevcnt;
- file->private_data = st;
- mask = POLLIN;
+ poll_wait(file, &deviceconndiscwq, wait);
+ if (file->f_version != conndiscevcnt) {
+ file->f_version = conndiscevcnt;
+ return POLLIN | POLLRDNORM;
}
- if (file->f_mode & FMODE_READ)
- poll_wait(file, &deviceconndiscwq, wait);
- if (st->lastev != conndiscevcnt)
- mask |= POLLIN;
- st->lastev = conndiscevcnt;
- mutex_unlock(&usbfs_mutex);
- return mask;
-}
-
-static int usb_device_open(struct inode *inode, struct file *file)
-{
- file->private_data = NULL;
- return 0;
-}
-
-static int usb_device_release(struct inode *inode, struct file *file)
-{
- kfree(file->private_data);
- file->private_data = NULL;
return 0;
}
@@ -699,6 +668,4 @@ const struct file_operations usbfs_devices_fops = {
.llseek = usb_device_lseek,
.read = usb_device_read,
.poll = usb_device_poll,
- .open = usb_device_open,
- .release = usb_device_release,
};
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
