netdev - Re: [PATCH] qmi_wwan: support modify usbnet's rx_urb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 3 Aug 2020 10:26:18 +0200
From:   Daniele Palmas <dnlplm@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     yzc666@...ease.com, Bjørn Mork <bjorn@...k.no>,
        David Miller <davem@...emloft.net>, kuba@...nel.org,
        netdev@...r.kernel.org, linux-usb <linux-usb@...r.kernel.org>,
        carl <carl.yin@...ctel.com>
Subject: Re: [PATCH] qmi_wwan: support modify usbnet's rx_urb_size

Hi Greg,

Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH
<gregkh@...uxfoundation.org> ha scritto:
>
> On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@...ease.com wrote:
> > From: carl <carl.yin@...ctel.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@...ctel.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