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

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

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