lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 9 Sep 2020 22:50:28 +0200
From:   Daniele Palmas <dnlplm@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Bjørn Mork <bjorn@...k.no>,
        Kristian Evensen <kristian.evensen@...il.com>,
        Paul Gildea <paul.gildea@...il.com>,
        Carl Yin <carl.yin@...ctel.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-usb <linux-usb@...r.kernel.org>
Subject: Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

Hi Greg,

Il giorno mer 9 set 2020 alle ore 14:28 Greg KH
<gregkh@...uxfoundation.org> ha scritto:
>
> On Wed, Sep 09, 2020 at 11:13:02AM +0200, Daniele Palmas wrote:
> > Add default rx_urb_size to support QMAP download data aggregation
> > without needing additional setup steps in userspace.
> >
> > The value chosen is the current highest one seen in available modems.
> >
> > The patch has the side-effect of fixing a babble issue in raw-ip mode
> > reported by multiple users.
> >
> > Signed-off-by: Daniele Palmas <dnlplm@...il.com>
>
> Any specific kernel commit that this "fixes:"?
>

Related to the aggregation, if I have to find a commit that would be
c6adf77953bcec0ad63d7782479452464e50f7a3 "net: usb: qmi_wwan: add qmap
mux protocol support", but I feel that this is an improvement rather
than a fix, since it avoids having userspace to configure the
rx_urb_size through changing the MTU of the master interface.

Related to the babble issue, my understanding is that it's not a
qmi_wwan issue, but a workaround for some chipsets' behavior.

> > ---
> > Resending with mailing lists added: sorry for the noise.
> >
> > Hi Bjørn and all,
> >
> > this patch tries to address the issue reported in the following threads
> >
> > https://www.spinics.net/lists/netdev/msg635944.html
> > https://www.spinics.net/lists/linux-usb/msg198846.html
> > https://www.spinics.net/lists/linux-usb/msg198025.html
> >
> > so I'm adding the people involved, maybe you can give it a try to
> > double check if this is good for you.
> >
> > On my side, I performed tests with different QC chipsets without
> > experiencing problems.
> >
> > Thanks,
> > Daniele
> > ---
> >  drivers/net/usb/qmi_wwan.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > index 07c42c0719f5..92d568f982b6 100644
> > --- a/drivers/net/usb/qmi_wwan.c
> > +++ b/drivers/net/usb/qmi_wwan.c
> > @@ -815,6 +815,10 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
> >       }
> >       dev->net->netdev_ops = &qmi_wwan_netdev_ops;
> >       dev->net->sysfs_groups[0] = &qmi_wwan_sysfs_attr_group;
> > +
> > +     /* Set rx_urb_size to allow QMAP rx data aggregation */
> > +     dev->rx_urb_size = 32768;
>
> Where did this "magic number" come from?
>

This is coming from the modem and it's the highest value I'm aware of
(Qualcomm SDX55 chipset). The problem is that different chipsets have
different maximum values, so while other modems have lower values
(e.g. 4096), if rx_urb_size can't be configured in user-space, the
maximum known value should be used to support all currently available
chipsets.

> And making an urb size that big can keep some pipelines full, it also
> comes at the expense of other potential issues, have you tested this to
> see that it really does help in throughput?
>

Yes, I had doubts about this: I could not test throughput for SDX55,
since at the moment I do not have the needed equipment. In the past I
tested a different value (16384) with 4G modems and I'm able to reach
the max dl throughput (1.1Gbit/s). Actually, enabling dl aggregation
is the only way to reach that throughput.

> And if it does, does this size really need to be that big?  What is it
> set to today, the endpoint size?  If so, that's a huge jump...
>

Since 16384 seems also the size used by the Windows driver, a
possibility is to use that value and add a comment in the driver
stating that higher values are not supported.

Today the default size is 1500, but it can be "configured" in user
space as a side-effect of changing the MTU.

To me that would still be acceptable since proved to be working fine
until now, but the problem is that to solve a babble issue seen with
recent chipsets the default rx_urb_size should be changed to an higher
value (QC suggested 2048) and this breaks the MTU workaround (see
function usbnet_change_mtu in usbnet.c).

Thanks,
Daniele

> thanks,
>
> greg k-h

Powered by blists - more mailing lists