Hi Bjørn, Il giorno mer 27 giu 2018 alle ore 10:01 Bjørn Mork <bj...@mork.no> ha scritto: > > Subash Abhinov Kasiviswanathan <subas...@codeaurora.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 <subas...@codeaurora.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