Re: [PATCH v2] usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg transfers

[Mathias Nyman <mathias.nyman@linux.intel.com>]

Hi

On 30.06.2015 04:54, Reyad Attiyat wrote:
> Hey Mathias,
> 
> The intention is to send an extra endpoint packet of length zero as my
> wireless card needs this to function properly. I have skimmed through
> the xhci spec and assumed that each td would generate a packet. That
> is why I do not chain the last trb or add a interrupt flag, since I
> don't want to call the urb completion function called twice or called
> with the incorrect td or length.

I just found in xhci 1.0 spec section 4.9.1 that "To generate a zero-length
USB transaction software shall explicitly define a TD with a single transfer
TRB, and its TRB transfer length field shall equal 0"

So with this in mind your approach was correct, we shouldn't chain the last
data containing TRB with the zero TRB. This way xhci treats it as a separate TD.

Xhci controller thinks we have two TDs, while the driver only sees one TD.
This TD will interrupt in the middle, and has transferred all data before its last TRB.
I suspect that this may cause some issues, especially if we stop at the zero trb
and the driver has already returned the URB before the last TRB is handled.  

If we continue with this option we need to make sure handle_tx_events(),
process_bulk_intr_td() and finish_td() work with with a SUCCESS bulk transfer
event in the middle of a TD. and that an transfer event for the zero transfer
received after URB is already returned doesn't mess anything up.

As the xhci specs in 4.9.1 requires us to define a TD with a single TRB for
the zero-packet, I think it would be better to add an additional TD to the bulk URB.

Then we should check if we need a zero transfer already in xchi_urb_enqueue(),
and make sure it allocates one more TD (doing size++ should be enough), and make sure
xhci_queue_bulk_tx() can handle bulk URBs with two TDs.

> 
> I have since tried a patch that just chains the trbs together, with
> the zero-length trb, and this still creates a zero-length packet. I
> was thinking I could remove the use of the last_trb variable I was
> using and simply chain all the trbs together and place the interupt
> flag on the zero-length trb if it exsits. Also I noticed that the
> other host controller drivers (ehci and ohci) check to ensure that the
> endpoint is sending data out and that the urb length is greater than
> zero. I will add these checks as well to keep in line with the their
> implementation.
> 

Adding the direction out check would be good.

> Do you think this is the best method for creating a zero-length
> packet, will every trb convert into at least one endpoint packet?

I think adding a new TD for the zero length transfer would be the best option.
This way we follow the specs. I started looking at zero-packet bulk issue
only after your first patch, so there might be many things I haven't taken into
consideration yet.

-Mathias