On Wed, 19 Jun 2013, Ming Lei wrote: > > The approach used in this patch is wrong. You shouldn't start the > > unlink, then wait, then finish the unlink. Consider what would happen > > What you mentioned above is just what the patch is doing, :-) > > start_unlink_intr() only sets the qh as QH_STATE_UNLINK_WAIT, puts > the qh into one wait list and starts a timer, if it is expired the qh will be > started to unlink, otherwise the qh can be recovered to QH_STATE_LINKED > immediately if there is one URB submitted. Okay, maybe I was fooled by your use of QH_STATE_UNLINK_WAIT. Setting the state to that value means that the QH is going to be unlinked after a time delay. However, that's not what you mean; you mean that after a time delay you will decide whether or not to unlink the QH. I think you should copy the approach used for the async QHs. > So unlinking intr qh becomes a 3-stage process: > > - wait(qh return to linked state if URB is submitted during the period, > otherwise go to start unlink) > - start unlink(do unlink, and wait for end of unlink) > - end unlink > > > if an URB were submitted during the delay: It would have to wait until > > If an URB were submitted during the delay, the previous wait is canceled > immediately, and the qh state is recovered to linked state, please see > cancel_unlink_wait_intr() called in intr_submit(). Right. But you're not allowed to do that after changing qh->state. One of the invariants of the driver is that once qh->state takes on any value other than QH_STATE_LINKED (or, temporarily, QH_STATE_COMPLETING), the QH must be unlinked. The state can't change back to QH_STATE_LINKED without first passing through QH_STATE_IDLE. > > Also, it's silly to check whether or not giveback uses a tasklet. We > > know that following the 6/6 patch it will. Even if it doesn't, there's > > no harm in waiting a little while before unlinking an empty interrupt > > QH. > > It is still for the test reason, since the patch may cause recursion if > HCD_BH isn't set. How can it cause recursion? The async unlink code doesn't. Alan Stern -- 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
