| Message ID | 1399736509-1159-2-git-send-email-bjorn@mork.no |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote: > Split out the part of setup dealing with updating the rx_max > and tx_max buffer sizes so that this code can be reused for > dynamically updating the limits. > > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > drivers/net/usb/cdc_ncm.c | 81 +++++++++++++++++++++++++++++------------------ > 1 file changed, 50 insertions(+), 31 deletions(-) > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c > index 549dbac710ed..87a32edf7ea5 100644 > --- a/drivers/net/usb/cdc_ncm.c > +++ b/drivers/net/usb/cdc_ncm.c > @@ -65,6 +65,54 @@ static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx); > static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer); > static struct usb_driver cdc_ncm_driver; > > +/* handle rx_max and tx_max changes */ > +static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx) > +{ > + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; > + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; > + u32 val, max, min; > + > + /* clamp new_rx to sane values */ > + min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > + max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); Are you sure this makes sense? min_t both times? Regards Oliver -- 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
Oliver Neukum <oneukum@suse.de> writes: > On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote: >> Split out the part of setup dealing with updating the rx_max >> and tx_max buffer sizes so that this code can be reused for >> dynamically updating the limits. >> >> Signed-off-by: Bjørn Mork <bjorn@mork.no> >> --- >> drivers/net/usb/cdc_ncm.c | 81 +++++++++++++++++++++++++++++------------------ >> 1 file changed, 50 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c >> index 549dbac710ed..87a32edf7ea5 100644 >> --- a/drivers/net/usb/cdc_ncm.c >> +++ b/drivers/net/usb/cdc_ncm.c >> @@ -65,6 +65,54 @@ static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx); >> static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer); >> static struct usb_driver cdc_ncm_driver; >> >> +/* handle rx_max and tx_max changes */ >> +static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx) >> +{ >> + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; >> + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; >> + u32 val, max, min; >> + >> + /* clamp new_rx to sane values */ >> + min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); >> + max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > > Are you sure this makes sense? min_t both times? Yes, I am sure. At least it made sense when I wrote it. I am more in doubt now. I guess you don't question the max calculation, but just so everyone else gets the idea: dwNtbInMaxSize is the buffer size suggested by the device. Some devices just specify an insanely large value (132kB has been observed). So we need to cap that to CDC_NCM_NTB_MAX_SIZE_RX, which is the absolutely largest buffer size we are prepared to support. USB_CDC_NCM_NTB_MIN_IN_SIZE is the minimum acceptable buffer size according to the spec. dwNtbInMaxSize is not allowed to be smaller than this. So if we assume that no device violates the spec, then the above should simple be min = USB_CDC_NCM_NTB_MIN_IN_SIZE; max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); which is the result for all spec conforming devices. The reason I put that min_t() there instead was an attempt to deal with the (not unlikely) event that some buggy device set dwNtbInMaxSize lower than this required minimum value. We then have the choices: a) fail to support the buggy device b) attempt to set a larger buffer size than the device supports c) accept the lower size So I chose c) in an attempt to be as gentle as possible. But I am open to go for a) instead if you think that is better. After all USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much more than the headers and a single full size ethernet frame. And I see now that we fail to do further sanity checking after this. What if dwNtbInMaxSize is 0? Or smaller than the necessary headers? Should I rewrite the above to do a) instead? I.e. min = USB_CDC_NCM_NTB_MIN_IN_SIZE; max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); if (min > max) fail; I don't think b) is a good idea. It might work, but it might also fail in surprising ways making it hard to debug. 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 Tue, 2014-05-13 at 10:49 +0200, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.de> writes: > > > On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote: > >> + /* clamp new_rx to sane values */ > >> + min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > >> + max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > > > > Are you sure this makes sense? min_t both times? > > Yes, I am sure. At least it made sense when I wrote it. I am more in > doubt now. I actually suspected a copy n' paste error; thence the formulation. > I guess you don't question the max calculation, but just so everyone > else gets the idea: dwNtbInMaxSize is the buffer size suggested by the > device. Some devices just specify an insanely large value (132kB has > been observed). So we need to cap that to CDC_NCM_NTB_MAX_SIZE_RX, which > is the absolutely largest buffer size we are prepared to support. Good > USB_CDC_NCM_NTB_MIN_IN_SIZE is the minimum acceptable buffer size > according to the spec. dwNtbInMaxSize is not allowed to be smaller than > this. So if we assume that no device violates the spec, then the above > should simple be > > min = USB_CDC_NCM_NTB_MIN_IN_SIZE; > max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > > which is the result for all spec conforming devices. > > The reason I put that min_t() there instead was an attempt to deal with > the (not unlikely) event that some buggy device set dwNtbInMaxSize lower > than this required minimum value. We then have the choices: > > a) fail to support the buggy device > b) attempt to set a larger buffer size than the device supports > c) accept the lower size My preference would be b) > a) > c) It seems to me that would should respect the spec and if the spec sets a lower limit then we don't go lower. > So I chose c) in an attempt to be as gentle as possible. But I am open > to go for a) instead if you think that is better. After all > USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much > more than the headers and a single full size ethernet frame. And I see > now that we fail to do further sanity checking after this. What if > dwNtbInMaxSize is 0? Or smaller than the necessary headers? Exactly. Some fool may simply overlook setting it at all. > Should I rewrite the above to do a) instead? I.e. > > min = USB_CDC_NCM_NTB_MIN_IN_SIZE; > max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > if (min > max) > fail; > > I don't think b) is a good idea. It might work, but it might also fail > in surprising ways making it hard to debug. Users may prefer working devices to clean failures, but I primarily care about conforming to spec. We just shouldn't do such violations in a general case. Regards Oliver -- 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
Oliver Neukum <oneukum@suse.de> writes: >> The reason I put that min_t() there instead was an attempt to deal with >> the (not unlikely) event that some buggy device set dwNtbInMaxSize lower >> than this required minimum value. We then have the choices: >> >> a) fail to support the buggy device >> b) attempt to set a larger buffer size than the device supports >> c) accept the lower size > > My preference would be b) > a) > c) > It seems to me that would should respect the spec and if the spec sets > a lower limit then we don't go lower. > >> So I chose c) in an attempt to be as gentle as possible. But I am open >> to go for a) instead if you think that is better. After all >> USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much >> more than the headers and a single full size ethernet frame. And I see >> now that we fail to do further sanity checking after this. What if >> dwNtbInMaxSize is 0? Or smaller than the necessary headers? > > Exactly. Some fool may simply overlook setting it at all. > >> Should I rewrite the above to do a) instead? I.e. >> >> min = USB_CDC_NCM_NTB_MIN_IN_SIZE; >> max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); >> if (min > max) >> fail; >> >> I don't think b) is a good idea. It might work, but it might also fail >> in surprising ways making it hard to debug. > > Users may prefer working devices to clean failures, but > I primarily care about conforming to spec. We just shouldn't > do such violations in a general case. Yes, I agree. Will change this. Let's try to go for b) then. I.e. min = USB_CDC_NCM_NTB_MIN_IN_SIZE; max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); if (max < min) max = min; 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 Tue, 2014-05-13 at 11:25 +0200, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.de> writes: > Yes, I agree. Will change this. Let's try to go for b) then. I.e. > > min = USB_CDC_NCM_NTB_MIN_IN_SIZE; > max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > if (max < min) > max = min; Thanks Regards Oliver -- 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 549dbac710ed..87a32edf7ea5 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -65,6 +65,54 @@ static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx); static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer); static struct usb_driver cdc_ncm_driver; +/* handle rx_max and tx_max changes */ +static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx) +{ + struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; + u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber; + u32 val, max, min; + + /* clamp new_rx to sane values */ + min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); + max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); + + val = clamp_t(u32, new_rx, min, max); + if (val != new_rx) { + dev_dbg(&dev->intf->dev, "rx_max must be in the [%u, %u] range. Using %u\n", + min, max, 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); + if (usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE, + USB_TYPE_CLASS | USB_DIR_OUT + | USB_RECIP_INTERFACE, + 0, iface_no, &dwNtbInMaxSize, 4) < 0) + dev_dbg(&dev->intf->dev, "Setting NTB Input Size failed\n"); + else + ctx->rx_max = val; + } + + /* clamp new_tx to sane values */ + min = CDC_NCM_MIN_HDR_SIZE + ctx->max_datagram_size; + max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)); + + /* some devices set dwNtbOutMaxSize too low for the above default */ + min = min(min, max); + + val = clamp_t(u32, new_tx, min, max); + if (val != new_tx) { + dev_dbg(&dev->intf->dev, "tx_max must be in the [%u, %u] range. Using %u\n", + min, max, val); + } + if (val != ctx->tx_max) + dev_info(&dev->intf->dev, "setting tx_max = %u\n", val); + ctx->tx_max = val; +} + static int cdc_ncm_setup(struct usbnet *dev) { struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; @@ -132,37 +180,8 @@ static int cdc_ncm_setup(struct usbnet *dev) (ctx->tx_max_datagrams > CDC_NCM_DPT_DATAGRAMS_MAX)) ctx->tx_max_datagrams = CDC_NCM_DPT_DATAGRAMS_MAX; - /* verify maximum size of received NTB in bytes */ - if (ctx->rx_max < USB_CDC_NCM_NTB_MIN_IN_SIZE) { - dev_dbg(&dev->intf->dev, "Using min receive length=%d\n", - USB_CDC_NCM_NTB_MIN_IN_SIZE); - ctx->rx_max = USB_CDC_NCM_NTB_MIN_IN_SIZE; - } - - if (ctx->rx_max > CDC_NCM_NTB_MAX_SIZE_RX) { - dev_dbg(&dev->intf->dev, "Using default maximum receive length=%d\n", - CDC_NCM_NTB_MAX_SIZE_RX); - ctx->rx_max = CDC_NCM_NTB_MAX_SIZE_RX; - } - - /* inform device about NTB input size changes */ - if (ctx->rx_max != le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)) { - __le32 dwNtbInMaxSize = cpu_to_le32(ctx->rx_max); - - err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_INPUT_SIZE, - USB_TYPE_CLASS | USB_DIR_OUT - | USB_RECIP_INTERFACE, - 0, iface_no, &dwNtbInMaxSize, 4); - if (err < 0) - dev_dbg(&dev->intf->dev, "Setting NTB Input Size failed\n"); - } - - /* verify maximum size of transmitted NTB in bytes */ - if (ctx->tx_max > CDC_NCM_NTB_MAX_SIZE_TX) { - dev_dbg(&dev->intf->dev, "Using default maximum transmit length=%d\n", - CDC_NCM_NTB_MAX_SIZE_TX); - ctx->tx_max = CDC_NCM_NTB_MAX_SIZE_TX; - } + /* clamp rx_max and tx_max and inform device */ + cdc_ncm_update_rxtx_max(dev, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize), le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)); /* * verify that the structure alignment is:
Split out the part of setup dealing with updating the rx_max and tx_max buffer sizes so that this code can be reused for dynamically updating the limits. Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/usb/cdc_ncm.c | 81 +++++++++++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 31 deletions(-)