| Message ID | 1350592867-25651-3-git-send-email-bjorn@mork.no |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote: > If the device refuses our updated value, then we must be prepared > to receive URBs as big as the device wants to send. Set rx_max > to the device provided value on error. Problematic in principle. How do you allocate a buffer of arbitrary size? 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 <oliver@neukum.org> writes: > On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote: >> If the device refuses our updated value, then we must be prepared >> to receive URBs as big as the device wants to send. Set rx_max >> to the device provided value on error. > > Problematic in principle. How do you allocate a buffer of arbitrary size? You cannot of course. You can only try and give up if it doesn't work. rx_submit would end up returning -ENOMEM, but we are not always checking that so it will most likely fail silently. But I don't think we can just continue with the smaller buffer size without having the device agree to that either. That is also likely to fail silently. Note that this patch was added exactly because one of the MBIM test devices did refuse the lower rx_max we tried to enforce. The device insists on using 128kB buffers. Maybe we should cap it at some arbitrary reasonable value, and just bail out from bind if the device insists on a larger buffer? Would that be OK? How big buffers are (semi-)reasonable? 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, Oct 19, 2012 at 12:09 AM, Bjørn Mork <bjorn@mork.no> wrote: > Oliver Neukum <oliver@neukum.org> writes: >> On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote: >>> If the device refuses our updated value, then we must be prepared >>> to receive URBs as big as the device wants to send. Set rx_max >>> to the device provided value on error. >> >> Problematic in principle. How do you allocate a buffer of arbitrary size? > > You cannot of course. You can only try and give up if it doesn't work. > rx_submit would end up returning -ENOMEM, but we are not always checking > that so it will most likely fail silently. > > But I don't think we can just continue with the smaller buffer size > without having the device agree to that either. That is also likely to > fail silently. Note that this patch was added exactly because one of > the MBIM test devices did refuse the lower rx_max we tried to enforce. > The device insists on using 128kB buffers. > > Maybe we should cap it at some arbitrary reasonable value, and just bail > out from bind if the device insists on a larger buffer? Would that be > OK? How big buffers are (semi-)reasonable? > I recommend to drop this.Vendor has to fix firmware. Current version of the driver supports 16-bit NTB, which means you can address (64K only - NTB header). So, how do you plan to use 64K-128K buffer space, if it can't be addressed by 16 bit offset? Another angle to big buffers, even while using 64K buffers your TCP connection will suffer, so what's the point making huge buffers? /alexey -- 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
Alexey Orishko <alexey.orishko@gmail.com> writes: > On Fri, Oct 19, 2012 at 12:09 AM, Bjørn Mork <bjorn@mork.no> wrote: >> Oliver Neukum <oliver@neukum.org> writes: >>> On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote: >>>> If the device refuses our updated value, then we must be prepared >>>> to receive URBs as big as the device wants to send. Set rx_max >>>> to the device provided value on error. >>> >>> Problematic in principle. How do you allocate a buffer of arbitrary size? >> >> You cannot of course. You can only try and give up if it doesn't work. >> rx_submit would end up returning -ENOMEM, but we are not always checking >> that so it will most likely fail silently. >> >> But I don't think we can just continue with the smaller buffer size >> without having the device agree to that either. That is also likely to >> fail silently. Note that this patch was added exactly because one of >> the MBIM test devices did refuse the lower rx_max we tried to enforce. >> The device insists on using 128kB buffers. >> >> Maybe we should cap it at some arbitrary reasonable value, and just bail >> out from bind if the device insists on a larger buffer? Would that be >> OK? How big buffers are (semi-)reasonable? >> > > I recommend to drop this. OK, will drop patch 01 (only necessary if some usbnet minidriver uses buffers > 60 * 1518) and 02 from the next version of this series. > Vendor has to fix firmware. I agree in principle, and I'll report the problem to them. But as usual I believe we have to support any weird firmware we encounter, if at all possible. > Current version of the driver supports 16-bit NTB, which means you can address > (64K only - NTB header). So, how do you plan to use 64K-128K buffer space, > if it can't be addressed by 16 bit offset? This is of course true. The device does obey the 16bit header choice, so I would hope that it does not send us buffers it can't use. But it does send buffers in the range 32K-64K, which makes the current driver fail in a rather ugly way. I assume the current CDC_NCM_NTB_MAX_SIZE_RX is set to 32K as a sane default, but how about allowing up to 64K for devices which does not accept this? The other options are - silently failing, or - refusing to load with an error the user cannot do anything with, and I don't think either of those are wanted, even if the NCM spec is quite clear that the device is wrong here. > Another angle to big buffers, even while using 64K buffers your TCP connection > will suffer, so what's the point making huge buffers? Agreed. There is no point. It's bloat. Which makes you kind of wonder why they bothered to define a separate 32bit header format at all, complicating the protocol quite a lot... Or why those writing the MBIM spec didn't take their opportunity to remove this useless complication? I am not holding it against you though ;-) A nice side effect of the refactoring done to support MBIM is that most of the 16bit header parsing has been isolated in separate functions, making it trivial to implement the missing 32bit header support. Maybe we should do that? 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
Bjørn Mork <bjorn@mork.no> writes: > Alexey Orishko <alexey.orishko@gmail.com> writes: > >> Vendor has to fix firmware. > > I agree in principle, and I'll report the problem to them. But as usual > I believe we have to support any weird firmware we encounter, if at all > possible. OK, I did some more experiments, and I am wondering if the real firmware problem is in the MBIM descriptor. It is CDC MBIM: bcdMBIMVersion 1.00 wMaxControlMessage 1536 bNumberFilters 16 bMaxFilterSize 40 wMaxSegmentSize 4096 bmNetworkCapabilities 0x20 8-byte ntb input size so we use the 8-byte version of USB_CDC_SET_NTB_INPUT_SIZE, which fails with -EPIPE. But forcing the 4-byte version seems to work. Hmm, I also see that the device returns 4 bytes in response to at USB_CDC_GET_NTB_INPUT_SIZE with an 8-byte buffer. Maybe we can auto-quirk based on that? I.e., if USB_CDC_GET_NTB_INPUT_SIZE returns only 4 bytes then we assume that the bmNetworkCapabilities flag is wrong. Is that acceptable? Then it seems we are able to inform this device of the reduced buffer, and the other problems can be ignored. 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, Oct 19, 2012 at 11:30 AM, Bjørn Mork <bjorn@mork.no> wrote: > Bjørn Mork <bjorn@mork.no> writes: >> Alexey Orishko <alexey.orishko@gmail.com> writes: >> > OK, I did some more experiments, and I am wondering if the real firmware > problem is in the MBIM descriptor. It is > > CDC MBIM: > bcdMBIMVersion 1.00 > wMaxControlMessage 1536 > bNumberFilters 16 > bMaxFilterSize 40 > wMaxSegmentSize 4096 > bmNetworkCapabilities 0x20 > 8-byte ntb input size > > > so we use the 8-byte version of USB_CDC_SET_NTB_INPUT_SIZE, which fails > with -EPIPE. But forcing the 4-byte version seems to work. Hmm, I also > see that the device returns 4 bytes in response to at > USB_CDC_GET_NTB_INPUT_SIZE with an 8-byte buffer. Maybe we can > auto-quirk based on that? I.e., if USB_CDC_GET_NTB_INPUT_SIZE returns > only 4 bytes then we assume that the bmNetworkCapabilities flag is > wrong. > > Is that acceptable? Then it seems we are able to inform this device of > the reduced buffer, and the other problems can be ignored. > Based on NCM errata, NCM functional descriptor: if Bit 5 is set, then device can (must) handle 8-byte form of Set/GetNtbInputSize. If they set a flag, but don't support the feature, hm.. is it a prototype device or commercially available one? At least device must support Set request, but for GetNtbInputSize we could happily live without wNtbInMaxDatagrams (i.e. use 4 byte variant) on Linux. Since we must anyway receive a complete NTB and making any number of skb buffers from received NTB is not a problem at all. regards, alexey -- 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
Alexey Orishko <alexey.orishko@gmail.com> writes: > On Fri, Oct 19, 2012 at 11:30 AM, Bjørn Mork <bjorn@mork.no> wrote: >> Bjørn Mork <bjorn@mork.no> writes: >>> Alexey Orishko <alexey.orishko@gmail.com> writes: >>> >> OK, I did some more experiments, and I am wondering if the real firmware >> problem is in the MBIM descriptor. It is >> >> CDC MBIM: >> bcdMBIMVersion 1.00 >> wMaxControlMessage 1536 >> bNumberFilters 16 >> bMaxFilterSize 40 >> wMaxSegmentSize 4096 >> bmNetworkCapabilities 0x20 >> 8-byte ntb input size >> >> >> so we use the 8-byte version of USB_CDC_SET_NTB_INPUT_SIZE, which fails >> with -EPIPE. But forcing the 4-byte version seems to work. Hmm, I also >> see that the device returns 4 bytes in response to at >> USB_CDC_GET_NTB_INPUT_SIZE with an 8-byte buffer. Maybe we can >> auto-quirk based on that? I.e., if USB_CDC_GET_NTB_INPUT_SIZE returns >> only 4 bytes then we assume that the bmNetworkCapabilities flag is >> wrong. >> >> Is that acceptable? Then it seems we are able to inform this device of >> the reduced buffer, and the other problems can be ignored. >> > > Based on NCM errata, NCM functional descriptor: if Bit 5 is set, then > device can (must) handle 8-byte form of Set/GetNtbInputSize. Yes, and this flag is copied with the same requirements in MBIM. So it is definitely a firmware bug. > If they set a flag, but don't support the feature, hm.. is it a > prototype device or > commercially available one? The firmware is not commercially availabe AFAIK, but based on experience I believe we should assume that any firmware bug ever seen will live forever. There are likely other devices with the same bug even if this firmware and this device, and all other devices from the same vendor, are fixed. I believe the problem here is that the USB descriptors are somewhat decoupled from the firmware functions. The firmware functions are usually implemented in firmware originating from the chipset vendor, while the descriptors are up to the device vendor. This has led to interesting situations before. But for us, I believe it means that we should put more trust in the responses to control messages than in functional descriptors. So if we can detect a mismatch like this one, then we do that and use the control message. > At least device must support Set request, but for GetNtbInputSize we > could happily > live without wNtbInMaxDatagrams (i.e. use 4 byte variant) on Linux. > Since we must anyway receive a complete NTB and making any number of skb > buffers from received NTB is not a problem at all. Yes, it doesn't matter to us whether we get the 8 byte variant or not. We are prepared to handle the 4 byte variant in any case, if the functional descriptor flag is not set. So I believe a fallback to 4 byte should not pose any problem. The only difference will be that we need to do the USB_CDC_GET_NTB_INPUT_SIZE to detect the bug in the first place. I'll cook up an alternative patch implementing this so you can evaluate the impact. 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
Alexey Orishko <alexey.orishko@gmail.com> writes: > Based on NCM errata, NCM functional descriptor: if Bit 5 is set, then > device can (must) handle 8-byte form of Set/GetNtbInputSize. > If they set a flag, but don't support the feature, hm.. is it a > prototype device or > commercially available one? > > At least device must support Set request, but for GetNtbInputSize we > could happily > live without wNtbInMaxDatagrams (i.e. use 4 byte variant) on Linux. > Since we must anyway receive a complete NTB and making any number of skb > buffers from received NTB is not a problem at all. OK, I may have misunderstood you here. Quoting the errata text: <quote> If bit D5 is set in the bmNetworkCapabilities field of function’s NCM Functional Descriptor, the host may set wLength either to 4 or to 8. If wLength is 4, the function shall assume that wNtbInMaxDatagrams is to be set to zero. If wLength is 8, then the function shall use the provided value as the limit. The function shall return an error response (a STALL PID) if wLength is set to any other value. </quote> So the 4 byte variant is always supported and we might as well just use it unconditionally because we don't set, or need to set, the wNtbInMaxDatagrams. Is that right? It will simplify the code even more without any loss of functionality, except for the possibility of failing on some other buggy device not supporting the 4 byte variant... 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, Oct 19, 2012 at 2:18 PM, Bjørn Mork <bjorn@mork.no> wrote: > > OK, I may have misunderstood you here. Quoting the errata text: > > <quote> > If bit D5 is set in the bmNetworkCapabilities field of function’s NCM > Functional Descriptor, the host may set wLength either to 4 or to > 8. If wLength is 4, the function shall assume that wNtbInMaxDatagrams > is to be set to zero. If wLength is 8, then the function shall use the > provided value as the limit. The function shall return an error > response (a STALL PID) if wLength is set to any other value. > </quote> > > So the 4 byte variant is always supported and we might as well just use > it unconditionally because we don't set, or need to set, the > wNtbInMaxDatagrams. > > Is that right? It will simplify the code even more without any loss of > functionality, except for the possibility of failing on some other buggy > device not supporting the 4 byte variant... Agree, since 4-byte version must be supported by all devices, we can drop 8-byte variant /alexey -- 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
Alexey Orishko <alexey.orishko@gmail.com> writes: > On Fri, Oct 19, 2012 at 2:18 PM, Bjørn Mork <bjorn@mork.no> wrote: >> >> OK, I may have misunderstood you here. Quoting the errata text: >> >> <quote> >> If bit D5 is set in the bmNetworkCapabilities field of function’s NCM >> Functional Descriptor, the host may set wLength either to 4 or to >> 8. If wLength is 4, the function shall assume that wNtbInMaxDatagrams >> is to be set to zero. If wLength is 8, then the function shall use the >> provided value as the limit. The function shall return an error >> response (a STALL PID) if wLength is set to any other value. >> </quote> >> >> So the 4 byte variant is always supported and we might as well just use >> it unconditionally because we don't set, or need to set, the >> wNtbInMaxDatagrams. >> >> Is that right? It will simplify the code even more without any loss of >> functionality, except for the possibility of failing on some other buggy >> device not supporting the 4 byte variant... > > Agree, since 4-byte version must be supported by all devices, > we can drop 8-byte variant Thanks. I'll implement that in the next version then, and drop the first patch as it is no longer needed. But I will hold off posting the updated series for a few more days to allow you all some time to digest the rest of this. There should be more issues to comment on here than this simple firmware bug workaround ;-) 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 4cd582a..6a65662 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -251,8 +251,11 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx) kfree(dwNtbInMaxSize); } size_err: - if (err < 0) - pr_debug("Setting NTB Input Size failed\n"); + if (err < 0) { + /* failed, so attempt to use the device suggested size */ + ctx->rx_max = le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize); + pr_debug("Setting NTB Input Size failed, reverting to %u\n", ctx->rx_max); + } } /* verify maximum size of transmitted NTB in bytes */
If the device refuses our updated value, then we must be prepared to receive URBs as big as the device wants to send. Set rx_max to the device provided value on error. Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/usb/cdc_ncm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)