| Message ID | 20200803065105.8997-1-yzc666@netease.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qmi_wwan: support modify usbnet's rx_urb_size | expand |
On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@netease.com wrote: > From: carl <carl.yin@quectel.com> > > 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@quectel.com> > --- > 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. thanks, greg k-h
Hi Greg, Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH <gregkh@linuxfoundation.org> ha scritto: > > On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@netease.com wrote: > > From: carl <carl.yin@quectel.com> > > > > 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@quectel.com> > > --- > > 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. 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. Thanks, Daniele > thanks, > > greg k-h
On 03.08.2020 9:51, yzc666@netease.com wrote: > From: carl <carl.yin@quectel.com> Prefrrably a full name, matching that one in the signoff tag. > > When QMUX enabled, the 'dl-datagram-max-size' can be 4KB/16KB/31KB depend on QUALCOMM's chipsets. No indentation here please, start at column 1 and respect the length limit of 75 columns as script/checkpatch.pl says... > 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@quectel.com> Need your full name. > --- > 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); Is dev->rx_urb_size really declared as size_t? > +} > + > +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; ... in this case, sholdn't this variable be also declared as size_t? > + 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, > &dev_attr_raw_ip.attr, > &dev_attr_add_mux.attr, > &dev_attr_del_mux.attr, >
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@linuxfoundation.org> ha scritto: > > > > On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@netease.com wrote: > > > From: carl <carl.yin@quectel.com> > > > > > > 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@quectel.com> > > > --- > > > 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? 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? :) > 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. thanks, greg k-h
Hi,
I love your patch! Yet something to improve:
[auto build test ERROR on sparc-next/master]
[also build test ERROR on linus/master v5.8 next-20200731]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/yzc666-netease-com/qmi_wwan-support-modify-usbnet-s-rx_urb_size/20200803-152459
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc-next.git master
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/net/usb/qmi_wwan.c:546:3: error: 'dev_attr_rx_urb_size' undeclared here (not in a function)
546 | &dev_attr_rx_urb_size.attr,
| ^~~~~~~~~~~~~~~~~~~~
drivers/net/usb/qmi_wwan.c:410:16: warning: 'rx_urb_size_store' defined but not used [-Wunused-function]
410 | static ssize_t rx_urb_size_store(struct device *d, struct device_attribute *attr,
| ^~~~~~~~~~~~~~~~~
drivers/net/usb/qmi_wwan.c:403:16: warning: 'rx_urb_size_show' defined but not used [-Wunused-function]
403 | static ssize_t rx_urb_size_show(struct device *d, struct device_attribute *attr, char *buf)
| ^~~~~~~~~~~~~~~~
vim +/dev_attr_rx_urb_size +546 drivers/net/usb/qmi_wwan.c
544
545 static struct attribute *qmi_wwan_sysfs_attrs[] = {
> 546 &dev_attr_rx_urb_size.attr,
547 &dev_attr_raw_ip.attr,
548 &dev_attr_add_mux.attr,
549 &dev_attr_del_mux.attr,
550 NULL,
551 };
552
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Daniele Palmas <dnlplm@gmail.com> writes: > Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH > <gregkh@linuxfoundation.org> ha scritto: > >> 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. Yes, but the driver "will know" (or "may assume") this based on the QMI_WWAN_FLAG_MUX flag, as long as we are using the driver internal (de)muxing. We should be able to automatically set a sane rx_urb_size value based on this? Not sure if the rmnet driver currently can be used on top of qmi_wwan? That will obviously need some other workaround. > 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. And this also requires an additional setup step for user/userspace, which we should try to avoid if possible. I'm all for a fully automatic solution. I don't think rx_urb_size should be directly configurable. And it it were, then it should be implemented in the usbnet framework. It is not a qmi_wwan specific attribute. Bjørn
Il giorno lun 3 ago 2020 alle ore 11:49 Greg KH <gregkh@linuxfoundation.org> 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@linuxfoundation.org> ha scritto: > > > > > > On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@netease.com wrote: > > > > From: carl <carl.yin@quectel.com> > > > > > > > > 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@quectel.com> > > > > --- > > > > 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
Hi Greg: I am carl, software engineer from a Chinese company ' Quectel Wireless Solutions Co., Ltd'. Chinese name is Yinzhangcheng. So English name carl.yin. My company’s products are LTE/5G modules base on QUALCOMM's chipset, like MDM9607/MDM9640/SDX24/SDX55...etc. On 2020-08-03，"Greg KH" <gregkh@linuxfoundation.org> wrote： >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@linuxfoundation.org> ha scritto: > > > > On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@netease.com wrote: > > > From: carl <carl.yin@quectel.com> > > > > > > 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@quectel.com> > > > --- > > > 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? > QMI_WDA_SET_DATA_FORMAT is from QUALLCOM's QMI protocol. Like USB NET MBIM protocol, ' QMI protocol ' also can transfer multiple IP Packets in one URB. The benefit is reduce USB interrupts, so can reduce CPU loading. QUALLCOM call it as QMAP, full name is QUALCOMM Multiplexing and Aggregation. The means of 'dl-datagram-max-size' in QMAP is 'Maximum size in bytes of a single aggregated packet allowed on downlink." For example, MDM9607 support up to 4KB, SDX20 support up to 16KB, SDX55 support up to 31KB. The 'dl-datagram-max-size' can by support is depend the chipset. But the userspace can use ' QMI_WDA_SET_DATA_FORMAT' to Negotiates an new size(less than or equal to 'dl-datagram-max-size'). Most of case, the userspace will use the max 'dl-datagram-max-size', for benefit of 'reduce CPU loding'. > 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? :) > > > 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. > Hi Daniele: I also understand the "limitation", do you mean need to send "USB ZERO length PACKET'. The function of MTU is not same as rx_urb_size, for example we can set rx_urb_size to 31KB, but MTU is still 1500B. > thanks, > > greg k-h
Daniele Palmas <dnlplm@gmail.com> writes: > Il giorno lun 3 ago 2020 alle ore 11:49 Greg KH > <gregkh@linuxfoundation.org> ha scritto: >> >> 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. Ah, right. Forgot about that. > 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? I think we have good enough reasons to increase the rx urb size by default. Let's try. Bjørn
bjorn@mork.no writes: > Daniele Palmas <dnlplm@gmail.com> writes: > > Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH > > <gregkh@linuxfoundation.org> ha scritto: > > > >> 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. > > Yes, but the driver "will know" (or "may assume") this based on the > QMI_WWAN_FLAG_MUX flag, as long as we are using the driver internal > (de)muxing. We should be able to automatically set a sane rx_urb_size value > based on this? > > Not sure if the rmnet driver currently can be used on top of qmi_wwan? > That will obviously need some other workaround. > > > 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. > > And this also requires an additional setup step for user/userspace, which we > should try to avoid if possible. > > I'm all for a fully automatic solution. I don't think rx_urb_size should be directly > configurable. And it it were, then it should be implemented in the usbnet > framework. It is not a qmi_wwan specific attribute. Hi Bjørn, You can check cdc_ncm.c. cdc ncm driver set 'rx_urb_size' on driver probe time, and the value read from' cdc_ncm_bind_common() 's USB_CDC_GET_NTB_FORMAT '. and also allow the userspace to modify 'rx_urb_size' by ' /sys/class/net/wwan0/cdc_ncm/rx_max'. > > > Bjørn
"Carl Yin(殷张成)" <carl.yin@quectel.com> writes: > Hi Bjørn, > You can check cdc_ncm.c. > cdc ncm driver set 'rx_urb_size' on driver probe time, and the value read from' cdc_ncm_bind_common() 's USB_CDC_GET_NTB_FORMAT '. > and also allow the userspace to modify 'rx_urb_size' by ' /sys/class/net/wwan0/cdc_ncm/rx_max'. And I must admit I wrote that code ;-) NCM has the concept of 'dwNtbInMaxSize' and 'dwNtbOutMaxSize' controlling the maximum USB buffer size in each direction. The values are set by the device and provided to the host driver when probing. The driver then knows it must be prepared to receive up to 'dwNtbInMaxSize' buffers and set 'rx_urb_size' to this value. This is fully automatic and will Just Work without user/userspace intervention for any sane NCM or MBIM device. 'rx_max' was introduced to handle the insane devices, wanting buffers larger than the host was prepared to give them. The limit used to be hard coded in the driver. But it was enough for some low end hosts, so I made it configurable using that sysfs knob. You are right that the rmnet aggregation situation is similar. But similar to NCM, I would like a solution which is fully automatic for most of the users. Or preferably all, if possible. A sysfs knob is a last resort thing. Let's try to do without it first. Bjørn
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); +} + +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, &dev_attr_raw_ip.attr, &dev_attr_add_mux.attr, &dev_attr_del_mux.attr,