Re: [PATCH net-next] net: qmi_wwan: Add pass through mode
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? Hi Bjørn The other approach was to explicitly check for the rx_handler of the qmi_wwan netdev and if it was attached by rmnet driver (rmnet_rx_handler). If it was indeed attached, then the packet would be queued to stack. I had dropped this option since it would mean that atleast the rmnet rx_handler would have to be exported and exposed through a new header. This also brings a tight coupling between qmi_wwan and rmnet driver. 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 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? 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. 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. There is no need testing for rawip here, since you enforce that in pass_through_store(). I can make these changes. With this, steps 1 and 2 are combined. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
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 ha scritto: > > Subash Abhinov Kasiviswanathan 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 > > --- > > 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 *)>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 >
Re: [PATCH net-next] net: qmi_wwan: Add pass through mode
Subash Abhinov Kasiviswanathan 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? > Signed-off-by: Subash Abhinov Kasiviswanathan > --- > 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 *)>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, )) > + return -EINVAL; > + > + info = (void *)>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
[PATCH net-next] net: qmi_wwan: Add pass through mode
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. Signed-off-by: Subash Abhinov Kasiviswanathan --- 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 *)>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) +{ + struct usbnet *dev = netdev_priv(to_net_dev(d)); + struct qmi_wwan_state *info; + bool enable; + int ret; + + if (strtobool(buf, )) + return -EINVAL; + + info = (void *)>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; + } + + 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); + 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[] = { _attr_raw_ip.attr, _attr_add_mux.attr, _attr_del_mux.attr, + _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)) { + 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); -- 1.9.1