HID: hiddev: use usb_find_interface, get rid of BKL
This removes the private hiddev_table in the usbhid driver and changes it to use usb_find_interface instead. The advantage is that we can avoid the race between usb_register_dev and usb_open and no longer need the big kernel lock. This doesn't introduce race condition -- the intf pointer could be invalidated only in hiddev_disconnect() through usb_deregister_dev(), but that will block on minor_rwsem and not actually remove the device until usb_open(). Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Jiri Kosina <jkosina@suse.cz> Cc: "Greg Kroah-Hartman" <gregkh@suse.de> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
This commit is contained in:
parent
1c5474a65b
commit
bd25f4dd69
|
@ -67,7 +67,7 @@ struct hiddev_list {
|
|||
struct mutex thread_lock;
|
||||
};
|
||||
|
||||
static struct hiddev *hiddev_table[HIDDEV_MINORS];
|
||||
static struct usb_driver hiddev_driver;
|
||||
|
||||
/*
|
||||
* Find a report, given the report's type and ID. The ID can be specified
|
||||
|
@ -265,22 +265,19 @@ static int hiddev_release(struct inode * inode, struct file * file)
|
|||
static int hiddev_open(struct inode *inode, struct file *file)
|
||||
{
|
||||
struct hiddev_list *list;
|
||||
int res, i;
|
||||
struct usb_interface *intf;
|
||||
struct hiddev *hiddev;
|
||||
int res;
|
||||
|
||||
/* See comment in hiddev_connect() for BKL explanation */
|
||||
lock_kernel();
|
||||
i = iminor(inode) - HIDDEV_MINOR_BASE;
|
||||
|
||||
if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
|
||||
intf = usb_find_interface(&hiddev_driver, iminor(inode));
|
||||
if (!intf)
|
||||
return -ENODEV;
|
||||
hiddev = usb_get_intfdata(intf);
|
||||
|
||||
if (!(list = kzalloc(sizeof(struct hiddev_list), GFP_KERNEL)))
|
||||
return -ENOMEM;
|
||||
mutex_init(&list->thread_lock);
|
||||
|
||||
list->hiddev = hiddev_table[i];
|
||||
|
||||
|
||||
list->hiddev = hiddev;
|
||||
file->private_data = list;
|
||||
|
||||
/*
|
||||
|
@ -289,7 +286,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
|
|||
*/
|
||||
if (list->hiddev->exist) {
|
||||
if (!list->hiddev->open++) {
|
||||
res = usbhid_open(hiddev_table[i]->hid);
|
||||
res = usbhid_open(hiddev->hid);
|
||||
if (res < 0) {
|
||||
res = -EIO;
|
||||
goto bail;
|
||||
|
@ -301,12 +298,12 @@ static int hiddev_open(struct inode *inode, struct file *file)
|
|||
}
|
||||
|
||||
spin_lock_irq(&list->hiddev->list_lock);
|
||||
list_add_tail(&list->node, &hiddev_table[i]->list);
|
||||
list_add_tail(&list->node, &hiddev->list);
|
||||
spin_unlock_irq(&list->hiddev->list_lock);
|
||||
|
||||
if (!list->hiddev->open++)
|
||||
if (list->hiddev->exist) {
|
||||
struct hid_device *hid = hiddev_table[i]->hid;
|
||||
struct hid_device *hid = hiddev->hid;
|
||||
res = usbhid_get_power(hid);
|
||||
if (res < 0) {
|
||||
res = -EIO;
|
||||
|
@ -314,13 +311,10 @@ static int hiddev_open(struct inode *inode, struct file *file)
|
|||
}
|
||||
usbhid_open(hid);
|
||||
}
|
||||
|
||||
unlock_kernel();
|
||||
return 0;
|
||||
bail:
|
||||
file->private_data = NULL;
|
||||
kfree(list);
|
||||
unlock_kernel();
|
||||
return res;
|
||||
}
|
||||
|
||||
|
@ -894,37 +888,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
|
|||
hid->hiddev = hiddev;
|
||||
hiddev->hid = hid;
|
||||
hiddev->exist = 1;
|
||||
|
||||
/*
|
||||
* BKL here is used to avoid race after usb_register_dev().
|
||||
* Once the device node has been created, open() could happen on it.
|
||||
* The code below will then fail, as hiddev_table hasn't been
|
||||
* updated.
|
||||
*
|
||||
* The obvious fix -- introducing mutex to guard hiddev_table[]
|
||||
* doesn't work, as usb_open() and usb_register_dev() both take
|
||||
* minor_rwsem, thus we'll have ABBA deadlock.
|
||||
*
|
||||
* Before BKL pushdown, usb_open() had been acquiring it in right
|
||||
* order, so _open() was safe to use it to protect from this race.
|
||||
* Now the order is different, but AB-BA deadlock still doesn't occur
|
||||
* as BKL is dropped on schedule() (i.e. while sleeping on
|
||||
* minor_rwsem). Fugly.
|
||||
*/
|
||||
lock_kernel();
|
||||
usb_set_intfdata(usbhid->intf, usbhid);
|
||||
retval = usb_register_dev(usbhid->intf, &hiddev_class);
|
||||
if (retval) {
|
||||
err_hid("Not able to get a minor for this device.");
|
||||
hid->hiddev = NULL;
|
||||
unlock_kernel();
|
||||
kfree(hiddev);
|
||||
return -1;
|
||||
} else {
|
||||
hid->minor = usbhid->intf->minor;
|
||||
hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
|
||||
}
|
||||
unlock_kernel();
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
@ -942,7 +913,6 @@ void hiddev_disconnect(struct hid_device *hid)
|
|||
hiddev->exist = 0;
|
||||
mutex_unlock(&hiddev->existancelock);
|
||||
|
||||
hiddev_table[hiddev->hid->minor - HIDDEV_MINOR_BASE] = NULL;
|
||||
usb_deregister_dev(usbhid->intf, &hiddev_class);
|
||||
|
||||
if (hiddev->open) {
|
||||
|
|
Loading…
Reference in a new issue