On Wed, Oct 31, 2012 at 12:31:43PM +0800, Shimmer Huang wrote:
> Sarah,
>
> This patch works for me. Would you please help submit and merge this patch ?
Yep, I'll get it merged shortly. It will be marked for stable kernels,
so it should make it into the Linux distribution kernels as well.
Sarah Sharp
>
> On Sat, Oct 27, 2012 at 1:51 AM, Sarah Sharp
> <sarah.a.sharp@xxxxxxxxxxxxxxx> wrote:
> > 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, this patch contains all the fixes we found in our bringup.
> We will continue to work with Linux xHCI driver and will report any issues
> we found in xHCI driver to you and the mailing list. Thanks.
>
> >
> > 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
>
>
>
> --
> - Shimmer
--
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
