| Message ID | 1401435070-26721-4-git-send-email-bjorn@mork.no |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Bjørn Mork > It doesn't matter whether the buffer size goes up or down. We have to > keep usbnet and device syncronized to be able to split transfers at the > correct boundaries. The spec allow skipping short packets when using > max sized transfers. If we don't tell usbnet about our new expected rx > buffer size, then it will merge and/or split NTBs. The driver does not > support this, and the result will be lots of framing errors. > > Fix by always reallocating usbnet rx buffers when the rx_max value > changes. I'm guessing that the rx_max value is the maximum size of the USB bulk data 'message' that the device generates? As such the URB only need to be longer that it. (Or multiples of the USB packet size, and the driver then merge URB when generating skbs.) Since you are now copying the data out of the URB's skb before passing the ethernet packet upstream, is there ever any real requirement to use a small rx_max? or ever change rx_max? David > Fixes: 68864abf08f0 ("net: cdc_ncm: support rx_max/tx_max updates when running") > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > drivers/net/usb/cdc_ncm.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > index ff5b3a854898..5bce86a0d063 100644 > --- a/drivers/net/usb/cdc_ncm.c > +++ b/drivers/net/usb/cdc_ncm.c > @@ -215,19 +215,12 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx) > min, max, val); > } > > - /* usbnet use these values for sizing rx queues */ > - dev->rx_urb_size = val; > - > /* inform device about NTB input size changes */ > if (val != ctx->rx_max) { > __le32 dwNtbInMaxSize = cpu_to_le32(val); > > dev_info(&dev->intf->dev, "setting rx_max = %u\n", val); > > - /* need to unlink rx urbs before increasing buffer size */ > - if (netif_running(dev->net) && dev->rx_urb_size > ctx->rx_max) > - usbnet_unlink_rx_urbs(dev); > - > /* tell device to use new size */ > if (usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE, > USB_TYPE_CLASS | USB_DIR_OUT > @@ -238,6 +231,13 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx) > ctx->rx_max = val; > } > > + /* usbnet use these values for sizing rx queues */ > + if (dev->rx_urb_size != ctx->rx_max) { > + dev->rx_urb_size = ctx->rx_max; > + if (netif_running(dev->net)) > + usbnet_unlink_rx_urbs(dev); > + } > + > /* clamp new_tx to sane values */ > min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth16); > max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)); > -- > 2.0.0.rc4
David Laight <David.Laight@ACULAB.COM> writes: > From: Bjørn Mork >> It doesn't matter whether the buffer size goes up or down. We have to >> keep usbnet and device syncronized to be able to split transfers at the >> correct boundaries. The spec allow skipping short packets when using >> max sized transfers. If we don't tell usbnet about our new expected rx >> buffer size, then it will merge and/or split NTBs. The driver does not >> support this, and the result will be lots of framing errors. >> >> Fix by always reallocating usbnet rx buffers when the rx_max value >> changes. > > I'm guessing that the rx_max value is the maximum size of the USB bulk > data 'message' that the device generates? > > As such the URB only need to be longer that it. So did I think too at first. That's how I added the bug fixed by this commit :-) The problem with NCM is that it explicitly allows (and encourage) using transfers which are multiples of the USB packet size, *without* any terminating short packet (0 or 1 byte). This means that the USB core won't know or care about the end of one transfer and the beginning of the next. Which is fine. But the cdc_ncm driver has to know, because it must split the transfers into frames it can decode. Now, the current cdc_ncm implementation has a built-in assumption that the size of the URB == rx_max. This lets it simplify the splitting into frames to nearly nothing: Any received URB contains exactly one frame. Therefore we need to keep the rx URB size strictly syncronized the rx_max. > (Or multiples of the USB packet size, and the driver then merge URB > when generating skbs.) > > Since you are now copying the data out of the URB's skb before > passing the ethernet packet upstream, is there ever any real > requirement to use a small rx_max? or ever change rx_max? Yes. usbnet doesn't currently recycle skbs. Continuously allocating 32 kB skbs (or even 64 kB truesize, which is the current cdc_ncm worst case) on memory constrained hosts is bound to fail eventually. Reducing rx_max will reduce the order of the allocations and thereby the probability of failing. I've had reports from OpenWRT users with Huawei modems (which tend to use extreme buffers), constantly hitting the usbnet allocation failure event 'EVENT_RX_MEMORY'. The result is of course terrible performance, where usbnet gets really busy trying to allocate buffers which just cannot be allocated... Reducing rx_max by simply rebuilding the old driver with a lower hard limit has pretty much eliminated these issues, but is not generally a solution because it makes some modem firmwares fail. True, copying instead of cloning is probably going to help this problem too, but I believe there are still reasons why you do not want to allow the 64 kB skb abuse. 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
David Laight <David.Laight@ACULAB.COM> writes: > From: Bjørn Mork >> David Laight <David.Laight@ACULAB.COM> writes: >> > From: Bjørn Mork >> >> It doesn't matter whether the buffer size goes up or down. We have to >> >> keep usbnet and device syncronized to be able to split transfers at the >> >> correct boundaries. The spec allow skipping short packets when using >> >> max sized transfers. If we don't tell usbnet about our new expected rx >> >> buffer size, then it will merge and/or split NTBs. The driver does not >> >> support this, and the result will be lots of framing errors. >> >> >> >> Fix by always reallocating usbnet rx buffers when the rx_max value >> >> changes. >> > >> > I'm guessing that the rx_max value is the maximum size of the USB bulk >> > data 'message' that the device generates? >> > >> > As such the URB only need to be longer that it. >> >> So did I think too at first. That's how I added the bug fixed by this >> commit :-) >> >> The problem with NCM is that it explicitly allows (and encourage) using >> transfers which are multiples of the USB packet size, *without* any >> terminating short packet (0 or 1 byte). This means that the USB core >> won't know or care about the end of one transfer and the beginning of >> the next. Which is fine. But the cdc_ncm driver has to know, because >> it must split the transfers into frames it can decode. >> >> Now, the current cdc_ncm implementation has a built-in assumption that >> the size of the URB == rx_max. This lets it simplify the splitting into >> frames to nearly nothing: Any received URB contains exactly one frame. >> Therefore we need to keep the rx URB size strictly syncronized the >> rx_max. > > Hmmm.... > So there is an ethernet packet size somewhere near 500 that exactly fills > a 512 byte USB2 frame. > If you receive one of those does the hardware send a 0 length terminating > fragment? > If not the then URB won't complete until after the next ethernet frame > arrives. > Receive three of them followed by a longer frame and you'll overrun the > end of the URB. > > At least one path through usbnet ensures that the rx buffers aren't > a multiple of the USB frame size. I'm not sure whether that really helps. > > Sounds like the hardware expects you to receive each USB bulk buffer > separately. Please, go read the NCM spec. 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
David Laight <David.Laight@ACULAB.COM> writes: > From: Bjørn Mork > ... >> > Since you are now copying the data out of the URB's skb before >> > passing the ethernet packet upstream, is there ever any real >> > requirement to use a small rx_max? or ever change rx_max? >> >> Yes. usbnet doesn't currently recycle skbs. Continuously allocating 32 >> kB skbs (or even 64 kB truesize, which is the current cdc_ncm worst >> case) on memory constrained hosts is bound to fail eventually. > > Getting usbnet to recycle skb shouldn't be too hard - and will > be an immediate gain for most of the drivers. Yes. This is one of those problems that really isn't that hard to solve. Until you start hitting all the corner cases... But I'm sure it is doable. Anyway, it is not going to be part of this patch series, which is merely touching the cdc_ncm minidriver within the existing usbnet framework. 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
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index ff5b3a854898..5bce86a0d063 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -215,19 +215,12 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx) min, max, val); } - /* usbnet use these values for sizing rx queues */ - dev->rx_urb_size = val; - /* inform device about NTB input size changes */ if (val != ctx->rx_max) { __le32 dwNtbInMaxSize = cpu_to_le32(val); dev_info(&dev->intf->dev, "setting rx_max = %u\n", val); - /* need to unlink rx urbs before increasing buffer size */ - if (netif_running(dev->net) && dev->rx_urb_size > ctx->rx_max) - usbnet_unlink_rx_urbs(dev); - /* tell device to use new size */ if (usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE, USB_TYPE_CLASS | USB_DIR_OUT @@ -238,6 +231,13 @@ static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx) ctx->rx_max = val; } + /* usbnet use these values for sizing rx queues */ + if (dev->rx_urb_size != ctx->rx_max) { + dev->rx_urb_size = ctx->rx_max; + if (netif_running(dev->net)) + usbnet_unlink_rx_urbs(dev); + } + /* clamp new_tx to sane values */ min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth16); max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
It doesn't matter whether the buffer size goes up or down. We have to keep usbnet and device syncronized to be able to split transfers at the correct boundaries. The spec allow skipping short packets when using max sized transfers. If we don't tell usbnet about our new expected rx buffer size, then it will merge and/or split NTBs. The driver does not support this, and the result will be lots of framing errors. Fix by always reallocating usbnet rx buffers when the rx_max value changes. Fixes: 68864abf08f0 ("net: cdc_ncm: support rx_max/tx_max updates when running") Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/usb/cdc_ncm.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)