On Wed, Apr 25, 2012 at 2:57 AM, Oliver Neukum <oliver@xxxxxxxxxx> wrote:
usb_submit_urb()
>>
>> This submit won't happen because HID_OUT_RUNNING is not cleared.
>
> I may be dense, but as far as I can tell a resubmit will happen, exactly if
> HID_OUT_RUNNING is _not_ cleared.
In fact, it should be a double unlink bug, usb_unlink_urb can handle
it correctly
if the lock is held. We also can deal with it easily by checking urb->unlinked,
so how about the below patch?
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index aa1c503..b530463 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -411,10 +411,10 @@ static void hid_irq_out(struct urb *urb)
{
struct hid_device *hid = urb->context;
struct usbhid_device *usbhid = hid->driver_data;
- unsigned long flags;
+ unsigned long status = urb->status;
int unplug = 0;
- switch (urb->status) {
+ switch (status) {
case 0: /* success */
break;
case -ESHUTDOWN: /* unplug */
@@ -428,8 +428,9 @@ static void hid_irq_out(struct urb *urb)
hid_warn(urb->dev, "output irq status %d received\n",
urb->status);
}
-
- spin_lock_irqsave(&usbhid->lock, flags);
+ if (status != -ECONNRESET)
+ spin_lock(&usbhid->unlink_lock);
+ spin_lock(&usbhid->lock);
if (unplug)
usbhid->outtail = usbhid->outhead;
@@ -438,12 +439,16 @@ static void hid_irq_out(struct urb *urb)
if (usbhid->outhead != usbhid->outtail && !hid_submit_out(hid)) {
/* Successfully submitted next urb in queue */
- spin_unlock_irqrestore(&usbhid->lock, flags);
+ spin_unlock(&usbhid->lock);
+ if (status != -ECONNRESET)
+ spin_unlock(&usbhid->unlink_lock);
return;
}
clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
- spin_unlock_irqrestore(&usbhid->lock, flags);
+ spin_unlock(&usbhid->lock);
+ if (status != -ECONNRESET)
+ spin_unlock(&usbhid->unlink_lock);
usb_autopm_put_interface_async(usbhid->intf);
wake_up(&usbhid->wait);
}
@@ -458,6 +463,8 @@ static void hid_ctrl(struct urb *urb)
struct usbhid_device *usbhid = hid->driver_data;
int unplug = 0, status = urb->status;
+ if (status != -ECONNRESET)
+ spin_lock(&usbhid->unlink_lock);
spin_lock(&usbhid->lock);
switch (status) {
@@ -487,11 +494,15 @@ static void hid_ctrl(struct urb *urb)
if (usbhid->ctrlhead != usbhid->ctrltail && !hid_submit_ctrl(hid)) {
/* Successfully submitted next urb in queue */
spin_unlock(&usbhid->lock);
+ if (status != -ECONNRESET)
+ spin_unlock(&usbhid->unlink_lock);
return;
}
clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
spin_unlock(&usbhid->lock);
+ if (status != -ECONNRESET)
+ spin_unlock(&usbhid->unlink_lock);
usb_autopm_put_interface_async(usbhid->intf);
wake_up(&usbhid->wait);
}
@@ -546,8 +557,13 @@ static void __usbhid_submit_report(struct
hid_device *hid, struct hid_report *re
* no race because this is called under
* spinlock
*/
- if (time_after(jiffies, usbhid->last_out + HZ * 5))
+
+ if (time_after(jiffies, usbhid->last_out + HZ * 5) &&
+ !usbhid->urbout->unlinked) {
+ spin_unlock(&usbhid->lock);
usb_unlink_urb(usbhid->urbout);
+ spin_lock(&usbhid->lock);
+ }
}
return;
}
@@ -594,8 +610,12 @@ static void __usbhid_submit_report(struct
hid_device *hid, struct hid_report *re
* no race because this is called under
* spinlock
*/
- if (time_after(jiffies, usbhid->last_ctrl + HZ * 5))
+ if (time_after(jiffies, usbhid->last_ctrl + HZ * 5) &&
+ !usbhid->urbctrl->unlinked) {
+ spin_unlock(&usbhid->lock);
usb_unlink_urb(usbhid->urbctrl);
+ spin_lock(&usbhid->lock);
+ }
}
}
@@ -604,9 +624,11 @@ void usbhid_submit_report(struct hid_device *hid,
struct hid_report *report, uns
struct usbhid_device *usbhid = hid->driver_data;
unsigned long flags;
- spin_lock_irqsave(&usbhid->lock, flags);
+ spin_lock_irqsave(&usbhid->unlink_lock, flags);
+ spin_lock(&usbhid->lock);
__usbhid_submit_report(hid, report, dir);
- spin_unlock_irqrestore(&usbhid->lock, flags);
+ spin_unlock(&usbhid->lock);
+ spin_unlock_irqrestore(&usbhid->unlink_lock, flags);
}
EXPORT_SYMBOL_GPL(usbhid_submit_report);
@@ -625,13 +647,15 @@ static void hid_led(struct work_struct *work)
return;
}
- spin_lock_irqsave(&usbhid->lock, flags);
+ spin_lock_irqsave(&usbhid->unlink_lock, flags);
+ spin_lock(&usbhid->lock);
if (!test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
usbhid->ledcount = hidinput_count_leds(hid);
hid_dbg(usbhid->hid, "New ledcount = %u\n", usbhid->ledcount);
__usbhid_submit_report(hid, field->report, USB_DIR_OUT);
}
- spin_unlock_irqrestore(&usbhid->lock, flags);
+ spin_unlock(&usbhid->lock);
+ spin_unlock_irqrestore(&usbhid->unlink_lock, flags);
}
static int usb_hidinput_input_event(struct input_dev *dev, unsigned
int type, unsigned int code, int value)
@@ -1299,6 +1323,7 @@ static int usbhid_probe(struct usb_interface
*intf, const struct usb_device_id *
INIT_WORK(&usbhid->reset_work, hid_reset);
setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
spin_lock_init(&usbhid->lock);
+ spin_lock_init(&usbhid->unlink_lock);
INIT_WORK(&usbhid->led_work, hid_led);
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 1883d7b..69af387 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -90,6 +90,7 @@ struct usbhid_device {
unsigned long last_out; /* record of last output for timeouts */
spinlock_t lock; /* fifo spinlock */
+ spinlock_t unlink_lock; /* unlink spinlock */
unsigned long iofl; /*
I/O flags (CTRL_RUNNING, OUT_RUNNING) */
struct timer_list io_retry; /*
Retry timer */
unsigned long stop_retry; /*
Time to give up, in jiffies */
Thanks,
--
Ming Lei
--
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
