Il giorno lun 3 ago 2020 alle ore 11:49 Greg KH
<gregkh@xxxxxxxxxxxxxxxxxxx> ha scritto:
>
> On Mon, Aug 03, 2020 at 10:26:18AM +0200, Daniele Palmas wrote:
> > Hi Greg,
> >
> > Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH
> > <gregkh@xxxxxxxxxxxxxxxxxxx> ha scritto:
> > >
> > > On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@xxxxxxxxxxx wrote:
> > > > From: carl <carl.yin@xxxxxxxxxxx>
> > > >
> > > > When QMUX enabled, the 'dl-datagram-max-size' can be 4KB/16KB/31KB depend on QUALCOMM's chipsets.
> > > > User can set 'dl-datagram-max-size' by 'QMI_WDA_SET_DATA_FORMAT'.
> > > > The usbnet's rx_urb_size must lager than or equal to the 'dl-datagram-max-size'.
> > > > This patch allow user to modify usbnet's rx_urb_size by next command.
> > > >
> > > > echo 4096 > /sys/class/net/wwan0/qmi/rx_urb_size
> > > >
> > > > Next commnds show how to set and query 'dl-datagram-max-size' by qmicli
> > > > # qmicli -d /dev/cdc-wdm1 --wda-set-data-format="link-layer-protocol=raw-ip, ul-protocol=qmap,
> > > > dl-protocol=qmap, dl-max-datagrams=32, dl-datagram-max-size=31744, ep-type=hsusb, ep-iface-number=4"
> > > > [/dev/cdc-wdm1] Successfully set data format
> > > > QoS flow header: no
> > > > Link layer protocol: 'raw-ip'
> > > > Uplink data aggregation protocol: 'qmap'
> > > > Downlink data aggregation protocol: 'qmap'
> > > > NDP signature: '0'
> > > > Downlink data aggregation max datagrams: '10'
> > > > Downlink data aggregation max size: '4096'
> > > >
> > > > # qmicli -d /dev/cdc-wdm1 --wda-get-data-format
> > > > [/dev/cdc-wdm1] Successfully got data format
> > > > QoS flow header: no
> > > > Link layer protocol: 'raw-ip'
> > > > Uplink data aggregation protocol: 'qmap'
> > > > Downlink data aggregation protocol: 'qmap'
> > > > NDP signature: '0'
> > > > Downlink data aggregation max datagrams: '10'
> > > > Downlink data aggregation max size: '4096'
> > > >
> > > > Signed-off-by: carl <carl.yin@xxxxxxxxxxx>
> > > > ---
> > > > drivers/net/usb/qmi_wwan.c | 39 ++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 39 insertions(+)
> > > >
> > > > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > > > index 07c42c0719f5b..8ea57fd99ae43 100644
> > > > --- a/drivers/net/usb/qmi_wwan.c
> > > > +++ b/drivers/net/usb/qmi_wwan.c
> > > > @@ -400,6 +400,44 @@ static ssize_t raw_ip_store(struct device *d, struct device_attribute *attr, co
> > > > return ret;
> > > > }
> > > >
> > > > +static ssize_t rx_urb_size_show(struct device *d, struct device_attribute *attr, char *buf)
> > > > +{
> > > > + struct usbnet *dev = netdev_priv(to_net_dev(d));
> > > > +
> > > > + return sprintf(buf, "%zd\n", dev->rx_urb_size);
> > >
> > > Why do you care about this?
> > >
> > > > +}
> > > > +
> > > > +static ssize_t rx_urb_size_store(struct device *d, struct device_attribute *attr,
> > > > + const char *buf, size_t len)
> > > > +{
> > > > + struct usbnet *dev = netdev_priv(to_net_dev(d));
> > > > + u32 rx_urb_size;
> > > > + int ret;
> > > > +
> > > > + if (kstrtou32(buf, 0, &rx_urb_size))
> > > > + return -EINVAL;
> > > > +
> > > > + /* no change? */
> > > > + if (rx_urb_size == dev->rx_urb_size)
> > > > + return len;
> > > > +
> > > > + if (!rtnl_trylock())
> > > > + return restart_syscall();
> > > > +
> > > > + /* we don't want to modify a running netdev */
> > > > + if (netif_running(dev->net)) {
> > > > + netdev_err(dev->net, "Cannot change a running device\n");
> > > > + ret = -EBUSY;
> > > > + goto err;
> > > > + }
> > > > +
> > > > + dev->rx_urb_size = rx_urb_size;
> > > > + ret = len;
> > > > +err:
> > > > + rtnl_unlock();
> > > > + return ret;
> > > > +}
> > > > +
> > > > static ssize_t add_mux_show(struct device *d, struct device_attribute *attr, char *buf)
> > > > {
> > > > struct net_device *dev = to_net_dev(d);
> > > > @@ -505,6 +543,7 @@ static DEVICE_ATTR_RW(add_mux);
> > > > static DEVICE_ATTR_RW(del_mux);
> > > >
> > > > static struct attribute *qmi_wwan_sysfs_attrs[] = {
> > > > + &dev_attr_rx_urb_size.attr,
> > >
> > > You added a driver-specific sysfs file and did not document in in
> > > Documentation/ABI? That's not ok, sorry, please fix up.
> > >
> > > Actually, no, this all should be done "automatically", do not change the
> > > urb size on the fly. Change it at probe time based on the device you
> > > are using, do not force userspace to "know" what to do here, as it will
> > > not know that at all.
> > >
> >
> > the problem with doing at probe time is that rx_urb_size is not fixed,
> > but depends on the configuration done at the userspace level with
> > QMI_WDA_SET_DATA_FORMAT, so the userspace knows that.
>
> Where does QMI_WDA_SET_DATA_FORMAT come from?
>
This is a request of Qualcomm proprietary protocol used, among other
things, to configure data aggregation for modems. There's an open
source userspace implementation in the libqmi project
(https://cgit.freedesktop.org/libqmi/tree/data/qmi-service-wda.json)
> And the commit log says that this "depends on the chipset being used",
> so why don't you know that at probe time, does the chipset change? :)
>
Me too does not understand this, I let the author explain...
> > Currently there's a workaround for setting rx_urb_size i.e. changing
> > the network interface MTU: this is fine for most uses with qmap, but
> > there's the limitation that certain values (multiple of the endpoint
> > size) are not allowed.
>
> Why not just set it really high to start with? That should not affect
> any older devices, as the urb size does not matter. The only thing is
> if it is too small that things can not move as fast as they might be
> able to.
>
Yes, this was proposed in the past by Bjørn
(https://lists.freedesktop.org/archives/libqmi-devel/2020-February/003221.html),
but I was not sure about issues with old modems.
Now I understand that there are no such issues, then I agree this is
the simplest solution: I've seen modems requiring as much as 49152,
but usually the default for qmap is <= 16384.
And, by the way, increasing the rx urb size is required also in
non-qmap mode, since the current value leads to babbling issues with
some chipsets (mine
https://www.spinics.net/lists/linux-usb/msg198025.html and Paul's
https://lists.freedesktop.org/archives/libqmi-devel/2020-February/003217.html),
so I think we should definitely increase this also for non-qmap mode.
Bjørn, what do you think?
Thanks,
Daniele
> thanks,
>
> greg k-h
