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.
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
