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

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?


> 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