* Re: [PATCH net-next] net: qmi_wwan: Add pass through mode
2018-06-27 8:00 ` Bjørn Mork
@ 2018-06-27 8:51 ` Daniele Palmas
2018-06-28 0:38 ` Subash Abhinov Kasiviswanathan
1 sibling, 0 replies; 4+ messages in thread
From: Daniele Palmas @ 2018-06-27 8:51 UTC (permalink / raw)
To: Bjørn Mork
Cc: Subash Abhinov Kasiviswanathan, Dan Williams, David Miller,
netdev, Aleksander Morgado
Hi Bjørn,
Il giorno mer 27 giu 2018 alle ore 10:01 Bjørn Mork <bjorn@mork.no> ha scritto:
>
> Subash Abhinov Kasiviswanathan <subashab@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 <subashab@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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net: qmi_wwan: Add pass through mode
2018-06-27 8:00 ` Bjørn Mork
2018-06-27 8:51 ` Daniele Palmas
@ 2018-06-28 0:38 ` Subash Abhinov Kasiviswanathan
1 sibling, 0 replies; 4+ messages in thread
From: Subash Abhinov Kasiviswanathan @ 2018-06-28 0:38 UTC (permalink / raw)
To: Bjørn Mork; +Cc: dnlplm, dcbw, davem, netdev, Aleksander Morgado
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread