From: "Bjørn Mork" <bjorn@mork.no>
To: Lee Jones <lee.jones@linaro.org>
Cc: Jakub Kicinski <kuba@kernel.org>,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
Oliver Neukum <oliver@neukum.org>,
"David S. Miller" <davem@davemloft.net>,
linux-usb@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH 1/1] net: cdc_ncm: Allow for dwNtbOutMaxSize to be unset or zero
Date: Fri, 03 Dec 2021 15:36:55 +0100 [thread overview]
Message-ID: <871r2tkb5k.fsf@miraculix.mork.no> (raw)
In-Reply-To: <YaoeKfmJrDPhMXWp@google.com> (Lee Jones's message of "Fri, 3 Dec 2021 13:39:53 +0000")
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.
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 supports
There 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.
> Perhaps we should use max_t() here instead of clamp?
No. That would allow userspace to set an unlimited buffer size.
Bjørn
next prev parent reply other threads:[~2021-12-03 14:37 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-02 14:34 [PATCH 1/1] net: cdc_ncm: Allow for dwNtbOutMaxSize to be unset or zero Lee Jones
2021-12-03 1:51 ` Jakub Kicinski
2021-12-03 10:29 ` Bjørn Mork
2021-12-03 11:25 ` Lee Jones
2021-12-03 12:57 ` Bjørn Mork
2021-12-03 13:39 ` Lee Jones
2021-12-03 14:36 ` Bjørn Mork [this message]
2021-12-03 14:46 ` Lee Jones
2021-12-03 14:52 ` Bjørn Mork
2021-12-04 0:57 ` Jakub Kicinski
2023-05-17 13:38 ` [PATCH] net: cdc_ncm: Deal with too low values of dwNtbOutMaxSize Tudor Ambarus
2023-05-17 13:38 ` Tudor Ambarus
2023-05-18 16:42 ` Simon Horman
2023-05-19 3:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871r2tkb5k.fsf@miraculix.mork.no \
--to=bjorn@mork.no \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=oliver@neukum.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.