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

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

[Vikas Bansal]

From: Sumit Batra <sumit.batra@samsung.com> 

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@samsung.com>
Signed-off-by: Vikas Bansal <vikas.bansal@samsung.com>
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 */
 			td->last_trb = ep_ring->enqueue;

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

[Sergei Shtylyov]

Hello.

On 12/21/2015 03:46 PM, Vikas Bansal wrote:

> From: Sumit Batra <sumit.batra@samsung.com>
>
> 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@samsung.com>
> Signed-off-by: Vikas Bansal <vikas.bansal@samsung.com>
> 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)

    Indent with 2 extra tabs ISO 1 please -- it'll be easier on the eyes 
(blends with the next statement otherwise).

> +				field |= TRB_CHAIN;
>   		} else {
>   			/* FIXME - add check for ZERO_PACKET flag before this */
>   			td->last_trb = ep_ring->enqueue;

MBR, Sergei

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

[Vikas Bansal]

From: Sumit Batra <sumit.batra@samsung.com> 

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@samsung.com>
Signed-off-by: Vikas Bansal <vikas.bansal@samsung.com>
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 */
 			td->last_trb = ep_ring->enqueue;

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@samsung.com>
>
> 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@samsung.com>
> Signed-off-by: Vikas Bansal <vikas.bansal@samsung.com>
> 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  

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

[Vikas Bansal]

From: Sumit Batra <sumit.batra@samsung.com> 

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@samsung.com>
Signed-off-by: Vikas Bansal <vikas.bansal@samsung.com>
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7d34cbf..862d7cd 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3110,7 +3110,8 @@ 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 */
 			td->last_trb = ep_ring->enqueue;

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

[Sergei Shtylyov]

Hello.

On 12/21/2015 3:15 PM, Vikas Bansal wrote:

> From: Sumit Batra <sumit.batra@samsung.com>
>
> 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@samsung.com>
> Signed-off-by: Vikas Bansal <vikas.bansal@samsung.com>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7d34cbf..862d7cd 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3110,7 +3110,8 @@ 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)

    Space needed after *if*. Please run your patches thru scripts/checkpatch.pl.

[...]

MBR, Sergei

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

[Vikas Bansal]

Pre-Condition
At the time of reset resume of a USB device, both self and bus powered devices might go to a low power state or power off state depending on the acceptable suspend time power of the system.    
In case the device experiences a power glitch or completely powers off during suspend-resume, the device will lose its internal state and hence it'll again need a set interface from class driver on reset resume operation.

Issue 
So far our experiments were based on a USB gadget working on cdc_eem protocol. 
We have seen that device is unable to continue the packet transfer on bulk endpoints after the reset resume operation.

Solution
We have added a .reset_resume function for cdc_eem protocol which sends a set interface command on the Control endpoint. And calls the existing usbnet_resume thereafter


Signed-off-by: Vikas Bansal <vikas.bansal@samsung.com>
Signed-off-by: Sumit Batra <sumit.batra@samsung.com>
diff --git a/drivers/net/usb/cdc_eem.c b/drivers/net/usb/cdc_eem.c
index f7180f8..1f6f7ea 100644
--- a/drivers/net/usb/cdc_eem.c
+++ b/drivers/net/usb/cdc_eem.c
@@ -342,6 +342,19 @@ next:
 	return 1;
 }
 
+static int cdc_eem_resume(struct usb_interface *intf)
+{
+	int ret = 0;
+	struct usbnet *dev = usb_get_intfdata(intf);
+
+	ret = usbnet_get_endpoints(dev, intf);
+	if(ret < 0)
+		goto err;
+	ret = usbnet_resume(intf);
+err:
+	return ret;
+}
+
 static const struct driver_info eem_info = {
 	.description =	"CDC EEM Device",
 	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT,
@@ -371,6 +384,7 @@ static struct usb_driver eem_driver = {
 	.disconnect =	usbnet_disconnect,
 	.suspend =	usbnet_suspend,
 	.resume =	usbnet_resume,
+	.reset_resume =	cdc_eem_resume,
 	.disable_hub_initiated_lpm = 1,
 };
 

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

[Oliver Neukum]

On Mon, 2015-12-21 at 17:34 +0530, Vikas Bansal wrote:
> Pre-Condition
> At the time of reset resume of a USB device, both self and bus powered devices might go to a low power state or power off state depending on the acceptable suspend time power of the system.    
> In case the device experiences a power glitch or completely powers off during suspend-resume, the device will lose its internal state and hence it'll again need a set interface from class driver on reset resume operation.
> 
> Issue 
> So far our experiments were based on a USB gadget working on cdc_eem protocol. 
> We have seen that device is unable to continue the packet transfer on bulk endpoints after the reset resume operation.
> 
> Solution
> We have added a .reset_resume function for cdc_eem protocol which sends a set interface command on the Control endpoint. And calls the existing usbnet_resume thereafte

Hi,

something is wrong with your subject line. Could you fix
that and resend?

	Regards
		Oliver