On Fri, 03 Dec 2021, Bjørn Mork wrote:
Lee Jones lee.jones@linaro.org writes:
On Fri, 03 Dec 2021, Bjørn Mork wrote:
This I don't understand. If we have for example
new_tx = 0 max = 0 min = 1514(=datagram) + 8(=ndp) + 2(=1+1) * 4(=dpe) + 12(=nth) = 1542
then
max = max(min, max) = 1542 val = clamp_t(u32, new_tx, min, max) = 1542
so we return 1542 and everything is fine.
I don't believe so.
#define clamp_t(type, val, lo, hi) \ min_t(type, max_t(type, val, lo), hi)
So: min_t(u32, max_t(u32, 0, 1542), 0)
I don't think so. If we have:
new_tx = 0 max = 0 min = 1514(=datagram) + 8(=ndp) + 2(=1+1) * 4(=dpe) + 12(=nth) = 1542 max = max(min, max) = 1542
Then we have
min_t(u32, max_t(u32, 0, 1542), 1542)
If it wasn't clear - My proposal was to change this:
- min = min(min, max);
- max = max(min, max);
in the original code.
Oh, I see. Yes, I missed the reallocation of 'max'.
I thought we were using original values and just changing min() to max().
But looking further I don't think that's a good idea either. I searched through old email and found this commit:
commit a6fe67087d7cb916e41b4ad1b3a57c91150edb88 Author: Bjørn Mork bjorn@mork.no Date: Fri Nov 1 11:17:01 2013 +0100
net: cdc_ncm: no not set tx_max higher than the device supportsThere are MBIM devices out there reporting dwNtbInMaxSize=2048 dwNtbOutMaxSize=2048 and since the spec require a datagram max size of at least 2048, this means that a full sized datagram will never fit. Still, sending larger NTBs than the device supports is not going to help. We do not have any other options than either a) refusing to bindi, or b) respect the insanely low value. Alternative b will at least make these devices work, so go for it. Cc: Alexey Orishko alexey.orishko@gmail.com Signed-off-by: Bjørn Mork bjorn@mork.no Signed-off-by: David S. Miller davem@davemloft.net
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index 4531f38fc0e5..11c703337577 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -159,8 +159,7 @@ static u8 cdc_ncm_setup(struct usbnet *dev) } /* verify maximum size of transmitted NTB in bytes */
if ((ctx->tx_max < (CDC_NCM_MIN_HDR_SIZE + ctx->max_datagram_size)) ||(ctx->tx_max > CDC_NCM_NTB_MAX_SIZE_TX)) {
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;So there are real devices depending on a dwNtbOutMaxSize which is too low. Our calculated minimum for MBIM will not fit.
So let's go back your original test for zero. It's better than nothing. I'll just ack that.
Sure, no problem.
Thanks for conversing with me.
Perhaps we should use max_t() here instead of clamp?
No. That would allow userspace to set an unlimited buffer size.
Right, I see.