Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size

["Bjørn Mork" <bjorn@mork.no>]

Daniele Palmas <dnlplm@gmail.com> writes:
> Il giorno mer 9 set 2020 alle ore 13:09 Carl Yin(殷张成)
> <carl.yin@quectel.com> ha scritto:
>>
>> Hi Deniele:
>>
>>         I have an idea, by now in order to use QMAP,
>>         must execute shell command 'echo mux_id > /sys/class/net/<iface>/add_mux' in user space,
>>         maybe we can expand usage of sys attribute 'add_mux', like next:
>>         'echo mux_id mux_size mux_version > /sys/class/net/<iface>/add_mux'.
>>         Users can set correct 'mux_size and mux_version' according to the response of 'QMI_WDA_SET_DATA_FORMAT '.
>>         If 'mux_size and mux_version' miss, qmi_wwan can use default values.
>>
>
> not sure this is something acceptable, let's wait for Bjørn to comment.

This breaks the "one value per file" rule.  Ref
https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt

And I really think this userspace ABI is complicated enough already
without adding yet another variable governed by strict rules.

I'd prefer this to just work, if possible.  I liked the simplicity of
your patch.  If it is necessary to have a different value when QMAP is
disabled, then I'd much rather see two fixed values, depending on
QMI_WWAN_FLAG_MUX.


>>         If fixed set as 32KB, but MDM9x07 chips only support 4KB, or uses do not enable QMAP,
>>         Maybe cannot reach max throughput for no enough rx urbs.
>>
>
> I did not test for performance, but you could be right since for
> example, if I'm not wrong, rx_qlen for an high-speed device would be 2
> with the changed rx_urb_size.

That's correct.  But I believe 2 URBs might be enough.  Still, would be
nice if some of you with a fast enough modem could test this.  At least
if throughput issues are going to be an argument for a more complicated
ABI.

> So, we'll probably need to modify also usbnet_update_max_qlen, but not
> sure about the direction (maybe fallback to a default value to
> guarantee a minimum number if rx_qlen is < than a threshold?). And
> this is also a change affecting all the drivers using usbnet, so it
> requires additional care.

I'm not sure we want to do that. We haven't done it for other
aggregating usbnet protocols.  There is a reason we have the hard limit.




Bjørn

>> > -----邮件原件-----
>> > 发件人: Daniele Palmas [mailto:dnlplm@gmail.com]
>> > 发送时间: Wednesday, September 09, 2020 5:13 PM
>> > 收件人: Bjørn Mork <bjorn@mork.no>
>> > 抄送: Kristian Evensen <kristian.evensen@gmail.com>; Paul Gildea
>> > <paul.gildea@gmail.com>; Carl Yin(殷张成) <carl.yin@quectel.com>; David S .
>> > Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
>> > netdev@vger.kernel.org; linux-usb@vger.kernel.org; Daniele Palmas
>> > <dnlplm@gmail.com>
>> > 主题: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size
>> >
>> > 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@gmail.com>
>> > ---
>> > 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;
>> > +
>> >  err:
>> >       return status;
>> >  }
>> > --
>> > 2.17.1
>>