On Mon, Jul 1, 2013 at 1:35 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Mon, 1 Jul 2013, Ming Lei wrote: > >> >> So I think we should unlink here to speed up the procedure as suggested >> >> in your previous email. >> > >> > You are right. If the QH's qtd_list isn't empty then we should WARN_ON >> > and return without doing anything -- just leak the QH. Otherwise, >> >> Yes, we can add the WARN_ON() because caller should have unlinked >> all requests, but looks we can handle the unlink here too without obvious >> side-effect. >> >> Currently, URB might be probably submitted to HCD too even after >> usb_hcd_flush_endpoint() completes since both accesses to dev->ep_in[epnum] >> and ep->enabled aren't protected by effective locks. > > The urb_list_lock in hcd.c serves to synchronize changes to > ep->enabled. An URB might be submitted after usb_hcd_flush_endpoint() > completes, but the submission will fail in usb_hcd_link_urb_to_ep(). But the lock isn't held when setting and clearing ep->enabled in usb_enable_endpoint and usb_disable_endpoint, is it? Also looks there is the similar problem about 'dev->can_submit' usage too. > >> So how about not adding the WARN_ON(!list_empty(&qh->qtd_list) >> now when qh->qh_state is QH_STATE_LINKED? > > I think we should add the WARN_ON. Or jump to the default case in that > switch statement -- maybe the error message there should include a > WARN_ON. How about just adding WARN_ON(!list_empty(&qh->qtd_list) but continue unlinking the qh like current code, which should be harmless and avoid the QH leak? > >> Also I am not sure if we should return without doing anything under the >> situation, at least when the QH is in IDLE and QH's qtd_list isn't empty, >> ep->hcpriv is cleared, then return. > > The only way for a QH to be in the IDLE state with a non-empty qtd_list > is if qh->clearing_tt is set. The code already checks for that. OK. 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
