[PATCH net-next 00/10] net: WWAN link creation improvements

[PATCH net-next 00/10] net: WWAN link creation improvements

[Sergey Ryazanov]

This series is intended to make the WWAN network links management easier
for WWAN device drivers.

The series begins with adding support for network links creation to the
WWAN HW simulator to facilitate code testing. Then there are a couple of
changes that prepe the WWAN core code for further modifications. The
following patches (4-6) simplify driver unregistering procedures by
performing the created links cleanup in the WWAN core. 7th patch is to
avoid the odd hold of a driver module. Next patches (8th and 9th) make
it easier for drivers to create a network interface for a default data
channel. Finally, 10th patch adds support for reporting of data link
(aka channel aka context) id to make user aware which network
interface is binded to which WWAN device data channel.

I have a quite busy last week, and I am sorry publishing these changes
so too late, after all frameworks and drivers have been merged to the
net-next tree. On the other hand, it may be good that we have all
drivers in the tree, so we have a more complete picture.

All core changes have been tested with the HW simulator. The MHI and
IOSM drivers were only compile tested as I have no access to this
hardware. So the coresponding patches require ACK from the driver
authors.

Sergey Ryazanov (10):
  wwan_hwsim: support network interface creation
  wwan: core: relocate ops registering code
  wwan: core: require WWAN netdev setup callback existence
  wwan: core: multiple netdevs deletion support
  wwan: core: remove all netdevs on ops unregistering
  net: iosm: drop custom netdev(s) removing
  wwan: core: no more hold netdev ops owning module
  wwan: core: support default netdev creation
  net: mhi_net: create default link via WWAN core
  wwan: core: add WWAN common private data for netdev

 drivers/net/mhi/net.c                 |  66 ++-----
 drivers/net/mhi/proto_mbim.c          |   5 +-
 drivers/net/wwan/iosm/iosm_ipc_wwan.c |  30 +--
 drivers/net/wwan/wwan_core.c          | 258 ++++++++++++++++++--------
 drivers/net/wwan/wwan_hwsim.c         |  47 +++++
 include/linux/wwan.h                  |  28 ++-
 6 files changed, 281 insertions(+), 153 deletions(-)

-- 
2.26.3

[PATCH net-next 01/10] wwan_hwsim: support network interface creation

[Sergey Ryazanov]

Add support for networking interface creation via the WWAN core by
registering the WWAN netdev creation ops for each simulated WWAN device.
Implemented minimalistic netdev support where the xmit callback just
consumes all egress skbs.

This should help with WWAN network interfaces creation testing.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_hwsim.c | 48 +++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
index 472cae544a2b..c1e850b9c087 100644
--- a/drivers/net/wwan/wwan_hwsim.c
+++ b/drivers/net/wwan/wwan_hwsim.c
@@ -14,10 +14,13 @@
 #include <linux/spinlock.h>
 #include <linux/list.h>
 #include <linux/skbuff.h>
+#include <linux/netdevice.h>
 #include <linux/wwan.h>
 #include <linux/debugfs.h>
 #include <linux/workqueue.h>
 
+#include <net/arp.h>
+
 static int wwan_hwsim_devsnum = 2;
 module_param_named(devices, wwan_hwsim_devsnum, int, 0444);
 MODULE_PARM_DESC(devices, "Number of simulated devices");
@@ -64,6 +67,38 @@ static const struct file_operations wwan_hwsim_debugfs_devdestroy_fops;
 static void wwan_hwsim_port_del_work(struct work_struct *work);
 static void wwan_hwsim_dev_del_work(struct work_struct *work);
 
+static netdev_tx_t wwan_hwsim_netdev_xmit(struct sk_buff *skb,
+					  struct net_device *ndev)
+{
+	ndev->stats.tx_packets++;
+	ndev->stats.tx_bytes += skb->len;
+	consume_skb(skb);
+	return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops wwan_hwsim_netdev_ops = {
+	.ndo_start_xmit = wwan_hwsim_netdev_xmit,
+};
+
+static void wwan_hwsim_netdev_setup(struct net_device *ndev)
+{
+	ndev->netdev_ops = &wwan_hwsim_netdev_ops;
+	ndev->needs_free_netdev = true;
+
+	ndev->mtu = ETH_DATA_LEN;
+	ndev->min_mtu = ETH_MIN_MTU;
+	ndev->max_mtu = ETH_MAX_MTU;
+
+	ndev->type = ARPHRD_NONE;
+	ndev->flags = IFF_POINTOPOINT | IFF_NOARP;
+}
+
+static const struct wwan_ops wwan_hwsim_wwan_rtnl_ops = {
+	.owner = THIS_MODULE,
+	.priv_size = 0,			/* No private data */
+	.setup = wwan_hwsim_netdev_setup,
+};
+
 static int wwan_hwsim_port_start(struct wwan_port *wport)
 {
 	struct wwan_hwsim_port *port = wwan_port_get_drvdata(wport);
@@ -254,6 +289,10 @@ static struct wwan_hwsim_dev *wwan_hwsim_dev_new(void)
 
 	INIT_WORK(&dev->del_work, wwan_hwsim_dev_del_work);
 
+	err = wwan_register_ops(&dev->dev, &wwan_hwsim_wwan_rtnl_ops, dev);
+	if (err)
+		goto err_unreg_dev;
+
 	dev->debugfs_topdir = debugfs_create_dir(dev_name(&dev->dev),
 						 wwan_hwsim_debugfs_topdir);
 	debugfs_create_file("destroy", 0200, dev->debugfs_topdir, dev,
@@ -265,6 +304,12 @@ static struct wwan_hwsim_dev *wwan_hwsim_dev_new(void)
 
 	return dev;
 
+err_unreg_dev:
+	device_unregister(&dev->dev);
+	/* Memory will be freed in the device release callback */
+
+	return ERR_PTR(err);
+
 err_free_dev:
 	kfree(dev);
 
@@ -290,6 +335,9 @@ static void wwan_hwsim_dev_del(struct wwan_hwsim_dev *dev)
 
 	debugfs_remove(dev->debugfs_topdir);
 
+	/* This will remove all child netdev(s) */
+	wwan_unregister_ops(&dev->dev);
+
 	/* Make sure that there is no pending deletion work */
 	if (current_work() != &dev->del_work)
 		cancel_work_sync(&dev->del_work);
-- 
2.26.3

[PATCH net-next 02/10] wwan: core: relocate ops registering code

[Sergey Ryazanov]

It is unlikely that RTNL callbacks will call WWAN ops (un-)register
functions, but it is highly likely that the ops (un-)register functions
will use RTNL link create/destroy handlers. So move the WWAN network
interface ops (un-)register functions below the RTNL callbacks to be
able to call them without forward declarations.

No functional changes, just code relocation.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_core.c | 142 +++++++++++++++++------------------
 1 file changed, 71 insertions(+), 71 deletions(-)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 7e728042fc41..00b514b01d91 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -781,77 +781,6 @@ static const struct file_operations wwan_port_fops = {
 	.llseek = noop_llseek,
 };
 
-/**
- * wwan_register_ops - register WWAN device ops
- * @parent: Device to use as parent and shared by all WWAN ports and
- *	created netdevs
- * @ops: operations to register
- * @ctxt: context to pass to operations
- *
- * Returns: 0 on success, a negative error code on failure
- */
-int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
-		      void *ctxt)
-{
-	struct wwan_device *wwandev;
-
-	if (WARN_ON(!parent || !ops))
-		return -EINVAL;
-
-	wwandev = wwan_create_dev(parent);
-	if (!wwandev)
-		return -ENOMEM;
-
-	if (WARN_ON(wwandev->ops)) {
-		wwan_remove_dev(wwandev);
-		return -EBUSY;
-	}
-
-	if (!try_module_get(ops->owner)) {
-		wwan_remove_dev(wwandev);
-		return -ENODEV;
-	}
-
-	wwandev->ops = ops;
-	wwandev->ops_ctxt = ctxt;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(wwan_register_ops);
-
-/**
- * wwan_unregister_ops - remove WWAN device ops
- * @parent: Device to use as parent and shared by all WWAN ports and
- *	created netdevs
- */
-void wwan_unregister_ops(struct device *parent)
-{
-	struct wwan_device *wwandev = wwan_dev_get_by_parent(parent);
-	bool has_ops;
-
-	if (WARN_ON(IS_ERR(wwandev)))
-		return;
-
-	has_ops = wwandev->ops;
-
-	/* put the reference obtained by wwan_dev_get_by_parent(),
-	 * we should still have one (that the owner is giving back
-	 * now) due to the ops being assigned, check that below
-	 * and return if not.
-	 */
-	put_device(&wwandev->dev);
-
-	if (WARN_ON(!has_ops))
-		return;
-
-	module_put(wwandev->ops->owner);
-
-	wwandev->ops = NULL;
-	wwandev->ops_ctxt = NULL;
-	wwan_remove_dev(wwandev);
-}
-EXPORT_SYMBOL_GPL(wwan_unregister_ops);
-
 static int wwan_rtnl_validate(struct nlattr *tb[], struct nlattr *data[],
 			      struct netlink_ext_ack *extack)
 {
@@ -966,6 +895,77 @@ static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
 	.policy = wwan_rtnl_policy,
 };
 
+/**
+ * wwan_register_ops - register WWAN device ops
+ * @parent: Device to use as parent and shared by all WWAN ports and
+ *	created netdevs
+ * @ops: operations to register
+ * @ctxt: context to pass to operations
+ *
+ * Returns: 0 on success, a negative error code on failure
+ */
+int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
+		      void *ctxt)
+{
+	struct wwan_device *wwandev;
+
+	if (WARN_ON(!parent || !ops))
+		return -EINVAL;
+
+	wwandev = wwan_create_dev(parent);
+	if (!wwandev)
+		return -ENOMEM;
+
+	if (WARN_ON(wwandev->ops)) {
+		wwan_remove_dev(wwandev);
+		return -EBUSY;
+	}
+
+	if (!try_module_get(ops->owner)) {
+		wwan_remove_dev(wwandev);
+		return -ENODEV;
+	}
+
+	wwandev->ops = ops;
+	wwandev->ops_ctxt = ctxt;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(wwan_register_ops);
+
+/**
+ * wwan_unregister_ops - remove WWAN device ops
+ * @parent: Device to use as parent and shared by all WWAN ports and
+ *	created netdevs
+ */
+void wwan_unregister_ops(struct device *parent)
+{
+	struct wwan_device *wwandev = wwan_dev_get_by_parent(parent);
+	bool has_ops;
+
+	if (WARN_ON(IS_ERR(wwandev)))
+		return;
+
+	has_ops = wwandev->ops;
+
+	/* put the reference obtained by wwan_dev_get_by_parent(),
+	 * we should still have one (that the owner is giving back
+	 * now) due to the ops being assigned, check that below
+	 * and return if not.
+	 */
+	put_device(&wwandev->dev);
+
+	if (WARN_ON(!has_ops))
+		return;
+
+	module_put(wwandev->ops->owner);
+
+	wwandev->ops = NULL;
+	wwandev->ops_ctxt = NULL;
+	wwan_remove_dev(wwandev);
+}
+EXPORT_SYMBOL_GPL(wwan_unregister_ops);
+
 static int __init wwan_init(void)
 {
 	int err;
-- 
2.26.3

[PATCH net-next 03/10] wwan: core: require WWAN netdev setup callback existence

[Sergey Ryazanov]

The setup callback will be unconditionally passed to the
alloc_netdev_mqs(), where the NULL pointer dereference will cause the
kernel panic. So refuse to register WWAN netdev ops with warning
generation if the setup callback is not provided.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 00b514b01d91..259d49f78998 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -909,7 +909,7 @@ int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
 {
 	struct wwan_device *wwandev;
 
-	if (WARN_ON(!parent || !ops))
+	if (WARN_ON(!parent || !ops || !ops->setup))
 		return -EINVAL;
 
 	wwandev = wwan_create_dev(parent);
-- 
2.26.3

[PATCH net-next 04/10] wwan: core: multiple netdevs deletion support

[Sergey Ryazanov]

Use unregister_netdevice_queue() instead of simple
unregister_netdevice() if the WWAN netdev ops does not provide a dellink
callback. This will help to accelerate deletion of multiple netdevs.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 259d49f78998..634f0a7a2dc6 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -874,7 +874,7 @@ static void wwan_rtnl_dellink(struct net_device *dev, struct list_head *head)
 	if (wwandev->ops->dellink)
 		wwandev->ops->dellink(wwandev->ops_ctxt, dev, head);
 	else
-		unregister_netdevice(dev);
+		unregister_netdevice_queue(dev, head);
 
 out:
 	/* release the reference */
-- 
2.26.3

[PATCH net-next 05/10] wwan: core: remove all netdevs on ops unregistering

[Sergey Ryazanov]

We use the ops owner module hold to protect against ops memory
disappearing. But this approach does not protect us from a driver that
unregisters ops but forgets to remove netdev(s) that were created using
this ops. In such case, we are left with netdev(s), which can not be
removed since ops is gone. Moreover, batch netdevs removing on
deinitialization is a desireable option for WWAN drivers as it is a
quite common task.

Implement deletion of all created links on WWAN netdev ops unregistering
in the same way that RTNL removes all links on RTNL ops unregistering.
Simply remove all child netdevs of a device whose WWAN netdev ops is
unregistering. This way we protecting the kernel from buggy drivers and
make it easier to write a driver deinitialization code.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/wwan/wwan_core.c | 40 ++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 634f0a7a2dc6..3f04e674cdaa 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -933,6 +933,17 @@ int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
 }
 EXPORT_SYMBOL_GPL(wwan_register_ops);
 
+/* Enqueue child netdev deletion */
+static int wwan_child_dellink(struct device *dev, void *data)
+{
+	struct list_head *kill_list = data;
+
+	if (dev->type == &wwan_type)
+		wwan_rtnl_dellink(to_net_dev(dev), kill_list);
+
+	return 0;
+}
+
 /**
  * wwan_unregister_ops - remove WWAN device ops
  * @parent: Device to use as parent and shared by all WWAN ports and
@@ -941,26 +952,37 @@ EXPORT_SYMBOL_GPL(wwan_register_ops);
 void wwan_unregister_ops(struct device *parent)
 {
 	struct wwan_device *wwandev = wwan_dev_get_by_parent(parent);
-	bool has_ops;
+	struct module *owner;
+	LIST_HEAD(kill_list);
 
 	if (WARN_ON(IS_ERR(wwandev)))
 		return;
-
-	has_ops = wwandev->ops;
+	if (WARN_ON(!wwandev->ops)) {
+		put_device(&wwandev->dev);
+		return;
+	}
 
 	/* put the reference obtained by wwan_dev_get_by_parent(),
 	 * we should still have one (that the owner is giving back
-	 * now) due to the ops being assigned, check that below
-	 * and return if not.
+	 * now) due to the ops being assigned.
 	 */
 	put_device(&wwandev->dev);
 
-	if (WARN_ON(!has_ops))
-		return;
+	owner = wwandev->ops->owner;	/* Preserve ops owner */
+
+	rtnl_lock();	/* Prevent concurent netdev(s) creation/destroying */
+
+	/* Remove all child netdev(s), using batch removing */
+	device_for_each_child(&wwandev->dev, &kill_list,
+			      wwan_child_dellink);
+	unregister_netdevice_many(&kill_list);
+
+	wwandev->ops = NULL;	/* Finally remove ops */
+
+	rtnl_unlock();
 
-	module_put(wwandev->ops->owner);
+	module_put(owner);
 
-	wwandev->ops = NULL;
 	wwandev->ops_ctxt = NULL;
 	wwan_remove_dev(wwandev);
 }
-- 
2.26.3

[PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Sergey Ryazanov]

Since the last commit, the WWAN core will remove all our network
interfaces for us at the time of the WWAN netdev ops unregistering.
Therefore, we can safely drop the custom code that cleaning the list of
created netdevs. Anyway it no longer removes any netdev, since all
netdevs were removed earlier in the wwan_unregister_ops() call.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
CC: M Chetan Kumar <m.chetan.kumar@intel.com>
CC: Intel Corporation <linuxwwan@intel.com>
---
 drivers/net/wwan/iosm/iosm_ipc_wwan.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
index 1711b79fc616..bee9b278223d 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
@@ -329,22 +329,9 @@ struct iosm_wwan *ipc_wwan_init(struct iosm_imem *ipc_imem, struct device *dev)
 
 void ipc_wwan_deinit(struct iosm_wwan *ipc_wwan)
 {
-	int if_id;
-
+	/* This call will remove all child netdev(s) */
 	wwan_unregister_ops(ipc_wwan->dev);
 
-	for (if_id = 0; if_id < ARRAY_SIZE(ipc_wwan->sub_netlist); if_id++) {
-		struct iosm_netdev_priv *priv;
-
-		priv = rcu_access_pointer(ipc_wwan->sub_netlist[if_id]);
-		if (!priv)
-			continue;
-
-		rtnl_lock();
-		ipc_wwan_dellink(ipc_wwan, priv->netdev, NULL);
-		rtnl_unlock();
-	}
-
 	mutex_destroy(&ipc_wwan->if_mutex);
 
 	kfree(ipc_wwan);
-- 
2.26.3

[PATCH net-next 07/10] wwan: core: no more hold netdev ops owning module

[Sergey Ryazanov]

The WWAN netdev ops owner holding was used to protect from the
unexpected memory disappear. This approach causes a dependency cycle
(driver -> core -> driver) and effectively prevents a WWAN driver
unloading. E.g. WWAN hwsim could not be unloaded until all simulated
devices are removed:

~# modprobe wwan_hwsim devices=2
~# lsmod | grep wwan
wwan_hwsim             16384  2
wwan                   20480  1 wwan_hwsim
~# rmmod wwan_hwsim
rmmod: ERROR: Module wwan_hwsim is in use
~# echo > /sys/kernel/debug/wwan_hwsim/hwsim0/destroy
~# echo > /sys/kernel/debug/wwan_hwsim/hwsim1/destroy
~# lsmod | grep wwan
wwan_hwsim             16384  0
wwan                   20480  1 wwan_hwsim
~# rmmod wwan_hwsim

For a real device driver this will cause an inability to unload module
until a served device is physically detached.

Since the last commit we are removing all child netdev(s) when a driver
unregister the netdev ops. This allows us to permit the driver
unloading, since any sane driver will call ops unregistering on a device
deinitialization. So, remove the holding of an ops owner to make it
easier to unload a driver module. The owner field has also beed removed
from the ops structure as there are no more users of this field.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/mhi/net.c         |  1 -
 drivers/net/wwan/wwan_core.c  | 10 ----------
 drivers/net/wwan/wwan_hwsim.c |  1 -
 include/linux/wwan.h          |  2 --
 4 files changed, 14 deletions(-)

diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
index 64af1e518484..ebd1dbf0a536 100644
--- a/drivers/net/mhi/net.c
+++ b/drivers/net/mhi/net.c
@@ -383,7 +383,6 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
 }
 
 const struct wwan_ops mhi_wwan_ops = {
-	.owner = THIS_MODULE,
 	.priv_size = sizeof(struct mhi_net_dev),
 	.setup = mhi_net_setup,
 	.newlink = mhi_net_newlink,
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 3f04e674cdaa..28590405172c 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -921,11 +921,6 @@ int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
 		return -EBUSY;
 	}
 
-	if (!try_module_get(ops->owner)) {
-		wwan_remove_dev(wwandev);
-		return -ENODEV;
-	}
-
 	wwandev->ops = ops;
 	wwandev->ops_ctxt = ctxt;
 
@@ -952,7 +947,6 @@ static int wwan_child_dellink(struct device *dev, void *data)
 void wwan_unregister_ops(struct device *parent)
 {
 	struct wwan_device *wwandev = wwan_dev_get_by_parent(parent);
-	struct module *owner;
 	LIST_HEAD(kill_list);
 
 	if (WARN_ON(IS_ERR(wwandev)))
@@ -968,8 +962,6 @@ void wwan_unregister_ops(struct device *parent)
 	 */
 	put_device(&wwandev->dev);
 
-	owner = wwandev->ops->owner;	/* Preserve ops owner */
-
 	rtnl_lock();	/* Prevent concurent netdev(s) creation/destroying */
 
 	/* Remove all child netdev(s), using batch removing */
@@ -981,8 +973,6 @@ void wwan_unregister_ops(struct device *parent)
 
 	rtnl_unlock();
 
-	module_put(owner);
-
 	wwandev->ops_ctxt = NULL;
 	wwan_remove_dev(wwandev);
 }
diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
index c1e850b9c087..a8582a58a385 100644
--- a/drivers/net/wwan/wwan_hwsim.c
+++ b/drivers/net/wwan/wwan_hwsim.c
@@ -94,7 +94,6 @@ static void wwan_hwsim_netdev_setup(struct net_device *ndev)
 }
 
 static const struct wwan_ops wwan_hwsim_wwan_rtnl_ops = {
-	.owner = THIS_MODULE,
 	.priv_size = 0,			/* No private data */
 	.setup = wwan_hwsim_netdev_setup,
 };
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 430a3a0817de..656a571b52ed 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -119,14 +119,12 @@ void *wwan_port_get_drvdata(struct wwan_port *port);
 
 /**
  * struct wwan_ops - WWAN device ops
- * @owner: module owner of the WWAN ops
  * @priv_size: size of private netdev data area
  * @setup: set up a new netdev
  * @newlink: register the new netdev
  * @dellink: remove the given netdev
  */
 struct wwan_ops {
-	struct module *owner;
 	unsigned int priv_size;
 	void (*setup)(struct net_device *dev);
 	int (*newlink)(void *ctxt, struct net_device *dev,
-- 
2.26.3

[PATCH net-next 08/10] wwan: core: support default netdev creation

[Sergey Ryazanov]

Most, if not each WWAN device driver will create a netdev for the
default data channel. Therefore, add an option for the WWAN netdev ops
registration function to create a default netdev for the WWAN device.

A WWAN device driver should pass a default data channel link id to the
ops registering function to request the creation of a default netdev, or
a special value WWAN_NO_DEFAULT_LINK to inform the WWAN core that the
default netdev should not be created.

For now, only wwan_hwsim utilize the default link creation option. Other
drivers will be reworked next.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
CC: M Chetan Kumar <m.chetan.kumar@intel.com>
CC: Intel Corporation <linuxwwan@intel.com>
---
 drivers/net/mhi/net.c                 |  3 +-
 drivers/net/wwan/iosm/iosm_ipc_wwan.c |  3 +-
 drivers/net/wwan/wwan_core.c          | 75 ++++++++++++++++++++++++++-
 drivers/net/wwan/wwan_hwsim.c         |  2 +-
 include/linux/wwan.h                  |  8 ++-
 5 files changed, 86 insertions(+), 5 deletions(-)

diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
index ebd1dbf0a536..b003003cbd42 100644
--- a/drivers/net/mhi/net.c
+++ b/drivers/net/mhi/net.c
@@ -397,7 +397,8 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
 	struct net_device *ndev;
 	int err;
 
-	err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev);
+	err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
+				WWAN_NO_DEFAULT_LINK);
 	if (err)
 		return err;
 
diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
index bee9b278223d..adb2bd40a404 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
@@ -317,7 +317,8 @@ struct iosm_wwan *ipc_wwan_init(struct iosm_imem *ipc_imem, struct device *dev)
 	ipc_wwan->dev = dev;
 	ipc_wwan->ipc_imem = ipc_imem;
 
-	if (wwan_register_ops(ipc_wwan->dev, &iosm_wwan_ops, ipc_wwan)) {
+	if (wwan_register_ops(ipc_wwan->dev, &iosm_wwan_ops, ipc_wwan,
+			      WWAN_NO_DEFAULT_LINK)) {
 		kfree(ipc_wwan);
 		return NULL;
 	}
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 28590405172c..b99a737a7d77 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -895,17 +895,81 @@ static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
 	.policy = wwan_rtnl_policy,
 };
 
+static void wwan_create_default_link(struct wwan_device *wwandev,
+				     u32 def_link_id)
+{
+	struct nlattr *tb[IFLA_MAX + 1], *linkinfo[IFLA_INFO_MAX + 1];
+	struct nlattr *data[IFLA_WWAN_MAX + 1];
+	struct net_device *dev;
+	struct nlmsghdr *nlh;
+	struct sk_buff *msg;
+
+	/* Forge attributes required to create a WWAN netdev. We first
+	 * build a netlink message and then parse it. This looks
+	 * odd, but such approach is less error prone.
+	 */
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (WARN_ON(!msg))
+		return;
+	nlh = nlmsg_put(msg, 0, 0, RTM_NEWLINK, 0, 0);
+	if (WARN_ON(!nlh))
+		goto free_attrs;
+
+	if (nla_put_string(msg, IFLA_PARENT_DEV_NAME, dev_name(&wwandev->dev)))
+		goto free_attrs;
+	tb[IFLA_LINKINFO] = nla_nest_start(msg, IFLA_LINKINFO);
+	if (!tb[IFLA_LINKINFO])
+		goto free_attrs;
+	linkinfo[IFLA_INFO_DATA] = nla_nest_start(msg, IFLA_INFO_DATA);
+	if (!linkinfo[IFLA_INFO_DATA])
+		goto free_attrs;
+	if (nla_put_u32(msg, IFLA_WWAN_LINK_ID, def_link_id))
+		goto free_attrs;
+	nla_nest_end(msg, linkinfo[IFLA_INFO_DATA]);
+	nla_nest_end(msg, tb[IFLA_LINKINFO]);
+
+	nlmsg_end(msg, nlh);
+
+	/* The next three parsing calls can not fail */
+	nlmsg_parse_deprecated(nlh, 0, tb, IFLA_MAX, NULL, NULL);
+	nla_parse_nested_deprecated(linkinfo, IFLA_INFO_MAX, tb[IFLA_LINKINFO],
+				    NULL, NULL);
+	nla_parse_nested_deprecated(data, IFLA_WWAN_MAX,
+				    linkinfo[IFLA_INFO_DATA], NULL, NULL);
+
+	rtnl_lock();
+
+	dev = rtnl_create_link(&init_net, "wwan%d", NET_NAME_ENUM,
+			       &wwan_rtnl_link_ops, tb, NULL);
+	if (WARN_ON(IS_ERR(dev)))
+		goto unlock;
+
+	if (WARN_ON(wwan_rtnl_newlink(&init_net, dev, tb, data, NULL))) {
+		free_netdev(dev);
+		goto unlock;
+	}
+
+unlock:
+	rtnl_unlock();
+
+free_attrs:
+	nlmsg_free(msg);
+}
+
 /**
  * wwan_register_ops - register WWAN device ops
  * @parent: Device to use as parent and shared by all WWAN ports and
  *	created netdevs
  * @ops: operations to register
  * @ctxt: context to pass to operations
+ * @def_link_id: id of the default link that will be automatically created by
+ *	the WWAN core for the WWAN device. The default link will not be created
+ *	if the passed value is WWAN_NO_DEFAULT_LINK.
  *
  * Returns: 0 on success, a negative error code on failure
  */
 int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
-		      void *ctxt)
+		      void *ctxt, u32 def_link_id)
 {
 	struct wwan_device *wwandev;
 
@@ -924,6 +988,15 @@ int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
 	wwandev->ops = ops;
 	wwandev->ops_ctxt = ctxt;
 
+	/* NB: we do not abort ops registration in case of default link
+	 * creation failure. Link ops is the management interface, while the
+	 * default link creation is a service option. And we should not prevent
+	 * a user from manually creating a link latter if service option failed
+	 * now.
+	 */
+	if (def_link_id != WWAN_NO_DEFAULT_LINK)
+		wwan_create_default_link(wwandev, def_link_id);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(wwan_register_ops);
diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
index a8582a58a385..5b62cf3b3c42 100644
--- a/drivers/net/wwan/wwan_hwsim.c
+++ b/drivers/net/wwan/wwan_hwsim.c
@@ -288,7 +288,7 @@ static struct wwan_hwsim_dev *wwan_hwsim_dev_new(void)
 
 	INIT_WORK(&dev->del_work, wwan_hwsim_dev_del_work);
 
-	err = wwan_register_ops(&dev->dev, &wwan_hwsim_wwan_rtnl_ops, dev);
+	err = wwan_register_ops(&dev->dev, &wwan_hwsim_wwan_rtnl_ops, dev, 1);
 	if (err)
 		goto err_unreg_dev;
 
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 656a571b52ed..14c9a19f3bf0 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -117,6 +117,12 @@ void wwan_port_txon(struct wwan_port *port);
  */
 void *wwan_port_get_drvdata(struct wwan_port *port);
 
+/**
+ * Used to indicate that the WWAN core should not create a default network
+ * link.
+ */
+#define WWAN_NO_DEFAULT_LINK		U32_MAX
+
 /**
  * struct wwan_ops - WWAN device ops
  * @priv_size: size of private netdev data area
@@ -134,7 +140,7 @@ struct wwan_ops {
 };
 
 int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
-		      void *ctxt);
+		      void *ctxt, u32 def_link_id);
 
 void wwan_unregister_ops(struct device *parent);
 
-- 
2.26.3

[PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

[Sergey Ryazanov]

Utilize the just introduced WWAN core feature to create a default netdev
for the default data channel. Since the netdev is now created via the
WWAN core, rely on it ability to destroy all child netdevs on ops
unregistering.

While at it, remove the RTNL lock acquiring hacks that were earlier used
to call addlink/dellink without holding the RTNL lock. Also make the
WWAN netdev ops structure static to make sparse happy.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 drivers/net/mhi/net.c | 54 +++++--------------------------------------
 1 file changed, 6 insertions(+), 48 deletions(-)

diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
index b003003cbd42..06253acecaa2 100644
--- a/drivers/net/mhi/net.c
+++ b/drivers/net/mhi/net.c
@@ -342,10 +342,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
 	/* Number of transfer descriptors determines size of the queue */
 	mhi_netdev->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
 
-	if (extack)
-		err = register_netdevice(ndev);
-	else
-		err = register_netdev(ndev);
+	err = register_netdevice(ndev);
 	if (err)
 		goto out_err;
 
@@ -370,10 +367,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
 	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
 	struct mhi_device *mhi_dev = ctxt;
 
-	if (head)
-		unregister_netdevice_queue(ndev, head);
-	else
-		unregister_netdev(ndev);
+	unregister_netdevice_queue(ndev, head);
 
 	mhi_unprepare_from_transfer(mhi_dev);
 
@@ -382,7 +376,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
 	dev_set_drvdata(&mhi_dev->dev, NULL);
 }
 
-const struct wwan_ops mhi_wwan_ops = {
+static const struct wwan_ops mhi_wwan_ops = {
 	.priv_size = sizeof(struct mhi_net_dev),
 	.setup = mhi_net_setup,
 	.newlink = mhi_net_newlink,
@@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {
 static int mhi_net_probe(struct mhi_device *mhi_dev,
 			 const struct mhi_device_id *id)
 {
-	const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
 	struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
-	struct net_device *ndev;
-	int err;
-
-	err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
-				WWAN_NO_DEFAULT_LINK);
-	if (err)
-		return err;
-
-	if (!create_default_iface)
-		return 0;
-
-	/* Create a default interface which is used as either RMNET real-dev,
-	 * MBIM link 0 or ip link 0)
-	 */
-	ndev = alloc_netdev(sizeof(struct mhi_net_dev), info->netname,
-			    NET_NAME_PREDICTABLE, mhi_net_setup);
-	if (!ndev) {
-		err = -ENOMEM;
-		goto err_unregister;
-	}
-
-	SET_NETDEV_DEV(ndev, &mhi_dev->dev);
 
-	err = mhi_net_newlink(mhi_dev, ndev, 0, NULL);
-	if (err)
-		goto err_release;
-
-	return 0;
-
-err_release:
-	free_netdev(ndev);
-err_unregister:
-	wwan_unregister_ops(&cntrl->mhi_dev->dev);
-
-	return err;
+	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
+				 create_default_iface ? 0 :
+				 WWAN_NO_DEFAULT_LINK);
 }
 
 static void mhi_net_remove(struct mhi_device *mhi_dev)
 {
-	struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
 	struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
 
 	/* rtnetlink takes care of removing remaining links */
 	wwan_unregister_ops(&cntrl->mhi_dev->dev);
-
-	if (create_default_iface)
-		mhi_net_dellink(mhi_dev, mhi_netdev->ndev, NULL);
 }
 
 static const struct mhi_device_info mhi_hwip0 = {
-- 
2.26.3

[PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Sergey Ryazanov]

The WWAN core not only multiplex the netdev configuration data, but
process it too, and needs some space to store its private data
associated with the netdev. Add a structure to keep common WWAN core
data. The structure will be stored inside the netdev private data before
WWAN driver private data and have a field to make it easier to access
the driver data. Also add a helper function that simplifies drivers
access to their data.

At the moment we use the common WWAN private data to store the WWAN data
link (channel) id at the time the link is created, and report it back to
user using the .fill_info() RTNL callback. This should help the user to
be aware which network interface is binded to which WWAN device data
channel.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
CC: M Chetan Kumar <m.chetan.kumar@intel.com>
CC: Intel Corporation <linuxwwan@intel.com>
---
 drivers/net/mhi/net.c                 | 12 +++++------
 drivers/net/mhi/proto_mbim.c          |  5 +++--
 drivers/net/wwan/iosm/iosm_ipc_wwan.c | 12 +++++------
 drivers/net/wwan/wwan_core.c          | 29 ++++++++++++++++++++++++++-
 include/linux/wwan.h                  | 18 +++++++++++++++++
 5 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
index 06253acecaa2..cff433d7b984 100644
--- a/drivers/net/mhi/net.c
+++ b/drivers/net/mhi/net.c
@@ -32,7 +32,7 @@ struct mhi_device_info {
 
 static int mhi_ndo_open(struct net_device *ndev)
 {
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	struct mhi_net_dev *mhi_netdev = wwan_netdev_drvpriv(ndev);
 
 	/* Feed the rx buffer pool */
 	schedule_delayed_work(&mhi_netdev->rx_refill, 0);
@@ -47,7 +47,7 @@ static int mhi_ndo_open(struct net_device *ndev)
 
 static int mhi_ndo_stop(struct net_device *ndev)
 {
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	struct mhi_net_dev *mhi_netdev = wwan_netdev_drvpriv(ndev);
 
 	netif_stop_queue(ndev);
 	netif_carrier_off(ndev);
@@ -58,7 +58,7 @@ static int mhi_ndo_stop(struct net_device *ndev)
 
 static int mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	struct mhi_net_dev *mhi_netdev = wwan_netdev_drvpriv(ndev);
 	const struct mhi_net_proto *proto = mhi_netdev->proto;
 	struct mhi_device *mdev = mhi_netdev->mdev;
 	int err;
@@ -93,7 +93,7 @@ static int mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
 static void mhi_ndo_get_stats64(struct net_device *ndev,
 				struct rtnl_link_stats64 *stats)
 {
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	struct mhi_net_dev *mhi_netdev = wwan_netdev_drvpriv(ndev);
 	unsigned int start;
 
 	do {
@@ -322,7 +322,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
 	if (dev_get_drvdata(&mhi_dev->dev))
 		return -EBUSY;
 
-	mhi_netdev = netdev_priv(ndev);
+	mhi_netdev = wwan_netdev_drvpriv(ndev);
 
 	dev_set_drvdata(&mhi_dev->dev, mhi_netdev);
 	mhi_netdev->ndev = ndev;
@@ -364,7 +364,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
 static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
 			    struct list_head *head)
 {
-	struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
+	struct mhi_net_dev *mhi_netdev = wwan_netdev_drvpriv(ndev);
 	struct mhi_device *mhi_dev = ctxt;
 
 	unregister_netdevice_queue(ndev, head);
diff --git a/drivers/net/mhi/proto_mbim.c b/drivers/net/mhi/proto_mbim.c
index fc72b3f6ec9e..bf1ad863237d 100644
--- a/drivers/net/mhi/proto_mbim.c
+++ b/drivers/net/mhi/proto_mbim.c
@@ -16,6 +16,7 @@
 #include <linux/ip.h>
 #include <linux/mii.h>
 #include <linux/netdevice.h>
+#include <linux/wwan.h>
 #include <linux/skbuff.h>
 #include <linux/usb.h>
 #include <linux/usb/cdc.h>
@@ -56,7 +57,7 @@ static void __mbim_errors_inc(struct mhi_net_dev *dev)
 
 static int mbim_rx_verify_nth16(struct sk_buff *skb)
 {
-	struct mhi_net_dev *dev = netdev_priv(skb->dev);
+	struct mhi_net_dev *dev = wwan_netdev_drvpriv(skb->dev);
 	struct mbim_context *ctx = dev->proto_data;
 	struct usb_cdc_ncm_nth16 *nth16;
 	int len;
@@ -102,7 +103,7 @@ static int mbim_rx_verify_nth16(struct sk_buff *skb)
 
 static int mbim_rx_verify_ndp16(struct sk_buff *skb, struct usb_cdc_ncm_ndp16 *ndp16)
 {
-	struct mhi_net_dev *dev = netdev_priv(skb->dev);
+	struct mhi_net_dev *dev = wwan_netdev_drvpriv(skb->dev);
 	int ret;
 
 	if (le16_to_cpu(ndp16->wLength) < USB_CDC_NCM_NDP16_LENGTH_MIN) {
diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
index adb2bd40a404..61ec48468b63 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
@@ -20,7 +20,7 @@
 #define IOSM_IF_ID_PAYLOAD 2
 
 /**
- * struct iosm_netdev_priv - netdev private data
+ * struct iosm_netdev_priv - netdev WWAN driver specific private data
  * @ipc_wwan:	Pointer to iosm_wwan struct
  * @netdev:	Pointer to network interface device structure
  * @if_id:	Interface id for device.
@@ -51,7 +51,7 @@ struct iosm_wwan {
 /* Bring-up the wwan net link */
 static int ipc_wwan_link_open(struct net_device *netdev)
 {
-	struct iosm_netdev_priv *priv = netdev_priv(netdev);
+	struct iosm_netdev_priv *priv = wwan_netdev_drvpriv(netdev);
 	struct iosm_wwan *ipc_wwan = priv->ipc_wwan;
 	int if_id = priv->if_id;
 	int ret;
@@ -88,7 +88,7 @@ static int ipc_wwan_link_open(struct net_device *netdev)
 /* Bring-down the wwan net link */
 static int ipc_wwan_link_stop(struct net_device *netdev)
 {
-	struct iosm_netdev_priv *priv = netdev_priv(netdev);
+	struct iosm_netdev_priv *priv = wwan_netdev_drvpriv(netdev);
 
 	netif_stop_queue(netdev);
 
@@ -105,7 +105,7 @@ static int ipc_wwan_link_stop(struct net_device *netdev)
 static int ipc_wwan_link_transmit(struct sk_buff *skb,
 				  struct net_device *netdev)
 {
-	struct iosm_netdev_priv *priv = netdev_priv(netdev);
+	struct iosm_netdev_priv *priv = wwan_netdev_drvpriv(netdev);
 	struct iosm_wwan *ipc_wwan = priv->ipc_wwan;
 	int if_id = priv->if_id;
 	int ret;
@@ -178,7 +178,7 @@ static int ipc_wwan_newlink(void *ctxt, struct net_device *dev,
 	    if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist))
 		return -EINVAL;
 
-	priv = netdev_priv(dev);
+	priv = wwan_netdev_drvpriv(dev);
 	priv->if_id = if_id;
 	priv->netdev = dev;
 	priv->ipc_wwan = ipc_wwan;
@@ -208,8 +208,8 @@ static int ipc_wwan_newlink(void *ctxt, struct net_device *dev,
 static void ipc_wwan_dellink(void *ctxt, struct net_device *dev,
 			     struct list_head *head)
 {
+	struct iosm_netdev_priv *priv = wwan_netdev_drvpriv(dev);
 	struct iosm_wwan *ipc_wwan = ctxt;
-	struct iosm_netdev_priv *priv = netdev_priv(dev);
 	int if_id = priv->if_id;
 
 	if (WARN_ON(if_id < IP_MUX_SESSION_START ||
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index b99a737a7d77..3b5545f32c0e 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -807,6 +807,7 @@ static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
 	const char *devname = nla_data(tb[IFLA_PARENT_DEV_NAME]);
 	struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
 	struct net_device *dev;
+	unsigned int priv_size;
 
 	if (IS_ERR(wwandev))
 		return ERR_CAST(wwandev);
@@ -817,7 +818,8 @@ static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
 		goto out;
 	}
 
-	dev = alloc_netdev_mqs(wwandev->ops->priv_size, ifname, name_assign_type,
+	priv_size = sizeof(struct wwan_netdev_priv) + wwandev->ops->priv_size;
+	dev = alloc_netdev_mqs(priv_size, ifname, name_assign_type,
 			       wwandev->ops->setup, num_tx_queues, num_rx_queues);
 
 	if (dev) {
@@ -837,6 +839,7 @@ static int wwan_rtnl_newlink(struct net *src_net, struct net_device *dev,
 {
 	struct wwan_device *wwandev = wwan_dev_get_by_parent(dev->dev.parent);
 	u32 link_id = nla_get_u32(data[IFLA_WWAN_LINK_ID]);
+	struct wwan_netdev_priv *priv = netdev_priv(dev);
 	int ret;
 
 	if (IS_ERR(wwandev))
@@ -848,6 +851,7 @@ static int wwan_rtnl_newlink(struct net *src_net, struct net_device *dev,
 		goto out;
 	}
 
+	priv->link_id = link_id;
 	if (wwandev->ops->newlink)
 		ret = wwandev->ops->newlink(wwandev->ops_ctxt, dev,
 					    link_id, extack);
@@ -881,6 +885,27 @@ static void wwan_rtnl_dellink(struct net_device *dev, struct list_head *head)
 	put_device(&wwandev->dev);
 }
 
+static size_t wwan_rtnl_get_size(const struct net_device *dev)
+{
+	return
+		nla_total_size(4) +	/* IFLA_WWAN_LINK_ID */
+		0;
+}
+
+static int wwan_rtnl_fill_info(struct sk_buff *skb,
+			       const struct net_device *dev)
+{
+	struct wwan_netdev_priv *priv = netdev_priv(dev);
+
+	if (nla_put_u32(skb, IFLA_WWAN_LINK_ID, priv->link_id))
+		goto nla_put_failure;
+
+	return 0;
+
+nla_put_failure:
+	return -EMSGSIZE;
+}
+
 static const struct nla_policy wwan_rtnl_policy[IFLA_WWAN_MAX + 1] = {
 	[IFLA_WWAN_LINK_ID] = { .type = NLA_U32 },
 };
@@ -892,6 +917,8 @@ static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
 	.validate = wwan_rtnl_validate,
 	.newlink = wwan_rtnl_newlink,
 	.dellink = wwan_rtnl_dellink,
+	.get_size = wwan_rtnl_get_size,
+	.fill_info = wwan_rtnl_fill_info,
 	.policy = wwan_rtnl_policy,
 };
 
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 14c9a19f3bf0..37a14af95845 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
+#include <linux/netdevice.h>
 
 /**
  * enum wwan_port_type - WWAN port types
@@ -117,6 +118,23 @@ void wwan_port_txon(struct wwan_port *port);
  */
 void *wwan_port_get_drvdata(struct wwan_port *port);
 
+/**
+ * struct wwan_netdev_priv - WWAN core network device private data
+ * @link_id: WWAN device data link id
+ * @drv_priv: driver private data area, size is determined in &wwan_ops
+ */
+struct wwan_netdev_priv {
+	u32 link_id;
+
+	/* must be last */
+	u8 drv_priv[] __aligned(sizeof(void *));
+};
+
+static inline void *wwan_netdev_drvpriv(struct net_device *dev)
+{
+	return ((struct wwan_netdev_priv *)netdev_priv(dev))->drv_priv;
+}
+
 /**
  * Used to indicate that the WWAN core should not create a default network
  * link.
-- 
2.26.3

Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

[Loic Poulain]

Hi Sergey,

On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Utilize the just introduced WWAN core feature to create a default netdev
> for the default data channel. Since the netdev is now created via the
> WWAN core, rely on it ability to destroy all child netdevs on ops
> unregistering.
>
> While at it, remove the RTNL lock acquiring hacks that were earlier used
> to call addlink/dellink without holding the RTNL lock. Also make the
> WWAN netdev ops structure static to make sparse happy.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> ---
>  drivers/net/mhi/net.c | 54 +++++--------------------------------------
>  1 file changed, 6 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
> index b003003cbd42..06253acecaa2 100644
> --- a/drivers/net/mhi/net.c
> +++ b/drivers/net/mhi/net.c
> @@ -342,10 +342,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
>         /* Number of transfer descriptors determines size of the queue */
>         mhi_netdev->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
>
> -       if (extack)
> -               err = register_netdevice(ndev);
> -       else
> -               err = register_netdev(ndev);
> +       err = register_netdevice(ndev);
>         if (err)
>                 goto out_err;
>
> @@ -370,10 +367,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>         struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
>         struct mhi_device *mhi_dev = ctxt;
>
> -       if (head)
> -               unregister_netdevice_queue(ndev, head);
> -       else
> -               unregister_netdev(ndev);
> +       unregister_netdevice_queue(ndev, head);
>
>         mhi_unprepare_from_transfer(mhi_dev);
>
> @@ -382,7 +376,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>         dev_set_drvdata(&mhi_dev->dev, NULL);
>  }
>
> -const struct wwan_ops mhi_wwan_ops = {
> +static const struct wwan_ops mhi_wwan_ops = {
>         .priv_size = sizeof(struct mhi_net_dev),
>         .setup = mhi_net_setup,
>         .newlink = mhi_net_newlink,
> @@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {
>  static int mhi_net_probe(struct mhi_device *mhi_dev,
>                          const struct mhi_device_id *id)
>  {
> -       const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
>         struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
> -       struct net_device *ndev;
> -       int err;
> -
> -       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
> -                               WWAN_NO_DEFAULT_LINK);
> -       if (err)
> -               return err;
> -
> -       if (!create_default_iface)
> -               return 0;
> -
> -       /* Create a default interface which is used as either RMNET real-dev,
> -        * MBIM link 0 or ip link 0)
> -        */
> -       ndev = alloc_netdev(sizeof(struct mhi_net_dev), info->netname,
> -                           NET_NAME_PREDICTABLE, mhi_net_setup);

I like the idea of the default link, but here we need to create the
netdev manually for several reasons:
- In case of QMAP/rmnet, this link is the lower netdev (transport
layer) and is not associated with any link id.
- In case of MBIM, it changes the netdev parent device from the MHI
dev to the WWAN dev, which (currently) breaks how ModemManager groups
ports/netdevs (based on bus).

For the last one, I don't think device hierarchy is considered as
UAPI, so we probably just need to add this new wwan link support to
user tools like MM. For the first one, I plan to split the mhi_net
driver into two different ones (mhi_net_qmap, mhi_net_mbim), and in
the case of qmap(rmnet) forward newlink/dellink call to rmnet
rtnetlink ops.

Regards,
Loic

Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Johannes Berg]

On Tue, 2021-06-15 at 03:30 +0300, Sergey Ryazanov wrote:
> The WWAN core not only multiplex the netdev configuration data, but
> process it too, and needs some space to store its private data
> associated with the netdev. Add a structure to keep common WWAN core
> data. The structure will be stored inside the netdev private data before
> WWAN driver private data and have a field to make it easier to access
> the driver data. Also add a helper function that simplifies drivers
> access to their data.
> 
> At the moment we use the common WWAN private data to store the WWAN data
> link (channel) id at the time the link is created, and report it back to
> user using the .fill_info() RTNL callback. This should help the user to
> be aware which network interface is binded to which WWAN device data

Nit: "binded" -> "bound".

> +static size_t wwan_rtnl_get_size(const struct net_device *dev)
> +{
> +	return
> +		nla_total_size(4) +	/* IFLA_WWAN_LINK_ID */
> +		0;
> +}
> 

Not sure I like that code style, but I guess I don't care much either :)

johannes

Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Loic Poulain]

Hi Sergey,

On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> The WWAN core not only multiplex the netdev configuration data, but
> process it too, and needs some space to store its private data
> associated with the netdev. Add a structure to keep common WWAN core
> data. The structure will be stored inside the netdev private data before
> WWAN driver private data and have a field to make it easier to access
> the driver data. Also add a helper function that simplifies drivers
> access to their data.

Would it be possible to store wwan_netdev_priv at the end of priv data instead?
That would allow drivers to use the standard netdev_priv without any change.
And would also simplify forwarding to rmnet (in mhi_net) since rmnet
uses netdev_priv.

Regards,
Loic

>
> At the moment we use the common WWAN private data to store the WWAN data
> link (channel) id at the time the link is created, and report it back to
> user using the .fill_info() RTNL callback. This should help the user to
> be aware which network interface is binded to which WWAN device data
> channel.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> CC: M Chetan Kumar <m.chetan.kumar@intel.com>
> CC: Intel Corporation <linuxwwan@intel.com>
> ---
>  drivers/net/mhi/net.c                 | 12 +++++------
>  drivers/net/mhi/proto_mbim.c          |  5 +++--
>  drivers/net/wwan/iosm/iosm_ipc_wwan.c | 12 +++++------
>  drivers/net/wwan/wwan_core.c          | 29 ++++++++++++++++++++++++++-
>  include/linux/wwan.h                  | 18 +++++++++++++++++
>  5 files changed, 61 insertions(+), 15 deletions(-)

Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

[Sergey Ryazanov]

Hi Loic,

CC Aleksander, as the talk drifts towards ModemManager.

On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> Utilize the just introduced WWAN core feature to create a default netdev
>> for the default data channel. Since the netdev is now created via the
>> WWAN core, rely on it ability to destroy all child netdevs on ops
>> unregistering.
>>
>> While at it, remove the RTNL lock acquiring hacks that were earlier used
>> to call addlink/dellink without holding the RTNL lock. Also make the
>> WWAN netdev ops structure static to make sparse happy.
>>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> ---
>>  drivers/net/mhi/net.c | 54 +++++--------------------------------------
>>  1 file changed, 6 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
>> index b003003cbd42..06253acecaa2 100644
>> --- a/drivers/net/mhi/net.c
>> +++ b/drivers/net/mhi/net.c
>> @@ -342,10 +342,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
>>         /* Number of transfer descriptors determines size of the queue */
>>         mhi_netdev->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
>>
>> -       if (extack)
>> -               err = register_netdevice(ndev);
>> -       else
>> -               err = register_netdev(ndev);
>> +       err = register_netdevice(ndev);
>>         if (err)
>>                 goto out_err;
>>
>> @@ -370,10 +367,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>>         struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
>>         struct mhi_device *mhi_dev = ctxt;
>>
>> -       if (head)
>> -               unregister_netdevice_queue(ndev, head);
>> -       else
>> -               unregister_netdev(ndev);
>> +       unregister_netdevice_queue(ndev, head);
>>
>>         mhi_unprepare_from_transfer(mhi_dev);
>>
>> @@ -382,7 +376,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>>         dev_set_drvdata(&mhi_dev->dev, NULL);
>>  }
>>
>> -const struct wwan_ops mhi_wwan_ops = {
>> +static const struct wwan_ops mhi_wwan_ops = {
>>         .priv_size = sizeof(struct mhi_net_dev),
>>         .setup = mhi_net_setup,
>>         .newlink = mhi_net_newlink,
>> @@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {
>>  static int mhi_net_probe(struct mhi_device *mhi_dev,
>>                          const struct mhi_device_id *id)
>>  {
>> -       const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
>>         struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
>> -       struct net_device *ndev;
>> -       int err;
>> -
>> -       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
>> -                               WWAN_NO_DEFAULT_LINK);
>> -       if (err)
>> -               return err;
>> -
>> -       if (!create_default_iface)
>> -               return 0;
>> -
>> -       /* Create a default interface which is used as either RMNET real-dev,
>> -        * MBIM link 0 or ip link 0)
>> -        */
>> -       ndev = alloc_netdev(sizeof(struct mhi_net_dev), info->netname,
>> -                           NET_NAME_PREDICTABLE, mhi_net_setup);
>
> I like the idea of the default link, but here we need to create the
> netdev manually for several reasons:
> - In case of QMAP/rmnet, this link is the lower netdev (transport
> layer) and is not associated with any link id.
> - In case of MBIM, it changes the netdev parent device from the MHI
> dev to the WWAN dev, which (currently) breaks how ModemManager groups
> ports/netdevs (based on bus).
>
> For the last one, I don't think device hierarchy is considered as
> UAPI, so we probably just need to add this new wwan link support to
> user tools like MM. For the first one, I plan to split the mhi_net
> driver into two different ones (mhi_net_qmap, mhi_net_mbim), and in
> the case of qmap(rmnet) forward newlink/dellink call to rmnet
> rtnetlink ops.

Looks like I missed the complexity of WWAN devices handling. Thank you
for pointing that out. Now I will drop this patch from the series.

Just curious, am I right to say that any network interface created
with wwan-core is not usable with ModemManager at the moment? AFAIU,
ModemManager is unable to bundle a control port and a netdev into a
common "modem" object, even if they both have the same parent Linux
device, just because that device is not a physical USB device.

--
Sergey

Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Sergey Ryazanov]

On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> The WWAN core not only multiplex the netdev configuration data, but
>> process it too, and needs some space to store its private data
>> associated with the netdev. Add a structure to keep common WWAN core
>> data. The structure will be stored inside the netdev private data before
>> WWAN driver private data and have a field to make it easier to access
>> the driver data. Also add a helper function that simplifies drivers
>> access to their data.
>
> Would it be possible to store wwan_netdev_priv at the end of priv data instead?
> That would allow drivers to use the standard netdev_priv without any change.
> And would also simplify forwarding to rmnet (in mhi_net) since rmnet
> uses netdev_priv.

I do not think that mimicking something by one subsystem for another
is generally a good idea. This could look good in a short term, but
finally it will become a headache due to involvement of too many
entities.

IMHO, a suitable approach to share the rmnet library and data
structures among drivers is to make the rmnet interface more generic.

E.g. consider such netdev/rtnl specific function:

static int rmnet_foo_action(struct net_device *dev, ...)
{
    struct rmnet_priv *rmdev = netdev_priv(dev);
    <do a foo action here>
}

It could be split into a wrapper and an actual handler:

int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
{
    <do a foo action here>
}
EXPORT_GPL(__rmnet_foo_action)

static int rmnet_foo_action(struct net_device *dev, ...)
{
    struct rmnet_priv *rmdev = netdev_priv(dev);
    return __rmnet_foo_action(rmdev, ...)
}

So a call from mhi_net to rmnet could looks like this:

static int mhi_net_foo_action(struct net_device *dev, ...)
{
    struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
    return __rmnet_foo_action(rmdev, ...)
}

In such a way, only the rmnet users know something special, while
other wwan core users and the core itself behave without any
surprises. E.g. any regular wwan core minidriver can access the
link_id field of the wwan common data by calling netdev_priv() without
further calculating the common data offset.

>> At the moment we use the common WWAN private data to store the WWAN data
>> link (channel) id at the time the link is created, and report it back to
>> user using the .fill_info() RTNL callback. This should help the user to
>> be aware which network interface is binded to which WWAN device data
>> channel.
>>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> CC: M Chetan Kumar <m.chetan.kumar@intel.com>
>> CC: Intel Corporation <linuxwwan@intel.com>
>> ---
>>  drivers/net/mhi/net.c                 | 12 +++++------
>>  drivers/net/mhi/proto_mbim.c          |  5 +++--
>>  drivers/net/wwan/iosm/iosm_ipc_wwan.c | 12 +++++------
>>  drivers/net/wwan/wwan_core.c          | 29 ++++++++++++++++++++++++++-
>>  include/linux/wwan.h                  | 18 +++++++++++++++++
>>  5 files changed, 61 insertions(+), 15 deletions(-)

-- 
Sergey

Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Sergey Ryazanov]

Hello Johannes,

On Tue, Jun 15, 2021 at 10:31 AM Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2021-06-15 at 03:30 +0300, Sergey Ryazanov wrote:
>> The WWAN core not only multiplex the netdev configuration data, but
>> process it too, and needs some space to store its private data
>> associated with the netdev. Add a structure to keep common WWAN core
>> data. The structure will be stored inside the netdev private data before
>> WWAN driver private data and have a field to make it easier to access
>> the driver data. Also add a helper function that simplifies drivers
>> access to their data.
>>
>> At the moment we use the common WWAN private data to store the WWAN data
>> link (channel) id at the time the link is created, and report it back to
>> user using the .fill_info() RTNL callback. This should help the user to
>> be aware which network interface is binded to which WWAN device data
>
> Nit: "binded" -> "bound".

Oh. Will fix it in V2.

>> +static size_t wwan_rtnl_get_size(const struct net_device *dev)
>> +{
>> +     return
>> +             nla_total_size(4) +     /* IFLA_WWAN_LINK_ID */
>> +             0;
>> +}
>>
>
> Not sure I like that code style, but I guess I don't care much either :)

Yeah. I am not happy with that code style either. But this is the only
git-blame friendly style known to me that allows us to add new lines
without touching existing ones :(

-- 
Sergey

Re: [PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Sergey Ryazanov]

Hello Chetan,

On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> Since the last commit, the WWAN core will remove all our network
> interfaces for us at the time of the WWAN netdev ops unregistering.
> Therefore, we can safely drop the custom code that cleaning the list of
> created netdevs. Anyway it no longer removes any netdev, since all
> netdevs were removed earlier in the wwan_unregister_ops() call.

Are you Ok with this change? I plan to submit a next version of the
series. If you have any objections, I can address them in V2.

BTW, if IOSM modems have a default data channel, I can add a separate
patch to the series to create a default network interface for IOSM if
you tell me which link id is used for the default data channel.

> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> CC: M Chetan Kumar <m.chetan.kumar@intel.com>
> CC: Intel Corporation <linuxwwan@intel.com>
> ---
>  drivers/net/wwan/iosm/iosm_ipc_wwan.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
> index 1711b79fc616..bee9b278223d 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
> @@ -329,22 +329,9 @@ struct iosm_wwan *ipc_wwan_init(struct iosm_imem *ipc_imem, struct device *dev)
>
>  void ipc_wwan_deinit(struct iosm_wwan *ipc_wwan)
>  {
> -       int if_id;
> -
> +       /* This call will remove all child netdev(s) */
>         wwan_unregister_ops(ipc_wwan->dev);
>
> -       for (if_id = 0; if_id < ARRAY_SIZE(ipc_wwan->sub_netlist); if_id++) {
> -               struct iosm_netdev_priv *priv;
> -
> -               priv = rcu_access_pointer(ipc_wwan->sub_netlist[if_id]);
> -               if (!priv)
> -                       continue;
> -
> -               rtnl_lock();
> -               ipc_wwan_dellink(ipc_wwan, priv->netdev, NULL);
> -               rtnl_unlock();
> -       }
> -
>         mutex_destroy(&ipc_wwan->if_mutex);
>
>         kfree(ipc_wwan);

-- 
Sergey

RE: [PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Kumar, M Chetan]

Hi Sergey,

> On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov
> <ryazanov.s.a@gmail.com> wrote:
> > Since the last commit, the WWAN core will remove all our network
> > interfaces for us at the time of the WWAN netdev ops unregistering.
> > Therefore, we can safely drop the custom code that cleaning the list
> > of created netdevs. Anyway it no longer removes any netdev, since all
> > netdevs were removed earlier in the wwan_unregister_ops() call.
> 
> Are you Ok with this change? I plan to submit a next version of the series. If
> you have any objections, I can address them in V2.

Changes looks fine.
 
> BTW, if IOSM modems have a default data channel, I can add a separate
> patch to the series to create a default network interface for IOSM if you tell
> me which link id is used for the default data channel.

Link id 1 is always associated as default data channel. 

Thanks,
Reviewed-by: M Chetan Kumar <m.chetan.kumar@intel.com>

Re: [PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Sergey Ryazanov]

On Sun, Jun 20, 2021 at 6:42 PM Kumar, M Chetan
<m.chetan.kumar@intel.com> wrote:
>> On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov
>> <ryazanov.s.a@gmail.com> wrote:
>>> Since the last commit, the WWAN core will remove all our network
>>> interfaces for us at the time of the WWAN netdev ops unregistering.
>>> Therefore, we can safely drop the custom code that cleaning the list
>>> of created netdevs. Anyway it no longer removes any netdev, since all
>>> netdevs were removed earlier in the wwan_unregister_ops() call.
>>
>> Are you Ok with this change? I plan to submit a next version of the series. If
>> you have any objections, I can address them in V2.
>
> Changes looks fine.
>
>> BTW, if IOSM modems have a default data channel, I can add a separate
>> patch to the series to create a default network interface for IOSM if you tell
>> me which link id is used for the default data channel.
>
> Link id 1 is always associated as default data channel.

Thank you, will add default interface creation with this Id in the V2 series.

> Thanks,
> Reviewed-by: M Chetan Kumar <m.chetan.kumar@intel.com>

-- 
Sergey

Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

[Loic Poulain]

Hi Sergey,

On Sun, 20 Jun 2021 at 15:51, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Hi Loic,
>
> CC Aleksander, as the talk drifts towards ModemManager.
>
> On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> > On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> >> Utilize the just introduced WWAN core feature to create a default netdev
> >> for the default data channel. Since the netdev is now created via the
> >> WWAN core, rely on it ability to destroy all child netdevs on ops
> >> unregistering.
> >>
> >> While at it, remove the RTNL lock acquiring hacks that were earlier used
> >> to call addlink/dellink without holding the RTNL lock. Also make the
> >> WWAN netdev ops structure static to make sparse happy.
> >>
> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> >> ---
> >>  drivers/net/mhi/net.c | 54 +++++--------------------------------------
> >>  1 file changed, 6 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
> >> index b003003cbd42..06253acecaa2 100644
> >> --- a/drivers/net/mhi/net.c
> >> +++ b/drivers/net/mhi/net.c
> >> @@ -342,10 +342,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
> >>         /* Number of transfer descriptors determines size of the queue */
> >>         mhi_netdev->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
> >>
> >> -       if (extack)
> >> -               err = register_netdevice(ndev);
> >> -       else
> >> -               err = register_netdev(ndev);
> >> +       err = register_netdevice(ndev);
> >>         if (err)
> >>                 goto out_err;
> >>
> >> @@ -370,10 +367,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
> >>         struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
> >>         struct mhi_device *mhi_dev = ctxt;
> >>
> >> -       if (head)
> >> -               unregister_netdevice_queue(ndev, head);
> >> -       else
> >> -               unregister_netdev(ndev);
> >> +       unregister_netdevice_queue(ndev, head);
> >>
> >>         mhi_unprepare_from_transfer(mhi_dev);
> >>
> >> @@ -382,7 +376,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
> >>         dev_set_drvdata(&mhi_dev->dev, NULL);
> >>  }
> >>
> >> -const struct wwan_ops mhi_wwan_ops = {
> >> +static const struct wwan_ops mhi_wwan_ops = {
> >>         .priv_size = sizeof(struct mhi_net_dev),
> >>         .setup = mhi_net_setup,
> >>         .newlink = mhi_net_newlink,
> >> @@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {
> >>  static int mhi_net_probe(struct mhi_device *mhi_dev,
> >>                          const struct mhi_device_id *id)
> >>  {
> >> -       const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
> >>         struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
> >> -       struct net_device *ndev;
> >> -       int err;
> >> -
> >> -       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
> >> -                               WWAN_NO_DEFAULT_LINK);
> >> -       if (err)
> >> -               return err;
> >> -
> >> -       if (!create_default_iface)
> >> -               return 0;
> >> -
> >> -       /* Create a default interface which is used as either RMNET real-dev,
> >> -        * MBIM link 0 or ip link 0)
> >> -        */
> >> -       ndev = alloc_netdev(sizeof(struct mhi_net_dev), info->netname,
> >> -                           NET_NAME_PREDICTABLE, mhi_net_setup);
> >
> > I like the idea of the default link, but here we need to create the
> > netdev manually for several reasons:
> > - In case of QMAP/rmnet, this link is the lower netdev (transport
> > layer) and is not associated with any link id.
> > - In case of MBIM, it changes the netdev parent device from the MHI
> > dev to the WWAN dev, which (currently) breaks how ModemManager groups
> > ports/netdevs (based on bus).
> >
> > For the last one, I don't think device hierarchy is considered as
> > UAPI, so we probably just need to add this new wwan link support to
> > user tools like MM. For the first one, I plan to split the mhi_net
> > driver into two different ones (mhi_net_qmap, mhi_net_mbim), and in
> > the case of qmap(rmnet) forward newlink/dellink call to rmnet
> > rtnetlink ops.
>
> Looks like I missed the complexity of WWAN devices handling. Thank you
> for pointing that out. Now I will drop this patch from the series.
>
> Just curious, am I right to say that any network interface created
> with wwan-core is not usable with ModemManager at the moment? AFAIU,
> ModemManager is unable to bundle a control port and a netdev into a
> common "modem" object, even if they both have the same parent Linux
> device, just because that device is not a physical USB device.

Right, there is an ongoing discussion about supporting iosm:
https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/issues/385#note_958408

So once we have it working for iosm, it should work for any WWAN
device using WWAN framework.

Regards,
Loic

Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Loic Poulain]

Hi Sergey,

On Sun, 20 Jun 2021 at 16:39, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> > On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> >> The WWAN core not only multiplex the netdev configuration data, but
> >> process it too, and needs some space to store its private data
> >> associated with the netdev. Add a structure to keep common WWAN core
> >> data. The structure will be stored inside the netdev private data before
> >> WWAN driver private data and have a field to make it easier to access
> >> the driver data. Also add a helper function that simplifies drivers
> >> access to their data.
> >
> > Would it be possible to store wwan_netdev_priv at the end of priv data instead?
> > That would allow drivers to use the standard netdev_priv without any change.
> > And would also simplify forwarding to rmnet (in mhi_net) since rmnet
> > uses netdev_priv.
>
> I do not think that mimicking something by one subsystem for another
> is generally a good idea. This could look good in a short term, but
> finally it will become a headache due to involvement of too many
> entities.
>
> IMHO, a suitable approach to share the rmnet library and data
> structures among drivers is to make the rmnet interface more generic.
>
> E.g. consider such netdev/rtnl specific function:
>
> static int rmnet_foo_action(struct net_device *dev, ...)
> {
>     struct rmnet_priv *rmdev = netdev_priv(dev);
>     <do a foo action here>
> }
>
> It could be split into a wrapper and an actual handler:
>
> int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
> {
>     <do a foo action here>
> }
> EXPORT_GPL(__rmnet_foo_action)
>
> static int rmnet_foo_action(struct net_device *dev, ...)
> {
>     struct rmnet_priv *rmdev = netdev_priv(dev);
>     return __rmnet_foo_action(rmdev, ...)
> }
>
> So a call from mhi_net to rmnet could looks like this:
>
> static int mhi_net_foo_action(struct net_device *dev, ...)
> {
>     struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
>     return __rmnet_foo_action(rmdev, ...)
> }
>
> In such a way, only the rmnet users know something special, while
> other wwan core users and the core itself behave without any
> surprises. E.g. any regular wwan core minidriver can access the
> link_id field of the wwan common data by calling netdev_priv() without
> further calculating the common data offset.

Yes, that would work, but it's an important refactoring since rmnet is
all built around the idea that netdev_priv is rmnet_priv, including rx
path (netdev_priv(skb->dev)).
My initial tests were based on this 'simple' change:
https://git.linaro.org/people/loic.poulain/linux.git/commit/?h=wwan_rmnet&id=6308d49790f10615bd33a38d56bc7f101646558f

Moreover, a driver like mhi_net also supports non WWAN local link
(called mhi_swip), which is a network link between the host and the
modem cpu (for modem hosted services...). This link is not managed by
the WWAN layer and is directly created by mhi_net. I could create a
different driver or set of handlers for this netdev, but it's
additional complexity.

> >> At the moment we use the common WWAN private data to store the WWAN data
> >> link (channel) id at the time the link is created, and report it back to
> >> user using the .fill_info() RTNL callback. This should help the user to
> >> be aware which network interface is binded to which WWAN device data
> >> channel.

I wonder if it would not be simpler to store the link ID into
netdev->dev_port, it's after all a kind of virtual/logical port.
That would only postpone the introduction of a wwan_netdev_priv struct though.

Regards,
Loic

Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

[Sergey Ryazanov]

Hi Loic,

On Mon, Jun 21, 2021 at 9:44 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> On Sun, 20 Jun 2021 at 15:51, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain <loic.poulain@linaro.org> wrote:
>>> On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>>>> Utilize the just introduced WWAN core feature to create a default netdev
>>>> for the default data channel. Since the netdev is now created via the
>>>> WWAN core, rely on it ability to destroy all child netdevs on ops
>>>> unregistering.
>>>>
>>>> While at it, remove the RTNL lock acquiring hacks that were earlier used
>>>> to call addlink/dellink without holding the RTNL lock. Also make the
>>>> WWAN netdev ops structure static to make sparse happy.
>>>>
>>>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>>>> ---
>>>>  drivers/net/mhi/net.c | 54 +++++--------------------------------------
>>>>  1 file changed, 6 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
>>>> index b003003cbd42..06253acecaa2 100644
>>>> --- a/drivers/net/mhi/net.c
>>>> +++ b/drivers/net/mhi/net.c
>>>> @@ -342,10 +342,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
>>>>         /* Number of transfer descriptors determines size of the queue */
>>>>         mhi_netdev->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
>>>>
>>>> -       if (extack)
>>>> -               err = register_netdevice(ndev);
>>>> -       else
>>>> -               err = register_netdev(ndev);
>>>> +       err = register_netdevice(ndev);
>>>>         if (err)
>>>>                 goto out_err;
>>>>
>>>> @@ -370,10 +367,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>>>>         struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
>>>>         struct mhi_device *mhi_dev = ctxt;
>>>>
>>>> -       if (head)
>>>> -               unregister_netdevice_queue(ndev, head);
>>>> -       else
>>>> -               unregister_netdev(ndev);
>>>> +       unregister_netdevice_queue(ndev, head);
>>>>
>>>>         mhi_unprepare_from_transfer(mhi_dev);
>>>>
>>>> @@ -382,7 +376,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>>>>         dev_set_drvdata(&mhi_dev->dev, NULL);
>>>>  }
>>>>
>>>> -const struct wwan_ops mhi_wwan_ops = {
>>>> +static const struct wwan_ops mhi_wwan_ops = {
>>>>         .priv_size = sizeof(struct mhi_net_dev),
>>>>         .setup = mhi_net_setup,
>>>>         .newlink = mhi_net_newlink,
>>>> @@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {
>>>>  static int mhi_net_probe(struct mhi_device *mhi_dev,
>>>>                          const struct mhi_device_id *id)
>>>>  {
>>>> -       const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
>>>>         struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
>>>> -       struct net_device *ndev;
>>>> -       int err;
>>>> -
>>>> -       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
>>>> -                               WWAN_NO_DEFAULT_LINK);
>>>> -       if (err)
>>>> -               return err;
>>>> -
>>>> -       if (!create_default_iface)
>>>> -               return 0;
>>>> -
>>>> -       /* Create a default interface which is used as either RMNET real-dev,
>>>> -        * MBIM link 0 or ip link 0)
>>>> -        */
>>>> -       ndev = alloc_netdev(sizeof(struct mhi_net_dev), info->netname,
>>>> -                           NET_NAME_PREDICTABLE, mhi_net_setup);
>>>
>>> I like the idea of the default link, but here we need to create the
>>> netdev manually for several reasons:
>>> - In case of QMAP/rmnet, this link is the lower netdev (transport
>>> layer) and is not associated with any link id.
>>> - In case of MBIM, it changes the netdev parent device from the MHI
>>> dev to the WWAN dev, which (currently) breaks how ModemManager groups
>>> ports/netdevs (based on bus).
>>>
>>> For the last one, I don't think device hierarchy is considered as
>>> UAPI, so we probably just need to add this new wwan link support to
>>> user tools like MM. For the first one, I plan to split the mhi_net
>>> driver into two different ones (mhi_net_qmap, mhi_net_mbim), and in
>>> the case of qmap(rmnet) forward newlink/dellink call to rmnet
>>> rtnetlink ops.
>>
>> Looks like I missed the complexity of WWAN devices handling. Thank you
>> for pointing that out. Now I will drop this patch from the series.
>>
>> Just curious, am I right to say that any network interface created
>> with wwan-core is not usable with ModemManager at the moment? AFAIU,
>> ModemManager is unable to bundle a control port and a netdev into a
>> common "modem" object, even if they both have the same parent Linux
>> device, just because that device is not a physical USB device.
>
> Right, there is an ongoing discussion about supporting iosm:
> https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/issues/385#note_958408
>
> So once we have it working for iosm, it should work for any WWAN
> device using WWAN framework.

Thank you for the clarification, I will keep in mind.

-- 
Sergey

Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Sergey Ryazanov]

Hi Loic,

On Mon, Jun 21, 2021 at 10:28 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> On Sun, 20 Jun 2021 at 16:39, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain <loic.poulain@linaro.org> wrote:
>>> On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>>>> The WWAN core not only multiplex the netdev configuration data, but
>>>> process it too, and needs some space to store its private data
>>>> associated with the netdev. Add a structure to keep common WWAN core
>>>> data. The structure will be stored inside the netdev private data before
>>>> WWAN driver private data and have a field to make it easier to access
>>>> the driver data. Also add a helper function that simplifies drivers
>>>> access to their data.
>>>
>>> Would it be possible to store wwan_netdev_priv at the end of priv data instead?
>>> That would allow drivers to use the standard netdev_priv without any change.
>>> And would also simplify forwarding to rmnet (in mhi_net) since rmnet
>>> uses netdev_priv.
>>
>> I do not think that mimicking something by one subsystem for another
>> is generally a good idea. This could look good in a short term, but
>> finally it will become a headache due to involvement of too many
>> entities.
>>
>> IMHO, a suitable approach to share the rmnet library and data
>> structures among drivers is to make the rmnet interface more generic.
>>
>> E.g. consider such netdev/rtnl specific function:
>>
>> static int rmnet_foo_action(struct net_device *dev, ...)
>> {
>>     struct rmnet_priv *rmdev = netdev_priv(dev);
>>     <do a foo action here>
>> }
>>
>> It could be split into a wrapper and an actual handler:
>>
>> int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
>> {
>>     <do a foo action here>
>> }
>> EXPORT_GPL(__rmnet_foo_action)
>>
>> static int rmnet_foo_action(struct net_device *dev, ...)
>> {
>>     struct rmnet_priv *rmdev = netdev_priv(dev);
>>     return __rmnet_foo_action(rmdev, ...)
>> }
>>
>> So a call from mhi_net to rmnet could looks like this:
>>
>> static int mhi_net_foo_action(struct net_device *dev, ...)
>> {
>>     struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
>>     return __rmnet_foo_action(rmdev, ...)
>> }
>>
>> In such a way, only the rmnet users know something special, while
>> other wwan core users and the core itself behave without any
>> surprises. E.g. any regular wwan core minidriver can access the
>> link_id field of the wwan common data by calling netdev_priv() without
>> further calculating the common data offset.
>
> Yes, that would work, but it's an important refactoring since rmnet is
> all built around the idea that netdev_priv is rmnet_priv, including rx
> path (netdev_priv(skb->dev)).
> My initial tests were based on this 'simple' change:
> https://git.linaro.org/people/loic.poulain/linux.git/commit/?h=wwan_rmnet&id=6308d49790f10615bd33a38d56bc7f101646558f
>
> Moreover, a driver like mhi_net also supports non WWAN local link
> (called mhi_swip), which is a network link between the host and the
> modem cpu (for modem hosted services...). This link is not managed by
> the WWAN layer and is directly created by mhi_net. I could create a
> different driver or set of handlers for this netdev, but it's
> additional complexity.

Correct me if I am wrong. I just checked the rmnet code and realized
that rmnet should work on top of mhi_net and not vice versa. mhi_net
should provide some kind of transportation for QMAP packets from a HW
device to rmnet. Then rmnet will perform demultiplexing, deaggregation
and decapsulation of QMAP packets to pure IP packets.

rmnet itself receives these QMAP packets via a network device. So any
driver that would like to provide the QMAP transport for rmnet should
create a network device for this task.

The main issue with the integration of mhi_net with the wwan is that
the mhi_net driver should pass its traffic through the rmnet demuxer.
While the network device that will be created by the rmnet demuxer
will not be a child of a MHI device or a wwan device. So to properly
integrate the mhi_net driver with the wwan core netdev capabilities,
you should begin to use rmnet not as an independent demux created on
top of a transport network interface, but as a library. Am I correctly
understanding?

Does the same issue appear when we begin a more tight integration of
the qmi_wwan USB driver with the wwan core?

I would like to say that one way or another, rmnet will be converted
to a quite abstract library that should avoid direct access to the
network device private data.

>>>> At the moment we use the common WWAN private data to store the WWAN data
>>>> link (channel) id at the time the link is created, and report it back to
>>>> user using the .fill_info() RTNL callback. This should help the user to
>>>> be aware which network interface is binded to which WWAN device data
>>>> channel.
>
> I wonder if it would not be simpler to store the link ID into
> netdev->dev_port, it's after all a kind of virtual/logical port.
> That would only postpone the introduction of a wwan_netdev_priv struct though.

I like this idea. This is likely to solve the link id storage problem.
But only if we plan to never extend the wwan core private data.
Otherwise, as you mention, this only postpones the wwan data structure
introduction to a moment when we will need to rework a lot of drivers.

Looks like we have no absolutely good solution. Only a set of
proposals, each which has its own shortcomings :(

-- 
Sergey

Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

[Loic Poulain]

On Mon, 21 Jun 2021 at 19:22, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> Hi Loic,
>
> On Mon, Jun 21, 2021 at 10:28 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> > On Sun, 20 Jun 2021 at 16:39, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> >> On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain <loic.poulain@linaro.org> wrote:
> >>> On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
> >>>> The WWAN core not only multiplex the netdev configuration data, but
> >>>> process it too, and needs some space to store its private data
> >>>> associated with the netdev. Add a structure to keep common WWAN core
> >>>> data. The structure will be stored inside the netdev private data before
> >>>> WWAN driver private data and have a field to make it easier to access
> >>>> the driver data. Also add a helper function that simplifies drivers
> >>>> access to their data.
> >>>
> >>> Would it be possible to store wwan_netdev_priv at the end of priv data instead?
> >>> That would allow drivers to use the standard netdev_priv without any change.
> >>> And would also simplify forwarding to rmnet (in mhi_net) since rmnet
> >>> uses netdev_priv.
> >>
> >> I do not think that mimicking something by one subsystem for another
> >> is generally a good idea. This could look good in a short term, but
> >> finally it will become a headache due to involvement of too many
> >> entities.
> >>
> >> IMHO, a suitable approach to share the rmnet library and data
> >> structures among drivers is to make the rmnet interface more generic.
> >>
> >> E.g. consider such netdev/rtnl specific function:
> >>
> >> static int rmnet_foo_action(struct net_device *dev, ...)
> >> {
> >>     struct rmnet_priv *rmdev = netdev_priv(dev);
> >>     <do a foo action here>
> >> }
> >>
> >> It could be split into a wrapper and an actual handler:
> >>
> >> int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
> >> {
> >>     <do a foo action here>
> >> }
> >> EXPORT_GPL(__rmnet_foo_action)
> >>
> >> static int rmnet_foo_action(struct net_device *dev, ...)
> >> {
> >>     struct rmnet_priv *rmdev = netdev_priv(dev);
> >>     return __rmnet_foo_action(rmdev, ...)
> >> }
> >>
> >> So a call from mhi_net to rmnet could looks like this:
> >>
> >> static int mhi_net_foo_action(struct net_device *dev, ...)
> >> {
> >>     struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
> >>     return __rmnet_foo_action(rmdev, ...)
> >> }
> >>
> >> In such a way, only the rmnet users know something special, while
> >> other wwan core users and the core itself behave without any
> >> surprises. E.g. any regular wwan core minidriver can access the
> >> link_id field of the wwan common data by calling netdev_priv() without
> >> further calculating the common data offset.
> >
> > Yes, that would work, but it's an important refactoring since rmnet is
> > all built around the idea that netdev_priv is rmnet_priv, including rx
> > path (netdev_priv(skb->dev)).
> > My initial tests were based on this 'simple' change:
> > https://git.linaro.org/people/loic.poulain/linux.git/commit/?h=wwan_rmnet&id=6308d49790f10615bd33a38d56bc7f101646558f
> >
> > Moreover, a driver like mhi_net also supports non WWAN local link
> > (called mhi_swip), which is a network link between the host and the
> > modem cpu (for modem hosted services...). This link is not managed by
> > the WWAN layer and is directly created by mhi_net. I could create a
> > different driver or set of handlers for this netdev, but it's
> > additional complexity.
>
> Correct me if I am wrong. I just checked the rmnet code and realized
> that rmnet should work on top of mhi_net and not vice versa. mhi_net
> should provide some kind of transportation for QMAP packets from a HW
> device to rmnet. Then rmnet will perform demultiplexing, deaggregation
> and decapsulation of QMAP packets to pure IP packets.

Exact mhi_net act as the transport layer in that case.

> rmnet itself receives these QMAP packets via a network device. So any
> driver that would like to provide the QMAP transport for rmnet should
> create a network device for this task.

Yes, this is what they call the 'real' device (or lower_dev).

> The main issue with the integration of mhi_net with the wwan is that
> the mhi_net driver should pass its traffic through the rmnet demuxer.
> While the network device that will be created by the rmnet demuxer
> will not be a child of a MHI device or a wwan device. So to properly
> integrate the mhi_net driver with the wwan core netdev capabilities,
> you should begin to use rmnet not as an independent demux created on
> top of a transport network interface, but as a library. Am I correctly
> understanding?

Once the link is created, packets received by the 'real' ndev are
automatically forwarded to the upper layer (in that case, rmnet). So
the 'transport' netdev doesn't even need to know about the upper layer
details.

> Does the same issue appear when we begin a more tight integration of
> the qmi_wwan USB driver with the wwan core?

That should be handled the same way as for mhi_net, indeed.

> I would like to say that one way or another, rmnet will be converted
> to a quite abstract library that should avoid direct access to the
> network device private data.
>
> >>>> At the moment we use the common WWAN private data to store the WWAN data
> >>>> link (channel) id at the time the link is created, and report it back to
> >>>> user using the .fill_info() RTNL callback. This should help the user to
> >>>> be aware which network interface is binded to which WWAN device data
> >>>> channel.
> >
> > I wonder if it would not be simpler to store the link ID into
> > netdev->dev_port, it's after all a kind of virtual/logical port.
> > That would only postpone the introduction of a wwan_netdev_priv struct though.
>
> I like this idea. This is likely to solve the link id storage problem.
> But only if we plan to never extend the wwan core private data.
> Otherwise, as you mention, this only postpones the wwan data structure
> introduction to a moment when we will need to rework a lot of drivers.
>
> Looks like we have no absolutely good solution. Only a set of
> proposals, each which has its own shortcomings :(
>
> --

Regards,
Loic

Re: [PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Loic Poulain]

Hi Chetan,

On Sun, 20 Jun 2021 at 17:42, Kumar, M Chetan <m.chetan.kumar@intel.com> wrote:
>
> Hi Sergey,
>
> > On Tue, Jun 15, 2021 at 3:30 AM Sergey Ryazanov
> > <ryazanov.s.a@gmail.com> wrote:
> > > Since the last commit, the WWAN core will remove all our network
> > > interfaces for us at the time of the WWAN netdev ops unregistering.
> > > Therefore, we can safely drop the custom code that cleaning the list
> > > of created netdevs. Anyway it no longer removes any netdev, since all
> > > netdevs were removed earlier in the wwan_unregister_ops() call.
> >
> > Are you Ok with this change? I plan to submit a next version of the series. If
> > you have any objections, I can address them in V2.
>
> Changes looks fine.
>
> > BTW, if IOSM modems have a default data channel, I can add a separate
> > patch to the series to create a default network interface for IOSM if you tell
> > me which link id is used for the default data channel.
>
> Link id 1 is always associated as default data channel.

Quick question, Isn't your driver use MBIM session IDs? with
session-ID 0 as the default one?

Regards,
Loic

Re: [PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Kumar, M Chetan]

Hi Loic,

On 6/29/2021 7:44 PM, Loic Poulain wrote:

>>> BTW, if IOSM modems have a default data channel, I can add a separate
>>> patch to the series to create a default network interface for IOSM if you tell
>>> me which link id is used for the default data channel.
>>
>> Link id 1 is always associated as default data channel.
> 
> Quick question, Isn't your driver use MBIM session IDs? with
> session-ID 0 as the default one?

Link Id from 1 to 8 are treated as valid link ids. These ids are
decremented by 1 to match session id.

In this case link id 1 would be mapped to session id 0. So have
requested link id 1 to be set as default data channel.

Regards,
Chetan

Re: [PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Loic Poulain]

Hi Chetan,

On Tue, 29 Jun 2021 at 16:56, Kumar, M Chetan <m.chetan.kumar@intel.com> wrote:
>
> Hi Loic,
>
> On 6/29/2021 7:44 PM, Loic Poulain wrote:
>
> >>> BTW, if IOSM modems have a default data channel, I can add a separate
> >>> patch to the series to create a default network interface for IOSM if you tell
> >>> me which link id is used for the default data channel.
> >>
> >> Link id 1 is always associated as default data channel.
> >
> > Quick question, Isn't your driver use MBIM session IDs? with
> > session-ID 0 as the default one?
>
> Link Id from 1 to 8 are treated as valid link ids. These ids are
> decremented by 1 to match session id.
>
> In this case link id 1 would be mapped to session id 0. So have
> requested link id 1 to be set as default data channel.

Oh ok, but why? it seems quite confusing, that means a user creating a
MBIM session 0 has to create a link with ID 1?
It seems to be quite specific to your driver, can't you simply handle
ID 0 from user? to keep aligned with other drivers.

Regards,
Loic

Re: [PATCH net-next 06/10] net: iosm: drop custom netdev(s) removing

[Kumar, M Chetan]

Hi Loic,

On 6/29/2021 8:59 PM, Loic Poulain wrote:
> Hi Chetan,
> 
> On Tue, 29 Jun 2021 at 16:56, Kumar, M Chetan <m.chetan.kumar@intel.com> wrote:
>>
>> Hi Loic,
>>
>> On 6/29/2021 7:44 PM, Loic Poulain wrote:
>>
>>>>> BTW, if IOSM modems have a default data channel, I can add a separate
>>>>> patch to the series to create a default network interface for IOSM if you tell
>>>>> me which link id is used for the default data channel.
>>>>
>>>> Link id 1 is always associated as default data channel.
>>>
>>> Quick question, Isn't your driver use MBIM session IDs? with
>>> session-ID 0 as the default one?
>>
>> Link Id from 1 to 8 are treated as valid link ids. These ids are
>> decremented by 1 to match session id.
>>
>> In this case link id 1 would be mapped to session id 0. So have
>> requested link id 1 to be set as default data channel.
> 
> Oh ok, but why? it seems quite confusing, that means a user creating a
> MBIM session 0 has to create a link with ID 1?
> It seems to be quite specific to your driver, can't you simply handle
> ID 0 from user? to keep aligned with other drivers.

Thought link id 0 is not a valid id so had considered it from 1 :(

Sure, We will correct it to be intact with other drivers.

Regards,
Chetan