All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] net: qmi_wwan: Add pass through mode
@ 2018-06-27  2:30 Subash Abhinov Kasiviswanathan
  2018-06-27  8:00 ` Bjørn Mork
  0 siblings, 1 reply; 4+ messages in thread
From: Subash Abhinov Kasiviswanathan @ 2018-06-27  2:30 UTC (permalink / raw)
  To: bjorn, dnlplm, dcbw, davem, netdev; +Cc: Subash Abhinov Kasiviswanathan

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 <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)
+{
+	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;
+	}
+
+	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[] = {
 	&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)) {
+		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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net-next] net: qmi_wwan: Add pass through mode
  2018-06-27  2:30 [PATCH net-next] net: qmi_wwan: Add pass through mode Subash Abhinov Kasiviswanathan
@ 2018-06-27  8:00 ` Bjørn Mork
  2018-06-27  8:51   ` Daniele Palmas
  2018-06-28  0:38   ` Subash Abhinov Kasiviswanathan
  0 siblings, 2 replies; 4+ messages in thread
From: Bjørn Mork @ 2018-06-27  8:00 UTC (permalink / raw)
  To: Subash Abhinov Kasiviswanathan
  Cc: dnlplm, dcbw, davem, netdev, Aleksander Morgado

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?


> 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: 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

end of thread, other threads:[~2018-06-28  0:38 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-27  2:30 [PATCH net-next] net: qmi_wwan: Add pass through mode Subash Abhinov Kasiviswanathan
2018-06-27  8:00 ` Bjørn Mork
2018-06-27  8:51   ` Daniele Palmas
2018-06-28  0:38   ` Subash Abhinov Kasiviswanathan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.