Re: [BUG, Regression, bisected] USB mouse causes bug on 1st insert, ignored on 2nd insert, lsusb stuck at usbdev_open

Jiri Kosina
2010-09-21T13:57:46Z

On Tue, 21 Sep 2010, Jiri Kosina wrote:

> > [ 22.705849] HID debug: hiddev_open(): hid: ffff88013a7c0000, hiddev: (null), intf: ffff880137d0ac00
>
> hiddev_open() has been called. usb_find_interface(&hid_driver, minor)
> returned interface pointer 0xffff880137d0ac00. Which is bogus! Interface
> 0xffff880137d0ac00 is not registered with hiddev at all, we should have
> received 0xffff880137d0a400.
>
> We currently register minors only for those usbhid devices (through
> usb_register_dev() in hiddev_connect()) which are going to be claimed by
> hiddev. It doesn't seem to be problematic to me, and I don't undersntand
> why usb_find_interface() returns wrong interface.
>
> I have just found out that it's actually CONFIG_USB_DYNAMIC_MINORS which
> makes the difference. When unset, the problem doesn't trigger, and
> usb_find_interface() always returns the proper interface. When
> CONFIG_USB_DYNAMIC_MINORS is being used, the oops happen.
>
> I'll look into that.

Could you guys please verify whether the patch below fixes the issues you
were seeing and puts everything back into shape again? Thanks.

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3f72924..616b449 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1154,6 +1154,7 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
char buf[64];
unsigned int i;
int len;
+ int hiddev_minor = 0;

if (hdev->quirks & HID_QUIRK_HIDDEV_FORCE)
connect_mask |= (HID_CONNECT_HIDDEV_FORCE | HID_CONNECT_HIDDEV);
@@ -1168,8 +1169,8 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
connect_mask & HID_CONNECT_HIDINPUT_FORCE))
hdev->claimed |= HID_CLAIMED_INPUT;
if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
- !hdev->hiddev_connect(hdev,
- connect_mask & HID_CONNECT_HIDDEV_FORCE))
+ ((hiddev_minor = hdev->hiddev_connect(hdev,
+ connect_mask & HID_CONNECT_HIDDEV_FORCE)) >= 0))
hdev->claimed |= HID_CLAIMED_HIDDEV;
if ((connect_mask & HID_CONNECT_HIDRAW) && !hidraw_connect(hdev))
hdev->claimed |= HID_CLAIMED_HIDRAW;
@@ -1189,7 +1190,7 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
len += sprintf(buf + len, "input");
if (hdev->claimed & HID_CLAIMED_HIDDEV)
len += sprintf(buf + len, "%shiddev%d", len ? "," : "",
- hdev->minor);
+ hiddev_minor);
if (hdev->claimed & HID_CLAIMED_HIDRAW)
len += sprintf(buf + len, "%shidraw%d", len ? "," : "",
((struct hidraw *)hdev->hidraw)->minor);
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 599041a..1f0a770 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1201,6 +1201,9 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
hid->driver_data = usbhid;
usbhid->hid = hid;
usbhid->intf = intf;
+ /* Only hiddev-claimed devices will have corresponding minor number.
+ * We can't leave it to 0, as that is valid minor as well */
+ usbhid->intf->minor = -1;
usbhid->ifnum = interface->desc.bInterfaceNumber;

init_waitqueue_head(&usbhid->wait);
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 681e620..7a81c1f 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -899,7 +899,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
kfree(hiddev);
return -1;
}
- return 0;
+ return usbhid->intf->minor;
}

/*

--
Jiri Kosina
SUSE Labs, Novell Inc.