* [PATCH v3 0/4] USB & USBNET: loose SG check and support usbnet DMA SG @ 2013-08-06 0:52 Ming Lei 2013-08-06 0:52 ` [PATCH v3 1/4] USB: introduce usb_device_no_sg_constraint() helper Ming Lei ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Ming Lei @ 2013-08-06 0:52 UTC (permalink / raw) To: David S. Miller, Greg Kroah-Hartman Cc: Oliver Neukum, Sarah Sharp, netdev, linux-usb 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/4] USB: introduce usb_device_no_sg_constraint() helper 2013-08-06 0:52 [PATCH v3 0/4] USB & USBNET: loose SG check and support usbnet DMA SG Ming Lei @ 2013-08-06 0:52 ` Ming Lei 2013-08-06 0:52 ` [PATCH v3 2/4] USB: XHCI: mark no_sg_constraint Ming Lei ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Ming Lei @ 2013-08-06 0:52 UTC (permalink / raw) To: David S. Miller, Greg Kroah-Hartman Cc: Oliver Neukum, Sarah Sharp, netdev, linux-usb, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/4] USB: XHCI: mark no_sg_constraint 2013-08-06 0:52 [PATCH v3 0/4] USB & USBNET: loose SG check and support usbnet DMA SG Ming Lei 2013-08-06 0:52 ` [PATCH v3 1/4] USB: introduce usb_device_no_sg_constraint() helper Ming Lei @ 2013-08-06 0:52 ` Ming Lei 2013-08-06 0:52 ` [PATCH v3 3/4] USBNET: support DMA SG Ming Lei 2013-08-06 0:52 ` [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma Ming Lei 3 siblings, 0 replies; 14+ messages in thread From: Ming Lei @ 2013-08-06 0:52 UTC (permalink / raw) To: David S. Miller, Greg Kroah-Hartman Cc: Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ming Lei, Alan Stern 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/4] USBNET: support DMA SG 2013-08-06 0:52 [PATCH v3 0/4] USB & USBNET: loose SG check and support usbnet DMA SG Ming Lei 2013-08-06 0:52 ` [PATCH v3 1/4] USB: introduce usb_device_no_sg_constraint() helper Ming Lei 2013-08-06 0:52 ` [PATCH v3 2/4] USB: XHCI: mark no_sg_constraint Ming Lei @ 2013-08-06 0:52 ` Ming Lei 2013-08-06 12:08 ` Oliver Neukum 2013-08-06 0:52 ` [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma Ming Lei 3 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2013-08-06 0:52 UTC (permalink / raw) To: David S. Miller, Greg Kroah-Hartman Cc: Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ming Lei, Eric Dumazet, Ben Hutchings, Grant Grundler, Freddy Xin, Alan Stern 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] USBNET: support DMA SG 2013-08-06 0:52 ` [PATCH v3 3/4] USBNET: support DMA SG Ming Lei @ 2013-08-06 12:08 ` Oliver Neukum 0 siblings, 0 replies; 14+ messages in thread From: Oliver Neukum @ 2013-08-06 12:08 UTC (permalink / raw) To: Ming Lei Cc: David S. Miller, Greg Kroah-Hartman, Sarah Sharp, netdev, linux-usb, Eric Dumazet, Ben Hutchings, Grant Grundler, Freddy Xin, Alan Stern 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-06 0:52 [PATCH v3 0/4] USB & USBNET: loose SG check and support usbnet DMA SG Ming Lei ` (2 preceding siblings ...) 2013-08-06 0:52 ` [PATCH v3 3/4] USBNET: support DMA SG Ming Lei @ 2013-08-06 0:52 ` Ming Lei 2013-08-06 12:22 ` Eric Dumazet 3 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2013-08-06 0:52 UTC (permalink / raw) To: David S. Miller, Greg Kroah-Hartman Cc: Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ming Lei, Eric Dumazet, Ben Hutchings, Grant Grundler, Alan Stern, Freddy Xin 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-06 0:52 ` [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma Ming Lei @ 2013-08-06 12:22 ` Eric Dumazet 2013-08-06 15:07 ` Ming Lei 2013-08-06 17:09 ` Grant Grundler 0 siblings, 2 replies; 14+ messages in thread From: Eric Dumazet @ 2013-08-06 12:22 UTC (permalink / raw) To: Ming Lei Cc: David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ben Hutchings, Grant Grundler, Alan Stern, Freddy Xin 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-06 12:22 ` Eric Dumazet @ 2013-08-06 15:07 ` Ming Lei 2013-08-06 17:09 ` Grant Grundler 1 sibling, 0 replies; 14+ messages in thread From: Ming Lei @ 2013-08-06 15:07 UTC (permalink / raw) To: Eric Dumazet Cc: David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA, Ben Hutchings, Grant Grundler, Alan Stern, Freddy Xin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-06 12:22 ` Eric Dumazet 2013-08-06 15:07 ` Ming Lei @ 2013-08-06 17:09 ` Grant Grundler 2013-08-07 0:41 ` Ming Lei 1 sibling, 1 reply; 14+ messages in thread From: Grant Grundler @ 2013-08-06 17:09 UTC (permalink / raw) To: Eric Dumazet Cc: Ming Lei, David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ben Hutchings, Alan Stern, Freddy Xin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-06 17:09 ` Grant Grundler @ 2013-08-07 0:41 ` Ming Lei 2013-08-08 17:25 ` Grant Grundler 0 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2013-08-07 0:41 UTC (permalink / raw) To: Grant Grundler Cc: Eric Dumazet, David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ben Hutchings, Alan Stern, Freddy Xin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-07 0:41 ` Ming Lei @ 2013-08-08 17:25 ` Grant Grundler 2013-08-08 23:48 ` Ming Lei 0 siblings, 1 reply; 14+ messages in thread From: Grant Grundler @ 2013-08-08 17:25 UTC (permalink / raw) To: Ming Lei Cc: Eric Dumazet, David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev, linux-usb-u79uwXL29TY76Z2rM5mHXA, Ben Hutchings, Alan Stern, Freddy Xin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-08 17:25 ` Grant Grundler @ 2013-08-08 23:48 ` Ming Lei 2013-08-09 0:18 ` Grant Grundler 0 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2013-08-08 23:48 UTC (permalink / raw) To: Grant Grundler Cc: Eric Dumazet, David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ben Hutchings, Alan Stern, Freddy Xin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-08 23:48 ` Ming Lei @ 2013-08-09 0:18 ` Grant Grundler 2013-08-09 1:44 ` Ming Lei 0 siblings, 1 reply; 14+ messages in thread From: Grant Grundler @ 2013-08-09 0:18 UTC (permalink / raw) To: Ming Lei Cc: Eric Dumazet, David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ben Hutchings, Alan Stern, Freddy Xin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma 2013-08-09 0:18 ` Grant Grundler @ 2013-08-09 1:44 ` Ming Lei 0 siblings, 0 replies; 14+ messages in thread From: Ming Lei @ 2013-08-09 1:44 UTC (permalink / raw) To: Grant Grundler Cc: Eric Dumazet, David S. Miller, Greg Kroah-Hartman, Oliver Neukum, Sarah Sharp, netdev, linux-usb, Ben Hutchings, Alan Stern, Freddy Xin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-08-09 1:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-06 0:52 [PATCH v3 0/4] USB & USBNET: loose SG check and support usbnet DMA SG Ming Lei 2013-08-06 0:52 ` [PATCH v3 1/4] USB: introduce usb_device_no_sg_constraint() helper Ming Lei 2013-08-06 0:52 ` [PATCH v3 2/4] USB: XHCI: mark no_sg_constraint Ming Lei 2013-08-06 0:52 ` [PATCH v3 3/4] USBNET: support DMA SG Ming Lei 2013-08-06 12:08 ` Oliver Neukum 2013-08-06 0:52 ` [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma Ming Lei 2013-08-06 12:22 ` Eric Dumazet 2013-08-06 15:07 ` Ming Lei 2013-08-06 17:09 ` Grant Grundler 2013-08-07 0:41 ` Ming Lei 2013-08-08 17:25 ` Grant Grundler 2013-08-08 23:48 ` Ming Lei 2013-08-09 0:18 ` Grant Grundler 2013-08-09 1:44 ` Ming Lei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).