[RFC] usb: xhci: add Immediate Data Transfers support

[RFC] usb: xhci: add Immediate Data Transfers support

[Nicolas Saenz Julienne]

Immediate data transfers (IDT) allow the HCD to copy small chunks of
data (up to 8bits) directly into its output transfer TRBs. This avoids
the somewhat expensive DMA mappings that are performed by default on
most URBs submissions.

In the case an URB was suitable for IDT. The data is directly copied
into the "Data Buffer Pointer" region of the TRB and the IDT flag is
set. Instead of triggering memory accesses the HC will use the data
directly.

An additional URB flag was created to mark whenever the URB doesn't need
any DMA mapping. Ideally it would have been nice to use a private flag
as this is specific to XHCI. Yet it's not possible as the URB private
area is allocated only after the DMA mapping is done.

Isochronous transfers are not implemented so far as it's hard to find a
device that will send such small packets.

This was tested using USB/serial adapter and by controlling the leds on
an XBOX controller. There where no disruptions on the rest of USB
devices attached on the system.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/usb/host/xhci-ring.c |  6 ++++++
 drivers/usb/host/xhci.c      | 37 ++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h      |  2 ++
 include/linux/usb.h          |  2 ++
 4 files changed, 47 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c4d041..dd9805fb0566 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 			field |= TRB_IOC;
 			more_trbs_coming = false;
 			td->last_trb = ring->enqueue;
+
+			if (urb->transfer_flags & URB_NO_DMA_MAP) {
+				memcpy(&send_addr, urb->transfer_buffer,
+				       full_len);
+				field |= TRB_IDT;
+			}
 		}
 
 		/* Only set interrupt on short packet for IN endpoints */
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608..ce3b6663f940 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
 
 /*-------------------------------------------------------------------------*/
 
+static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+	if (urb->transfer_flags & URB_NO_DMA_MAP)
+		urb->transfer_flags &= ~URB_NO_DMA_MAP;
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
+static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+				gfp_t mem_flags)
+{
+	int maxp = usb_endpoint_maxp(&urb->ep->desc);
+	int len = urb->transfer_buffer_length;
+	int ret = 0;
+
+	/*
+	 * Checks if URB is suitable for Immediate Transfer (IDT): instead of
+	 * mapping the buffer for DMA and passing the address to the host
+	 * controller, we copy the actual data into the TRB address register.
+	 * This is limited to transfers up to 8 bytes.
+	 *
+	 * IDT is only supported for Bulk and Interrupt endpoints. Apart from
+	 * the size constraints special care is taken to avoid cases where
+	 * wMaxPacketSize is smaller than 8 bytes as it's not supported.
+	 */
+	if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
+	    usb_endpoint_is_bulk_out(&urb->ep->desc)) &&
+	    maxp >= TRB_IDT_MAX_SIZE && len <= TRB_IDT_MAX_SIZE)
+		urb->transfer_flags |= URB_NO_DMA_MAP;
+	else
+		ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+
+	return ret;
+}
+
 /**
  * xhci_get_endpoint_index - Used for passing endpoint bitmasks between the core and
  * HCDs.  Find the index for an endpoint given its descriptor.  Use the return
@@ -5155,6 +5190,8 @@ static const struct hc_driver xhci_hc_driver = {
 	/*
 	 * managing i/o requests and associated device resources
 	 */
+	.map_urb_for_dma =      xhci_map_urb_for_dma,
+	.unmap_urb_for_dma =    xhci_unmap_urb_for_dma,
 	.urb_enqueue =		xhci_urb_enqueue,
 	.urb_dequeue =		xhci_urb_dequeue,
 	.alloc_dev =		xhci_alloc_dev,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36e3012..1b51999794b3 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
 #define TRB_IOC			(1<<5)
 /* The buffer pointer contains immediate data */
 #define TRB_IDT			(1<<6)
+/* TDs smaller 64bits this might use immediate data */
+#define TRB_IDT_MAX_SIZE	8
 
 /* Block Event Interrupt */
 #define	TRB_BEI			(1<<9)
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 5e49e82c4368..1a6b2dfe7c72 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1331,6 +1331,8 @@ extern int usb_disabled(void);
 #define URB_SETUP_MAP_LOCAL	0x00200000	/* HCD-local setup packet */
 #define URB_DMA_SG_COMBINED	0x00400000	/* S-G entries were combined */
 #define URB_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
+#define URB_NO_DMA_MAP		0x01000000	/* Data directly transferred to
+						 * HC */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;
-- 
2.20.1

[RFC] usb: xhci: add Immediate Data Transfers support

[Nicolas Saenz Julienne]

Immediate data transfers (IDT) allow the HCD to copy small chunks of
data (up to 8bits) directly into its output transfer TRBs. This avoids
the somewhat expensive DMA mappings that are performed by default on
most URBs submissions.

In the case an URB was suitable for IDT. The data is directly copied
into the "Data Buffer Pointer" region of the TRB and the IDT flag is
set. Instead of triggering memory accesses the HC will use the data
directly.

An additional URB flag was created to mark whenever the URB doesn't need
any DMA mapping. Ideally it would have been nice to use a private flag
as this is specific to XHCI. Yet it's not possible as the URB private
area is allocated only after the DMA mapping is done.

Isochronous transfers are not implemented so far as it's hard to find a
device that will send such small packets.

This was tested using USB/serial adapter and by controlling the leds on
an XBOX controller. There where no disruptions on the rest of USB
devices attached on the system.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/usb/host/xhci-ring.c |  6 ++++++
 drivers/usb/host/xhci.c      | 37 ++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h      |  2 ++
 include/linux/usb.h          |  2 ++
 4 files changed, 47 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c4d041..dd9805fb0566 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 			field |= TRB_IOC;
 			more_trbs_coming = false;
 			td->last_trb = ring->enqueue;
+
+			if (urb->transfer_flags & URB_NO_DMA_MAP) {
+				memcpy(&send_addr, urb->transfer_buffer,
+				       full_len);
+				field |= TRB_IDT;
+			}
 		}
 
 		/* Only set interrupt on short packet for IN endpoints */
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608..ce3b6663f940 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
 
 /*-------------------------------------------------------------------------*/
 
+static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+	if (urb->transfer_flags & URB_NO_DMA_MAP)
+		urb->transfer_flags &= ~URB_NO_DMA_MAP;
+	else
+		usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
+static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+				gfp_t mem_flags)
+{
+	int maxp = usb_endpoint_maxp(&urb->ep->desc);
+	int len = urb->transfer_buffer_length;
+	int ret = 0;
+
+	/*
+	 * Checks if URB is suitable for Immediate Transfer (IDT): instead of
+	 * mapping the buffer for DMA and passing the address to the host
+	 * controller, we copy the actual data into the TRB address register.
+	 * This is limited to transfers up to 8 bytes.
+	 *
+	 * IDT is only supported for Bulk and Interrupt endpoints. Apart from
+	 * the size constraints special care is taken to avoid cases where
+	 * wMaxPacketSize is smaller than 8 bytes as it's not supported.
+	 */
+	if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
+	    usb_endpoint_is_bulk_out(&urb->ep->desc)) &&
+	    maxp >= TRB_IDT_MAX_SIZE && len <= TRB_IDT_MAX_SIZE)
+		urb->transfer_flags |= URB_NO_DMA_MAP;
+	else
+		ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+
+	return ret;
+}
+
 /**
  * xhci_get_endpoint_index - Used for passing endpoint bitmasks between the core and
  * HCDs.  Find the index for an endpoint given its descriptor.  Use the return
@@ -5155,6 +5190,8 @@ static const struct hc_driver xhci_hc_driver = {
 	/*
 	 * managing i/o requests and associated device resources
 	 */
+	.map_urb_for_dma =      xhci_map_urb_for_dma,
+	.unmap_urb_for_dma =    xhci_unmap_urb_for_dma,
 	.urb_enqueue =		xhci_urb_enqueue,
 	.urb_dequeue =		xhci_urb_dequeue,
 	.alloc_dev =		xhci_alloc_dev,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36e3012..1b51999794b3 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
 #define TRB_IOC			(1<<5)
 /* The buffer pointer contains immediate data */
 #define TRB_IDT			(1<<6)
+/* TDs smaller 64bits this might use immediate data */
+#define TRB_IDT_MAX_SIZE	8
 
 /* Block Event Interrupt */
 #define	TRB_BEI			(1<<9)
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 5e49e82c4368..1a6b2dfe7c72 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1331,6 +1331,8 @@ extern int usb_disabled(void);
 #define URB_SETUP_MAP_LOCAL	0x00200000	/* HCD-local setup packet */
 #define URB_DMA_SG_COMBINED	0x00400000	/* S-G entries were combined */
 #define URB_ALIGNED_TEMP_BUFFER	0x00800000	/* Temp buffer was alloc'd */
+#define URB_NO_DMA_MAP		0x01000000	/* Data directly transferred to
+						 * HC */
 
 struct usb_iso_packet_descriptor {
 	unsigned int offset;

Re: [RFC] usb: xhci: add Immediate Data Transfers support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4695 bytes --]


Hi,

Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes:
> Immediate data transfers (IDT) allow the HCD to copy small chunks of
> data (up to 8bits) directly into its output transfer TRBs. This avoids
              ^^^^^
              8 bytes

> the somewhat expensive DMA mappings that are performed by default on
> most URBs submissions.
>
> In the case an URB was suitable for IDT. The data is directly copied
> into the "Data Buffer Pointer" region of the TRB and the IDT flag is
> set. Instead of triggering memory accesses the HC will use the data
> directly.
>
> An additional URB flag was created to mark whenever the URB doesn't need
> any DMA mapping. Ideally it would have been nice to use a private flag
> as this is specific to XHCI. Yet it's not possible as the URB private
> area is allocated only after the DMA mapping is done.
>
> Isochronous transfers are not implemented so far as it's hard to find a
> device that will send such small packets.
>
> This was tested using USB/serial adapter and by controlling the leds on
> an XBOX controller. There where no disruptions on the rest of USB
> devices attached on the system.
>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/usb/host/xhci-ring.c |  6 ++++++
>  drivers/usb/host/xhci.c      | 37 ++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/xhci.h      |  2 ++
>  include/linux/usb.h          |  2 ++
>  4 files changed, 47 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 40fa25c4d041..dd9805fb0566 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  			field |= TRB_IOC;
>  			more_trbs_coming = false;
>  			td->last_trb = ring->enqueue;
> +
> +			if (urb->transfer_flags & URB_NO_DMA_MAP) {

do you really need the flag? Why don't you do this unconditionally as
long as urb->transfer_length is <= 8?

> +				memcpy(&send_addr, urb->transfer_buffer,
> +				       full_len);
> +				field |= TRB_IDT;
> +			}
>  		}
>  
>  		/* Only set interrupt on short packet for IN endpoints */
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 005e65922608..ce3b6663f940 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> +{
> +	if (urb->transfer_flags & URB_NO_DMA_MAP)
> +		urb->transfer_flags &= ~URB_NO_DMA_MAP;
> +	else
> +		usb_hcd_unmap_urb_for_dma(hcd, urb);
> +}
> +
> +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> +				gfp_t mem_flags)
> +{
> +	int maxp = usb_endpoint_maxp(&urb->ep->desc);
> +	int len = urb->transfer_buffer_length;
> +	int ret = 0;
> +
> +	/*
> +	 * Checks if URB is suitable for Immediate Transfer (IDT): instead of
> +	 * mapping the buffer for DMA and passing the address to the host
> +	 * controller, we copy the actual data into the TRB address register.
> +	 * This is limited to transfers up to 8 bytes.
> +	 *
> +	 * IDT is only supported for Bulk and Interrupt endpoints. Apart from
> +	 * the size constraints special care is taken to avoid cases where
> +	 * wMaxPacketSize is smaller than 8 bytes as it's not supported.
> +	 */
> +	if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
> +	    usb_endpoint_is_bulk_out(&urb->ep->desc)) &&

I don't understand the check for endpoint type. IDT is, actually,
already used for control endpoints because setup packets are composed of
8 bytes. You're also showing that this works for INT and BULK types. It
would be a surprise if it doesn't work for ISOC.

> +	    maxp >= TRB_IDT_MAX_SIZE && len <= TRB_IDT_MAX_SIZE)

why the maxp check? What if I'm an interrupt endpoint with maxp of 2?
Is there really a limitation that you couldn't use IDT for those?

> +		urb->transfer_flags |= URB_NO_DMA_MAP;

do we really need this? I wonder if returning zero here would be enough,
then you could:

...map_urb_for_dma(...)
{
	ret = 0;

	if (urb->transfer_length > 8)
        	ret = usb_hcd_map_urb_for_dma(hcd, urb, flags);

	return ret;
}

...unmap_urb_for_dma(...)
{
	if urb->transfer_length > 8)
        	usb_hcd_unmap_urb_for_dma(hcd, urb);
}

...xhci_queue_bulk_tx(...)
{
	[...]

	if (urb->transfer_length <= 8) {
		memcpy(&addr, urb->transfer_buffer, 8)l
		field |= TRB_IDT;
        }

	[...]
}

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[RFC] usb: xhci: add Immediate Data Transfers support

[Felipe Balbi]

Hi,

Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes:
> Immediate data transfers (IDT) allow the HCD to copy small chunks of
> data (up to 8bits) directly into its output transfer TRBs. This avoids
              ^^^^^
              8 bytes

> the somewhat expensive DMA mappings that are performed by default on
> most URBs submissions.
>
> In the case an URB was suitable for IDT. The data is directly copied
> into the "Data Buffer Pointer" region of the TRB and the IDT flag is
> set. Instead of triggering memory accesses the HC will use the data
> directly.
>
> An additional URB flag was created to mark whenever the URB doesn't need
> any DMA mapping. Ideally it would have been nice to use a private flag
> as this is specific to XHCI. Yet it's not possible as the URB private
> area is allocated only after the DMA mapping is done.
>
> Isochronous transfers are not implemented so far as it's hard to find a
> device that will send such small packets.
>
> This was tested using USB/serial adapter and by controlling the leds on
> an XBOX controller. There where no disruptions on the rest of USB
> devices attached on the system.
>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/usb/host/xhci-ring.c |  6 ++++++
>  drivers/usb/host/xhci.c      | 37 ++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/xhci.h      |  2 ++
>  include/linux/usb.h          |  2 ++
>  4 files changed, 47 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 40fa25c4d041..dd9805fb0566 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  			field |= TRB_IOC;
>  			more_trbs_coming = false;
>  			td->last_trb = ring->enqueue;
> +
> +			if (urb->transfer_flags & URB_NO_DMA_MAP) {

do you really need the flag? Why don't you do this unconditionally as
long as urb->transfer_length is <= 8?

> +				memcpy(&send_addr, urb->transfer_buffer,
> +				       full_len);
> +				field |= TRB_IDT;
> +			}
>  		}
>  
>  		/* Only set interrupt on short packet for IN endpoints */
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 005e65922608..ce3b6663f940 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> +{
> +	if (urb->transfer_flags & URB_NO_DMA_MAP)
> +		urb->transfer_flags &= ~URB_NO_DMA_MAP;
> +	else
> +		usb_hcd_unmap_urb_for_dma(hcd, urb);
> +}
> +
> +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> +				gfp_t mem_flags)
> +{
> +	int maxp = usb_endpoint_maxp(&urb->ep->desc);
> +	int len = urb->transfer_buffer_length;
> +	int ret = 0;
> +
> +	/*
> +	 * Checks if URB is suitable for Immediate Transfer (IDT): instead of
> +	 * mapping the buffer for DMA and passing the address to the host
> +	 * controller, we copy the actual data into the TRB address register.
> +	 * This is limited to transfers up to 8 bytes.
> +	 *
> +	 * IDT is only supported for Bulk and Interrupt endpoints. Apart from
> +	 * the size constraints special care is taken to avoid cases where
> +	 * wMaxPacketSize is smaller than 8 bytes as it's not supported.
> +	 */
> +	if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
> +	    usb_endpoint_is_bulk_out(&urb->ep->desc)) &&

I don't understand the check for endpoint type. IDT is, actually,
already used for control endpoints because setup packets are composed of
8 bytes. You're also showing that this works for INT and BULK types. It
would be a surprise if it doesn't work for ISOC.

> +	    maxp >= TRB_IDT_MAX_SIZE && len <= TRB_IDT_MAX_SIZE)

why the maxp check? What if I'm an interrupt endpoint with maxp of 2?
Is there really a limitation that you couldn't use IDT for those?

> +		urb->transfer_flags |= URB_NO_DMA_MAP;

do we really need this? I wonder if returning zero here would be enough,
then you could:

...map_urb_for_dma(...)
{
	ret = 0;

	if (urb->transfer_length > 8)
        	ret = usb_hcd_map_urb_for_dma(hcd, urb, flags);

	return ret;
}

...unmap_urb_for_dma(...)
{
	if urb->transfer_length > 8)
        	usb_hcd_unmap_urb_for_dma(hcd, urb);
}

...xhci_queue_bulk_tx(...)
{
	[...]

	if (urb->transfer_length <= 8) {
		memcpy(&addr, urb->transfer_buffer, 8)l
		field |= TRB_IDT;
        }

	[...]
}

Re: [RFC] usb: xhci: add Immediate Data Transfers support

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 5770 bytes --]

Hi Felipe, thanks for the review :)

On Fri, 2019-02-01 at 15:31 +0200, Felipe Balbi wrote:
> Hi,
> 
> Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes:
> > Immediate data transfers (IDT) allow the HCD to copy small chunks of
> > data (up to 8bits) directly into its output transfer TRBs. This avoids
>               ^^^^^
>               8 bytes

Obviously, noted.

> 
> > the somewhat expensive DMA mappings that are performed by default on
> > most URBs submissions.
> > 
> > In the case an URB was suitable for IDT. The data is directly copied
> > into the "Data Buffer Pointer" region of the TRB and the IDT flag is
> > set. Instead of triggering memory accesses the HC will use the data
> > directly.
> > 
> > An additional URB flag was created to mark whenever the URB doesn't need
> > any DMA mapping. Ideally it would have been nice to use a private flag
> > as this is specific to XHCI. Yet it's not possible as the URB private
> > area is allocated only after the DMA mapping is done.
> > 
> > Isochronous transfers are not implemented so far as it's hard to find a
> > device that will send such small packets.
> > 
> > This was tested using USB/serial adapter and by controlling the leds on
> > an XBOX controller. There where no disruptions on the rest of USB
> > devices attached on the system.
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/usb/host/xhci-ring.c |  6 ++++++
> >  drivers/usb/host/xhci.c      | 37 ++++++++++++++++++++++++++++++++++++
> >  drivers/usb/host/xhci.h      |  2 ++
> >  include/linux/usb.h          |  2 ++
> >  4 files changed, 47 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 40fa25c4d041..dd9805fb0566 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> >  			field |= TRB_IOC;
> >  			more_trbs_coming = false;
> >  			td->last_trb = ring->enqueue;
> > +
> > +			if (urb->transfer_flags & URB_NO_DMA_MAP) {
> 
> do you really need the flag? Why don't you do this unconditionally as
> long as urb->transfer_length is <= 8?

There is a set of limitations, see my comments below.

> 
> > +				memcpy(&send_addr, urb->transfer_buffer,
> > +				       full_len);
> > +				field |= TRB_IDT;
> > +			}
> >  		}
> >  
> >  		/* Only set interrupt on short packet for IN endpoints */
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 005e65922608..ce3b6663f940 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
> >  
> >  /*-------------------------------------------------------------------------
> > */
> >  
> > +static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> > +{
> > +	if (urb->transfer_flags & URB_NO_DMA_MAP)
> > +		urb->transfer_flags &= ~URB_NO_DMA_MAP;
> > +	else
> > +		usb_hcd_unmap_urb_for_dma(hcd, urb);
> > +}
> > +
> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > +				gfp_t mem_flags)
> > +{
> > +	int maxp = usb_endpoint_maxp(&urb->ep->desc);
> > +	int len = urb->transfer_buffer_length;
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * Checks if URB is suitable for Immediate Transfer (IDT): instead of
> > +	 * mapping the buffer for DMA and passing the address to the host
> > +	 * controller, we copy the actual data into the TRB address register.
> > +	 * This is limited to transfers up to 8 bytes.
> > +	 *
> > +	 * IDT is only supported for Bulk and Interrupt endpoints. Apart from
> > +	 * the size constraints special care is taken to avoid cases where
> > +	 * wMaxPacketSize is smaller than 8 bytes as it's not supported.
> > +	 */
> > +	if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
> > +	    usb_endpoint_is_bulk_out(&urb->ep->desc)) &&
> 
> I don't understand the check for endpoint type. IDT is, actually,
> already used for control endpoints because setup packets are composed of
> 8 bytes. You're also showing that this works for INT and BULK types. It
> would be a surprise if it doesn't work for ISOC.

Isoc should work, but I've been unable to find a device with such small
transfer size. As the standard says if IDT is set only a Transfer/Isoc TRB is
allowed per TD (see Table 6-22 on the spec). So I guess it makes it unlikely
for an Isoc transfer to match the constraints. That said I'll have a more in
depth look at it.

> 
> > +	    maxp >= TRB_IDT_MAX_SIZE && len <= TRB_IDT_MAX_SIZE)
> 
> why the maxp check? What if I'm an interrupt endpoint with maxp of 2?
> Is there really a limitation that you couldn't use IDT for those?

Same table in specs. As I state in the patch's comment above, IDT with
wMaxPacketSize < 8 it's not supported.

> 
> > +		urb->transfer_flags |= URB_NO_DMA_MAP;
> 
> do we really need this? I wonder if returning zero here would be enough,
> then you could:

I agree that with Isoc included we could lose the flag and implement it as you
state below. Only length and ep's wMaxPacketSize should be checked.

> 
> ...map_urb_for_dma(...)
> {
> 	ret = 0;
> 
> 	if (urb->transfer_length > 8)
>         	ret = usb_hcd_map_urb_for_dma(hcd, urb, flags);
> 
> 	return ret;
> }
> 
> ...unmap_urb_for_dma(...)
> {
> 	if urb->transfer_length > 8)
>         	usb_hcd_unmap_urb_for_dma(hcd, urb);
> }
> 
> ...xhci_queue_bulk_tx(...)
> {
> 	[...]
> 
> 	if (urb->transfer_length <= 8) {
> 		memcpy(&addr, urb->transfer_buffer, 8)l
> 		field |= TRB_IDT;
>         }
> 
> 	[...]
> }
> 

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC] usb: xhci: add Immediate Data Transfers support

[Nicolas Saenz Julienne]

Hi Felipe, thanks for the review :)

On Fri, 2019-02-01 at 15:31 +0200, Felipe Balbi wrote:
> Hi,
> 
> Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes:
> > Immediate data transfers (IDT) allow the HCD to copy small chunks of
> > data (up to 8bits) directly into its output transfer TRBs. This avoids
>               ^^^^^
>               8 bytes

Obviously, noted.

> 
> > the somewhat expensive DMA mappings that are performed by default on
> > most URBs submissions.
> > 
> > In the case an URB was suitable for IDT. The data is directly copied
> > into the "Data Buffer Pointer" region of the TRB and the IDT flag is
> > set. Instead of triggering memory accesses the HC will use the data
> > directly.
> > 
> > An additional URB flag was created to mark whenever the URB doesn't need
> > any DMA mapping. Ideally it would have been nice to use a private flag
> > as this is specific to XHCI. Yet it's not possible as the URB private
> > area is allocated only after the DMA mapping is done.
> > 
> > Isochronous transfers are not implemented so far as it's hard to find a
> > device that will send such small packets.
> > 
> > This was tested using USB/serial adapter and by controlling the leds on
> > an XBOX controller. There where no disruptions on the rest of USB
> > devices attached on the system.
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/usb/host/xhci-ring.c |  6 ++++++
> >  drivers/usb/host/xhci.c      | 37 ++++++++++++++++++++++++++++++++++++
> >  drivers/usb/host/xhci.h      |  2 ++
> >  include/linux/usb.h          |  2 ++
> >  4 files changed, 47 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 40fa25c4d041..dd9805fb0566 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> >  			field |= TRB_IOC;
> >  			more_trbs_coming = false;
> >  			td->last_trb = ring->enqueue;
> > +
> > +			if (urb->transfer_flags & URB_NO_DMA_MAP) {
> 
> do you really need the flag? Why don't you do this unconditionally as
> long as urb->transfer_length is <= 8?

There is a set of limitations, see my comments below.

> 
> > +				memcpy(&send_addr, urb->transfer_buffer,
> > +				       full_len);
> > +				field |= TRB_IDT;
> > +			}
> >  		}
> >  
> >  		/* Only set interrupt on short packet for IN endpoints */
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 005e65922608..ce3b6663f940 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
> >  
> >  /*-------------------------------------------------------------------------
> > */
> >  
> > +static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> > +{
> > +	if (urb->transfer_flags & URB_NO_DMA_MAP)
> > +		urb->transfer_flags &= ~URB_NO_DMA_MAP;
> > +	else
> > +		usb_hcd_unmap_urb_for_dma(hcd, urb);
> > +}
> > +
> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > +				gfp_t mem_flags)
> > +{
> > +	int maxp = usb_endpoint_maxp(&urb->ep->desc);
> > +	int len = urb->transfer_buffer_length;
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * Checks if URB is suitable for Immediate Transfer (IDT): instead of
> > +	 * mapping the buffer for DMA and passing the address to the host
> > +	 * controller, we copy the actual data into the TRB address register.
> > +	 * This is limited to transfers up to 8 bytes.
> > +	 *
> > +	 * IDT is only supported for Bulk and Interrupt endpoints. Apart from
> > +	 * the size constraints special care is taken to avoid cases where
> > +	 * wMaxPacketSize is smaller than 8 bytes as it's not supported.
> > +	 */
> > +	if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
> > +	    usb_endpoint_is_bulk_out(&urb->ep->desc)) &&
> 
> I don't understand the check for endpoint type. IDT is, actually,
> already used for control endpoints because setup packets are composed of
> 8 bytes. You're also showing that this works for INT and BULK types. It
> would be a surprise if it doesn't work for ISOC.

Isoc should work, but I've been unable to find a device with such small
transfer size. As the standard says if IDT is set only a Transfer/Isoc TRB is
allowed per TD (see Table 6-22 on the spec). So I guess it makes it unlikely
for an Isoc transfer to match the constraints. That said I'll have a more in
depth look at it.

> 
> > +	    maxp >= TRB_IDT_MAX_SIZE && len <= TRB_IDT_MAX_SIZE)
> 
> why the maxp check? What if I'm an interrupt endpoint with maxp of 2?
> Is there really a limitation that you couldn't use IDT for those?

Same table in specs. As I state in the patch's comment above, IDT with
wMaxPacketSize < 8 it's not supported.

> 
> > +		urb->transfer_flags |= URB_NO_DMA_MAP;
> 
> do we really need this? I wonder if returning zero here would be enough,
> then you could:

I agree that with Isoc included we could lose the flag and implement it as you
state below. Only length and ep's wMaxPacketSize should be checked.

> 
> ...map_urb_for_dma(...)
> {
> 	ret = 0;
> 
> 	if (urb->transfer_length > 8)
>         	ret = usb_hcd_map_urb_for_dma(hcd, urb, flags);
> 
> 	return ret;
> }
> 
> ...unmap_urb_for_dma(...)
> {
> 	if urb->transfer_length > 8)
>         	usb_hcd_unmap_urb_for_dma(hcd, urb);
> }
> 
> ...xhci_queue_bulk_tx(...)
> {
> 	[...]
> 
> 	if (urb->transfer_length <= 8) {
> 		memcpy(&addr, urb->transfer_buffer, 8)l
> 		field |= TRB_IDT;
>         }
> 
> 	[...]
> }
> 

Regards,
Nicolas