>Is this the second version of this patch? If it is, you should say so
>in the subject line. For example:
>
>Subject: [PATCH v.2] USB: OHCI: ...
I will take care of this in future.
>> There is a race condition between finish_unlinks->finish_urb()
>> function and usb_kill_urb() in ohci controller case.
>> The finish_urb is called after calling spin_unlock(&ohci->lock),
>
>Not quite right. finish_urb() itself calls spin_unlock().
yes, right. This code path become lock free in usb_hcd_giveback_urb().
>Your analysis is correct, but I don't like the solution. The code you
>quoted above uses *last after finish_urb(), which is not a good idea.
>
>In my opinion, it would be better to go back to the way things were
>before commit 977dcfdc6031 and remove ed from the ed_rm_list before
>finishing any URBs. Then at the end, we can add ed back to the list
>if necessary.
>
>How does the patch below look to you? Does it fix the problem?
yes, this change is also doing the same which i wanted to do.
I think it is better way of implementing the required change.
Should i submit the changed patch for required change ?
Thanks & Regards,
Aman Deep
------- Original Message -------
Sender : Alan Stern
Date : Jul 07, 2015 23:36 (GMT+06:00)
Title : Re: [USB][OHCI]BugFix:Insert new ed at end of ohci->ed_rm_list to accomdate race condition
Is this the second version of this patch? If it is, you should say so
in the subject line. For example:
Subject: [PATCH v.2] USB: OHCI: ...
On Tue, 7 Jul 2015, AMAN DEEP wrote:
> There is a race condition between finish_unlinks->finish_urb()
> function and usb_kill_urb() in ohci controller case.
> The finish_urb is called after calling spin_unlock(&ohci->lock),
Not quite right. finish_urb() itself calls spin_unlock().
> then if during this time, usb_kill_urb is called for another
> endpoint, then new ed will be added to ed_rm_list at beginning
> for unlink. and ed_rm_list will point to newly added ed.
>
> When finish_urb() is completed in finish_unlinks() and
> ed->td_list becomes empty as in below code (in finish_unlinks() function)
> if (list_empty(&ed->td_list)) {
> *last = ed->ed_next;
> ed->ed_next = NULL;
> } else if (ohci->rh_state == OHCI_RH_RUNNING) {
> *last = ed->ed_next;
> ed->ed_next = NULL;
> ed_schedule(ohci, ed);
> }
> *last = ed->ed_next will make ed_rm_list to point to ed->ed_next and
> previously added ed by usb_kill_urb will be left unreferenced by
> ed_rm_list. This causes usb_kill_urb() hang forever waiting for
> finish_unlink to remove added ed from ed_rm_list.
>
> The main reason for hang in this race condtion is addition and removal
> of ed from ed_rm_list in the beginning. This patch adds new ed in
> ed_rm_list at the end and it solves the usb_kill_urb hang issue for
> ohci controller.
Your analysis is correct, but I don't like the solution. The code you
quoted above uses *last after finish_urb(), which is not a good idea.
In my opinion, it would be better to go back to the way things were
before commit 977dcfdc6031 and remove ed from the ed_rm_list before
finishing any URBs. Then at the end, we can add ed back to the list
if necessary.
How does the patch below look to you? Does it fix the problem?
Alan Stern
drivers/usb/host/ohci-q.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
Index: usb-4.1/drivers/usb/host/ohci-q.c
===================================================================
--- usb-4.1.orig/drivers/usb/host/ohci-q.c
+++ usb-4.1/drivers/usb/host/ohci-q.c
@@ -1016,6 +1016,8 @@ skip_ed:
* have modified this list. normally it's just prepending
* entries (which we'd ignore), but paranoia won't hurt.
*/
+ *last = ed->ed_next;
+ ed->ed_next = NULL;
modified = 0;
/* unlink urbs as requested, but rescan the list after
@@ -1074,21 +1076,22 @@ rescan_this:
goto rescan_this;
/*
- * If no TDs are queued, take ED off the ed_rm_list.
+ * If no TDs are queued, ED is now idle.
* Otherwise, if the HC is running, reschedule.
- * If not, leave it on the list for further dequeues.
+ * If the HC isn't running, add ED back to the
+ * start of the list for later processing.
*/
if (list_empty(&ed->td_list)) {
- *last = ed->ed_next;
- ed->ed_next = NULL;
ed->state = ED_IDLE;
list_del(&ed->in_use_list);
} else if (ohci->rh_state == OHCI_RH_RUNNING) {
- *last = ed->ed_next;
- ed->ed_next = NULL;
ed_schedule(ohci, ed);
} else {
- last = &ed->ed_next;
+ ed->ed_next = ohci->ed_rm_list;
+ ohci->ed_rm_list = ed;
+ /* Don't loop on the same ED */
+ if (last == &ohci->ed_rm_list)
+ last = &ed->ed_next;
}
if (modified)ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ¥Šwÿº{.nÇ+‰·¥Š{±þëþ)í…æèw* jg¬±¨ ¶‰šŽŠÝ¢jÿ¾«þG«?éÿ¢¸ ¢·¦j:+v‰¨ŠwèjØm¶Ÿÿþø ¯ù ®w¥þŠàþf£¢·hš?â?úÿ†Ù¥
