This commmit checks for the URB_ZERO_PACKET flag and creates an extra
zero-length td if the urb transfer length is a multiple of the endpoint's
max packet length.
Signed-off-by: Reyad Attiyat <[email protected]>
---
drivers/usb/host/xhci-ring.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7d34cbf..3d57a7a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3040,7 +3040,9 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
int num_sgs;
int trb_buff_len, this_sg_len, running_total;
unsigned int total_packet_count;
+ bool zero_length_needed;
bool first_trb;
+ int last_trb;
u64 addr;
bool more_trbs_coming;
@@ -3056,6 +3058,14 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
total_packet_count = DIV_ROUND_UP(urb->transfer_buffer_length,
usb_endpoint_maxp(&urb->ep->desc));
+ /* Deal with URB_ZERO_PACKET - need one more td/trb */
+ zero_length_needed = (urb->transfer_flags & URB_ZERO_PACKET)
+ && !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc));
+ if(zero_length_needed){
+ num_trbs++;
+ xhci_dbg(xhci, "Creating zero length td.\n");
+ }
+
trb_buff_len = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
num_trbs, urb, 0, mem_flags);
@@ -3092,6 +3102,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
trb_buff_len = urb->transfer_buffer_length;
first_trb = true;
+ last_trb = zero_length_needed ? 2 : 1;
/* Queue the first TRB, even if it's zero-length */
do {
u32 field = 0;
@@ -3109,12 +3120,13 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
/* Chain all the TRBs together; clear the chain bit in the last
* TRB to indicate it's the last TRB in the chain.
*/
- if (num_trbs > 1) {
+ if (num_trbs > last_trb) {
field |= TRB_CHAIN;
- } else {
- /* FIXME - add check for ZERO_PACKET flag before this */
+ } else if (num_trbs == last_trb) {
td->last_trb = ep_ring->enqueue;
field |= TRB_IOC;
+ } else if (zero_length_needed && num_trbs == 1) {
+ trb_buff_len = 0;
}
/* Only set interrupt on short packet for IN endpoints */
@@ -3176,7 +3188,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
if (running_total + trb_buff_len > urb->transfer_buffer_length)
trb_buff_len =
urb->transfer_buffer_length - running_total;
- } while (running_total < urb->transfer_buffer_length);
+ } while (num_trbs > 0);
check_trb_math(urb, num_trbs, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
@@ -3194,7 +3206,9 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
int num_trbs;
struct xhci_generic_trb *start_trb;
bool first_trb;
+ int last_trb;
bool more_trbs_coming;
+ bool zero_length_needed;
int start_cycle;
u32 field, length_field;
@@ -3225,7 +3239,14 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
num_trbs++;
running_total += TRB_MAX_BUFF_SIZE;
}
- /* FIXME: this doesn't deal with URB_ZERO_PACKET - need one more */
+
+ /* Deal with URB_ZERO_PACKET - need one more td/trb */
+ zero_length_needed = (urb->transfer_flags & URB_ZERO_PACKET)
+ && !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc));
+ if(zero_length_needed){
+ num_trbs++;
+ xhci_dbg(xhci, "Creating zero length td.\n");
+ }
ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
@@ -3255,7 +3276,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
trb_buff_len = urb->transfer_buffer_length;
first_trb = true;
-
+ last_trb = zero_length_needed ? 2 : 1;
/* Queue the first TRB, even if it's zero-length */
do {
u32 remainder = 0;
@@ -3272,12 +3293,14 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
/* Chain all the TRBs together; clear the chain bit in the last
* TRB to indicate it's the last TRB in the chain.
*/
- if (num_trbs > 1) {
+
+ if (num_trbs > last_trb) {
field |= TRB_CHAIN;
- } else {
- /* FIXME - add check for ZERO_PACKET flag before this */
+ } else if (num_trbs == last_trb) {
td->last_trb = ep_ring->enqueue;
field |= TRB_IOC;
+ } else if (zero_length_needed && num_trbs == 1) {
+ trb_buff_len = 0;
}
/* Only set interrupt on short packet for IN endpoints */
@@ -3315,7 +3338,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
trb_buff_len = urb->transfer_buffer_length - running_total;
if (trb_buff_len > TRB_MAX_BUFF_SIZE)
trb_buff_len = TRB_MAX_BUFF_SIZE;
- } while (running_total < urb->transfer_buffer_length);
+ } while (num_trbs > 0);
check_trb_math(urb, num_trbs, running_total);
giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
--
2.4.3
Hi
On 29.06.2015 03:53, Reyad Attiyat wrote:
> This commmit checks for the URB_ZERO_PACKET flag and creates an extra
> zero-length td if the urb transfer length is a multiple of the endpoint's
> max packet length.
>
> Signed-off-by: Reyad Attiyat <[email protected]>
> ---
Thanks for the patch.
Generic idea and implementation looks good, there are some opens though
See comments and questions inline.
> drivers/usb/host/xhci-ring.c | 43 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7d34cbf..3d57a7a 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3040,7 +3040,9 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> int num_sgs;
> int trb_buff_len, this_sg_len, running_total;
> unsigned int total_packet_count;
> + bool zero_length_needed;
> bool first_trb;
> + int last_trb;
last_trb isn't a really a good name as it might be confused with td->last_trb.
It's used for different purposes here.
> u64 addr;
> bool more_trbs_coming;
>
> @@ -3056,6 +3058,14 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> total_packet_count = DIV_ROUND_UP(urb->transfer_buffer_length,
> usb_endpoint_maxp(&urb->ep->desc));
>
> + /* Deal with URB_ZERO_PACKET - need one more td/trb */
> + zero_length_needed = (urb->transfer_flags & URB_ZERO_PACKET)
> + && !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc));
Please move the "&&" to end of previous line.
(minor thing but helps readability)
Checkpatch also complains about missing whitespaces in the if () statements.
> + if(zero_length_needed){
> + num_trbs++;
> + xhci_dbg(xhci, "Creating zero length td.\n");
> + }
> +
> trb_buff_len = prepare_transfer(xhci, xhci->devs[slot_id],
> ep_index, urb->stream_id,
> num_trbs, urb, 0, mem_flags);
> @@ -3092,6 +3102,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> trb_buff_len = urb->transfer_buffer_length;
>
> first_trb = true;
> + last_trb = zero_length_needed ? 2 : 1;
> /* Queue the first TRB, even if it's zero-length */
> do {
> u32 field = 0;
> @@ -3109,12 +3120,13 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> /* Chain all the TRBs together; clear the chain bit in the last
> * TRB to indicate it's the last TRB in the chain.
> */
> - if (num_trbs > 1) {
> + if (num_trbs > last_trb) {
> field |= TRB_CHAIN;
> - } else {
> - /* FIXME - add check for ZERO_PACKET flag before this */
> + } else if (num_trbs == last_trb) {
> td->last_trb = ep_ring->enqueue;
> field |= TRB_IOC;
> + } else if (zero_length_needed && num_trbs == 1) {
> + trb_buff_len = 0;
> }
Normally chain bits are set for all TRBs except the last TRB, and the IOC (interrupt on completion)
is usually set for only the last TRB.
In case last_trb == 2, the chain bit is now not set between the TRB containing the last data
and the actual last zero TRB, which is the last TRB in the TD.
It now also sets the interrupt on completion (IOC) for the TRB with the last data,
but not for the final last, zero lengt TRB in the TD.
Is this intentional and how we want zero packet bulk transfers to behave?
-Mathias
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 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.
Do you think this is the best method for creating a zero-length
packet, will every trb convert into at least one endpoint packet?
Thank you,
Reyad Attiyat
On Mon, Jun 29, 2015 at 10:48 AM, Mathias Nyman <[email protected]> wrote:
> Hi
>
> On 29.06.2015 03:53, Reyad Attiyat wrote:
>> This commmit checks for the URB_ZERO_PACKET flag and creates an extra
>> zero-length td if the urb transfer length is a multiple of the endpoint's
>> max packet length.
>>
>> Signed-off-by: Reyad Attiyat <[email protected]>
>> ---
>
> Thanks for the patch.
> Generic idea and implementation looks good, there are some opens though
> See comments and questions inline.
>
>> drivers/usb/host/xhci-ring.c | 43 +++++++++++++++++++++++++++++++++----------
>> 1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 7d34cbf..3d57a7a 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -3040,7 +3040,9 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>> int num_sgs;
>> int trb_buff_len, this_sg_len, running_total;
>> unsigned int total_packet_count;
>> + bool zero_length_needed;
>> bool first_trb;
>> + int last_trb;
>
> last_trb isn't a really a good name as it might be confused with td->last_trb.
> It's used for different purposes here.
>
>> u64 addr;
>> bool more_trbs_coming;
>>
>> @@ -3056,6 +3058,14 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>> total_packet_count = DIV_ROUND_UP(urb->transfer_buffer_length,
>> usb_endpoint_maxp(&urb->ep->desc));
>>
>> + /* Deal with URB_ZERO_PACKET - need one more td/trb */
>> + zero_length_needed = (urb->transfer_flags & URB_ZERO_PACKET)
>> + && !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc));
>
> Please move the "&&" to end of previous line.
> (minor thing but helps readability)
>
> Checkpatch also complains about missing whitespaces in the if () statements.
>
>> + if(zero_length_needed){
>> + num_trbs++;
>> + xhci_dbg(xhci, "Creating zero length td.\n");
>> + }
>> +
>> trb_buff_len = prepare_transfer(xhci, xhci->devs[slot_id],
>> ep_index, urb->stream_id,
>> num_trbs, urb, 0, mem_flags);
>> @@ -3092,6 +3102,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>> trb_buff_len = urb->transfer_buffer_length;
>>
>> first_trb = true;
>> + last_trb = zero_length_needed ? 2 : 1;
>> /* Queue the first TRB, even if it's zero-length */
>> do {
>> u32 field = 0;
>> @@ -3109,12 +3120,13 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>> /* Chain all the TRBs together; clear the chain bit in the last
>> * TRB to indicate it's the last TRB in the chain.
>> */
>> - if (num_trbs > 1) {
>> + if (num_trbs > last_trb) {
>> field |= TRB_CHAIN;
>> - } else {
>> - /* FIXME - add check for ZERO_PACKET flag before this */
>> + } else if (num_trbs == last_trb) {
>> td->last_trb = ep_ring->enqueue;
>> field |= TRB_IOC;
>> + } else if (zero_length_needed && num_trbs == 1) {
>> + trb_buff_len = 0;
>> }
>
> Normally chain bits are set for all TRBs except the last TRB, and the IOC (interrupt on completion)
> is usually set for only the last TRB.
>
> In case last_trb == 2, the chain bit is now not set between the TRB containing the last data
> and the actual last zero TRB, which is the last TRB in the TD.
> It now also sets the interrupt on completion (IOC) for the TRB with the last data,
> but not for the final last, zero lengt TRB in the TD.
>
> Is this intentional and how we want zero packet bulk transfers to behave?
>
> -Mathias
>
>
>
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