> -----Original Message----- > From: Andiry Xu [mailto:andiry@xxxxxxxxx] > Sent: 05 November 2013 17:40 > To: David Laight > Cc: Linux-USB; Sarah Sharp > Subject: Re: [PATCH] usb: xhci: Optimise setup of isochronous transfers > > On Tue, Nov 5, 2013 at 6:21 AM, David Laight <David.Laight@xxxxxxxxxx> wrote: > > > > Close inspection shows that xhci_queue_isoc_tx() can only fail if > > prepare_transfer() gets an error from usb_hcd_link_urb_to_ep() for > > the first TD. The call to prepare_ring() cannot fail because an initial > > check is made to ensure the ring has enough space for all the TD. ... > > - ret = prepare_transfer(xhci, xhci->devs[slot_id], ep_index, > > - urb->stream_id, trbs_per_td, urb, i, mem_flags); > > - if (ret < 0) { > > - if (i == 0) > > - return ret; > > - goto cleanup; > > - } > > + /* prepare_transfer() was called earlier for the first TRB. > > + * It cannot fail for later TRBs. > > + */ > > + if (i != 0) > > + prepare_transfer(xhci, xhci->devs[slot_id], ep_index, > > + urb->stream_id, trbs_per_td, urb, i, > > + mem_flags); ... > > -cleanup: > > - /* Clean up a partially enqueued isoc transfer. */ > > - > > - for (i--; i >= 0; i--) > > - list_del_init(&urb_priv->td[i].td_list); > > - > > - /* Use the first TD as a temporary variable to turn the TDs we've queued > > - * into No-ops with a software-owned cycle bit. That way the hardware > > - * won't accidentally start executing bogus TDs when we partially > > - * overwrite them. td->first_trb and td->start_seg are already set. > > - */ > > - urb_priv->td[0].last_trb = ep_ring->enqueue; > > - /* Every TRB except the first & last will have its cycle bit flipped. */ > > - td_to_noop(xhci, ep_ring, &urb_priv->td[0], true); > > - > > - /* Reset the ring enqueue back to the first TRB and its cycle bit. */ > > - ep_ring->enqueue = urb_priv->td[0].first_trb; > > - ep_ring->enq_seg = urb_priv->td[0].start_seg; > > - /* The cycle bit in the first TRB won't be modified, get its inverse. */ > > - ep_ring->cycle_state = (ep_ring->enqueue->generic.field[3] & > > - TRB_CYCLE) ^ TRB_CYCLE; > > - ep_ring->num_trbs_free = ep_ring->num_trbs_free_temp; > > - usb_hcd_unlink_urb_from_ep(bus_to_hcd(urb->dev->bus), urb); > > - return ret; > > } > > This code is there for a reason. See commit > 522989a27c7badb608155b1f1dea3487ed431f74 for details. I presume you means this bit: When an isochronous transfer is enqueued, xhci_queue_isoc_tx_prepare() will ensure that there is enough room on the transfer rings for all of the isochronous TDs for that URB. However, when xhci_queue_isoc_tx() is enqueueing individual isoc TDs, the prepare_transfer() function can fail if the endpoint state has changed to disabled, error, or some other unknown state. The check for the endpoint state is inside prepare_ring() when called from prepare_transfer(). This is now only called for the first TD so cannot fail for subsequent ones. So if there is an error it will be handled after the ring (etc) contains all of the TD and they will be tidied up together. > Also, do you remove the return 0 sentence on purpose? Yes, function return type is now 'void'. David -- 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
