Re: Endpoint is not halted

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sarah,

This patch works for me. Would you please help submit and merge this patch ?

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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux