| Message ID | 1424979823.4444.46.camel@xylophone.i.decadent.org.uk |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Ben Hutchings <ben.hutchings@codethink.co.uk> writes: > cdc_ncm disagrees with usbnet about how much framing overhead should > be counted in the tx_bytes statistics, and tries 'fix' this by > decrementing tx_bytes on the transmit path. But statistics must never > be decremented except due to roll-over; this will thoroughly confuse > user-space. Also, tx_bytes is only incremented by usbnet in the > completion path. > > Fix this by requiring drivers that set FLAG_MULTI_FRAME to set a > tx_bytes delta along with the tx_packets count. > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > --- > I noticed this bug while trying to fix the tx_packets statistic in asix. > It depends on the patch I just sent for that. I don't have any hardware > to test this with, or any need to make it work. If you want this fix, > please test and re-submit it yoursef. I tested this on an MBIM device, and it worked perfectly as-is. Please submit it without the RFC prefix. This fixes a real and reported problem with the cdc_ncm driver, so I'd claim it's "net" material along with a stable Cc and Fixes: beeecd42c3b4 ("net: cdc_ncm/cdc_mbim: adding NCM protocol statistics") But you and David decide that, of course... Fixing this was actually on my TODO-list after a recent report showing one of the problems with the previous hack: The decremented tx_bytes counter was never corrected if the URB transmission failed, possibly ending up with a negative (i.e. very large since it is unsigned) tx_bytes counter. Thanks a lot. I didn't know how to do this without introducing a new callback or something. Your solutions is very nice, and so obvious when I see it. Just brilliant :-) Tested-by: Bjørn Mork <bjorn@mork.no> And you might also want (if this is OK for Sami): Reported-by: Sami Farin <hvtaifwkbgefbaei@gmail.com> Sami wrote: > I have this USB ID 12d1:157d, Sonera Opengate LTE. > It says all is okay but no packets are moving. .. > ifconfig > wwp3s0u1c2: flags=4291<UP,BROADCAST,RUNNING,NOARP,MULTICAST> mtu 1500 > inet 46.132.188.224 netmask 255.255.255.192 broadcast 46.132.188.255 > inet6 fe80:: prefixlen 64 scopeid 0x20<link> > ether 32:35:3a:64:2e:25 txqueuelen 1000 (Ethernet) > RX packets 0 bytes 0 (0.0 B) > RX errors 17764 dropped 0 overruns 0 frame 0 > TX packets 6714 bytes 18446744073709268911 (1638.3 PiB) > TX errors 1427 dropped 0 overruns 0 carrier 0 collisions 0 > Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-02-27 at 10:12 +0100, Bjørn Mork wrote: > Ben Hutchings <ben.hutchings@codethink.co.uk> writes: > > > cdc_ncm disagrees with usbnet about how much framing overhead should > > be counted in the tx_bytes statistics, and tries 'fix' this by > > decrementing tx_bytes on the transmit path. But statistics must never > > be decremented except due to roll-over; this will thoroughly confuse > > user-space. Also, tx_bytes is only incremented by usbnet in the > > completion path. > > > > Fix this by requiring drivers that set FLAG_MULTI_FRAME to set a > > tx_bytes delta along with the tx_packets count. > > > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > > --- > > I noticed this bug while trying to fix the tx_packets statistic in asix. > > It depends on the patch I just sent for that. I don't have any hardware > > to test this with, or any need to make it work. If you want this fix, > > please test and re-submit it yoursef. > > I tested this on an MBIM device, and it worked perfectly as-is. Please > submit it without the RFC prefix. This fixes a real and reported > problem with the cdc_ncm driver, so I'd claim it's "net" material along > with a stable Cc and > > Fixes: beeecd42c3b4 ("net: cdc_ncm/cdc_mbim: adding NCM protocol statistics") > > But you and David decide that, of course... > > > Fixing this was actually on my TODO-list after a recent report showing > one of the problems with the previous hack: The decremented tx_bytes > counter was never corrected if the URB transmission failed, possibly > ending up with a negative (i.e. very large since it is unsigned) > tx_bytes counter. > > Thanks a lot. I didn't know how to do this without introducing a new > callback or something. Your solutions is very nice, and so obvious when > I see it. Just brilliant :-) > > Tested-by: Bjørn Mork <bjorn@mork.no> > > And you might also want (if this is OK for Sami): > > Reported-by: Sami Farin <hvtaifwkbgefbaei@gmail.com> Acked-by: Oliver Neukum <oliver@neukum.org> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index 724a9b50df7a..75d6f26729a3 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -189,7 +189,7 @@ struct sk_buff *asix_tx_fixup(struct usbnet *dev, struct sk_buff *skb, skb_put(skb, sizeof(padbytes)); } - usbnet_set_skb_tx_stats(skb, 1); + usbnet_set_skb_tx_stats(skb, 1, 0); return skb; } diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 70cbea551139..c3e4da9e79ca 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -1177,13 +1177,12 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign) ctx->tx_overhead += skb_out->len - ctx->tx_curr_frame_payload; ctx->tx_ntbs++; - /* usbnet has already counted all the framing overhead. + /* usbnet will count all the framing overhead by default. * Adjust the stats so that the tx_bytes counter show real * payload data instead. */ - dev->net->stats.tx_bytes -= skb_out->len - ctx->tx_curr_frame_payload; - - usbnet_set_skb_tx_stats(skb_out, n); + usbnet_set_skb_tx_stats(skb_out, n, + ctx->tx_curr_frame_payload - skb_out->len); return skb_out; diff --git a/drivers/net/usb/sr9800.c b/drivers/net/usb/sr9800.c index 7650cdc8fe6b..953de13267df 100644 --- a/drivers/net/usb/sr9800.c +++ b/drivers/net/usb/sr9800.c @@ -144,7 +144,7 @@ static struct sk_buff *sr_tx_fixup(struct usbnet *dev, struct sk_buff *skb, skb_put(skb, sizeof(padbytes)); } - usbnet_set_skb_tx_stats(skb, 1); + usbnet_set_skb_tx_stats(skb, 1, 0); return skb; } diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 0f3ff285f6a1..777757ae1973 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1346,9 +1346,19 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, } else urb->transfer_flags |= URB_ZERO_PACKET; } - entry->length = urb->transfer_buffer_length = length; - if (!(info->flags & FLAG_MULTI_PACKET)) - usbnet_set_skb_tx_stats(skb, 1); + urb->transfer_buffer_length = length; + + if (info->flags & FLAG_MULTI_PACKET) { + /* Driver has set number of packets and a length delta. + * Calculate the complete length and ensure that it's + * positive. + */ + entry->length += length; + if (WARN_ON_ONCE(entry->length <= 0)) + entry->length = length; + } else { + usbnet_set_skb_tx_stats(skb, 1, length); + } spin_lock_irqsave(&dev->txq.lock, flags); retval = usb_autopm_get_interface_async(dev->intf); diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index ff3fb2bd0e90..6e0ce8c7b8cb 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -227,7 +227,7 @@ struct skb_data { /* skb->cb is one of these */ struct urb *urb; struct usbnet *dev; enum skb_state state; - size_t length; + long length; unsigned long packets; }; @@ -235,11 +235,13 @@ struct skb_data { /* skb->cb is one of these */ * tx_fixup method before returning an skb. */ static inline void -usbnet_set_skb_tx_stats(struct sk_buff *skb, unsigned long packets) +usbnet_set_skb_tx_stats(struct sk_buff *skb, + unsigned long packets, long bytes_delta) { struct skb_data *entry = (struct skb_data *) skb->cb; entry->packets = packets; + entry->length = bytes_delta; } extern int usbnet_open(struct net_device *net);
cdc_ncm disagrees with usbnet about how much framing overhead should be counted in the tx_bytes statistics, and tries 'fix' this by decrementing tx_bytes on the transmit path. But statistics must never be decremented except due to roll-over; this will thoroughly confuse user-space. Also, tx_bytes is only incremented by usbnet in the completion path. Fix this by requiring drivers that set FLAG_MULTI_FRAME to set a tx_bytes delta along with the tx_packets count. Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> --- I noticed this bug while trying to fix the tx_packets statistic in asix. It depends on the patch I just sent for that. I don't have any hardware to test this with, or any need to make it work. If you want this fix, please test and re-submit it yoursef. Ben. drivers/net/usb/asix_common.c | 2 +- drivers/net/usb/cdc_ncm.c | 7 +++---- drivers/net/usb/sr9800.c | 2 +- drivers/net/usb/usbnet.c | 16 +++++++++++++--- include/linux/usb/usbnet.h | 6 ++++-- 5 files changed, 22 insertions(+), 11 deletions(-)