[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 09 Sep 2020 14:49:30 +0200
From: Bjørn Mork <bjorn@...k.no>
To: Daniele Palmas <dnlplm@...il.com>
Cc: Carl Yin(殷张成) <carl.yin@...ctel.com>,
Kristian Evensen <kristian.evensen@...il.com>,
Paul Gildea <paul.gildea@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
"netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
"linux-usb\@vger.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH net-next 1/1] net: usb: qmi_wwan: add default rx_urb_size
Daniele Palmas <dnlplm@...il.com> writes:
> Il giorno mer 9 set 2020 alle ore 13:09 Carl Yin(殷张成)
> <carl.yin@...ctel.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@...il.com]
>> > 发送时间: Wednesday, September 09, 2020 5:13 PM
>> > 收件人: 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@...r.kernel.org; Daniele Palmas
>> > <dnlplm@...il.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@...il.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
>>
Powered by blists - more mailing lists