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

[PATCH v3 0/4] USB & USBNET: loose 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(such ax xHCI) support
this kind of sg buffers.

Previously we added check[1] on the situation and don't allow these sg buffers
passed to USB HCD, looks the check is too strict to make use of new feature of
new hardware(xHCI) for some applications(such as network stack) which can't
provide this kind of sg buffers to usbnet driver, so the patch looses the check
in case that the host controller supports it.

Patch 3/4 implements DMA SG on usbnet driver, and patch 4/4 uses it on ax88179_178a
USB3 NIC for supporting TSO, so both CPU utilization and tx throughput can be
improved with TSO and DMA SG in case of the USB NIC is attached to xHCI controller.

This patchset depends on both net-next and usb-next tree, so hope David and Greg
to figure out one elegent way to merge it.

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

V3:
	- save 3 lines code for usb_device_no_sg_constraint() as suggested by Alan
	- fix urb->sg leak in xmit failure path

V2:
        - add missed kfree(urb->sg) in 3/4
        - rename no_sg_limit as no_sg_constraint as suggested by Alan

V1:
        - introduce and apply usb_device_no_sg_limit() helper as suggested by Greg
        - simplify patch 4/4 against Eric Dumazet's patch(ax88179_178a: avoid copy of tx tcp packets)
        - don't pass usbnet header as sg buffer

 drivers/net/usb/ax88179_178a.c |   12 +++++++++++
 drivers/net/usb/usbnet.c       |   45 +++++++++++++++++++++++++++++++++++++---
 drivers/usb/core/urb.c         |    3 ++-
 drivers/usb/host/xhci.c        |    4 ++++
 include/linux/usb.h            |    8 ++++++-
 include/linux/usb/usbnet.h     |    1 +
 6 files changed, 68 insertions(+), 5 deletions(-)


Thanks,
--
Ming Lei

[PATCH v3 1/4] USB: introduce usb_device_no_sg_constraint() helper

[Ming Lei]

Some host controllers(such as xHCI) can support building
packet from discontinuous buffers, so introduce one flag
and helper for this kind of host controllers, then the
feature can help some applications(such as usbnet) by
supporting arbitrary length of sg buffers.

Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/core/urb.c |    3 ++-
 include/linux/usb.h    |    8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index e75115a..c77ec78 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_constraint &&
+			dev->speed != USB_SPEED_WIRELESS) {
 		struct scatterlist *sg;
 		int i;
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 84f14e2..bbd2c8d 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_constraint:1;	/* no sg constraint */
 	unsigned sg_tablesize;		/* 0 or largest number of sg list entries */
 
 	int devnum_next;		/* Next open device number in
@@ -684,6 +685,11 @@ static inline bool usb_device_supports_ltm(struct usb_device *udev)
 	return udev->bos->ss_cap->bmAttributes & USB_LTM_SUPPORT;
 }
 
+static inline bool usb_device_no_sg_constraint(struct usb_device *udev)
+{
+	return udev && udev->bus && udev->bus->no_sg_constraint;
+}
+
 
 /*-------------------------------------------------------------------------*/
 
@@ -1249,7 +1255,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_constraint 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 v3 2/4] USB: XHCI: mark no_sg_constraint

[Ming Lei]

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

Cc: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Signed-off-by: Ming Lei <ming.lei@canonical.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..6e2ac57 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_constraint = 1;
+
 	/* XHCI controllers don't stop the ep queue on short packets :| */
 	hcd->self.no_stop_on_short = 1;
 
-- 
1.7.9.5

[PATCH v3 3/4] USBNET: support DMA SG

[Ming Lei]

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

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

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: Grant Grundler <grundler@google.com>
Cc: Freddy Xin <freddy@asix.com.tw>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/usbnet.c   |   45 +++++++++++++++++++++++++++++++++++++++++---
 include/linux/usb/usbnet.h |    1 +
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e4811d7..534b60b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1232,6 +1232,37 @@ 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;
+
+	num_sgs = skb_shinfo(skb)->nr_frags + 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);
+
+	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 +1289,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 +1298,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
@@ -1340,7 +1374,10 @@ drop:
 not_drop:
 		if (skb)
 			dev_kfree_skb_any (skb);
-		usb_free_urb (urb);
+		if (urb) {
+			kfree(urb->sg);
+			usb_free_urb(urb);
+		}
 	} else
 		netif_dbg(dev, tx_queued, dev->net,
 			  "> tx, len %d, type 0x%x\n", length, skb->protocol);
@@ -1391,6 +1428,7 @@ static void usbnet_bh (unsigned long param)
 			rx_process (dev, skb);
 			continue;
 		case tx_done:
+			kfree(entry->urb->sg);
 		case rx_cleanup:
 			usb_free_urb (entry->urb);
 			dev_kfree_skb (skb);
@@ -1727,6 +1765,7 @@ int usbnet_resume (struct usb_interface *intf)
 			retval = usb_submit_urb(res, GFP_ATOMIC);
 			if (retval < 0) {
 				dev_kfree_skb_any(skb);
+				kfree(res->sg);
 				usb_free_urb(res);
 				usb_autopm_put_interface_async(dev->intf);
 			} else {
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 8fbc008..9cb2fe8 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;
-- 
1.7.9.5

[PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb 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 TSO can be enabled
and skb fragment buffers can be passed to usb stack via urb->sg
directly.

With the patch, system CPU utilization decreased ~50% and throughput
increased by ~10% when doing iperf client test on one ARM A15 dual
core board.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: Grant Grundler <grundler@google.com>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Freddy Xin <freddy@asix.com.tw>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/ax88179_178a.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index fb0caa2..7a96326 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 (usb_device_no_sg_constraint(dev->udev))
+		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;
@@ -1310,6 +1318,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 v3 3/4] USBNET: support DMA SG

[Oliver Neukum]

On Tue, 2013-08-06 at 08:52 +0800, Ming Lei wrote:
> This patch introduces support of DMA SG if the USB host controller
> which usbnet device is attached to is capable of building packet from
> discontinuous buffers.
> 
> The patch supports passing the skb fragment buffers to usb stack directly
> via urb->sg.
> 
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> Cc: Grant Grundler <grundler@google.com>
> Cc: Freddy Xin <freddy@asix.com.tw>
> Cc: Oliver Neukum <oneukum@suse.de>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
Acked-by: Oliver Neukum <oneukum@suse.de>

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

[Eric Dumazet]

On Tue, 2013-08-06 at 08:52 +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 TSO can be enabled
> and skb fragment buffers can be passed to usb stack via urb->sg
> directly.
> 
> With the patch, system CPU utilization decreased ~50% and throughput
> increased by ~10% when doing iperf client test on one ARM A15 dual
> core board.
> 

Nice ;)

>  	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
> @@ -1310,6 +1318,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;
> +	}
>  

My concern with setting TSO on reset() is the following :

Admin can disable TSO with

ethtool -K ethX tso off


Then, one hour later, or one month later, a reset happens, and this code
magically re-enables TSO

So, I really think this part should be removed from your patch.

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

[Ming Lei]

On Tue, Aug 6, 2013 at 8:22 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, 2013-08-06 at 08:52 +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 TSO can be enabled
>> and skb fragment buffers can be passed to usb stack via urb->sg
>> directly.
>>
>> With the patch, system CPU utilization decreased ~50% and throughput
>> increased by ~10% when doing iperf client test on one ARM A15 dual
>> core board.
>>
>
> Nice ;)
>
>>              AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
>> @@ -1310,6 +1318,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;
>> +     }
>>
>
> My concern with setting TSO on reset() is the following :
>
> Admin can disable TSO with
>
> ethtool -K ethX tso off
>
>
> Then, one hour later, or one month later, a reset happens, and this code
> magically re-enables TSO

The reset only happens during open(), and TSO can't be re-enabled magically,
unless the interface is re-opened by Admin.

>
> So, I really think this part should be removed from your patch.

OK, will remove it.


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 v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma

[Grant Grundler]

On Tue, Aug 6, 2013 at 5:22 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
...
>> @@ -1310,6 +1318,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;
>> +     }
>>
>
> My concern with setting TSO on reset() is the following :
>
> Admin can disable TSO with
>
> ethtool -K ethX tso off
>
>
> Then, one hour later, or one month later, a reset happens, and this code
> magically re-enables TSO
>
> So, I really think this part should be removed from your patch.

Following that logic, shouldn't all the features/hw_features settings
be removed from reset code path?

hw_features shouldn't change since power up.

FWIW, I do agree with you.

I'll note that any "hiccup" in the USB side that causes the device to
get dropped and re-probed will cause the same symptom. There is
nothing the driver can do about it in this case. Perhaps add some udev
rules to preserve ethtool settings the same way I've seen udev rules
to record MAC address to enumerate devices (eth0, eth1, etc.)

cheers,
grant

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

[Ming Lei]

On Wed, Aug 7, 2013 at 1:09 AM, Grant Grundler <grundler@google.com> wrote:
> On Tue, Aug 6, 2013 at 5:22 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> ...
>>> @@ -1310,6 +1318,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;
>>> +     }
>>>
>>
>> My concern with setting TSO on reset() is the following :
>>
>> Admin can disable TSO with
>>
>> ethtool -K ethX tso off
>>
>>
>> Then, one hour later, or one month later, a reset happens, and this code
>> magically re-enables TSO
>>
>> So, I really think this part should be removed from your patch.
>
> Following that logic, shouldn't all the features/hw_features settings
> be removed from reset code path?

This patch won't touch other settings because that isn't related with
this patch.

>
> hw_features shouldn't change since power up.
> FWIW, I do agree with you.
>
> I'll note that any "hiccup" in the USB side that causes the device to
> get dropped and re-probed will cause the same symptom. There is

I am afraid that PCI network devices' setting still won't survive unbound&
re-probed, will they?

> nothing the driver can do about it in this case. Perhaps add some udev
> rules to preserve ethtool settings the same way I've seen udev rules
> to record MAC address to enumerate devices (eth0, eth1, etc.)

Some usbnet devices may have random MAC address assigned in every
probe().


Thanks,
--
Ming Lei

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

[Grant Grundler]

On Tue, Aug 6, 2013 at 5:41 PM, Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> On Wed, Aug 7, 2013 at 1:09 AM, Grant Grundler <grundler-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
...
>> Following that logic, shouldn't all the features/hw_features settings
>> be removed from reset code path?
>
> This patch won't touch other settings because that isn't related with
> this patch.

Sorry - I didn't mean to imply your patch should do this. I was hoping
for a yes/no answer from you, Eric, or Dave.

...
>> I'll note that any "hiccup" in the USB side that causes the device to
>> get dropped and re-probed will cause the same symptom. There is
>
> I am afraid that PCI network devices' setting still won't survive unbound&
> re-probed, will they?

Correct - but PCI isn't as prone to "dropping off the bus" like USB
is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
that's never been true for any USB device.

>> nothing the driver can do about it in this case. Perhaps add some udev
>> rules to preserve ethtool settings the same way I've seen udev rules
>> to record MAC address to enumerate devices (eth0, eth1, etc.)
>
> Some usbnet devices may have random MAC address assigned in every
> probe().

Ugh - ok. Then those scripts are broken for those devices. C'est la Vie.

My point is the mechanism (udev) exists to preserve any settings
ethtool can read/record the state of.

cheers,
grant

ps. thanks again for posting the USBNET sg dma series - I will likely
back port those to our chromeos-3.8 tree in the near future.
--
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 v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma

[Ming Lei]

On Fri, Aug 9, 2013 at 1:25 AM, Grant Grundler <grundler@google.com> wrote:
> On Tue, Aug 6, 2013 at 5:41 PM, Ming Lei <ming.lei@canonical.com> wrote:
>> On Wed, Aug 7, 2013 at 1:09 AM, Grant Grundler <grundler@google.com> wrote:
> ...
>>> Following that logic, shouldn't all the features/hw_features settings
>>> be removed from reset code path?
>>
>> This patch won't touch other settings because that isn't related with
>> this patch.
>
> Sorry - I didn't mean to imply your patch should do this. I was hoping
> for a yes/no answer from you, Eric, or Dave.
>
> ...
>>> I'll note that any "hiccup" in the USB side that causes the device to
>>> get dropped and re-probed will cause the same symptom. There is
>>
>> I am afraid that PCI network devices' setting still won't survive unbound&
>> re-probed, will they?
>
> Correct - but PCI isn't as prone to "dropping off the bus" like USB

As far as I know, USB device still won't be disconnected easily, and
reset is possible, but we can make setting survive reset by implementing
.pre_reset() and .post_reset() callback. Or do you have other situation
of USB 'dropping off the bus'?

> is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
> that's never been true for any USB device.

I mean rmmod & modprobe still can reset setting of one PCI network
device after powering on the device, can't it?


Thanks,
--
Ming Lei

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

[Grant Grundler]

Ming,
We are splitting hairs now. :) I want to be clear I think your changes
are good and the rest of this conversation is just to learn something
new.

On Thu, Aug 8, 2013 at 4:48 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Fri, Aug 9, 2013 at 1:25 AM, Grant Grundler <grundler@google.com> wrote:
...
>>> I am afraid that PCI network devices' setting still won't survive unbound&
>>> re-probed, will they?
>>
>> Correct - but PCI isn't as prone to "dropping off the bus" like USB
>
> As far as I know, USB device still won't be disconnected easily, and
> reset is possible, but we can make setting survive reset by implementing
> .pre_reset() and .post_reset() callback. Or do you have other situation
> of USB 'dropping off the bus'?

So far only older USB core bugs like this one:
    https://codereview.chromium.org/4687002/show

I agree USB won't be disconnected easily.

>> is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
>> that's never been true for any USB device.
>
> I mean rmmod & modprobe still can reset setting of one PCI network
> device after powering on the device, can't it?

Definitely. But this isn't something that will "randomly" happen and
will leave tracks all over the place of it happening. So I'm not
worried about trying to debug this scenario.

thanks,
grant

>
>
> Thanks,
> --
> Ming Lei

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

[Ming Lei]

On Fri, Aug 9, 2013 at 8:18 AM, Grant Grundler <grundler@google.com> wrote:
> Ming,
> We are splitting hairs now. :) I want to be clear I think your changes
> are good and the rest of this conversation is just to learn something
> new.
>
> On Thu, Aug 8, 2013 at 4:48 PM, Ming Lei <ming.lei@canonical.com> wrote:
>> On Fri, Aug 9, 2013 at 1:25 AM, Grant Grundler <grundler@google.com> wrote:
> ...
>>>> I am afraid that PCI network devices' setting still won't survive unbound&
>>>> re-probed, will they?
>>>
>>> Correct - but PCI isn't as prone to "dropping off the bus" like USB
>>
>> As far as I know, USB device still won't be disconnected easily, and
>> reset is possible, but we can make setting survive reset by implementing
>> .pre_reset() and .post_reset() callback. Or do you have other situation
>> of USB 'dropping off the bus'?
>
> So far only older USB core bugs like this one:
>     https://codereview.chromium.org/4687002/show

This happens in configuration change situation, which is seldom
triggered, and also not "randomly" happen per your standpoint,
just like rmmod/modprobe , :-)


> I agree USB won't be disconnected easily.
>
>>> is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
>>> that's never been true for any USB device.
>>
>> I mean rmmod & modprobe still can reset setting of one PCI network
>> device after powering on the device, can't it?
>
> Definitely. But this isn't something that will "randomly" happen and
> will leave tracks all over the place of it happening. So I'm not
> worried about trying to debug this scenario.

Thanks,
--
Ming Lei