On Thu, May 09, 2013 at 09:42:51PM -0700, Julius Werner wrote: > The TD Size field in an XHCI TRB is calculated in two different ways, > depending on the XHCI version. The current code does this version check > in every caller, which makes it easy to forget (such as in the function > for control transfers, which still erroneously uses the old calculation > for its data stage TRB). The 1.0 calculation has also been corrected and > amended several times, growing slightly more complicated than necessary.
Does this patch fix the control transfer data stage issue? Please break this into a bug fix for that, with your refactoring on top of that. We need to backport the bug fix to the stable kernels, and queue the refactoring for 3.11. Sarah Sharp > > This patch moves the XHCI version check into a single, unified (and > slightly simplified) xhci_td_remainder() function, which deduplicates > code and looks cleaner in the callers. > > Signed-off-by: Julius Werner <[email protected]> > --- > drivers/usb/host/xhci-ring.c | 111 > +++++++++++-------------------------------- > 1 file changed, 29 insertions(+), 82 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 1969c00..32629b2 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -3074,54 +3074,33 @@ int xhci_queue_intr_tx(struct xhci_hcd *xhci, gfp_t > mem_flags, > } > > /* > - * The TD size is the number of bytes remaining in the TD (including this > TRB), > - * right shifted by 10. > - * It must fit in bits 21:17, so it can't be bigger than 31. > - */ > -static u32 xhci_td_remainder(unsigned int remainder) > -{ > - u32 max = (1 << (21 - 17 + 1)) - 1; > - > - if ((remainder >> 10) >= max) > - return max << 17; > - else > - return (remainder >> 10) << 17; > -} > - > -/* > - * For xHCI 1.0 host controllers, TD size is the number of max packet sized > - * packets remaining in the TD (*not* including this TRB). > - * > - * Total TD packet count = total_packet_count = > - * DIV_ROUND_UP(TD size in bytes / wMaxPacketSize) > + * Calculate the TD Size field of a TRB, shifted to its correct position > + * in bits 21:17. In pre-1.0 XHCI versions, this field contains the > + * number of bytes left in the TD, including the current TRB, in kB. > * > - * Packets transferred up to and including this TRB = packets_transferred = > - * rounddown(total bytes transferred including this TRB / wMaxPacketSize) > + * In XHCI 1.0, it contains the amount of packets left in the TD, > + * excluding the current TRB. Even though the spec explains this in a > + * roundabout way, the end result is just the amount of bytes left after > + * this TRB divided by the maximum packet size, rounded up. For the last > + * TRB this is naturally zero since there cannot be any more bytes left. > * > - * TD size = total_packet_count - packets_transferred > - * > - * It must fit in bits 21:17, so it can't be bigger than 31. > - * The last TRB in a TD must have the TD size set to zero. > + * @running_total: amount of bytes in the TD before this TRB > + * @trb_len: amount of bytes in the current TRB > + * @total_len: total amount of bytes in the TD > */ > -static u32 xhci_v1_0_td_remainder(int running_total, int trb_buff_len, > - unsigned int total_packet_count, struct urb *urb, > - unsigned int num_trbs_left) > +static u32 xhci_td_remainder(int running_total, int trb_len, int total_len, > + struct urb *urb) > { > - int packets_transferred; > + u32 result; > > - /* One TRB with a zero-length data packet. */ > - if (num_trbs_left == 0 || (running_total == 0 && trb_buff_len == 0)) > - return 0; > - > - /* All the TRB queueing functions don't count the current TRB in > - * running_total. > - */ > - packets_transferred = (running_total + trb_buff_len) / > - GET_MAX_PACKET(usb_endpoint_maxp(&urb->ep->desc)); > + if (hcd_to_xhci(bus_to_hcd(urb->dev->bus))->hci_version < 0x100) > + result = (total_len - running_total) >> 10; > + else > + result = DIV_ROUND_UP(total_len - running_total - trb_len, > + GET_MAX_PACKET(usb_endpoint_maxp(&urb->ep->desc))); > > - if ((total_packet_count - packets_transferred) > 31) > - return 31 << 17; > - return (total_packet_count - packets_transferred) << 17; > + /* TD size field is only 5 bit wide, so cap this at 31 */ > + return min(result, 31u) << 17; > } > > static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags, > @@ -3134,7 +3113,6 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, > gfp_t mem_flags, > struct scatterlist *sg; > int num_sgs; > int trb_buff_len, this_sg_len, running_total; > - unsigned int total_packet_count; > bool first_trb; > u64 addr; > bool more_trbs_coming; > @@ -3148,8 +3126,6 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, > gfp_t mem_flags, > > num_trbs = count_sg_trbs_needed(xhci, urb); > num_sgs = urb->num_mapped_sgs; > - total_packet_count = DIV_ROUND_UP(urb->transfer_buffer_length, > - usb_endpoint_maxp(&urb->ep->desc)); > > trb_buff_len = prepare_transfer(xhci, xhci->devs[slot_id], > ep_index, urb->stream_id, > @@ -3191,7 +3167,6 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, > gfp_t mem_flags, > do { > u32 field = 0; > u32 length_field = 0; > - u32 remainder = 0; > > /* Don't change the cycle bit of the first TRB until later */ > if (first_trb) { > @@ -3225,17 +3200,9 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, > gfp_t mem_flags, > } > > /* Set the TRB length, TD size, and interrupter fields. */ > - if (xhci->hci_version < 0x100) { > - remainder = xhci_td_remainder( > - urb->transfer_buffer_length - > - running_total); > - } else { > - remainder = xhci_v1_0_td_remainder(running_total, > - trb_buff_len, total_packet_count, urb, > - num_trbs - 1); > - } > length_field = TRB_LEN(trb_buff_len) | > - remainder | > + xhci_td_remainder(running_total, trb_buff_len, > + urb->transfer_buffer_length, urb) | > TRB_INTR_TARGET(0); > > if (num_trbs > 1) > @@ -3294,7 +3261,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t > mem_flags, > u32 field, length_field; > > int running_total, trb_buff_len, ret; > - unsigned int total_packet_count; > u64 addr; > > if (urb->num_sgs) > @@ -3340,8 +3306,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t > mem_flags, > start_cycle = ep_ring->cycle_state; > > running_total = 0; > - total_packet_count = DIV_ROUND_UP(urb->transfer_buffer_length, > - usb_endpoint_maxp(&urb->ep->desc)); > /* How much data is in the first TRB? */ > addr = (u64) urb->transfer_dma; > trb_buff_len = TRB_MAX_BUFF_SIZE - > @@ -3353,7 +3317,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t > mem_flags, > > /* Queue the first TRB, even if it's zero-length */ > do { > - u32 remainder = 0; > field = 0; > > /* Don't change the cycle bit of the first TRB until later */ > @@ -3380,17 +3343,9 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t > mem_flags, > field |= TRB_ISP; > > /* Set the TRB length, TD size, and interrupter fields. */ > - if (xhci->hci_version < 0x100) { > - remainder = xhci_td_remainder( > - urb->transfer_buffer_length - > - running_total); > - } else { > - remainder = xhci_v1_0_td_remainder(running_total, > - trb_buff_len, total_packet_count, urb, > - num_trbs - 1); > - } > length_field = TRB_LEN(trb_buff_len) | > - remainder | > + xhci_td_remainder(running_total, trb_buff_len, > + urb->transfer_buffer_length, urb) | > TRB_INTR_TARGET(0); > > if (num_trbs > 1) > @@ -3502,7 +3457,8 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t > mem_flags, > field = TRB_TYPE(TRB_DATA); > > length_field = TRB_LEN(urb->transfer_buffer_length) | > - xhci_td_remainder(urb->transfer_buffer_length) | > + xhci_td_remainder(0, urb->transfer_buffer_length, > + urb->transfer_buffer_length, urb) | > TRB_INTR_TARGET(0); > if (urb->transfer_buffer_length > 0) { > if (setup->bRequestType & USB_DIR_IN) > @@ -3673,7 +3629,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, > gfp_t mem_flags, > > td = urb_priv->td[i]; > for (j = 0; j < trbs_per_td; j++) { > - u32 remainder = 0; > field = 0; > > if (first_trb) { > @@ -3726,17 +3681,9 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, > gfp_t mem_flags, > trb_buff_len = td_remain_len; > > /* Set the TRB length, TD size, & interrupter fields. */ > - if (xhci->hci_version < 0x100) { > - remainder = xhci_td_remainder( > - td_len - running_total); > - } else { > - remainder = xhci_v1_0_td_remainder( > - running_total, trb_buff_len, > - total_packet_count, urb, > - (trbs_per_td - j - 1)); > - } > length_field = TRB_LEN(trb_buff_len) | > - remainder | > + xhci_td_remainder(running_total, trb_buff_len, > + td_len, urb) | > TRB_INTR_TARGET(0); > > queue_trb(xhci, ep_ring, more_trbs_coming, > -- > 1.7.12.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
