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  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 12:33:03 +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

Il giorno lun 3 ago 2020 alle ore 11:49 Greg KH
<gregkh@...uxfoundation.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@...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.
>
> 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

Powered by blists - more mailing lists