On Fri, 03 Dec 2021, Bjørn Mork wrote:
> Lee Jones <lee.jones@xxxxxxxxxx> 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@xxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Bjørn Mork <bjorn@xxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
>
> 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.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
