[RFC 0/4] wwan framework netdev creation

[RFC 0/4] wwan framework netdev creation

[Johannes Berg]

Here's a respin of the series to create netdevs through the WWAN
framework. I haven't tested it at all, since I don't have any
such hardware, so I guess there will be some bugs ... It'd be
best if somebody else takes over here, Loic, maybe I can talk
you into getting the generic bits done if you have a test case? :)

This applies on top of the IOSM driver series posted here:
https://lore.kernel.org/r/20210520140158.10132-1-m.chetan.kumar@intel.com

I've included the first bugfix patch only so it actually all
can apply properly, not really needed for review.

johannes

[RFC 1/4] iosm: fix stats and RCU bugs in RX

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/wwan/iosm/iosm_ipc_wwan.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
index 02c35bc86674..719c88d9b2e9 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
@@ -312,7 +312,8 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
 		     bool dss, int if_id)
 {
 	struct sk_buff *skb = skb_arg;
-	struct net_device_stats stats;
+	struct net_device_stats *stats;
+	struct iosm_net_link *priv;
 	int ret;
 
 	if ((skb->data[0] & IOSM_IP_TYPE_MASK) == IOSM_IP_TYPE_IPV4)
@@ -325,19 +326,27 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
 
 	if (if_id < (IP_MUX_SESSION_START - 1) ||
 	    if_id > (IP_MUX_SESSION_END - 1)) {
-		dev_kfree_skb(skb);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto free;
 	}
 
 	rcu_read_lock();
-	skb->dev = rcu_dereference(ipc_wwan->sub_netlist[if_id])->netdev;
-	stats = rcu_dereference(ipc_wwan->sub_netlist[if_id])->netdev->stats;
-	stats.rx_packets++;
-	stats.rx_bytes += skb->len;
+	priv = rcu_dereference(ipc_wwan->sub_netlist[if_id]);
+	if (!priv) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+	skb->dev = priv->netdev;
+	stats = &priv->netdev->stats;
+	stats->rx_packets++;
+	stats->rx_bytes += skb->len;
 
 	ret = netif_rx(skb);
+	skb = NULL;
+unlock:
 	rcu_read_unlock();
-
+free:
+	dev_kfree_skb(skb);
 	return ret;
 }
 
-- 
2.31.1

[RFC 2/4] rtnetlink: add alloc() method to rtnl_link_ops

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

In order to make rtnetlink ops that can create different
kinds of devices, like what we want to add to the WWAN
framework, the priv_size and setup parameters aren't quite
sufficient. Make this easier to manage by allowing ops to
allocate their own netdev via an @alloc method that gets
both the tb and data.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/bareudp.c   |  2 +-
 drivers/net/can/vxcan.c |  2 +-
 drivers/net/geneve.c    |  2 +-
 drivers/net/veth.c      |  2 +-
 drivers/net/vxlan.c     |  2 +-
 include/net/rtnetlink.h | 10 ++++++++++
 net/core/rtnetlink.c    | 17 ++++++++++++++---
 net/ipv4/ip_gre.c       |  2 +-
 8 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
index edfad93e7b68..a694f6d8eb21 100644
--- a/drivers/net/bareudp.c
+++ b/drivers/net/bareudp.c
@@ -727,7 +727,7 @@ struct net_device *bareudp_dev_create(struct net *net, const char *name,
 
 	memset(tb, 0, sizeof(tb));
 	dev = rtnl_create_link(net, name, name_assign_type,
-			       &bareudp_link_ops, tb, NULL);
+			       &bareudp_link_ops, tb, NULL, NULL);
 	if (IS_ERR(dev))
 		return dev;
 
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8861a7d875e7..f904c78b4c23 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -204,7 +204,7 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 		return PTR_ERR(peer_net);
 
 	peer = rtnl_create_link(peer_net, ifname, name_assign_type,
-				&vxcan_link_ops, tbp, extack);
+				&vxcan_link_ops, tbp, NULL, extack);
 	if (IS_ERR(peer)) {
 		put_net(peer_net);
 		return PTR_ERR(peer);
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 1ab94b5f9bbf..130397110623 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1840,7 +1840,7 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 
 	memset(tb, 0, sizeof(tb));
 	dev = rtnl_create_link(net, name, name_assign_type,
-			       &geneve_link_ops, tb, NULL);
+			       &geneve_link_ops, tb, NULL, NULL);
 	if (IS_ERR(dev))
 		return dev;
 
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index bdb7ce3cb054..788faa8faa98 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1498,7 +1498,7 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 		return PTR_ERR(net);
 
 	peer = rtnl_create_link(net, ifname, name_assign_type,
-				&veth_link_ops, tbp, extack);
+				&veth_link_ops, tbp, NULL, extack);
 	if (IS_ERR(peer)) {
 		put_net(net);
 		return PTR_ERR(peer);
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 02a14f1b938a..b8a63a8bcb73 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4483,7 +4483,7 @@ struct net_device *vxlan_dev_create(struct net *net, const char *name,
 	memset(&tb, 0, sizeof(tb));
 
 	dev = rtnl_create_link(net, name, name_assign_type,
-			       &vxlan_link_ops, tb, NULL);
+			       &vxlan_link_ops, tb, NULL, NULL);
 	if (IS_ERR(dev))
 		return dev;
 
diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 479f60ef54c0..75426ad5a05b 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -37,6 +37,9 @@ static inline int rtnl_msg_family(const struct nlmsghdr *nlh)
  *	@maxtype: Highest device specific netlink attribute number
  *	@policy: Netlink policy for device specific attribute validation
  *	@validate: Optional validation function for netlink/changelink parameters
+ *	@alloc: netdev allocation function, can be %NULL and is then used
+ *		in place of alloc_netdev_mqs(), in this case @priv_size
+ *		and @setup are unused. Returns a netdev or ERR_PTR().
  *	@priv_size: sizeof net_device private space
  *	@setup: net_device setup function
  *	@newlink: Function for configuring and registering a new device
@@ -63,6 +66,12 @@ struct rtnl_link_ops {
 	const char		*kind;
 
 	size_t			priv_size;
+	struct net_device	*(*alloc)(struct nlattr *tb[],
+					  struct nlattr *data[],
+					  const char *ifname,
+					  unsigned char name_assign_type,
+					  unsigned int num_tx_queues,
+					  unsigned int num_rx_queues);
 	void			(*setup)(struct net_device *dev);
 
 	bool			netns_refund;
@@ -162,6 +171,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
 				    unsigned char name_assign_type,
 				    const struct rtnl_link_ops *ops,
 				    struct nlattr *tb[],
+				    struct nlattr *data[],
 				    struct netlink_ext_ack *extack);
 int rtnl_delete_link(struct net_device *dev);
 int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 714d5fa38546..9da38639f088 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3151,6 +3151,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
 				    unsigned char name_assign_type,
 				    const struct rtnl_link_ops *ops,
 				    struct nlattr *tb[],
+				    struct nlattr *data[],
 				    struct netlink_ext_ack *extack)
 {
 	struct net_device *dev;
@@ -3177,8 +3178,17 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
 		return ERR_PTR(-EINVAL);
 	}
 
-	dev = alloc_netdev_mqs(ops->priv_size, ifname, name_assign_type,
-			       ops->setup, num_tx_queues, num_rx_queues);
+	if (ops->alloc) {
+		dev = ops->alloc(tb, data, ifname, name_assign_type,
+				 num_tx_queues, num_rx_queues);
+		if (IS_ERR(dev))
+			return dev;
+	} else {
+		dev = alloc_netdev_mqs(ops->priv_size, ifname,
+				       name_assign_type, ops->setup,
+				       num_tx_queues, num_rx_queues);
+	}
+
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
 
@@ -3440,7 +3450,8 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	dev = rtnl_create_link(link_net ? : dest_net, ifname,
-			       name_assign_type, ops, tb, extack);
+			       name_assign_type, ops, tb, data,
+			       extack);
 	if (IS_ERR(dev)) {
 		err = PTR_ERR(dev);
 		goto out;
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index a68bf4c6fe9b..7680f5b87f61 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1637,7 +1637,7 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
 	memset(&tb, 0, sizeof(tb));
 
 	dev = rtnl_create_link(net, name, name_assign_type,
-			       &ipgre_tap_ops, tb, NULL);
+			       &ipgre_tap_ops, tb, NULL, NULL);
 	if (IS_ERR(dev))
 		return dev;
 
-- 
2.31.1

[RFC 3/4] wwan: add interface creation support

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Add support to create (and destroy) interfaces via a new
rtnetlink kind "wwan". The responsible driver has to use
the new wwan_register_ops() to make this possible.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/wwan/wwan_core.c | 219 ++++++++++++++++++++++++++++++++++-
 include/linux/wwan.h         |  36 ++++++
 include/uapi/linux/wwan.h    |  17 +++
 3 files changed, 267 insertions(+), 5 deletions(-)
 create mode 100644 include/uapi/linux/wwan.h

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index cff04e532c1e..b1ad78f386bc 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -13,6 +13,8 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/wwan.h>
+#include <net/rtnetlink.h>
+#include <uapi/linux/wwan.h>
 
 #define WWAN_MAX_MINORS 256 /* 256 minors allowed with register_chrdev() */
 
@@ -524,24 +526,231 @@ static const struct file_operations wwan_port_fops = {
 	.llseek = noop_llseek,
 };
 
+struct wwan_dev_reg {
+	struct list_head list;
+	struct device *dev;
+	const struct wwan_ops *ops;
+	void *ctxt;
+};
+
+static DEFINE_MUTEX(wwan_mtx);
+static LIST_HEAD(wwan_devs);
+
+int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
+		      void *ctxt)
+{
+	struct wwan_dev_reg *reg;
+	int ret;
+
+	if (WARN_ON(!parent || !ops))
+		return -EINVAL;
+
+	mutex_lock(&wwan_mtx);
+	list_for_each_entry(reg, &wwan_devs, list) {
+		if (WARN_ON(reg->dev == parent)) {
+			ret = -EBUSY;
+			goto out;
+		}
+	}
+
+	reg = kzalloc(sizeof(*reg), GFP_KERNEL);
+	if (!reg) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	reg->dev = parent;
+	reg->ops = ops;
+	reg->ctxt = ctxt;
+	list_add_tail(&reg->list, &wwan_devs);
+
+	ret = 0;
+
+out:
+	mutex_unlock(&wwan_mtx);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(wwan_register_ops);
+
+void wwan_unregister_ops(struct device *parent)
+{
+	struct wwan_dev_reg *tmp;
+
+	mutex_lock(&wwan_mtx);
+	list_for_each_entry(tmp, &wwan_devs, list) {
+		if (tmp->dev == parent) {
+			list_del(&tmp->list);
+			break;
+		}
+	}
+	mutex_unlock(&wwan_mtx);
+}
+EXPORT_SYMBOL_GPL(wwan_unregister_ops);
+
+static struct wwan_dev_reg *wwan_find_by_name(const char *name)
+{
+	struct wwan_dev_reg *tmp;
+
+	lockdep_assert_held(&wwan_mtx);
+
+	list_for_each_entry(tmp, &wwan_devs, list) {
+		if (strcmp(name, dev_name(tmp->dev)) == 0)
+			return tmp;
+	}
+
+	return NULL;
+}
+
+static struct wwan_dev_reg *wwan_find_by_dev(struct device *dev)
+{
+	struct wwan_dev_reg *tmp;
+
+	lockdep_assert_held(&wwan_mtx);
+
+	list_for_each_entry(tmp, &wwan_devs, list) {
+		if (tmp->dev == dev)
+			return tmp;
+	}
+
+	return NULL;
+}
+
+static int wwan_rtnl_validate(struct nlattr *tb[], struct nlattr *data[],
+			      struct netlink_ext_ack *extack)
+{
+	if (!data)
+		return -EINVAL;
+
+	if (!data[IFLA_WWAN_LINK_ID] || !data[IFLA_WWAN_DEV_NAME])
+		return -EINVAL;
+
+	return 0;
+}
+
+static struct device_type wwan_type = { .name = "wwan" };
+
+static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
+					  struct nlattr *data[],
+					  const char *ifname,
+					  unsigned char name_assign_type,
+					  unsigned int num_tx_queues,
+					  unsigned int num_rx_queues)
+{
+	const char *devname = nla_data(data[IFLA_WWAN_DEV_NAME]);
+	struct wwan_dev_reg *reg;
+	struct net_device *dev;
+
+	mutex_lock(&wwan_mtx);
+	reg = wwan_find_by_name(devname);
+	if (!reg) {
+		mutex_unlock(&wwan_mtx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	dev = alloc_netdev_mqs(reg->ops->priv_size, ifname, name_assign_type,
+			       reg->ops->setup, num_tx_queues, num_rx_queues);
+
+	mutex_unlock(&wwan_mtx);
+
+	if (dev) {
+		SET_NETDEV_DEV(dev, reg->dev);
+		SET_NETDEV_DEVTYPE(dev, &wwan_type);
+	}
+
+	return dev;
+}
+
+static int wwan_rtnl_newlink(struct net *src_net, struct net_device *dev,
+			     struct nlattr *tb[], struct nlattr *data[],
+			     struct netlink_ext_ack *extack)
+{
+	struct wwan_dev_reg *reg;
+	u32 link_id = nla_get_u32(data[IFLA_WWAN_LINK_ID]);
+	int ret;
+
+	mutex_lock(&wwan_mtx);
+	reg = wwan_find_by_dev(dev->dev.parent);
+	if (!reg) {
+		mutex_unlock(&wwan_mtx);
+		return -EINVAL;
+	}
+
+	if (reg->ops->newlink)
+		ret = reg->ops->newlink(reg->ctxt, dev, link_id, extack);
+	else
+		ret = register_netdevice(dev);
+
+	mutex_unlock(&wwan_mtx);
+
+	return ret;
+}
+
+static void wwan_rtnl_dellink(struct net_device *dev, struct list_head *head)
+{
+	struct wwan_dev_reg *reg;
+
+	mutex_lock(&wwan_mtx);
+	reg = wwan_find_by_dev(dev->dev.parent);
+	if (!reg) {
+		mutex_unlock(&wwan_mtx);
+		return;
+	}
+
+	if (reg->ops->dellink)
+		reg->ops->dellink(reg->ctxt, dev, head);
+	else
+		unregister_netdevice(dev);
+
+	mutex_unlock(&wwan_mtx);
+}
+
+static const struct nla_policy wwan_rtnl_policy[IFLA_WWAN_MAX + 1] = {
+	[IFLA_WWAN_DEV_NAME] = { .type = NLA_NUL_STRING },
+	[IFLA_WWAN_LINK_ID] = { .type = NLA_U32 },
+};
+
+static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
+	.kind = "wwan",
+	.maxtype = __IFLA_WWAN_MAX,
+	.alloc = wwan_rtnl_alloc,
+	.validate = wwan_rtnl_validate,
+	.newlink = wwan_rtnl_newlink,
+	.dellink = wwan_rtnl_dellink,
+	.policy = wwan_rtnl_policy,
+};
+
 static int __init wwan_init(void)
 {
+	int err;
+
+	err = rtnl_link_register(&wwan_rtnl_link_ops);
+	if (err)
+		return err;
+
 	wwan_class = class_create(THIS_MODULE, "wwan");
-	if (IS_ERR(wwan_class))
-		return PTR_ERR(wwan_class);
+	if (IS_ERR(wwan_class)) {
+		err = PTR_ERR(wwan_class);
+		goto unregister;
+	}
 
 	/* chrdev used for wwan ports */
 	wwan_major = register_chrdev(0, "wwan_port", &wwan_port_fops);
 	if (wwan_major < 0) {
-		class_destroy(wwan_class);
-		return wwan_major;
+		err = wwan_major;
+		goto destroy;
 	}
 
-	return 0;
+	err = 0;
+destroy:
+	class_destroy(wwan_class);
+unregister:
+	rtnl_link_unregister(&wwan_rtnl_link_ops);
+	return err;
 }
 
 static void __exit wwan_exit(void)
 {
+	rtnl_link_unregister(&wwan_rtnl_link_ops);
 	unregister_chrdev(wwan_major, "wwan_port");
 	class_destroy(wwan_class);
 }
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index aa05a253dcf9..d07301962ff7 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -7,6 +7,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
+#include <linux/netlink.h>
 
 /**
  * enum wwan_port_type - WWAN port types
@@ -108,4 +109,39 @@ void wwan_port_txon(struct wwan_port *port);
  */
 void *wwan_port_get_drvdata(struct wwan_port *port);
 
+/**
+ * struct wwan_ops - WWAN device 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 {
+	unsigned int priv_size;
+	void (*setup)(struct net_device *dev);
+	int (*newlink)(void *ctxt, struct net_device *dev,
+		       u32 if_id, struct netlink_ext_ack *extack);
+	void (*dellink)(void *ctxt, struct net_device *dev,
+			struct list_head *head);
+};
+
+/**
+ * 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);
+
+/**
+ * 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);
+
 #endif /* __WWAN_H */
diff --git a/include/uapi/linux/wwan.h b/include/uapi/linux/wwan.h
new file mode 100644
index 000000000000..a134c823cfbd
--- /dev/null
+++ b/include/uapi/linux/wwan.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+/*
+ * Copyright (C) 2021 Intel Corporation.
+ */
+#ifndef _UAPI_WWAN_H_
+#define _UAPI_WWAN_H_
+
+enum {
+	IFLA_WWAN_UNSPEC,
+	IFLA_WWAN_DEV_NAME, /* nul-string */
+	IFLA_WWAN_LINK_ID, /* u32 */
+
+	__IFLA_WWAN_MAX
+};
+#define IFLA_WWAN_MAX (__IFLA_WWAN_MAX - 1)
+
+#endif /* _UAPI_WWAN_H_ */
-- 
2.31.1

[RFC 4/4] iosm: convert to generic wwan ops

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/wwan/iosm/iosm_ipc_imem_ops.h |   1 -
 drivers/net/wwan/iosm/iosm_ipc_pcie.c     |   7 -
 drivers/net/wwan/iosm/iosm_ipc_pcie.h     |   2 -
 drivers/net/wwan/iosm/iosm_ipc_wwan.c     | 310 +++++++---------------
 include/uapi/linux/if_link.h              |  10 -
 tools/include/uapi/linux/if_link.h        |  10 -
 6 files changed, 103 insertions(+), 237 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
index 6677a82be77b..84087cf33329 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h
@@ -29,7 +29,6 @@
 /* IP MUX channel range */
 #define IP_MUX_SESSION_START 1
 #define IP_MUX_SESSION_END 8
-#define MAX_IP_MUX_SESSION IP_MUX_SESSION_END
 
 /**
  * ipc_imem_sys_port_open - Open a port link to CP.
diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index 0c26047ebc1c..ac6baddfde61 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
@@ -567,18 +567,11 @@ static int __init iosm_ipc_driver_init(void)
 		return -1;
 	}
 
-	if (rtnl_link_register(&iosm_netlink)) {
-		pr_err("IOSM RTNL register failed");
-		pci_unregister_driver(&iosm_ipc_driver);
-		return -1;
-	}
-
 	return 0;
 }
 
 static void __exit iosm_ipc_driver_exit(void)
 {
-	rtnl_link_unregister(&iosm_netlink);
 	pci_unregister_driver(&iosm_ipc_driver);
 }
 
diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.h b/drivers/net/wwan/iosm/iosm_ipc_pcie.h
index 839809fee3dd..7d1f0cd7364c 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.h
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.h
@@ -12,8 +12,6 @@
 
 #include "iosm_ipc_irq.h"
 
-extern struct rtnl_link_ops iosm_netlink;
-
 /* Device ID */
 #define INTEL_CP_DEVICE_7560_ID 0x7560
 
diff --git a/drivers/net/wwan/iosm/iosm_ipc_wwan.c b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
index 719c88d9b2e9..daf3d36edc4e 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_wwan.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_wwan.c
@@ -6,8 +6,7 @@
 #include <linux/etherdevice.h>
 #include <linux/if_arp.h>
 #include <linux/if_link.h>
-#include <net/rtnetlink.h>
-
+#include <linux/wwan.h>
 #include "iosm_ipc_chnl_cfg.h"
 #include "iosm_ipc_imem_ops.h"
 #include "iosm_ipc_wwan.h"
@@ -18,65 +17,52 @@
 
 #define IOSM_IF_ID_PAYLOAD 2
 
-static struct device_type wwan_type = { .name = "wwan" };
-
-static const struct nla_policy ipc_wwan_policy[IFLA_IOSM_MAX + 1] = {
-	[IFLA_IOSM_IF_ID]	= { .type = NLA_U16 },
-};
-
 /**
- * struct iosm_net_link - This structure includes information about interface
- *			  dev.
+ * struct iosm_netdev_priv - netdev private data
  * @if_id:	Interface id for device.
  * @ch_id:	IPC channel number for which interface device is created.
- * @netdev:	Pointer to network interface device structure
  * @ipc_wwan:	Pointer to iosm_wwan struct
  */
-
-struct iosm_net_link {
+struct iosm_netdev_priv {
+	struct iosm_wwan *ipc_wwan;
+	struct net_device *netdev;
 	int if_id;
 	int ch_id;
-	struct net_device *netdev;
-	struct iosm_wwan *ipc_wwan;
 };
 
 /**
  * struct iosm_wwan - This structure contains information about WWAN root device
- *		     and interface to the IPC layer.
- * @netdev:		Pointer to network interface device structure.
- * @sub_netlist:	List of netlink interfaces
+ *		      and interface to the IPC layer.
  * @ipc_imem:		Pointer to imem data-struct
+ * @sub_netlist:	List of active netdevs
  * @dev:		Pointer device structure
  * @if_mutex:		Mutex used for add and remove interface id
- * @is_registered:	Registration status with netdev
  */
 struct iosm_wwan {
-	struct net_device *netdev;
-	struct iosm_net_link __rcu *sub_netlist[MAX_IP_MUX_SESSION];
 	struct iosm_imem *ipc_imem;
+	struct iosm_netdev_priv __rcu *sub_netlist[IP_MUX_SESSION_END + 1];
 	struct device *dev;
 	struct mutex if_mutex; /* Mutex used for add and remove interface id */
-	u8 is_registered:1;
 };
 
 /* Bring-up the wwan net link */
 static int ipc_wwan_link_open(struct net_device *netdev)
 {
-	struct iosm_net_link *netlink = netdev_priv(netdev);
-	struct iosm_wwan *ipc_wwan = netlink->ipc_wwan;
-	int if_id = netlink->if_id;
-	int ret = -EINVAL;
+	struct iosm_netdev_priv *priv = netdev_priv(netdev);
+	struct iosm_wwan *ipc_wwan = priv->ipc_wwan;
+	int if_id = priv->if_id;
+	int ret;
 
-	if (if_id < IP_MUX_SESSION_START || if_id > IP_MUX_SESSION_END)
-		return ret;
+	if (if_id < IP_MUX_SESSION_START ||
+	    if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist))
+		return -EINVAL;
 
 	mutex_lock(&ipc_wwan->if_mutex);
 
 	/* get channel id */
-	netlink->ch_id =
-		ipc_imem_sys_wwan_open(ipc_wwan->ipc_imem, if_id);
+	priv->ch_id = ipc_imem_sys_wwan_open(ipc_wwan->ipc_imem, if_id);
 
-	if (netlink->ch_id < 0) {
+	if (priv->ch_id < 0) {
 		dev_err(ipc_wwan->dev,
 			"cannot connect wwan0 & id %d to the IPC mem layer",
 			if_id);
@@ -88,7 +74,7 @@ static int ipc_wwan_link_open(struct net_device *netdev)
 	netif_start_queue(netdev);
 
 	dev_dbg(ipc_wwan->dev, "Channel id %d allocated to if_id %d",
-		netlink->ch_id, netlink->if_id);
+		priv->ch_id, priv->if_id);
 
 	ret = 0;
 out:
@@ -99,14 +85,15 @@ 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_net_link *netlink = netdev_priv(netdev);
+	struct iosm_netdev_priv *priv = netdev_priv(netdev);
 
 	netif_stop_queue(netdev);
 
-	mutex_lock(&netlink->ipc_wwan->if_mutex);
-	ipc_imem_sys_wwan_close(netlink->ipc_wwan->ipc_imem, netlink->if_id,
-				netlink->ch_id);
-	mutex_unlock(&netlink->ipc_wwan->if_mutex);
+	mutex_lock(&priv->ipc_wwan->if_mutex);
+	ipc_imem_sys_wwan_close(priv->ipc_wwan->ipc_imem, priv->if_id,
+				priv->ch_id);
+	priv->ch_id = -1;
+	mutex_unlock(&priv->ipc_wwan->if_mutex);
 
 	return 0;
 }
@@ -115,20 +102,21 @@ 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_net_link *netlink = netdev_priv(netdev);
-	struct iosm_wwan *ipc_wwan = netlink->ipc_wwan;
-	int if_id = netlink->if_id;
-	int ret = -EINVAL;
+	struct iosm_netdev_priv *priv = netdev_priv(netdev);
+	struct iosm_wwan *ipc_wwan = priv->ipc_wwan;
+	int if_id = priv->if_id;
+	int ret;
 
 	/* Interface IDs from 1 to 8 are for IP data
 	 * & from 257 to 261 are for non-IP data
 	 */
-	if (if_id < IP_MUX_SESSION_START || if_id > IP_MUX_SESSION_END)
-		goto exit;
+	if (if_id < IP_MUX_SESSION_START ||
+	    if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist))
+		return -EINVAL;
 
 	/* Send the SKB to device for transmission */
 	ret = ipc_imem_sys_wwan_transmit(ipc_wwan->ipc_imem,
-					 if_id, netlink->ch_id, skb);
+					 if_id, priv->ch_id, skb);
 
 	/* Return code of zero is success */
 	if (ret == 0) {
@@ -175,137 +163,72 @@ static void ipc_wwan_setup(struct net_device *iosm_dev)
 	iosm_dev->netdev_ops = &ipc_inm_ops;
 }
 
-static struct device_type inm_type = { .name = "iosmdev" };
-
 /* Create new wwan net link */
-static int ipc_wwan_newlink(struct net *src_net, struct net_device *dev,
-			    struct nlattr *tb[], struct nlattr *data[],
-			    struct netlink_ext_ack *extack)
+static int ipc_wwan_newlink(void *ctxt, struct net_device *dev,
+			    u32 if_id, struct netlink_ext_ack *extack)
 {
-	struct iosm_net_link *netlink = netdev_priv(dev);
-	struct iosm_wwan *ipc_wwan;
-	struct net_device *netdev;
-	int err = -EINVAL;
-	int index;
-
-	if (!data[IFLA_IOSM_IF_ID]) {
-		pr_err("Interface ID not specified");
-		goto out;
-	}
-
-	if (!tb[IFLA_LINK]) {
-		pr_err("Link not specified");
-		goto out;
-	}
-
-	netlink->netdev = dev;
-
-	netdev = __dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
-
-	netlink->ipc_wwan = netdev_priv(netdev);
-
-	ipc_wwan = netlink->ipc_wwan;
+	struct iosm_wwan *ipc_wwan = ctxt;
+	struct iosm_netdev_priv *priv;
+	int err;
 
-	if (ipc_wwan->netdev != netdev)
-		goto out;
-
-	netlink->if_id = nla_get_u16(data[IFLA_IOSM_IF_ID]);
-	index = netlink->if_id;
-
-	/* Complete all memory stores before this point */
-	smp_mb();
-	if (index < IP_MUX_SESSION_START || index > IP_MUX_SESSION_END)
-		goto out;
+	if (if_id < IP_MUX_SESSION_START ||
+	    if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist))
+		return -EINVAL;
 
-	rcu_read_lock();
+	priv = netdev_priv(dev);
+	priv->if_id = if_id;
 
-	if (rcu_access_pointer(ipc_wwan->sub_netlist[index - 1])) {
-		pr_err("IOSM interface ID already in use");
-		goto out_free_lock;
+	mutex_lock(&ipc_wwan->if_mutex);
+	if (rcu_access_pointer(ipc_wwan->sub_netlist[if_id])) {
+		err = -EBUSY;
+		goto out_unlock;
 	}
 
-	SET_NETDEV_DEVTYPE(dev, &inm_type);
-
-	eth_hw_addr_random(dev);
 	err = register_netdevice(dev);
-	if (err) {
-		dev_err(ipc_wwan->dev, "register netlink failed.\n");
-		goto out_free_lock;
-	}
-
-	err = netdev_upper_dev_link(ipc_wwan->netdev, dev, extack);
+	if (err)
+		goto out_unlock;
 
-	if (err) {
-		dev_err(ipc_wwan->dev, "netdev linking with parent failed.\n");
-		goto netlink_err;
-	}
+	rcu_assign_pointer(ipc_wwan->sub_netlist[if_id], priv);
+	mutex_unlock(&ipc_wwan->if_mutex);
 
-	rcu_assign_pointer(ipc_wwan->sub_netlist[index - 1], netlink);
 	netif_device_attach(dev);
-	rcu_read_unlock();
 
 	return 0;
 
-netlink_err:
-	unregister_netdevice(dev);
-out_free_lock:
-	rcu_read_unlock();
-out:
+out_unlock:
+	mutex_unlock(&ipc_wwan->if_mutex);
 	return err;
 }
 
-/* Delete new wwan net link */
-static void ipc_wwan_dellink(struct net_device *dev, struct list_head *head)
+static void ipc_wwan_dellink(void *ctxt, struct net_device *dev,
+			     struct list_head *head)
 {
-	struct iosm_net_link *netlink = netdev_priv(dev);
-	u16 index = netlink->if_id;
-
-	netdev_upper_dev_unlink(netlink->ipc_wwan->netdev, dev);
-	unregister_netdevice(dev);
+	struct iosm_wwan *ipc_wwan = ctxt;
+	struct iosm_netdev_priv *priv = netdev_priv(dev);
+	int if_id = priv->if_id;
 
-	mutex_lock(&netlink->ipc_wwan->if_mutex);
-	rcu_assign_pointer(netlink->ipc_wwan->sub_netlist[index - 1], NULL);
-	mutex_unlock(&netlink->ipc_wwan->if_mutex);
-}
+	if (WARN_ON(if_id < IP_MUX_SESSION_START ||
+		    if_id >= ARRAY_SIZE(ipc_wwan->sub_netlist)))
+		return;
 
-/* Get size for iosm net link payload*/
-static size_t ipc_wwan_get_size(const struct net_device *dev)
-{
-	return nla_total_size(IOSM_IF_ID_PAYLOAD);
-}
-
-/* Validate the input parameters for wwan net link */
-static int ipc_wwan_validate(struct nlattr *tb[], struct nlattr *data[],
-			     struct netlink_ext_ack *extack)
-{
-	u16 if_id;
-
-	if (!data || !data[IFLA_IOSM_IF_ID]) {
-		NL_SET_ERR_MSG_MOD(extack, "IF ID not specified");
-		return -EINVAL;
-	}
+	mutex_lock(&ipc_wwan->if_mutex);
 
-	if_id = nla_get_u16(data[IFLA_IOSM_IF_ID]);
+	if (WARN_ON(rcu_access_pointer(ipc_wwan->sub_netlist[if_id]) != priv))
+		goto unlock;
 
-	if (if_id < IP_MUX_SESSION_START || if_id > IP_MUX_SESSION_END) {
-		NL_SET_ERR_MSG_MOD(extack, "Invalid Interface");
-		return -ERANGE;
-	}
+	RCU_INIT_POINTER(ipc_wwan->sub_netlist[if_id], NULL);
+	/* unregistering includes synchronize_net() */
+	unregister_netdevice(dev);
 
-	return 0;
+unlock:
+	mutex_unlock(&ipc_wwan->if_mutex);
 }
 
-/* RT Net link ops structure for new wwan net link */
-struct rtnl_link_ops iosm_netlink __read_mostly = {
-	.kind           = "iosm",
-	.maxtype        = __IFLA_IOSM_MAX,
-	.priv_size      = sizeof(struct iosm_net_link),
-	.setup          = ipc_wwan_setup,
-	.validate       = ipc_wwan_validate,
-	.newlink        = ipc_wwan_newlink,
-	.dellink        = ipc_wwan_dellink,
-	.get_size       = ipc_wwan_get_size,
-	.policy         = ipc_wwan_policy,
+static const struct wwan_ops iosm_wwan_ops = {
+	.priv_size = sizeof(struct iosm_netdev_priv),
+	.setup = ipc_wwan_setup,
+	.newlink = ipc_wwan_newlink,
+	.dellink = ipc_wwan_dellink,
 };
 
 int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
@@ -313,7 +236,7 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
 {
 	struct sk_buff *skb = skb_arg;
 	struct net_device_stats *stats;
-	struct iosm_net_link *priv;
+	struct iosm_netdev_priv *priv;
 	int ret;
 
 	if ((skb->data[0] & IOSM_IP_TYPE_MASK) == IOSM_IP_TYPE_IPV4)
@@ -353,10 +276,17 @@ int ipc_wwan_receive(struct iosm_wwan *ipc_wwan, struct sk_buff *skb_arg,
 void ipc_wwan_tx_flowctrl(struct iosm_wwan *ipc_wwan, int if_id, bool on)
 {
 	struct net_device *netdev;
+	struct iosm_netdev_priv *priv;
 	bool is_tx_blk;
 
 	rcu_read_lock();
-	netdev = rcu_dereference(ipc_wwan->sub_netlist[if_id])->netdev;
+	priv = rcu_dereference(ipc_wwan->sub_netlist[if_id]);
+	if (!priv) {
+		rcu_read_unlock();
+		return;
+	}
+
+	netdev = priv->netdev;
 
 	is_tx_blk = netif_queue_stopped(netdev);
 
@@ -371,78 +301,44 @@ void ipc_wwan_tx_flowctrl(struct iosm_wwan *ipc_wwan, int if_id, bool on)
 	rcu_read_unlock();
 }
 
-static void ipc_netdev_setup(struct net_device *dev) {}
-
 struct iosm_wwan *ipc_wwan_init(struct iosm_imem *ipc_imem, struct device *dev)
 {
-	static const struct net_device_ops iosm_wwandev_ops = {};
 	struct iosm_wwan *ipc_wwan;
-	struct net_device *netdev;
 
-	netdev = alloc_netdev(sizeof(*ipc_wwan), "wwan%d", NET_NAME_ENUM,
-			      ipc_netdev_setup);
-
-	if (!netdev)
+	ipc_wwan = kzalloc(sizeof(*ipc_wwan), GFP_KERNEL);
+	if (!ipc_wwan)
 		return NULL;
 
-	ipc_wwan = netdev_priv(netdev);
-
 	ipc_wwan->dev = dev;
-	ipc_wwan->netdev = netdev;
-	ipc_wwan->is_registered = false;
-
 	ipc_wwan->ipc_imem = ipc_imem;
 
-	mutex_init(&ipc_wwan->if_mutex);
-
-	/* allocate random ethernet address */
-	eth_random_addr(netdev->dev_addr);
-	netdev->addr_assign_type = NET_ADDR_RANDOM;
-
-	netdev->netdev_ops = &iosm_wwandev_ops;
-	netdev->flags |= IFF_NOARP;
-
-	SET_NETDEV_DEVTYPE(netdev, &wwan_type);
-
-	if (register_netdev(netdev)) {
-		dev_err(ipc_wwan->dev, "register_netdev failed");
-		goto reg_fail;
+	if (wwan_register_ops(ipc_wwan->dev, &iosm_wwan_ops, ipc_wwan)) {
+		kfree(ipc_wwan);
+		return NULL;
 	}
 
-	ipc_wwan->is_registered = true;
-
-	netif_device_attach(netdev);
+	mutex_init(&ipc_wwan->if_mutex);
 
 	return ipc_wwan;
-
-reg_fail:
-	free_netdev(netdev);
-	return NULL;
 }
 
 void ipc_wwan_deinit(struct iosm_wwan *ipc_wwan)
 {
-	struct iosm_net_link *netlink;
-	int i;
-
-	if (ipc_wwan->is_registered) {
-		rcu_read_lock();
-		for (i = IP_MUX_SESSION_START; i <= IP_MUX_SESSION_END; i++) {
-			if (rcu_access_pointer(ipc_wwan->sub_netlist[i - 1])) {
-				netlink =
-				rcu_dereference(ipc_wwan->sub_netlist[i - 1]);
-				rtnl_lock();
-				netdev_upper_dev_unlink(ipc_wwan->netdev,
-							netlink->netdev);
-				unregister_netdevice(netlink->netdev);
-				rtnl_unlock();
-				rcu_assign_pointer(ipc_wwan->sub_netlist[i - 1],
-						   NULL);
-			}
-		}
-		rcu_read_unlock();
+	int if_id;
 
-		unregister_netdev(ipc_wwan->netdev);
-		free_netdev(ipc_wwan->netdev);
+	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;
+
+		ipc_wwan_dellink(ipc_wwan, priv->netdev, NULL);
 	}
+
+	mutex_destroy(&ipc_wwan->if_mutex);
+
+	kfree(ipc_wwan);
 }
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index f7c3beebb074..cd5b382a4138 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1251,14 +1251,4 @@ struct ifla_rmnet_flags {
 	__u32	mask;
 };
 
-/* IOSM Section */
-
-enum {
-	IFLA_IOSM_UNSPEC,
-	IFLA_IOSM_IF_ID,
-	__IFLA_IOSM_MAX,
-};
-
-#define IFLA_IOSM_MAX  (__IFLA_IOSM_MAX - 1)
-
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
index cb496d0de39e..d208b2af697f 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -1046,14 +1046,4 @@ struct ifla_rmnet_flags {
 	__u32	mask;
 };
 
-/* IOSM Section */
-
-enum {
-	IFLA_IOSM_UNSPEC,
-	IFLA_IOSM_IF_ID,
-	__IFLA_IOSM_MAX,
-};
-
-#define IFLA_IOSM_MAX  (__IFLA_IOSM_MAX - 1)
-
 #endif /* _UAPI_LINUX_IF_LINK_H */
-- 
2.31.1

Re: [RFC 3/4] wwan: add interface creation support

[Loic Poulain]

Hi Johannes,

On Tue, 1 Jun 2021 at 10:05, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> Add support to create (and destroy) interfaces via a new
> rtnetlink kind "wwan". The responsible driver has to use
> the new wwan_register_ops() to make this possible.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  drivers/net/wwan/wwan_core.c | 219 ++++++++++++++++++++++++++++++++++-
>  include/linux/wwan.h         |  36 ++++++
>  include/uapi/linux/wwan.h    |  17 +++
>  3 files changed, 267 insertions(+), 5 deletions(-)
>  create mode 100644 include/uapi/linux/wwan.h
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index cff04e532c1e..b1ad78f386bc 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -13,6 +13,8 @@
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/wwan.h>
> +#include <net/rtnetlink.h>
> +#include <uapi/linux/wwan.h>
>
>  #define WWAN_MAX_MINORS 256 /* 256 minors allowed with register_chrdev() */
>
> @@ -524,24 +526,231 @@ static const struct file_operations wwan_port_fops = {
>         .llseek = noop_llseek,
>  };
>
> +struct wwan_dev_reg {
> +       struct list_head list;
> +       struct device *dev;
> +       const struct wwan_ops *ops;
> +       void *ctxt;
> +};
> +
> +static DEFINE_MUTEX(wwan_mtx);
> +static LIST_HEAD(wwan_devs);
> +
> +int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
> +                     void *ctxt)
> +{
> +       struct wwan_dev_reg *reg;
> +       int ret;
> +
> +       if (WARN_ON(!parent || !ops))
> +               return -EINVAL;
> +
> +       mutex_lock(&wwan_mtx);
> +       list_for_each_entry(reg, &wwan_devs, list) {
> +               if (WARN_ON(reg->dev == parent)) {
> +                       ret = -EBUSY;
> +                       goto out;
> +               }
> +       }

Thanks for this, overall it looks good to me, but just checking why
you're not using the wwan_dev internally to create-or-pick wwan_dev
(wwan_dev_create) and register ops to it, instead of having a global
new wwan_devs list.

> +
> +       reg = kzalloc(sizeof(*reg), GFP_KERNEL);
> +       if (!reg) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       reg->dev = parent;
> +       reg->ops = ops;
> +       reg->ctxt = ctxt;
> +       list_add_tail(&reg->list, &wwan_devs);
> +
> +       ret = 0;
> +
> +out:
> +       mutex_unlock(&wwan_mtx);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(wwan_register_ops);

Regards,
Loic

Re: [RFC 3/4] wwan: add interface creation support

[Johannes Berg]

Hi,

> > +int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
> > +                     void *ctxt)
> > +{
> > +       struct wwan_dev_reg *reg;
> > +       int ret;
> > +
> > +       if (WARN_ON(!parent || !ops))
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&wwan_mtx);
> > +       list_for_each_entry(reg, &wwan_devs, list) {
> > +               if (WARN_ON(reg->dev == parent)) {
> > +                       ret = -EBUSY;
> > +                       goto out;
> > +               }
> > +       }
> 
> Thanks for this, overall it looks good to me, but just checking why
> you're not using the wwan_dev internally to create-or-pick wwan_dev
> (wwan_dev_create) and register ops to it, instead of having a global
> new wwan_devs list.

Uh, no good reason. I just missed that all that infrastructure is
already there, oops.

johannes

Re: [RFC 3/4] wwan: add interface creation support

[Sergey Ryazanov]

Hello Johannes and Loic,

On Tue, Jun 1, 2021 at 11:07 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> Add support to create (and destroy) interfaces via a new
> rtnetlink kind "wwan". The responsible driver has to use
> the new wwan_register_ops() to make this possible.

Wow, this is a perfect solution! I just could not help but praise this
proposal :)

When researching the latest WWAN device drivers and related
discussions, I began to assume that implementing the netdev management
API without the dummy (no-op) netdev is only possible using genetlink.
But this usage of a regular device specified by its name as a parent
for netdev creation is so natural and clear that I believe in RTNL
again.

Let me place my 2 cents. Maybe the parent device attribute should be
made generic? E.g. call it IFLA_PARENT_DEV_NAME, with usage semantics
similar to the IFLA_LINK attribute for VLAN interfaces. The case when
a user needs to create a netdev on behalf of a regular device is not
WWAN specific, IMHO. So, other drivers could benefit from such
attribute too. To be honest, I can not recall any driver that could
immediately start using such attribute, but the situation with the
need for such attribute seems to be quite common.

-- 
Sergey

Re: [RFC 3/4] wwan: add interface creation support

[Loic Poulain]

On Tue, 1 Jun 2021 at 12:35, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi,
>
> > > +int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
> > > +                     void *ctxt)
> > > +{
> > > +       struct wwan_dev_reg *reg;
> > > +       int ret;
> > > +
> > > +       if (WARN_ON(!parent || !ops))
> > > +               return -EINVAL;
> > > +
> > > +       mutex_lock(&wwan_mtx);
> > > +       list_for_each_entry(reg, &wwan_devs, list) {
> > > +               if (WARN_ON(reg->dev == parent)) {
> > > +                       ret = -EBUSY;
> > > +                       goto out;
> > > +               }
> > > +       }
> >
> > Thanks for this, overall it looks good to me, but just checking why
> > you're not using the wwan_dev internally to create-or-pick wwan_dev
> > (wwan_dev_create) and register ops to it, instead of having a global
> > new wwan_devs list.
>
> Uh, no good reason. I just missed that all that infrastructure is
> already there, oops.

OK no prob ;-), are you going to resubmit something or do you want I
take care of this?

Regards,
Loic

Re: [RFC 3/4] wwan: add interface creation support

[Johannes Berg]

Hi Sergey,

> Wow, this is a perfect solution! I just could not help but praise this
> proposal :)

Heh.

> When researching the latest WWAN device drivers and related
> discussions, I began to assume that implementing the netdev management
> API without the dummy (no-op) netdev is only possible using genetlink.
> But this usage of a regular device specified by its name as a parent
> for netdev creation is so natural and clear that I believe in RTNL
> again.
> 
> Let me place my 2 cents. Maybe the parent device attribute should be
> made generic? E.g. call it IFLA_PARENT_DEV_NAME, with usage semantics
> similar to the IFLA_LINK attribute for VLAN interfaces. The case when
> a user needs to create a netdev on behalf of a regular device is not
> WWAN specific, IMHO. So, other drivers could benefit from such
> attribute too. To be honest, I can not recall any driver that could
> immediately start using such attribute, but the situation with the
> need for such attribute seems to be quite common.

That's a good question/thought.

I mean, in principle this is trivial, right? Just add a
IFLA_PARENT_DEV_NAME like you say, and use it instead of
IFLA_WWAN_DEV_NAME.

It'd come out of tb[] instead of data[] and in this case would remove
the need to add the additional data[] argument to rtnl_create_link() in
my patch, since it's in tb[] then.

The only thing I'd be worried about is that different implementations
use it for different meanings, but I guess that's not that big a deal?

johannes

Re: [RFC 3/4] wwan: add interface creation support

[Johannes Berg]

On Wed, 2021-06-02 at 08:52 +0200, Loic Poulain wrote:
> 
> OK no prob ;-), are you going to resubmit something or do you want I
> take care of this?

I just respun a v2, but I'm still not able to test any of this (I'm in a
completely different group than Chetan, just have been helping/advising
them, so I don't even have their HW).

So if you want to take over at some point and are able to test it, I'd
much appreciate it.

johannes

Re: [RFC 3/4] wwan: add interface creation support

[Sergey Ryazanov]

Hello Johannes,

On Wed, Jun 2, 2021 at 10:38 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>> Wow, this is a perfect solution! I just could not help but praise this
>> proposal :)
>
> Heh.
>
>> When researching the latest WWAN device drivers and related
>> discussions, I began to assume that implementing the netdev management
>> API without the dummy (no-op) netdev is only possible using genetlink.
>> But this usage of a regular device specified by its name as a parent
>> for netdev creation is so natural and clear that I believe in RTNL
>> again.
>>
>> Let me place my 2 cents. Maybe the parent device attribute should be
>> made generic? E.g. call it IFLA_PARENT_DEV_NAME, with usage semantics
>> similar to the IFLA_LINK attribute for VLAN interfaces. The case when
>> a user needs to create a netdev on behalf of a regular device is not
>> WWAN specific, IMHO. So, other drivers could benefit from such
>> attribute too. To be honest, I can not recall any driver that could
>> immediately start using such attribute, but the situation with the
>> need for such attribute seems to be quite common.
>
> That's a good question/thought.
>
> I mean, in principle this is trivial, right? Just add a
> IFLA_PARENT_DEV_NAME like you say, and use it instead of
> IFLA_WWAN_DEV_NAME.
>
> It'd come out of tb[] instead of data[] and in this case would remove
> the need to add the additional data[] argument to rtnl_create_link() in
> my patch, since it's in tb[] then.

Yep, exactly.

> The only thing I'd be worried about is that different implementations
> use it for different meanings, but I guess that's not that big a deal?

The spectrum of sane use of the IFLA_PARENT_DEV_NAME attribute by
various subsystems and (or) drivers will be quite narrow. It should do
exactly what its name says - identify a parent device.

We can not handle the attribute in the common rtnetlink code since
rtnetlink does not know the HW configuration details. That is why
IFLA_PARENT_DEV_NAME should be handled by the RTNL ->newlink()
callback. But after all the processing, the device that is identified
by the IFLA_PARENT_DEV_NAME attribute should appear in the
netdev->dev.parent field with help of SET_NETDEV_DEV(). Eventually
RTNL will be able to fill IFLA_PARENT_DEV_NAME during the netdevs dump
on its own, taking data from netdev->dev.parent.

I assume that IFLA_PARENT_DEV_NAME could replace the IFLA_LINK
attribute usage in such drivers as MBIM and RMNET. But the best way to
evolve these drivers is to make them WWAN-subsystem-aware using the
WWAN interface configuration API from your proposal, IMHO.

-- 
Sergey

Re: [RFC 3/4] wwan: add interface creation support

[Johannes Berg]

Hi Sergey,

> > The only thing I'd be worried about is that different implementations
> > use it for different meanings, but I guess that's not that big a deal?
> 
> The spectrum of sane use of the IFLA_PARENT_DEV_NAME attribute by
> various subsystems and (or) drivers will be quite narrow. It should do
> exactly what its name says - identify a parent device.

Sure, I was more worried there could be multiple interpretations as to
what "a parent device" is, since userspace does nothing but pass a
string in. But we can say it should be a 'struct device' in the kernel.

> We can not handle the attribute in the common rtnetlink code since
> rtnetlink does not know the HW configuration details. That is why
> IFLA_PARENT_DEV_NAME should be handled by the RTNL ->newlink()
> callback. But after all the processing, the device that is identified
> by the IFLA_PARENT_DEV_NAME attribute should appear in the
> netdev->dev.parent field with help of SET_NETDEV_DEV(). Eventually
> RTNL will be able to fill IFLA_PARENT_DEV_NAME during the netdevs dump
> on its own, taking data from netdev->dev.parent.

I didn't do that second part, but I guess that makes sense.

Want to send a follow-up patch to my other patch? I guess you should've
gotten it, but if not the new series is here:

https://lore.kernel.org/netdev/20210602082840.85828-1-johannes@sipsolutions.net/T/#t

> I assume that IFLA_PARENT_DEV_NAME could replace the IFLA_LINK
> attribute usage in such drivers as MBIM and RMNET. But the best way to
> evolve these drivers is to make them WWAN-subsystem-aware using the
> WWAN interface configuration API from your proposal, IMHO.

Right.

johannes

Re: [RFC 3/4] wwan: add interface creation support

[Sergey Ryazanov]

On Wed, Jun 2, 2021 at 3:56 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>>> The only thing I'd be worried about is that different implementations
>>> use it for different meanings, but I guess that's not that big a deal?
>>
>> The spectrum of sane use of the IFLA_PARENT_DEV_NAME attribute by
>> various subsystems and (or) drivers will be quite narrow. It should do
>> exactly what its name says - identify a parent device.
>
> Sure, I was more worried there could be multiple interpretations as to
> what "a parent device" is, since userspace does nothing but pass a
> string in. But we can say it should be a 'struct device' in the kernel.
>
>> We can not handle the attribute in the common rtnetlink code since
>> rtnetlink does not know the HW configuration details. That is why
>> IFLA_PARENT_DEV_NAME should be handled by the RTNL ->newlink()
>> callback. But after all the processing, the device that is identified
>> by the IFLA_PARENT_DEV_NAME attribute should appear in the
>> netdev->dev.parent field with help of SET_NETDEV_DEV(). Eventually
>> RTNL will be able to fill IFLA_PARENT_DEV_NAME during the netdevs dump
>> on its own, taking data from netdev->dev.parent.
>
> I didn't do that second part, but I guess that makes sense.
>
> Want to send a follow-up patch to my other patch? I guess you should've
> gotten it, but if not the new series is here:
>
> https://lore.kernel.org/netdev/20210602082840.85828-1-johannes@sipsolutions.net/T/#t

Yes, I saw the second version of your RFC and even attempted to
provide a full picture of why this attribute should be generic.

I will send a follow-up series tonight with parent device exporting
support and with some usage examples.

>> I assume that IFLA_PARENT_DEV_NAME could replace the IFLA_LINK
>> attribute usage in such drivers as MBIM and RMNET. But the best way to
>> evolve these drivers is to make them WWAN-subsystem-aware using the
>> WWAN interface configuration API from your proposal, IMHO.
>
> Right.

-- 
Sergey

Re: [RFC 3/4] wwan: add interface creation support

[Loic Poulain]

On Wed, 2 Jun 2021 at 10:29, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2021-06-02 at 08:52 +0200, Loic Poulain wrote:
> >
> > OK no prob ;-), are you going to resubmit something or do you want I
> > take care of this?
>
> I just respun a v2, but I'm still not able to test any of this (I'm in a
> completely different group than Chetan, just have been helping/advising
> them, so I don't even have their HW).
>
> So if you want to take over at some point and are able to test it, I'd
> much appreciate it.

Thanks for this work, yes I can try testing this with mhi_net.

Chetan, would you be able to test that as well? basically with the two
kernel series (Johannes, Sergey) applied on top of your IOSM one + the
iproute2 changes for the userspace side (Sergey), that should work,
but let us know if any issues.

Regards,
Loic

RE: [RFC 3/4] wwan: add interface creation support

[Kumar, M Chetan]

Hi Loic,

> > > OK no prob ;-), are you going to resubmit something or do you want I
> > > take care of this?
> >
> > I just respun a v2, but I'm still not able to test any of this (I'm in
> > a completely different group than Chetan, just have been
> > helping/advising them, so I don't even have their HW).
> >
> > So if you want to take over at some point and are able to test it, I'd
> > much appreciate it.
> 
> Thanks for this work, yes I can try testing this with mhi_net.
> 
> Chetan, would you be able to test that as well? basically with the two kernel
> series (Johannes, Sergey) applied on top of your IOSM one + the
> iproute2 changes for the userspace side (Sergey), that should work, but let us
> know if any issues.

Sure. I will test these changes with the hardware and share my observation.

Regards,
Chetan