Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1173596 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg transfers Reyad Attiyat <reyad.attiyat@gmail.com> - 2015-06-29 03:00 +0200
    Re: [PATCH v2] usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg  transfers Mathias Nyman <mathias.nyman@intel.com> - 2015-06-29 17:50 +0200
      Re: [PATCH v2] usb: xhci: Add support for URB_ZERO_PACKET to bulk/sg transfers Reyad Attiyat <reyad.attiyat@gmail.com> - 2015-06-30 04:00 +0200

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

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 <reyad.attiyat@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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 <reyad.attiyat@gmail.com>
> ---

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1174185

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 <mathias.nyman@intel.com> 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 <reyad.attiyat@gmail.com>
>> ---
>
> 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
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web