[PATCH 0/4] USB & USBNET: loose DMA SG check and support usbnet DMA SG

[PATCH 0/4] USB & USBNET: loose DMA SG check and support usbnet DMA SG

[Ming Lei]

Hi,

This patchset allows drivers to pass sg buffers which size can't be divided
by max packet size of endpoint if the host controllers support this kind
of sg buffers.

Previously we add check[1] on the situation and don't allow these sg buffers
passed to USB HCD, looks the check is too strict because some applications(such
as network stack) can't provide this sg buffers which size can be divided by
max packet size, so this patch looses the check in case that the host controllers
support it.

Also patch 3/4 implements DMA SG on usbnet driver, and patch 4/4
enables it on ax88179_178a USB3 NIC, so CPU utilization can be
decreased much with usbnet DMA SG when the USB3 NIC is attached to
xHCI controller.

[1], http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=10e232c597ac757e7f8600649f7e872e86de190f

 drivers/net/usb/ax88179_178a.c |   30 +++++++++++++++++++++++++
 drivers/net/usb/usbnet.c       |   48 ++++++++++++++++++++++++++++++++++++++--
 drivers/usb/core/urb.c         |    3 ++-
 drivers/usb/host/xhci.c        |    4 ++++
 include/linux/usb.h            |    3 ++-
 include/linux/usb/usbnet.h     |    4 ++++
 6 files changed, 88 insertions(+), 4 deletions(-)


Thanks,
--
Ming Lei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] USB: introduce no_sg_limit field into usb_bus

[Ming Lei]

Some host controllers(such as xHCI) can support building
packet from discontinuous buffers, so introduce the flag
for this kind of host controllers, then the feature can
help for some applications(such as, usbnet)

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/core/urb.c |    3 ++-
 include/linux/usb.h    |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index e75115a..7a88ae8 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -414,7 +414,8 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 			urb->iso_frame_desc[n].status = -EXDEV;
 			urb->iso_frame_desc[n].actual_length = 0;
 		}
-	} else if (dev->speed != USB_SPEED_WIRELESS && urb->num_sgs) {
+	} else if (urb->num_sgs && !urb->dev->bus->no_sg_limit &&
+			dev->speed != USB_SPEED_WIRELESS) {
 		struct scatterlist *sg;
 		int i;
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 84f14e2..981fe52 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -337,6 +337,7 @@ struct usb_bus {
 					 * the ep queue on a short transfer
 					 * with the URB_SHORT_NOT_OK flag set.
 					 */
+	unsigned no_sg_limit:1;		/* no sg list limit */
 	unsigned sg_tablesize;		/* 0 or largest number of sg list entries */
 
 	int devnum_next;		/* Next open device number in
@@ -1249,7 +1250,7 @@ typedef void (*usb_complete_t)(struct urb *);
  *	transfer_buffer.
  * @sg: scatter gather buffer list, the buffer size of each element in
  * 	the list (except the last) must be divisible by the endpoint's
- * 	max packet size
+ * 	max packet size if no_sg_limit isn't set in 'struct usb_bus'
  * @num_mapped_sgs: (internal) number of mapped sg entries
  * @num_sgs: number of entries in the sg list
  * @transfer_buffer_length: How big is transfer_buffer.  The transfer may
-- 
1.7.9.5

[PATCH 3/4] USBNET: support DMA SG

[Ming Lei]

This patch introduces support of DMA SG if the USB host controller
attached by usbnet device is capable of building packet from
discontinuous buffers.

Firstly, one header for usb transfer is often needed for device, and
this patch introduces one extra small buffer to hold the header, then
we can avoid move or copy data over the whole skb.

Secondaly, the patch supports passing the skb fragment buffers to
usb stack directly via urb->sg.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/usbnet.c   |   48 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/usb/usbnet.h |    4 ++++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e4811d7..c04682b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1232,6 +1232,45 @@ EXPORT_SYMBOL_GPL(usbnet_tx_timeout);
 
 /*-------------------------------------------------------------------------*/
 
+static int build_dma_sg(const struct sk_buff *skb, struct urb *urb)
+{
+	unsigned num_sgs, total_len = 0;
+	int i, s = 0;
+	struct skb_data *entry = (struct skb_data *)skb->cb;
+	int has_header = !!entry->length;
+
+	num_sgs = skb_shinfo(skb)->nr_frags + has_header + 1;
+	if (num_sgs == 1)
+		return 0;
+
+	urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+	if (!urb->sg)
+		return -ENOMEM;
+
+	urb->num_sgs = num_sgs;
+	sg_init_table(urb->sg, urb->num_sgs);
+
+	if (has_header) {
+		sg_set_buf(&urb->sg[s++], entry->header,
+				entry->length);
+		total_len += entry->length;
+	}
+
+	sg_set_buf(&urb->sg[s++], skb->data, skb_headlen(skb));
+	total_len += skb_headlen(skb);
+
+	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+		struct skb_frag_struct *f = &skb_shinfo(skb)->frags[i];
+
+		total_len += skb_frag_size(f);
+		sg_set_page(&urb->sg[i + s], f->page.p, f->size,
+				f->page_offset);
+	}
+	urb->transfer_buffer_length = total_len;
+
+	return 1;
+}
+
 netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 				     struct net_device *net)
 {
@@ -1258,7 +1297,6 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 			goto drop;
 		}
 	}
-	length = skb->len;
 
 	if (!(urb = usb_alloc_urb (0, GFP_ATOMIC))) {
 		netif_dbg(dev, tx_err, dev->net, "no urb\n");
@@ -1268,10 +1306,14 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 	entry = (struct skb_data *) skb->cb;
 	entry->urb = urb;
 	entry->dev = dev;
-	entry->length = length;
 
 	usb_fill_bulk_urb (urb, dev->udev, dev->out,
 			skb->data, skb->len, tx_complete, skb);
+	if (dev->can_dma_sg) {
+		if (build_dma_sg(skb, urb) < 0)
+			goto drop;
+	}
+	entry->length = length = urb->transfer_buffer_length;
 
 	/* don't assume the hardware handles USB_ZERO_PACKET
 	 * NOTE:  strictly conforming cdc-ether devices should expect
@@ -1391,6 +1433,8 @@ static void usbnet_bh (unsigned long param)
 			rx_process (dev, skb);
 			continue;
 		case tx_done:
+			kfree(entry->header);
+			kfree(entry->urb->sg);
 		case rx_cleanup:
 			usb_free_urb (entry->urb);
 			dev_kfree_skb (skb);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 8fbc008..51aa466 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -35,6 +35,7 @@ struct usbnet {
 	unsigned char		suspend_count;
 	unsigned char		pkt_cnt, pkt_err;
 	unsigned short		rx_qlen, tx_qlen;
+	unsigned		can_dma_sg:1;
 
 	/* i/o info: pipes etc */
 	unsigned		in, out;
@@ -217,7 +218,10 @@ enum skb_state {
 struct skb_data {	/* skb->cb is one of these */
 	struct urb		*urb;
 	struct usbnet		*dev;
+	void			*header;
 	enum skb_state		state;
+
+	/* either header length for dma sg or transfer buffer lenght */
 	size_t			length;
 };
 
-- 
1.7.9.5

[PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Ming Lei]

This patch enables 'can_dma_sg' flag for ax88179_178a device
if the attached host controller supports building packet from
discontinuous buffers(DMA SG is possible), so both frame header
and skb data buffers can be passed to usb stack via urb->sg,
then skb data copy can be saved.

With the patch, CPU utilization decreased much in iperf test at
client mode.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/ax88179_178a.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 5a468f3..c75bded 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1031,12 +1031,20 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->mii.phy_id = 0x03;
 	dev->mii.supports_gmii = 1;
 
+	if (dev->udev->bus->no_sg_limit)
+		dev->can_dma_sg = 1;
+
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM;
 
+	if (dev->can_dma_sg) {
+		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
+		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
+	}
+
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
 	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
@@ -1170,12 +1178,30 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 	int mss = skb_shinfo(skb)->gso_size;
 	int headroom;
 	int tailroom;
+	struct skb_data *entry = (struct skb_data *)skb->cb;
 
 	tx_hdr1 = skb->len;
 	tx_hdr2 = mss;
 	if (((skb->len + 8) % frame_size) == 0)
 		tx_hdr2 |= 0x80008000;	/* Enable padding */
 
+	if (dev->can_dma_sg) {
+		if (!(entry->header = kmalloc(8, GFP_ATOMIC)))
+			goto no_sg;
+
+		entry->length = 8;
+		cpu_to_le32s(&tx_hdr1);
+		cpu_to_le32s(&tx_hdr2);
+		memcpy(entry->header, &tx_hdr1, 4);
+		memcpy(entry->header + 4, &tx_hdr2, 4);
+
+		return skb;
+	} else {
+no_sg:
+		entry->header = NULL;
+		entry->length = 0;
+	}
+
 	headroom = skb_headroom(skb);
 	tailroom = skb_tailroom(skb);
 
@@ -1323,6 +1349,10 @@ static int ax88179_reset(struct usbnet *dev)
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM;
+	if (dev->can_dma_sg) {
+		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
+		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
+	}
 
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
-- 
1.7.9.5

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Greg Kroah-Hartman]

On Wed, Jul 31, 2013 at 06:51:49PM +0800, Ming Lei wrote:
> This patch enables 'can_dma_sg' flag for ax88179_178a device
> if the attached host controller supports building packet from
> discontinuous buffers(DMA SG is possible), so both frame header
> and skb data buffers can be passed to usb stack via urb->sg,
> then skb data copy can be saved.
> 
> With the patch, CPU utilization decreased much in iperf test at
> client mode.

What is "much"?

> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1031,12 +1031,20 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->mii.phy_id = 0x03;
>  	dev->mii.supports_gmii = 1;
>  
> +	if (dev->udev->bus->no_sg_limit)
> +		dev->can_dma_sg = 1;

I don't feel comfortable with USB drivers poking about in the USB host
controller structures like this.  If we really do this, please provide a
function call for it to make to determine this.

But even then, is this really something that we want/need to do at all?
If only some xhci controller support this, we will start to see more
driver-specific changes just to handle one type of host controller.  Why
not have the USB core handle it instead and if the host controller can
not do sg lists like this, fall back to a slower implementation?

thanks,

greg k-h

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Ming Lei]

On Wed, Jul 31, 2013 at 8:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jul 31, 2013 at 06:51:49PM +0800, Ming Lei wrote:
>> This patch enables 'can_dma_sg' flag for ax88179_178a device
>> if the attached host controller supports building packet from
>> discontinuous buffers(DMA SG is possible), so both frame header
>> and skb data buffers can be passed to usb stack via urb->sg,
>> then skb data copy can be saved.
>>
>> With the patch, CPU utilization decreased much in iperf test at
>> client mode.
>
> What is "much"?

The data is platform dependent, on one dual-core ARM A15 board
at my hand, ~30~40% CPU utilization is saved with the patchset
when doing 'iperf -c' test on this board.

>
>> --- a/drivers/net/usb/ax88179_178a.c
>> +++ b/drivers/net/usb/ax88179_178a.c
>> @@ -1031,12 +1031,20 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>>       dev->mii.phy_id = 0x03;
>>       dev->mii.supports_gmii = 1;
>>
>> +     if (dev->udev->bus->no_sg_limit)
>> +             dev->can_dma_sg = 1;
>
> I don't feel comfortable with USB drivers poking about in the USB host
> controller structures like this.  If we really do this, please provide a
> function call for it to make to determine this.

OK, that is fine.

>
> But even then, is this really something that we want/need to do at all?
> If only some xhci controller support this, we will start to see more

It should be all xhci controllers support this, per xHCI spec.

> driver-specific changes just to handle one type of host controller.  Why

We already have this kind of changes, such as usbfs and mass storage
driver, the two drivers support both sg list and non-sg list cases(depends
on HCD).

> not have the USB core handle it instead and if the host controller can
> not do sg lists like this, fall back to a slower implementation?

In the usbnet case, the driver already supports non-sg well. Actually,
all current drivers should support non-sg well because urb->sg wasn't
introduced for very long time. We can think it as a new feature or DMA
enhancement for xHCI controller.

If you mean buffer debounce for dma sg support on ehci/uhci/ohci/..,
maybe we need to discuss and evaluate further.

Thanks,
--
Ming Lei

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Oliver Neukum]

On Wed, 2013-07-31 at 21:50 +0800, Ming Lei wrote:

> In the usbnet case, the driver already supports non-sg well. Actually,
> all current drivers should support non-sg well because urb->sg wasn't
> introduced for very long time. We can think it as a new feature or DMA
> enhancement for xHCI controller.
> 
> If you mean buffer debounce for dma sg support on ehci/uhci/ohci/..,
> maybe we need to discuss and evaluate further.

We cannot lie to the networking layer. Either we can do sg in hardware
or we cannot. This is unfortunately determined by the HC not the device
on the bus. How else but from the host driver would we get the
information?

	Regards
		Oliver

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Wed, 2013-07-31 at 16:02 +0200, Oliver Neukum wrote:
> On Wed, 2013-07-31 at 21:50 +0800, Ming Lei wrote:
> 
> > In the usbnet case, the driver already supports non-sg well. Actually,
> > all current drivers should support non-sg well because urb->sg wasn't
> > introduced for very long time. We can think it as a new feature or DMA
> > enhancement for xHCI controller.
> > 
> > If you mean buffer debounce for dma sg support on ehci/uhci/ohci/..,
> > maybe we need to discuss and evaluate further.
> 
> We cannot lie to the networking layer. Either we can do sg in hardware
> or we cannot. This is unfortunately determined by the HC not the device
> on the bus. How else but from the host driver would we get the
> information?
> 

Hmm, I would rather make sure SG is really supported before adding TSO
support.

TCP stack can build skb with fragments of any size, not multiple
of 512 or 1024 bytes.

commit 20f0170377264e8449b6987041f0bcc4d746d3ed

    usbnet: do not pretend to support SG/TSO
    
    usbnet doesn't support yet SG, so drivers should not advertise SG or TSO
    capabilities, as they allow TCP stack to build large TSO packets that
    need to be linearized and might use order-5 pages.
    
    This adds an extra copy overhead and possible allocation failures.
    
    Current code ignore skb_linearize() return code so crashes are even
    possible.
    
    Best is to not pretend SG/TSO is supported, and add this again when/if
    usbnet really supports SG for devices who could get a performance gain.
    


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Ming Lei]

On Wed, Jul 31, 2013 at 11:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2013-07-31 at 16:02 +0200, Oliver Neukum wrote:
>> On Wed, 2013-07-31 at 21:50 +0800, Ming Lei wrote:
>>
>> > In the usbnet case, the driver already supports non-sg well. Actually,
>> > all current drivers should support non-sg well because urb->sg wasn't
>> > introduced for very long time. We can think it as a new feature or DMA
>> > enhancement for xHCI controller.
>> >
>> > If you mean buffer debounce for dma sg support on ehci/uhci/ohci/..,
>> > maybe we need to discuss and evaluate further.
>>
>> We cannot lie to the networking layer. Either we can do sg in hardware
>> or we cannot. This is unfortunately determined by the HC not the device
>> on the bus. How else but from the host driver would we get the
>> information?
>>
>
> Hmm, I would rather make sure SG is really supported before adding TSO
> support.
>
> TCP stack can build skb with fragments of any size, not multiple
> of 512 or 1024 bytes.

For this SG buffer, only xHCI USB host controller supports it, and other
host controllers can't support it if they don't use bounce buffer, but usbnet
devices may be connected to all these host controllers.

This patchset only enables SG/TSO support when ax88179_178a device
connects to xHCI host controller.

Thanks,
--
Ming Lei

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Ming Lei]

On Wed, Jul 31, 2013 at 11:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2013-07-31 at 16:02 +0200, Oliver Neukum wrote:
>
> Hmm, I would rather make sure SG is really supported before adding TSO
> support.
>
> TCP stack can build skb with fragments of any size, not multiple
> of 512 or 1024 bytes.

Now Sarah Sharp(xHCD maintainer) has Acked patch 2/4, and confirmed
SG is supported on xHCD, so I appreciate that you network guys may
review the network related changes in patch 3/4 and 4/4, then we
can move on, :-)


Thanks,
--
Ming Lei

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Wed, 2013-07-31 at 18:51 +0800, Ming Lei wrote:
> This patch enables 'can_dma_sg' flag for ax88179_178a device
> if the attached host controller supports building packet from
> discontinuous buffers(DMA SG is possible), so both frame header
> and skb data buffers can be passed to usb stack via urb->sg,
> then skb data copy can be saved.
> 
> With the patch, CPU utilization decreased much in iperf test at
> client mode.
> 
> Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  drivers/net/usb/ax88179_178a.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 5a468f3..c75bded 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1031,12 +1031,20 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->mii.phy_id = 0x03;
>  	dev->mii.supports_gmii = 1;
>  
> +	if (dev->udev->bus->no_sg_limit)
> +		dev->can_dma_sg = 1;
> +
>  	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  			      NETIF_F_RXCSUM;
>  
>  	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  				 NETIF_F_RXCSUM;
>  
> +	if (dev->can_dma_sg) {
> +		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
> +		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
> +	}
> +
>  	/* Enable checksum offload */
>  	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
>  	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
> @@ -1170,12 +1178,30 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>  	int mss = skb_shinfo(skb)->gso_size;
>  	int headroom;
>  	int tailroom;
> +	struct skb_data *entry = (struct skb_data *)skb->cb;
>  
>  	tx_hdr1 = skb->len;
>  	tx_hdr2 = mss;
>  	if (((skb->len + 8) % frame_size) == 0)
>  		tx_hdr2 |= 0x80008000;	/* Enable padding */
>  
> +	if (dev->can_dma_sg) {



> +		if (!(entry->header = kmalloc(8, GFP_ATOMIC)))
> +			goto no_sg;
> +
> +		entry->length = 8;
> +		cpu_to_le32s(&tx_hdr1);
> +		cpu_to_le32s(&tx_hdr2);

You could do these cpu_to_le32s() calls before the if (dev->can_dma_sg)
and remove them before the skb_copy_to_linear_data() calls.

> +		memcpy(entry->header, &tx_hdr1, 4);
> +		memcpy(entry->header + 4, &tx_hdr2, 4);
> +
> +		return skb;
> +	} else {
> +no_sg:
> +		entry->header = NULL;
> +		entry->length = 0;
> +	}
> +
>  	headroom = skb_headroom(skb);
>  	tailroom = skb_tailroom(skb);
>  
> @@ -1323,6 +1349,10 @@ static int ax88179_reset(struct usbnet *dev)
>  
>  	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  				 NETIF_F_RXCSUM;
> +	if (dev->can_dma_sg) {
> +		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
> +		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
> +	}

Are you sure this part is needed ?

Also it looks like that you are going through this sg allocation/setup
even for linear skb ?

For linear skb, with 8+ bytes of headroom, I guess this adds significant
overhead (2 kmalloc() + sg setup)



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Ming Lei]

Hi Eric,

Thanks for your review.

On Thu, Aug 1, 2013 at 11:51 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2013-07-31 at 18:51 +0800, Ming Lei wrote:
>> This patch enables 'can_dma_sg' flag for ax88179_178a device
>> if the attached host controller supports building packet from
>> discontinuous buffers(DMA SG is possible), so both frame header
>> and skb data buffers can be passed to usb stack via urb->sg,
>> then skb data copy can be saved.
>>
>> With the patch, CPU utilization decreased much in iperf test at
>> client mode.
>>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  drivers/net/usb/ax88179_178a.c |   30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
>> index 5a468f3..c75bded 100644
>> --- a/drivers/net/usb/ax88179_178a.c
>> +++ b/drivers/net/usb/ax88179_178a.c
>> @@ -1031,12 +1031,20 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>>       dev->mii.phy_id = 0x03;
>>       dev->mii.supports_gmii = 1;
>>
>> +     if (dev->udev->bus->no_sg_limit)
>> +             dev->can_dma_sg = 1;
>> +
>>       dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>>                             NETIF_F_RXCSUM;
>>
>>       dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>>                                NETIF_F_RXCSUM;
>>
>> +     if (dev->can_dma_sg) {
>> +             dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
>> +             dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
>> +     }
>> +
>>       /* Enable checksum offload */
>>       *tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
>>              AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
>> @@ -1170,12 +1178,30 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>>       int mss = skb_shinfo(skb)->gso_size;
>>       int headroom;
>>       int tailroom;
>> +     struct skb_data *entry = (struct skb_data *)skb->cb;
>>
>>       tx_hdr1 = skb->len;
>>       tx_hdr2 = mss;
>>       if (((skb->len + 8) % frame_size) == 0)
>>               tx_hdr2 |= 0x80008000;  /* Enable padding */
>>
>> +     if (dev->can_dma_sg) {
>
>
>
>> +             if (!(entry->header = kmalloc(8, GFP_ATOMIC)))
>> +                     goto no_sg;
>> +
>> +             entry->length = 8;
>> +             cpu_to_le32s(&tx_hdr1);
>> +             cpu_to_le32s(&tx_hdr2);
>
> You could do these cpu_to_le32s() calls before the if (dev->can_dma_sg)
> and remove them before the skb_copy_to_linear_data() calls.

OK.

>
>> +             memcpy(entry->header, &tx_hdr1, 4);
>> +             memcpy(entry->header + 4, &tx_hdr2, 4);
>> +
>> +             return skb;
>> +     } else {
>> +no_sg:
>> +             entry->header = NULL;
>> +             entry->length = 0;
>> +     }
>> +
>>       headroom = skb_headroom(skb);
>>       tailroom = skb_tailroom(skb);
>>
>> @@ -1323,6 +1349,10 @@ static int ax88179_reset(struct usbnet *dev)
>>
>>       dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>>                                NETIF_F_RXCSUM;
>> +     if (dev->can_dma_sg) {
>> +             dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
>> +             dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
>> +     }
>
> Are you sure this part is needed ?

I add flags here because the part existed previously, and if you are sure
it isn't needed anymore, I can remove this part.

>
> Also it looks like that you are going through this sg allocation/setup
> even for linear skb ?
>
> For linear skb, with 8+ bytes of headroom, I guess this adds significant
> overhead (2 kmalloc() + sg setup)

>From my trace result, lots of linear SKBs are cloned or header-cloned, so
it needs skb copy too.

Is it normal in xmit path to see cloned SKBs for driver? If not, I can add check
to avoid allocation of 8 bytes header for non-cloned skb.

But for the kmalloc of urb->sg, I don't think there is one way to save that.

Thanks,
--
Ming Lei

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Thu, 2013-08-01 at 12:41 +0800, Ming Lei wrote:

> From my trace result, lots of linear SKBs are cloned or header-cloned, so
> it needs skb copy too.
> 
> Is it normal in xmit path to see cloned SKBs for driver? If not, I can add check
> to avoid allocation of 8 bytes header for non-cloned skb.

Existing code is not very friendly and very complex.

Sure TCP stack does a clone for every skb from socket write queue,
but header should be available for pushing 8 bytes.

The !skb_cloned(skb) test should be removed if the memmove() is not
needed.

Could you try following patch ?

 drivers/net/usb/ax88179_178a.c |   21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 2bc87e3..e2120d6 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1166,31 +1166,18 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 	int frame_size = dev->maxpacket;
 	int mss = skb_shinfo(skb)->gso_size;
 	int headroom;
-	int tailroom;
 
 	tx_hdr1 = skb->len;
 	tx_hdr2 = mss;
 	if (((skb->len + 8) % frame_size) == 0)
 		tx_hdr2 |= 0x80008000;	/* Enable padding */
 
-	headroom = skb_headroom(skb);
-	tailroom = skb_tailroom(skb);
+	headroom = skb_headroom(skb) - 8;
 
-	if (!skb_header_cloned(skb) &&
-	    !skb_cloned(skb) &&
-	    (headroom + tailroom) >= 8) {
-		if (headroom < 8) {
-			skb->data = memmove(skb->head + 8, skb->data, skb->len);
-			skb_set_tail_pointer(skb, skb->len);
-		}
-	} else {
-		struct sk_buff *skb2;
-
-		skb2 = skb_copy_expand(skb, 8, 0, flags);
+	if ((skb_header_cloned(skb) || headroom < 0) &&
+	    pskb_expand_head(skb, headroom < 0 ? 8 : 0, 0, GFP_ATOMIC)) {
 		dev_kfree_skb_any(skb);
-		skb = skb2;
-		if (!skb)
-			return NULL;
+		return NULL;
 	}
 
 	skb_push(skb, 4);

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Ming Lei]

On Thu, Aug 1, 2013 at 1:04 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, 2013-08-01 at 12:41 +0800, Ming Lei wrote:
>
>> From my trace result, lots of linear SKBs are cloned or header-cloned, so
>> it needs skb copy too.
>>
>> Is it normal in xmit path to see cloned SKBs for driver? If not, I can add check
>> to avoid allocation of 8 bytes header for non-cloned skb.
>
> Existing code is not very friendly and very complex.
>
> Sure TCP stack does a clone for every skb from socket write queue,
> but header should be available for pushing 8 bytes.
>
> The !skb_cloned(skb) test should be removed if the memmove() is not
> needed.
>
> Could you try following patch ?

Tested-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

This patch does work, and it will make the patch 4/4 become very
simple, :-)

I will rewrite this one against your patch.

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Thu, 2013-08-01 at 16:10 +0800, Ming Lei wrote:
> On Thu, Aug 1, 2013 at 1:04 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Thu, 2013-08-01 at 12:41 +0800, Ming Lei wrote:
> >
> >> From my trace result, lots of linear SKBs are cloned or header-cloned, so
> >> it needs skb copy too.
> >>
> >> Is it normal in xmit path to see cloned SKBs for driver? If not, I can add check
> >> to avoid allocation of 8 bytes header for non-cloned skb.
> >
> > Existing code is not very friendly and very complex.
> >
> > Sure TCP stack does a clone for every skb from socket write queue,
> > but header should be available for pushing 8 bytes.
> >
> > The !skb_cloned(skb) test should be removed if the memmove() is not
> > needed.
> >
> > Could you try following patch ?
> 
> Tested-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> 
> This patch does work, and it will make the patch 4/4 become very
> simple, :-)
> 
> I will rewrite this one against your patch.

OK I'll post an official patch then. I presume you got performance
increase, since we no longer do a copy of the TCP packets.



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next] ax88179_178a: avoid copy of tx tcp packets

[Eric Dumazet]

From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

ax88179_tx_fixup() has quite complex code trying to push 8 bytes
of control data (len/mss), but fails to do it properly for TCP packets,
incurring an extra copy and point of memory allocation failure.

Lets use the simple and approved way.

dev->needed_headroom being 8, all frames should have 8 bytes of
headroom, so the extra copy should be unlikely anyway.

This patch should improve performance for TCP xmits.

Reported-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Tested-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 drivers/net/usb/ax88179_178a.c |   21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 2bc87e3..e2120d6 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1166,31 +1166,18 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 	int frame_size = dev->maxpacket;
 	int mss = skb_shinfo(skb)->gso_size;
 	int headroom;
-	int tailroom;
 
 	tx_hdr1 = skb->len;
 	tx_hdr2 = mss;
 	if (((skb->len + 8) % frame_size) == 0)
 		tx_hdr2 |= 0x80008000;	/* Enable padding */
 
-	headroom = skb_headroom(skb);
-	tailroom = skb_tailroom(skb);
+	headroom = skb_headroom(skb) - 8;
 
-	if (!skb_header_cloned(skb) &&
-	    !skb_cloned(skb) &&
-	    (headroom + tailroom) >= 8) {
-		if (headroom < 8) {
-			skb->data = memmove(skb->head + 8, skb->data, skb->len);
-			skb_set_tail_pointer(skb, skb->len);
-		}
-	} else {
-		struct sk_buff *skb2;

Re: [PATCH net-next] ax88179_178a: avoid copy of tx tcp packets

[Eric Dumazet]

On Thu, 2013-08-01 at 06:49 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> 
> ax88179_tx_fixup() has quite complex code trying to push 8 bytes
> of control data (len/mss), but fails to do it properly for TCP packets,
> incurring an extra copy and point of memory allocation failure.

I forgot to say this patch is for net-next, but depends on following fix
from net tree being applied.

commit 20f0170377264e8449b6987041f0bcc4d746d3ed
("usbnet: do not pretend to support SG/TSO")



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] ax88179_178a: avoid copy of tx tcp packets

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 01 Aug 2013 06:52:45 -0700

> On Thu, 2013-08-01 at 06:49 -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>> 
>> ax88179_tx_fixup() has quite complex code trying to push 8 bytes
>> of control data (len/mss), but fails to do it properly for TCP packets,
>> incurring an extra copy and point of memory allocation failure.
> 
> I forgot to say this patch is for net-next, but depends on following fix
> from net tree being applied.
> 
> commit 20f0170377264e8449b6987041f0bcc4d746d3ed
> ("usbnet: do not pretend to support SG/TSO")

Thanks for letting me know.

Applied, thanks Eric.

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Grant Grundler]

On Wed, Jul 31, 2013 at 3:51 AM, Ming Lei <ming.lei@canonical.com> wrote:
> This patch enables 'can_dma_sg' flag for ax88179_178a device
> if the attached host controller supports building packet from
> discontinuous buffers(DMA SG is possible), so both frame header
> and skb data buffers can be passed to usb stack via urb->sg,
> then skb data copy can be saved.
>
> With the patch, CPU utilization decreased much in iperf test at
> client mode.
>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/net/usb/ax88179_178a.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 5a468f3..c75bded 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1031,12 +1031,20 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>         dev->mii.phy_id = 0x03;
>         dev->mii.supports_gmii = 1;
>
> +       if (dev->udev->bus->no_sg_limit)
> +               dev->can_dma_sg = 1;
> +
>         dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>                               NETIF_F_RXCSUM;
>
>         dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>                                  NETIF_F_RXCSUM;
>
> +       if (dev->can_dma_sg) {
> +               dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
> +               dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
> +       }
> +
>         /* Enable checksum offload */
>         *tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
>                AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
> @@ -1170,12 +1178,30 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>         int mss = skb_shinfo(skb)->gso_size;
>         int headroom;
>         int tailroom;
> +       struct skb_data *entry = (struct skb_data *)skb->cb;
>
>         tx_hdr1 = skb->len;
>         tx_hdr2 = mss;
>         if (((skb->len + 8) % frame_size) == 0)
>                 tx_hdr2 |= 0x80008000;  /* Enable padding */
>
> +       if (dev->can_dma_sg) {
> +               if (!(entry->header = kmalloc(8, GFP_ATOMIC)))
> +                       goto no_sg;
> +
> +               entry->length = 8;
> +               cpu_to_le32s(&tx_hdr1);
> +               cpu_to_le32s(&tx_hdr2);

http://lxr.free-electrons.com/source/include/linux/byteorder/generic.h#L111
http://lxr.free-electrons.com/ident?i=__cpu_to_le32s

IIRC, cpu_to_leXX() macros return the endian "corrected" value.
In other words, they need to be assigned to something, no?

thanks,
grant

> +               memcpy(entry->header, &tx_hdr1, 4);
> +               memcpy(entry->header + 4, &tx_hdr2, 4);
> +
> +               return skb;
> +       } else {
> +no_sg:
> +               entry->header = NULL;
> +               entry->length = 0;
> +       }
> +
>         headroom = skb_headroom(skb);
>         tailroom = skb_tailroom(skb);
>
> @@ -1323,6 +1349,10 @@ static int ax88179_reset(struct usbnet *dev)
>
>         dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>                                  NETIF_F_RXCSUM;
> +       if (dev->can_dma_sg) {
> +               dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
> +               dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
> +       }
>
>         /* Enable checksum offload */
>         *tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
> --
> 1.7.9.5
>

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Thu, 2013-08-01 at 08:30 -0700, Grant Grundler wrote:

> http://lxr.free-electrons.com/source/include/linux/byteorder/generic.h#L111
> http://lxr.free-electrons.com/ident?i=__cpu_to_le32s
> 
> IIRC, cpu_to_leXX() macros return the endian "corrected" value.
> In other words, they need to be assigned to something, no?
> 

Nope, this in in-place byte swapping (for Big Endian only)

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Grant Grundler]

On Thu, Aug 1, 2013 at 8:36 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
...
>> IIRC, cpu_to_leXX() macros return the endian "corrected" value.
>> In other words, they need to be assigned to something, no?
>
> Nope, this in in-place byte swapping (for Big Endian only)

Ah ok - thanks for clarifying. I couldn't find the implementation and
this documentation also helped:
   http://kernelnewbies.org/EndianIssues

I don't remember implementing in-place endian correction for
arch/parisc (which is BE). I was expecting to find a generic
definition that did a load/swap/store and didn't.

thanks,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] USB & USBNET: loose DMA SG check and support usbnet DMA SG

[Oliver Neukum]

On Wed, 2013-07-31 at 18:51 +0800, Ming Lei wrote:
> Hi,
> 
> This patchset allows drivers to pass sg buffers which size can't be divided
> by max packet size of endpoint if the host controllers support this kind
> of sg buffers.

Cool. One question though. It makes sense to use sg only
if all elements are in memory capable of DMA. It seems to be possible
to have HCs which can do unlimited sg but are limited to 32bit DMA.

	Regards
		Oliver

Re: [PATCH 0/4] USB & USBNET: loose DMA SG check and support usbnet DMA SG

[Ming Lei]

On Wed, Jul 31, 2013 at 7:12 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Wed, 2013-07-31 at 18:51 +0800, Ming Lei wrote:
>> Hi,
>>
>> This patchset allows drivers to pass sg buffers which size can't be divided
>> by max packet size of endpoint if the host controllers support this kind
>> of sg buffers.
>
> Cool. One question though. It makes sense to use sg only
> if all elements are in memory capable of DMA. It seems to be possible
> to have HCs which can do unlimited sg but are limited to 32bit DMA.

For current usbnet devices which supports TSO, one header need to add
before skb->data from network stack, so basically the first two elements aren't
OK for other HCs(EHCI/UHCI/...) to DMA from SG.


Thanks,
--
Ming Le

[PATCH 2/4] USB: XHCI: mark no_sg_limit

[Ming Lei]

This patch marks all xHCI controllers as no_sg_limit since
xHCI supports building packet from discontinuous buffers.

Cc: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/usb/host/xhci.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2c49f00..541b155 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4841,6 +4841,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 
 	/* Accept arbitrarily long scatter-gather lists */
 	hcd->self.sg_tablesize = ~0;
+
+	/* support to build packet from discontinuous buffers */
+	hcd->self.no_sg_limit = 1;
+
 	/* XHCI controllers don't stop the ep queue on short packets :| */
 	hcd->self.no_stop_on_short = 1;
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] USB: XHCI: mark no_sg_limit

[Sarah Sharp]

On Wed, Jul 31, 2013 at 06:51:47PM +0800, Ming Lei wrote:
> This patch marks all xHCI controllers as no_sg_limit since
> xHCI supports building packet from discontinuous buffers.
> 
> Cc: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>

Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>

> ---
>  drivers/usb/host/xhci.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 2c49f00..541b155 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4841,6 +4841,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  
>  	/* Accept arbitrarily long scatter-gather lists */
>  	hcd->self.sg_tablesize = ~0;
> +
> +	/* support to build packet from discontinuous buffers */
> +	hcd->self.no_sg_limit = 1;
> +
>  	/* XHCI controllers don't stop the ep queue on short packets :| */
>  	hcd->self.no_stop_on_short = 1;
>  
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] USB: XHCI: mark no_sg_limit

[Greg Kroah-Hartman]

On Wed, Jul 31, 2013 at 09:40:26AM -0700, Sarah Sharp wrote:
> On Wed, Jul 31, 2013 at 06:51:47PM +0800, Ming Lei wrote:
> > This patch marks all xHCI controllers as no_sg_limit since
> > xHCI supports building packet from discontinuous buffers.
> > 
> > Cc: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> 
> Acked-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Is it a requirement that all xhci controllers support sg?  I know we are
starting to see other controllers (the platform code?) so would they
need to support this as well?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] USB: XHCI: mark no_sg_limit

[Oliver Neukum]

On Thu, 2013-08-01 at 15:30 +0800, Greg Kroah-Hartman wrote:
> On Wed, Jul 31, 2013 at 09:40:26AM -0700, Sarah Sharp wrote:
> > On Wed, Jul 31, 2013 at 06:51:47PM +0800, Ming Lei wrote:
> > > This patch marks all xHCI controllers as no_sg_limit since
> > > xHCI supports building packet from discontinuous buffers.
> > > 
> > > Cc: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> > > Signed-off-by: Ming Lei <ming.lei@canonical.com>
> > 
> > Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> 
> Is it a requirement that all xhci controllers support sg?  I know we are
> starting to see other controllers (the platform code?) so would they
> need to support this as well?

The way XHCI describes transfers allows them to be arbitrary.

	Regards
		Oliver

Re: [PATCH 2/4] USB: XHCI: mark no_sg_limit

[Sarah Sharp]

On Thu, Aug 01, 2013 at 10:15:51AM +0200, Oliver Neukum wrote:
> On Thu, 2013-08-01 at 15:30 +0800, Greg Kroah-Hartman wrote:
> > On Wed, Jul 31, 2013 at 09:40:26AM -0700, Sarah Sharp wrote:
> > > On Wed, Jul 31, 2013 at 06:51:47PM +0800, Ming Lei wrote:
> > > > This patch marks all xHCI controllers as no_sg_limit since
> > > > xHCI supports building packet from discontinuous buffers.
> > > > 
> > > > Cc: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> > > > Signed-off-by: Ming Lei <ming.lei@canonical.com>
> > > 
> > > Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> > 
> > Is it a requirement that all xhci controllers support sg?  I know we are
> > starting to see other controllers (the platform code?) so would they
> > need to support this as well?
> 
> The way XHCI describes transfers allows them to be arbitrary.

Yes, Oliver is right.  The xHCI host can handle arbitrary length buffers
chained together into one transfer, and now that we have the xHCI driver
ring expansion code in place, we should be able to make them as long as
the USB core will allow.

Sarah Sharp

Re: [PATCH 0/4] USB & USBNET: loose DMA SG check and support usbnet DMA SG

[Eric Dumazet]

On Wed, 2013-07-31 at 18:51 +0800, Ming Lei wrote:
> Hi,
> 
> This patchset allows drivers to pass sg buffers which size can't be divided
> by max packet size of endpoint if the host controllers support this kind
> of sg buffers.
> 
> Previously we add check[1] on the situation and don't allow these sg buffers
> passed to USB HCD, looks the check is too strict because some applications(such
> as network stack) can't provide this sg buffers which size can be divided by
> max packet size, so this patch looses the check in case that the host controllers
> support it.
> 
> Also patch 3/4 implements DMA SG on usbnet driver, and patch 4/4
> enables it on ax88179_178a USB3 NIC, so CPU utilization can be
> decreased much with usbnet DMA SG when the USB3 NIC is attached to
> xHCI controller.

This looks very promising to me, thanks a lot for doing this Ming.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html