Hi Dmitry,
Can you please respin this patch to not include initialization in
variable declarations? In general, I'd like to avoid anything but
simple initialization for things like setting return values to 0 or
pointers to NULL, although I know some of the code in the xHCI driver is
not consistent about that.
I especially want to avoid initialization-during-declaration lines that
include function calls, because it makes it really hard to notice what's
happening in the code. Some of the lines in your patch are also
triggering "over-80 character line" warnings from checkpatch.pl.
Comments inline below. I'll test this patch on the hardware and let you
know if it works.
Sarah Sharp
On Fri, Mar 18, 2011 at 01:34:08AM -0700, Dmitry Torokhov wrote:
> The logic of the handling Missed Service Error Events was pretty
> confusing as we were checking the same condition several times.
> In addition, it caused compiler warning since the compiler could
> not figure out that event_trb is actually unused in case we are
> skipping current TD.
>
> Fix that by rearranging "skip" condition checks, and factor out
> skip_isoc_td() so that it is called explicitly.
>
> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 186 +++++++++++++++++++++---------------------
> 1 files changed, 94 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index cfc1ad9..7ec7acd 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1671,75 +1671,50 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
> union xhci_trb *event_trb, struct xhci_transfer_event *event,
> struct xhci_virt_ep *ep, int *status)
> {
> - struct xhci_ring *ep_ring;
> - struct urb_priv *urb_priv;
> - int idx;
> + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer);
> + struct urb_priv *urb_priv = td->urb->hcpriv;
> + int idx = urb_priv->td_cnt;
> + struct usb_iso_packet_descriptor *frame = &td->urb->iso_frame_desc[idx];
The ep_ring and frame initialization is particularly ugly here, but I
really want all this initialization taken out of the variable
declaration area. The trb_comp_code can stay, I guess, since it's
initialized that way elsewhere.
> int len = 0;
> - int skip_td = 0;
> union xhci_trb *cur_trb;
> struct xhci_segment *cur_seg;
> - u32 trb_comp_code;
> + u32 trb_comp_code = GET_COMP_CODE(event->transfer_len);
> + bool skip_td = false;
>
> - ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer);
> - trb_comp_code = GET_COMP_CODE(event->transfer_len);
> - urb_priv = td->urb->hcpriv;
> - idx = urb_priv->td_cnt;
> -
> - if (ep->skip) {
> - /* The transfer is partly done */
> - *status = -EXDEV;
> - td->urb->iso_frame_desc[idx].status = -EXDEV;
> - } else {
> - /* handle completion code */
> - switch (trb_comp_code) {
> - case COMP_SUCCESS:
> - td->urb->iso_frame_desc[idx].status = 0;
> - xhci_dbg(xhci, "Successful isoc transfer!\n");
> - break;
> - case COMP_SHORT_TX:
> - if (td->urb->transfer_flags & URB_SHORT_NOT_OK)
> - td->urb->iso_frame_desc[idx].status =
> - -EREMOTEIO;
> - else
> - td->urb->iso_frame_desc[idx].status = 0;
> - break;
> - case COMP_BW_OVER:
> - td->urb->iso_frame_desc[idx].status = -ECOMM;
> - skip_td = 1;
> - break;
> - case COMP_BUFF_OVER:
> - case COMP_BABBLE:
> - td->urb->iso_frame_desc[idx].status = -EOVERFLOW;
> - skip_td = 1;
> - break;
> - case COMP_STALL:
> - td->urb->iso_frame_desc[idx].status = -EPROTO;
> - skip_td = 1;
> - break;
> - case COMP_STOP:
> - case COMP_STOP_INVAL:
> - break;
> - default:
> - td->urb->iso_frame_desc[idx].status = -1;
> - break;
> - }
> - }
> -
> - /* calc actual length */
> - if (ep->skip) {
> - td->urb->iso_frame_desc[idx].actual_length = 0;
> - /* Update ring dequeue pointer */
> - while (ep_ring->dequeue != td->last_trb)
> - inc_deq(xhci, ep_ring, false);
> - inc_deq(xhci, ep_ring, false);
> - return finish_td(xhci, td, event_trb, event, ep, status, true);
> + /* handle completion code */
> + switch (trb_comp_code) {
> + case COMP_SUCCESS:
> + frame->status = 0;
> + xhci_dbg(xhci, "Successful isoc transfer!\n");
> + break;
> + case COMP_SHORT_TX:
> + frame->status = td->urb->transfer_flags & URB_SHORT_NOT_OK ?
> + -EREMOTEIO : 0;
> + break;
> + case COMP_BW_OVER:
> + frame->status = -ECOMM;
> + skip_td = true;
> + break;
> + case COMP_BUFF_OVER:
> + case COMP_BABBLE:
> + frame->status = -EOVERFLOW;
> + skip_td = true;
> + break;
> + case COMP_STALL:
> + frame->status = -EPROTO;
> + skip_td = true;
> + break;
> + case COMP_STOP:
> + case COMP_STOP_INVAL:
> + break;
> + default:
> + frame->status = -1;
> + break;
> }
>
> - if (trb_comp_code == COMP_SUCCESS || skip_td == 1) {
> - td->urb->iso_frame_desc[idx].actual_length =
> - td->urb->iso_frame_desc[idx].length;
> - td->urb->actual_length +=
> - td->urb->iso_frame_desc[idx].length;
> + if (trb_comp_code == COMP_SUCCESS || skip_td) {
> + frame->actual_length = frame->length;
> + td->urb->actual_length += frame->length;
> } else {
> for (cur_trb = ep_ring->dequeue,
> cur_seg = ep_ring->deq_seg; cur_trb != event_trb;
> @@ -1755,7 +1730,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
> TRB_LEN(event->transfer_len);
>
> if (trb_comp_code != COMP_STOP_INVAL) {
> - td->urb->iso_frame_desc[idx].actual_length = len;
> + frame->actual_length = len;
> td->urb->actual_length += len;
> }
> }
> @@ -1766,6 +1741,30 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
> return finish_td(xhci, td, event_trb, event, ep, status, false);
> }
>
> +static int skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
> + struct xhci_transfer_event *event,
> + struct xhci_virt_ep *ep, int *status)
> +{
> + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer);
> + struct urb_priv *urb_priv = td->urb->hcpriv;
> + int idx = urb_priv->td_cnt;
> + struct usb_iso_packet_descriptor *frame = &td->urb->iso_frame_desc[idx];
Same here for these four variables.
> +
> + /* The transfer is partly done */
> + *status = -EXDEV;
> + frame->status = -EXDEV;
> +
> + /* calc actual length */
> + frame->actual_length = 0;
> +
> + /* Update ring dequeue pointer */
> + while (ep_ring->dequeue != td->last_trb)
> + inc_deq(xhci, ep_ring, false);
> + inc_deq(xhci, ep_ring, false);
> +
> + return finish_td(xhci, td, NULL, event, ep, status, true);
> +}
> +
> /*
> * Process bulk and interrupt tds, update urb status and actual_length.
> */
> @@ -1773,13 +1772,10 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td,
> union xhci_trb *event_trb, struct xhci_transfer_event *event,
> struct xhci_virt_ep *ep, int *status)
> {
> - struct xhci_ring *ep_ring;
> + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer);
Also here.
> union xhci_trb *cur_trb;
> struct xhci_segment *cur_seg;
> - u32 trb_comp_code;
> -
> - ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer);
> - trb_comp_code = GET_COMP_CODE(event->transfer_len);
> + u32 trb_comp_code = GET_COMP_CODE(event->transfer_len);
>
> switch (trb_comp_code) {
> case COMP_SUCCESS:
> @@ -2024,36 +2020,42 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> }
>
> td = list_entry(ep_ring->td_list.next, struct xhci_td, td_list);
> +
> /* Is this a TRB in the currently executing TD? */
> event_seg = trb_in_td(ep_ring->deq_seg, ep_ring->dequeue,
> td->last_trb, event_dma);
> - if (event_seg && ep->skip) {
> + if (!event_seg) {
> + if (!ep->skip ||
> + !usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> + /* HC is busted, give up! */
> + xhci_err(xhci,
> + "ERROR Transfer event TRB DMA ptr not "
> + "part of current TD\n");
> + return -ESHUTDOWN;
> + }
> +
> + ret = skip_isoc_td(xhci, td, event, ep, &status);
> + goto cleanup;
> + }
> +
> + if (ep->skip) {
> xhci_dbg(xhci, "Found td. Clear skip flag.\n");
> ep->skip = false;
> }
> - if (!event_seg &&
> - (!ep->skip || !usb_endpoint_xfer_isoc(&td->urb->ep->desc))) {
> - /* HC is busted, give up! */
> - xhci_err(xhci, "ERROR Transfer event TRB DMA ptr not "
> - "part of current TD\n");
> - return -ESHUTDOWN;
> - }
>
> - if (event_seg) {
> - event_trb = &event_seg->trbs[(event_dma -
> - event_seg->dma) / sizeof(*event_trb)];
> - /*
> - * No-op TRB should not trigger interrupts.
> - * If event_trb is a no-op TRB, it means the
> - * corresponding TD has been cancelled. Just ignore
> - * the TD.
> - */
> - if ((event_trb->generic.field[3] & TRB_TYPE_BITMASK)
> - == TRB_TYPE(TRB_TR_NOOP)) {
> - xhci_dbg(xhci, "event_trb is a no-op TRB. "
> - "Skip it\n");
> - goto cleanup;
> - }
> + event_trb = &event_seg->trbs[(event_dma - event_seg->dma) /
> + sizeof(*event_trb)];
> + /*
> + * No-op TRB should not trigger interrupts.
> + * If event_trb is a no-op TRB, it means the
> + * corresponding TD has been cancelled. Just ignore
> + * the TD.
> + */
> + if ((event_trb->generic.field[3] & TRB_TYPE_BITMASK)
> + == TRB_TYPE(TRB_TR_NOOP)) {
> + xhci_dbg(xhci,
> + "event_trb is a no-op TRB. Skip it\n");
> + goto cleanup;
> }
>
> /* Now update the urb's actual_length and give back to
> --
> 1.7.3.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
