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 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
Oliver Neukum [off-list ref] writes:
On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote:
quoted
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
On Fri, Oct 19, 2012 at 12:09 AM, Bjørn Mork [off-list ref] wrote:
Oliver Neukum [off-list ref] writes:
quoted
On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote:
quoted
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
Alexey Orishko [off-list ref] writes:
On Fri, Oct 19, 2012 at 12:09 AM, Bjørn Mork [off-list ref] wrote:
quoted
Oliver Neukum [off-list ref] writes:
quoted
On Thursday 18 October 2012 22:40:55 Bjørn Mork wrote:
quoted
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
Bjørn Mork [off-list ref] writes:
Alexey Orishko [off-list ref] writes:
quoted
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