Hi Greg, Il giorno mer 9 set 2020 alle ore 14:28 Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> > > 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
