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.
>
> The check for 'num_tds' being negative is pointless here, and the
> check of the 'running_total' is looking for a simple coding error,
> both can be deleted.
>
> Isoc setup is the only code path that calls prepare_transfer() with
> ts_index != 0 so there is no point calling prepare_ring() again.
>
> prepare_ring() is still called twice for the first TD.
> However since prepare_tranfser() doesn't depend on the number of TD
> the initial prepare_ring() can be replaced by prepare_transfer()
> and the existing prepare_transfer() call only made for subsequent TD.
>
> This all means that xhci_queue_isoc_tx() can no longer fail.
> Delete the (possibly dubious) 'cleanup' code and change the return
> type to void.
>
> Signed-off-by: David Laight <david.laight@xxxxxxxxxx>
> ---
> This is compile tested only because I don't have an isoc device.
> However it should be ok.
> I've just ordered a cheap USB 'sound card' - presumably that
> will let me test isochronous transfers.
>
> Note: This patch will only apply cleanly if my earlier patches are applied first.
> ---
> drivers/usb/host/xhci-ring.c | 90 +++++++++++++-------------------------------
> drivers/usb/host/xhci.h | 1 -
> 2 files changed, 27 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d0ef8b8..5480215 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2991,7 +2991,7 @@ static int prepare_transfer(struct xhci_hcd *xhci,
> struct urb_priv *urb_priv;
> struct xhci_td *td;
> struct xhci_ring *ep_ring;
> - struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
> + struct xhci_ep_ctx *ep_ctx;
>
> ep_ring = xhci_stream_id_to_ring(xdev, ep_index, stream_id);
> if (!ep_ring) {
> @@ -3000,24 +3000,26 @@ static int prepare_transfer(struct xhci_hcd *xhci,
> return -EINVAL;
> }
>
> - ret = prepare_ring(xhci, ep_ring,
> - le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
> - num_trbs, mem_flags);
> - if (ret)
> - return ret;
> -
> - urb_priv = urb->hcpriv;
> - td = &urb_priv->td[td_index];
> + if (td_index == 0) {
> + ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
>
> - INIT_LIST_HEAD(&td->td_list);
> - INIT_LIST_HEAD(&td->cancelled_td_list);
> + ret = prepare_ring(xhci, ep_ring,
> + le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
> + num_trbs, mem_flags);
> + if (unlikely(ret))
> + return ret;
>
> - if (td_index == 0) {
> ret = usb_hcd_link_urb_to_ep(bus_to_hcd(urb->dev->bus), urb);
> if (unlikely(ret))
> return ret;
> }
>
> + urb_priv = urb->hcpriv;
> + td = &urb_priv->td[td_index];
> +
> + INIT_LIST_HEAD(&td->td_list);
> + INIT_LIST_HEAD(&td->cancelled_td_list);
> +
> td->urb = urb;
> /* Add this TD to the tail of the endpoint ring's TD list */
> list_add_tail(&td->td_list, &ep_ring->td_list);
> @@ -3647,7 +3649,7 @@ static unsigned int xhci_get_last_burst_packet_count(struct xhci_hcd *xhci,
> }
>
> /* This is for isoc transfer */
> -static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> +static void xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> struct urb *urb, int slot_id, unsigned int ep_index)
> {
> struct xhci_ring *ep_ring;
> @@ -3657,7 +3659,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> struct xhci_generic_trb *start_trb;
> bool first_trb;
> u32 field, length_field;
> - int running_total, trb_buff_len, td_len, td_remain_len, ret;
> + int running_total, trb_buff_len, td_len, td_remain_len;
> u64 start_addr, addr;
> int i, j;
> bool more_trbs_coming;
> @@ -3665,10 +3667,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
>
> num_tds = urb->number_of_packets;
> - if (num_tds < 1) {
> - xhci_dbg(xhci, "Isoc URB with zero packets?\n");
> - return -EINVAL;
> - }
>
> start_addr = (u64) urb->transfer_dma;
> start_trb = &ep_ring->enqueue->generic;
> @@ -3698,13 +3696,13 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>
> trbs_per_td = count_isoc_trbs_needed(xhci, urb, i);
>
> - 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);
>
> td = &urb_priv->td[i];
> for (j = 0; j < trbs_per_td; j++) {
> @@ -3782,13 +3780,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> addr += trb_buff_len;
> td_remain_len -= trb_buff_len;
> }
> -
> - /* Check TD length */
> - if (running_total != td_len) {
> - xhci_err(xhci, "ISOC TD length unmatch\n");
> - ret = -EINVAL;
> - goto cleanup;
> - }
> }
>
> if (xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs == 0) {
> @@ -3798,31 +3789,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs++;
>
> giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id, start_trb);
> - return 0;
> -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.
Also, do you remove the return 0 sentence on purpose?
Thanks,
Andiry
>
> /*
> @@ -3836,7 +3802,6 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
> struct urb *urb, int slot_id, unsigned int ep_index)
> {
> struct xhci_virt_device *xdev;
> - struct xhci_ring *ep_ring;
> struct xhci_ep_ctx *ep_ctx;
> int start_frame;
> int xhci_interval;
> @@ -3845,7 +3810,6 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
> int ret;
>
> xdev = xhci->devs[slot_id];
> - ep_ring = xdev->eps[ep_index].ring;
> ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
>
> num_trbs = 0;
> @@ -3856,8 +3820,8 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
> /* Check the ring to guarantee there is enough room for the whole urb.
> * Do not insert any td of the urb to the ring if the check failed.
> */
> - ret = prepare_ring(xhci, ep_ring, le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
> - num_trbs, mem_flags);
> + ret = prepare_transfer(xhci, xdev, ep_index,
> + urb->stream_id, num_trbs, urb, 0, mem_flags);
> if (ret)
> return ret;
>
> @@ -3889,9 +3853,9 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci, gfp_t mem_flags,
> urb->dev->speed == USB_SPEED_FULL)
> urb->interval /= 8;
> }
> - ep_ring->num_trbs_free_temp = ep_ring->num_trbs_free;
>
> - return xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);
> + xhci_queue_isoc_tx(xhci, mem_flags, urb, slot_id, ep_index);
> + return 0;
> }
>
> /**** Command Ring Operations ****/
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index a7d8fdd..118f90b 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1331,7 +1331,6 @@ struct xhci_ring {
> unsigned int stream_id;
> unsigned int num_segs;
> unsigned int num_trbs_free;
> - unsigned int num_trbs_free_temp;
> enum xhci_ring_type type;
> bool last_td_was_short;
> };
> --
> 1.8.1.2
>
>
>
> --
> 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
--
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
