Re: [PATCH] BugFix in XHCI controller driver for scatter gather DMA

[Mathias Nyman]

On 23.12.2015 13:58, Vikas Bansal wrote:
> From: Sumit Batra <sumit.batra@xxxxxxxxxxx>
>
> Pre-Condition
> URB with Scatter Gather list is queued to bulk OUT endpoint.
> Every buffer in scatter gather list is not a multiple of maximum packet
> size for that endpoint(short packet).
> CHAIN bit is set for all TRBs in a TD so that the DMA happens to all of
> them at once.
>
> Issue
> DMA operation copies all the CHAINED TRBs at contiguous device memory.
> But since the original packet was a short packet, so the actual data is
> re-aligned after this DMA operation.
> At device end this re-aligned data causes data integrity issue with
> applications like ICMP ping.
>
> Solution
> Don't set the CHAINED bit for these TRBs, if their buffers are not a
> multiple of maximum packet size.
> This will reduce the benefit in throughput as required from a scatter
> gather implementation, but this reduces the CPU utilization.
> And solves the data integrity issue on Device End
>
>
> Signed-off-by: Sumit Batra <sumit.batra@xxxxxxxxxxx>
> Signed-off-by: Vikas Bansal <vikas.bansal@xxxxxxxxxxx>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7d34cbf..7363dee 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3110,7 +3110,9 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>   		 * TRB to indicate it's the last TRB in the chain.
>   		 */
>   		if (num_trbs > 1) {
> -			field |= TRB_CHAIN;
> +			if (this_sg_len %
> +					usb_endpoint_maxp(&urb->ep->desc) == 0)
> +				field |= TRB_CHAIN;
>   		} else {
>   			/* FIXME - add check for ZERO_PACKET flag before this */

Hi

I don't fully understand the issue here yet, and I need to look into this more.
I believe removing the CHAIN bit from a TRB mid TD would make the xhc interpret
it as two separate TDs, and in this case the TD size fields of the TRBs will be wrong.

xhci supports Scatter/Gather transfers where the TRBs of a Multi-TRB TD have different
length fields. specs say "(xhc) form a concatenated data buffer from separate buffers
that reside in memory. If the Transfer Ring was associated with an OUT Endpoint then the
concatenated data buffer would be sent to the USB Device as single transfer"

"Note that no constraints are placed on the TRB Length fields in a Scatter/Gather list.
Classically all the buffers pointed to by a scatter gather list were required to be “page size”
in length except for the first and last (as illustrated by the example above).
The xHCI does not require this constraint. Any buffer pointed to by a Normal, Data Stage,
or Isoch TRB in a TD may be any size between 0 and 64K bytes in size."

"Note: A USB packet may be comprised of the data from many TRBs, or many USB packets may be
required to transfer a single TRB.
Note: No relationship is assumed between USB packet boundaries and TRB data buffer boundaries."

Is the case here that a TRB Length field of a Scatter/Father is less than max packet size,
or just not aligned with max packet size boundaries?

Is it possible this is about TD fragments? xhci has some requirements on how TDs
should be fragmented, it's possible the driver doesn't live up to all these requirements.

See xhci specs section 4.11.7.1, TD Fragments
  
I need to dig into this after the holidays,
I'll be back 7 January 2016.

Don't be afraid to ping me about this issue after that.

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