答复: [PATCH] qmi_wwan: support modify usbnet's rx_urb_size

["Carl Yin(殷张成)" <carl.yin@quectel.com>]

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