Date:   Wed, 27 Jun 2018 10:51:04 +0200
From:   Daniele Palmas <dnlplm@...il.com>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>,
        Dan Williams <dcbw@...hat.com>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        Aleksander Morgado <aleksander@...ksander.es>
Subject: Re: [PATCH net-next] net: qmi_wwan: Add pass through mode

Hi Bjørn,

Il giorno mer 27 giu 2018 alle ore 10:01 Bjørn Mork <bjorn@...k.no> ha scritto:
>
> Subash Abhinov Kasiviswanathan <subashab@...eaurora.org> writes:
>
> > Pass through mode is to allow packets in MAP format to be passed
> > on to the stack. rmnet driver can be used to process and demultiplex
> > these packets. Note that pass through mode can be enabled when the
> > device is in raw ip mode only.
>
> The concepte looks fine to me, but I have a few comments to the
> implementation below.
>
> First: I missed the last part of the discussion around automatic
> detection of passthrough mode.  Could you give us a short summary of the
> alternatives you tried and why they were dropped?
>
> IIUC, userspace will be responsible for doing something like this to set
> up a rmnet map interface:
>
>  1) set the qmi_wwan netdev mode to raw-ip using sysfs
>  2) set the qmi_wwan netdev mode to pass-through using syfs
>  3) bind an rmnet netdev to the qmi_wwan netdev using netlink
>  4) configure the device for raw-ip using qmi
>  5) configure the device for map using qmi
>
> It would be good to have some sanity check by the ones having to deal
> with all that in userspace before carving it in stone.  Note that I'm
> not looking for a commitment to actually implement anything :-)
>
> I know Dan already is involved, so I am sure this is taken care of. But
> I'm including Aleksander in the CC as well just in case he sees any
> issues the rest of us fail to see.  The above procedure will probably
> not scare any of you?  Most of it is due to the driver being completely
> control protocol agnostic.  And I don't think the order matters much in
> practice, except for the raw-ip before pass-through enforced by the
> below patch?
>

For your information this is how I'm testing (patch without the
pass_through sysfs attribute):

qmicli -d /dev/cdc-wdm0 --set-expected-data-format=raw-ip
qmicli -d /dev/cdc-wdm0
--wda-set-data-format=link-layer-protocol=raw-ip,ul-protocol=qmap,dl-protocol=qmap
ip link set wwp0s20u5i2 up

ip link add link wwp0s20u5i2 name rmnet0 type rmnet mux_id 1
ip link add link wwp0s20u5i2 name rmnet1 type rmnet mux_id 2

qmicli -d /dev/cdc-wdm0 --wds-noop --client-no-release-cid
qmicli -d /dev/cdc-wdm0
--wds-bind-mux-data-port=mux-id=1,ep-iface-number=2
--client-no-release-cid --client-cid=${ccid1}
qmicli -p -d /dev/cdc-wdm0 --wds-start-network=apn=${apn1},ip-type=4
--client-no-release-cid --client-cid=${ccid1}
qmicli -p -d /dev/cdc-wdm0 --wds-get-current-settings
--client-no-release-cid --client-cid=${ccid1}

ip addr add ${ip1}/${bitmask1} dev rmnet0
ip link set rmnet0 up

qmicli -d /dev/cdc-wdm0 --wds-noop --client-no-release-cid
qmicli -d /dev/cdc-wdm0
--wds-bind-mux-data-port=mux-id=2,ep-iface-number=2
--client-no-release-cid --client-cid=${ccid2}
qmicli -p -d /dev/cdc-wdm0 --wds-start-network=apn=${apn2},ip-type=4
--client-no-release-cid --client-cid=${ccid2}
qmicli -p -d /dev/cdc-wdm0 --wds-get-current-settings
--client-no-release-cid --client-cid=${ccid2}

ip addr add ${ip2}/${bitmask2} dev rmnet1
ip link set rmnet1 up

I think that 3) implies 1) and 2), so maybe some steps could be merged.

Regards,
Daniele

>
> > Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
> > ---
> >  drivers/net/usb/qmi_wwan.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> >
> > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > index 8fac8e1..6eeec92 100644
> > --- a/drivers/net/usb/qmi_wwan.c
> > +++ b/drivers/net/usb/qmi_wwan.c
> > @@ -59,6 +59,7 @@ struct qmi_wwan_state {
> >  enum qmi_wwan_flags {
> >       QMI_WWAN_FLAG_RAWIP = 1 << 0,
> >       QMI_WWAN_FLAG_MUX = 1 << 1,
> > +     QMI_WWAN_FLAG_PASS_THROUGH = 1 << 2,
> >  };
> >
> >  enum qmi_wwan_quirks {
> > @@ -425,14 +426,82 @@ static ssize_t del_mux_store(struct device *d,  struct device_attribute *attr, c
> >       return ret;
> >  }
> >
> > +static ssize_t pass_through_show(struct device *d,
> > +                              struct device_attribute *attr,
> > +                              char *buf)
> > +{
> > +     struct usbnet *dev = netdev_priv(to_net_dev(d));
> > +     struct qmi_wwan_state *info;
> > +
> > +     info = (void *)&dev->data;
> > +     return sprintf(buf, "%c\n",
> > +                    info->flags & QMI_WWAN_FLAG_PASS_THROUGH ? 'Y' : 'N');
> > +}
> > +
> > +static ssize_t pass_through_store(struct device *d,
> > +                               struct device_attribute *attr,
> > +                               const char *buf, size_t len)
> > +{
>
>
> I've just had a quick glance at this, but this function looks like an
> almost exact copy of the raw_ip_store().  Why not share that code
> instead of copying it?
>
> And while you're at it:  There is nothing preventing us from turning on
> raw-ip here instead of failing if it is off, us there?  I.e. why not
> set QMI_WWAN_FLAG_RAWIP when QMI_WWAN_FLAG_PASS_THROUGH is being set,
> and clear QMI_WWAN_FLAG_PASS_THROUGH when QMI_WWAN_FLAG_RAWIP is being
> cleared?
>
>
> > +     struct usbnet *dev = netdev_priv(to_net_dev(d));
> > +     struct qmi_wwan_state *info;
> > +     bool enable;
> > +     int ret;
> > +
> > +     if (strtobool(buf, &enable))
> > +             return -EINVAL;
> > +
> > +     info = (void *)&dev->data;
> > +
> > +     /* no change? */
> > +     if (enable == (info->flags & QMI_WWAN_FLAG_PASS_THROUGH))
> > +             return len;
> > +
> > +     /* pass through mode can be set for raw ip devices only */
> > +     if (!(info->flags & QMI_WWAN_FLAG_RAWIP)) {
> > +             netdev_err(dev->net, "Cannot set pass through mode on non ip device\n");
> > +             return -EINVAL;
> > +     }
>
> You're missing the inverse relationship, aren't you?  There is nothing
> preventing the user from turning off raw-ip again after setting
> pass-through.
>
> > +
> > +     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;
> > +     }
> > +
> > +     /* let other drivers deny the change */
> > +     ret = call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE, dev->net);
> > +     ret = notifier_to_errno(ret);
> > +     if (ret) {
> > +             netdev_err(dev->net, "Type change was refused\n");
> > +             goto err;
> > +     }
> > +
> > +     if (enable)
> > +             info->flags |= QMI_WWAN_FLAG_PASS_THROUGH;
> > +     else
> > +             info->flags &= ~QMI_WWAN_FLAG_PASS_THROUGH;
> > +     qmi_wwan_netdev_setup(dev->net);
> > +     call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev->net);
>
>
> Do we really need all the notifier stuff here? You don't change the
> qmi_wwan netdev since that's already taken care of when setting
> QMI_WWAN_FLAG_RAWIP.
>
> AFAICS, QMI_WWAN_FLAG_PASS_THROUGH can be changed without any locking,
> notifications or netdev state restrictions. All it does is change the
> behaviour on rx, and there is no reason that can't be applied from the
> next packet even on a running interface.
>
> We could make pass_through_store just call raw_ip_store (or the part
> of it which matters, factored out into a separate function), if
> necessary. The rest of pass_through_store just sets or clears the flag.
> There is no need to do more than that, is there?
>
> And as noted above, raw_ip_store must also clear
> QMI_WWAN_FLAG_PASS_THROUGH if clearing QMI_WWAN_FLAG_RAWIP.
>
>
>
> > +     ret = len;
> > +err:
> > +     rtnl_unlock();
> > +     return ret;
> > +}
> > +
> >  static DEVICE_ATTR_RW(raw_ip);
> >  static DEVICE_ATTR_RW(add_mux);
> >  static DEVICE_ATTR_RW(del_mux);
> > +static DEVICE_ATTR_RW(pass_through);
> >
> >  static struct attribute *qmi_wwan_sysfs_attrs[] = {
> >       &dev_attr_raw_ip.attr,
> >       &dev_attr_add_mux.attr,
> >       &dev_attr_del_mux.attr,
> > +     &dev_attr_pass_through.attr,
> >       NULL,
> >  };
> >
> > @@ -479,6 +548,11 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> >       if (info->flags & QMI_WWAN_FLAG_MUX)
> >               return qmimux_rx_fixup(dev, skb);
> >
> > +     if (rawip && (info->flags & QMI_WWAN_FLAG_PASS_THROUGH)) {
>
>
> There is no need testing for rawip here, since you enforce that in
> pass_through_store().
>
>
>
> > +             skb->protocol = htons(ETH_P_MAP);
> > +             return (netif_rx(skb) == NET_RX_SUCCESS);
> > +     }
> > +
> >       switch (skb->data[0] & 0xf0) {
> >       case 0x40:
> >               proto = htons(ETH_P_IP);
>
>
>
>
>
> Bjørn

Powered by blists - more mailing lists