On Fri, Oct 26, 2012 at 02:50:20PM +0800, Shimmer Huang wrote:
> Sarah,
>
> We've found the TD_size issue when developing a new XHCI host controller also:
> 1. need fix xhci_td_remainder() and xhci_v1_0_td_remainder()
What needed to be fixed in xhci_td_remainder()?
> 2. we need to use DIV_ROUND_UP() instead of roundup() when we
> calculating total_packet_count .
> As in recent kernel versions, roundup() is defined as follow
> #define roundup(x, y) ( \
> { \
> const typeof(y) __y = y; \
> (((x) + (__y - 1)) / __y) * __y; \
> } \
> )
>
> And DIV_ROUND_UP is defined as
> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
>
> The function roundup() is used several times in xhci-ring.c to
> calculate the number of packets needed:
> total_packet_count = roundup(td_len,
> usb_endpoint_maxp(&urb->ep->desc));
>
> total_packet_count = roundup(urb->transfer_buffer_length,
> usb_endpoint_maxp(&urb->ep->desc));
Yes, you're right about needing to use DIV_ROUND_UP() instead of
roundup(). Obviously I didn't test this patch very well. :-/
Chintan, I've updated that branch with the DIV_ROUND_UP() fix.
Please test with that instead, by running:
git fetch orgin
git reset --hard for-usb-linus-queue
And then recompile and try it out.
Shimmer, the fix diff is attached. Please look it over and see if I've
missed any of the TD size fixes you found during your xHCI host bring
up. Did you happen to find any other issues in your testing?
Sarah Sharp
>
> On Fri, Oct 26, 2012 at 7:25 AM, Sarah Sharp
> <sarah.a.sharp@xxxxxxxxxxxxxxx> wrote:
> > Hi Chintan,
> >
> > I think I have a fix for the TD size issue. Can you install a custom
> > kernel and test it out on your host controller?
> >
> > The directions for building a custom kernel are here:
> > http://kernelnewbies.org/KernelBuild
> >
> > Instead of running any of the commands in "Which kernel to build?"
> > section, use these commands instead:
> >
> > git clone git://git.kernel.org/pub/scm/linux/kernel/git/sarah/xhci.git -b for-usb-linus-queue
> > cd xhci
> >
> > Use the "Duplicating your current config" section.
> >
> > If you have trouble booting the 3.7-rc2 kernel, let me know and I'll
> > rebase the patch against a stable kernel version.
> >
> > Sarah Sharp
> >
> >
> > On Thu, Oct 25, 2012 at 03:32:08PM -0700, Sarah Sharp wrote:
> >> Going back over your example, it does look there is a couple bugs in the
> >> Linux xHCI TD size calculations. Notes are below, I'll send you a patch
> >> to test out on your host controller shortly.
> >>
> >> Thanks for catching this!
> >>
> >> Sarah Sharp
> >>
> >> On Thu, Oct 25, 2012 at 02:24:04PM -0700, Sarah Sharp wrote:
> >> > On Fri, Oct 19, 2012 at 11:29:44AM +0530, Chintan Mehta wrote:
> >> > > > > > 2. For Bulk Endpoint:
> >> > > > > >
> >> > > > > > - *Driver can put a TD with total TD transfer size less than
> >> > > > maxpacket
> >> > > > > > size and more than 1 TRB?*
> >> > > > > > - For example, Maxpacketsize is 1K. And TD contains 3 TRBs as below:
> >> > > > > > - 1st trb with TRB transfer length 600 Bytes, chain bit 1 and
> >> > > > > > TDSize 0
> >> > > > > > - 2nd trb with TRB transfer length 200 Bytes, chain bit 1 and
> >> > > > > > TDSize 0
> >> > > > > > - 3rd trb with TRB transfer length 100 Bytes, chain bit 0 and
> >> > > > > > TDSize 0
> >> > > > > > - *What should be the value of TDSize in above TRBs of TD?*
> >> > > >
> >> > > > Again, see section 4.11.2.4.
> >> > > >
> >> > > > TRB 1 600 (600 + 200 + 100) >> 10 = 0
> >> > > > TRB 2 200 (200 + 100) >> 10 = 0
> >> > > > TRB 3 100 (100) >> 10 = 0
> >>
> >> Let's see what the TD size for a 1.0 host controller should be here.
> >>
> >> TD packet count =
> >> roundup(TD size / max packet size) =
> >> roundup(900 / 1024) = 1
> >>
> >> Packets Transferred (TRB 1) =
> >> rounddown(TRB length sum(n) / max packet size)
> >>
> >> where TRB length sum is the sum of the trb lengths up to and including
> >> this TRB, so
> >>
> >> Packets Transferred (TRB 1) = rounddown(600 / 1024) = 0
> >>
> >> TD size = (TD packet count - Packets Transferred)
> >>
> >> Therefore,
> >>
> >> TD size(TRB 1) = (1 - 0) = 1
> >>
> >> Packets Transferred (TRB 2) =
> >> rounddown((600 + 200) / 1024) = 0
> >> TD size(TRB 2) = (1 - 0) = 1
> >>
> >> The TD size for TRB 3 is supposed to be set to 0, since it is the last
> >> TRB in the TD.
> >>
> >> So, the final answer should be
> >> TRB 1: TD size = 1
> >> TRB 2: TD size = 1
> >> TRB 3: TD size = 0
> >>
> >> Now let's see what the xHCI driver actually does.
> >>
> >> 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;
> >> }
> >>
> >> static u32 xhci_v1_0_td_remainder(int running_total, int trb_buff_len,
> >> unsigned int total_packet_count, struct urb *urb)
> >> {
> >> int packets_transferred;
> >>
> >> /* One TRB with a zero-length data packet. */
> >> if (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) /
> >> usb_endpoint_maxp(&urb->ep->desc);
> >>
> >> return xhci_td_remainder(total_packet_count - packets_transferred);
> >> }
> >>
> >> That doesn't look right from the start, because passing the result to
> >> xhci_td_remainder() will left shift it by 10, which isn't what we want.
> >> I'll assume I've fixed that, and make sure the math is right from there.
> >>
> >> The total_packet_count passed to xhci_v1_0_td_remainder() looks sane,
> >> looking at how it's calculated in the isochronous and bulk queueing
> >> functions (which also handles the interrupt TD queueing).
> >>
> >> running_total is the number of bytes in the previous TRBs (not including
> >> this TRB), and trb_buff_len is the number of bytes in this TRB.
> >>
> >> So, for the first TRB, running_total = 0, trb_buff_len = 600, and
> >> total_packet_count = 1.
> >> packets_transferred = (0 + 600) / 1024 = 0
> >> TD size (TRB 1) = (1 - 0) = 1
> >>
> >> TRB 2:
> >> packets_transferred = (600 + 200) / 1024 = 0
> >> TD size (TRB 2) = (1 - 0) = 1
> >>
> >> TRB 3:
> >> packets_transferred = (600 + 200 + 100) / 1024 = 0
> >> TD size (TRB 3) = (1 - 0) = 1
> >>
> >> That last TD size is wrong, of course, since the xHCI spec says it has
> >> to be special-cased. Probably the URB enqueueing functions should
> >> special case that, since they know whether this this the last TRB in a
> >> TD.
> >>
> >> So, yes, there are two bugs in the Linux xHCI TD size code, and I'll
> >> send you a patch shortly to fix it.
> >>
> >> Sarah Sharp
> >> --
> >> 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
>
>
>
> --
> - Shimmer
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 77f1ace..cbb44b7 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3071,11 +3071,11 @@ static u32 xhci_td_remainder(unsigned int remainder)
}
/*
- * For xHCI 1.0 host controllers, TD size is the number of packets remaining in
- * the TD (*not* including this TRB).
+ * 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 =
- * roundup(TD size in bytes / wMaxPacketSize)
+ * DIV_ROUND_UP(TD size in bytes / wMaxPacketSize)
*
* Packets transferred up to and including this TRB = packets_transferred =
* rounddown(total bytes transferred including this TRB / wMaxPacketSize)
@@ -3083,15 +3083,16 @@ static u32 xhci_td_remainder(unsigned int remainder)
* 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.
*/
-
static u32 xhci_v1_0_td_remainder(int running_total, int trb_buff_len,
- unsigned int total_packet_count, struct urb *urb)
+ unsigned int total_packet_count, struct urb *urb,
+ unsigned int num_trbs_left)
{
int packets_transferred;
/* One TRB with a zero-length data packet. */
- if (running_total == 0 && trb_buff_len == 0)
+ 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
@@ -3100,7 +3101,9 @@ static u32 xhci_v1_0_td_remainder(int running_total, int trb_buff_len,
packets_transferred = (running_total + trb_buff_len) /
usb_endpoint_maxp(&urb->ep->desc);
- return xhci_td_remainder(total_packet_count - packets_transferred);
+ if ((total_packet_count - packets_transferred) > 31)
+ return 31 << 17;
+ return (total_packet_count - packets_transferred) << 17;
}
static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
@@ -3127,7 +3130,7 @@ 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 = roundup(urb->transfer_buffer_length,
+ 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],
@@ -3210,7 +3213,8 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
running_total);
} else {
remainder = xhci_v1_0_td_remainder(running_total,
- trb_buff_len, total_packet_count, urb);
+ trb_buff_len, total_packet_count, urb,
+ num_trbs - 1);
}
length_field = TRB_LEN(trb_buff_len) |
remainder |
@@ -3318,7 +3322,7 @@ 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 = roundup(urb->transfer_buffer_length,
+ 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;
@@ -3364,7 +3368,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
running_total);
} else {
remainder = xhci_v1_0_td_remainder(running_total,
- trb_buff_len, total_packet_count, urb);
+ trb_buff_len, total_packet_count, urb,
+ num_trbs - 1);
}
length_field = TRB_LEN(trb_buff_len) |
remainder |
@@ -3627,7 +3632,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
addr = start_addr + urb->iso_frame_desc[i].offset;
td_len = urb->iso_frame_desc[i].length;
td_remain_len = td_len;
- total_packet_count = roundup(td_len,
+ total_packet_count = DIV_ROUND_UP(td_len,
usb_endpoint_maxp(&urb->ep->desc));
/* A zero-length transfer still involves at least one packet. */
if (total_packet_count == 0)
@@ -3706,7 +3711,8 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
} else {
remainder = xhci_v1_0_td_remainder(
running_total, trb_buff_len,
- total_packet_count, urb);
+ total_packet_count, urb,
+ (trbs_per_td - j - 1));
}
length_field = TRB_LEN(trb_buff_len) |
remainder |
