[RFC v2 0/5] generic WWAN framework interface creation

[RFC v2 0/5] generic WWAN framework interface creation

[Johannes Berg]

Hi,

Spin two, with a new patch (IFLA_PARENT_DEV_NAME) and reworked
to use the existing struct wwan_device that I missed entirely.

Still includes the IOSM patches for reference, so the IOSM bits
here apply only with this set also applied:
https://lore.kernel.org/r/20210520140158.10132-1-m.chetan.kumar@intel.com

johannes

[RFC v2 1/5] 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 v2 2/5] 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
the tb netlink data.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
v2:
 * remove data[] argument and thus many changes
---
 include/net/rtnetlink.h |  8 ++++++++
 net/core/rtnetlink.c    | 13 +++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 479f60ef54c0..384e800665f2 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,11 @@ struct rtnl_link_ops {
 	const char		*kind;
 
 	size_t			priv_size;
+	struct net_device	*(*alloc)(struct nlattr *tb[],
+					  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;
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 714d5fa38546..4975dd91407d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3177,8 +3177,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, 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);
 
-- 
2.31.1

[RFC v2 3/5] rtnetlink: add IFLA_PARENT_DEV_NAME

[Johannes Berg]

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

In some cases, for example in the upcoming WWAN framework
changes, there's no natural "parent netdev", so sometimes
dummy netdevs are created or similar. IFLA_PARENT_DEV_NAME
is a new attribute intended to contain a device (sysfs,
struct device) name that can be used instead when creating
a new netdev, if the rtnetlink family implements it.

Suggested-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
v2:
 * new patch
---
 include/uapi/linux/if_link.h | 6 ++++++
 net/core/rtnetlink.c         | 1 +
 2 files changed, 7 insertions(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index f7c3beebb074..3455780193a4 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -341,6 +341,12 @@ enum {
 	IFLA_ALT_IFNAME, /* Alternative ifname */
 	IFLA_PERM_ADDRESS,
 	IFLA_PROTO_DOWN_REASON,
+
+	/* device (sysfs) name as parent, used instead
+	 * of IFLA_LINK where there's no parent netdev
+	 */
+	IFLA_PARENT_DEV_NAME,
+
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 4975dd91407d..49a27bf6e4a7 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1878,6 +1878,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_PERM_ADDRESS]	= { .type = NLA_REJECT },
 	[IFLA_PROTO_DOWN_REASON] = { .type = NLA_NESTED },
 	[IFLA_NEW_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
+	[IFLA_PARENT_DEV_NAME]	= { .type = NLA_NUL_STRING },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
-- 
2.31.1

[RFC v2 4/5] 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>
---
v2:
 * use wwan_create_dev() and friends
 * use IFLA_PARENT_DEV_NAME
---
 drivers/net/wwan/wwan_core.c | 220 +++++++++++++++++++++++++++++++++--
 include/linux/wwan.h         |  36 ++++++
 include/uapi/linux/wwan.h    |  16 +++
 3 files changed, 265 insertions(+), 7 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..e2490c73ac33 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() */
 
@@ -34,11 +36,15 @@ static int wwan_major;
  * @id: WWAN device unique ID.
  * @dev: Underlying device.
  * @port_id: Current available port ID to pick.
+ * @ops: wwan device ops
+ * @ops_ctxt: context to pass to ops
  */
 struct wwan_device {
 	unsigned int id;
 	struct device dev;
 	atomic_t port_id;
+	const struct wwan_ops *ops;
+	void *ops_ctxt;
 };
 
 /**
@@ -92,6 +98,23 @@ static struct wwan_device *wwan_dev_get_by_parent(struct device *parent)
 	return to_wwan_dev(dev);
 }
 
+static int wwan_dev_name_match(struct device *dev, const void *name)
+{
+	return dev->type == &wwan_dev_type &&
+	       strcmp(dev_name(dev), name) == 0;
+}
+
+static struct wwan_device *wwan_dev_get_by_name(const char *name)
+{
+	struct device *dev;
+
+	dev = class_find_device(wwan_class, NULL, name, wwan_dev_name_match);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	return to_wwan_dev(dev);
+}
+
 /* This function allocates and registers a new WWAN device OR if a WWAN device
  * already exist for the given parent, it gets a reference and return it.
  * This function is not exported (for now), it is called indirectly via
@@ -156,9 +179,14 @@ static void wwan_remove_dev(struct wwan_device *wwandev)
 	/* WWAN device is created and registered (get+add) along with its first
 	 * child port, and subsequent port registrations only grab a reference
 	 * (get). The WWAN device must then be unregistered (del+put) along with
-	 * its latest port, and reference simply dropped (put) otherwise.
+	 * its last port, and reference simply dropped (put) otherwise. In the
+	 * same fashion, we must not unregister it when the ops are still there.
 	 */
-	ret = device_for_each_child(&wwandev->dev, NULL, is_wwan_child);
+	if (wwandev->ops)
+		ret = 1;
+	else
+		ret = device_for_each_child(&wwandev->dev, NULL, is_wwan_child);
+
 	if (!ret)
 		device_unregister(&wwandev->dev);
 	else
@@ -524,24 +552,202 @@ static const struct file_operations wwan_port_fops = {
 	.llseek = noop_llseek,
 };
 
+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;
+	}
+
+	wwandev->ops = ops;
+	wwandev->ops_ctxt = ctxt;
+
+	return 0;
+}
+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;
+
+	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;
+
+	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)
+{
+	if (!data)
+		return -EINVAL;
+
+	if (!tb[IFLA_PARENT_DEV_NAME])
+		return -EINVAL;
+
+	if (!data[IFLA_WWAN_LINK_ID])
+		return -EINVAL;
+
+	return 0;
+}
+
+static struct device_type wwan_type = { .name = "wwan" };
+
+static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
+					  const char *ifname,
+					  unsigned char name_assign_type,
+					  unsigned int num_tx_queues,
+					  unsigned int num_rx_queues)
+{
+	const char *devname = nla_data(tb[IFLA_PARENT_DEV_NAME]);
+	struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
+	struct net_device *dev;
+
+	if (IS_ERR(wwandev))
+		return ERR_CAST(wwandev);
+
+	/* only supported if ops were registered (not just ports) */
+	if (!wwandev->ops) {
+		dev = ERR_PTR(-EOPNOTSUPP);
+		goto out;
+	}
+
+	dev = alloc_netdev_mqs(wwandev->ops->priv_size, ifname, name_assign_type,
+			       wwandev->ops->setup, num_tx_queues, num_rx_queues);
+
+	if (dev) {
+		SET_NETDEV_DEV(dev, &wwandev->dev);
+		SET_NETDEV_DEVTYPE(dev, &wwan_type);
+	}
+
+out:
+	/* release the reference */
+	put_device(&wwandev->dev);
+	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_device *wwandev = wwan_dev_get_by_parent(dev->dev.parent);
+	u32 link_id = nla_get_u32(data[IFLA_WWAN_LINK_ID]);
+	int ret;
+
+	if (IS_ERR(wwandev))
+		return PTR_ERR(wwandev);
+
+	/* shouldn't have a netdev (left) with us as parent so WARN */
+	if (WARN_ON(!wwandev->ops)) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (wwandev->ops->newlink)
+		ret = wwandev->ops->newlink(wwandev->ops_ctxt, dev,
+					    link_id, extack);
+	else
+		ret = register_netdevice(dev);
+
+out:
+	/* release the reference */
+	put_device(&wwandev->dev);
+	return ret;
+}
+
+static void wwan_rtnl_dellink(struct net_device *dev, struct list_head *head)
+{
+	struct wwan_device *wwandev = wwan_dev_get_by_parent(dev->dev.parent);
+
+	if (IS_ERR(wwandev))
+		return;
+
+	/* shouldn't have a netdev (left) with us as parent so WARN */
+	if (WARN_ON(!wwandev->ops))
+		goto out;
+
+	if (wwandev->ops->dellink)
+		wwandev->ops->dellink(wwandev->ops_ctxt, dev, head);
+	else
+		unregister_netdevice(dev);
+
+out:
+	/* release the reference */
+	put_device(&wwandev->dev);
+}
+
+static const struct nla_policy wwan_rtnl_policy[IFLA_WWAN_MAX + 1] = {
+	[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..32a2720b4d11
--- /dev/null
+++ b/include/uapi/linux/wwan.h
@@ -0,0 +1,16 @@
+/* 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_LINK_ID, /* u32 */
+
+	__IFLA_WWAN_MAX
+};
+#define IFLA_WWAN_MAX (__IFLA_WWAN_MAX - 1)
+
+#endif /* _UAPI_WWAN_H_ */
-- 
2.31.1

[RFC v2 5/5] 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 3455780193a4..0ac1f6ab5673 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1257,14 +1257,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 v2 3/5] rtnetlink: add IFLA_PARENT_DEV_NAME

[Sergey Ryazanov]

On Wed, Jun 2, 2021 at 11:28 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> In some cases, for example in the upcoming WWAN framework
> changes, there's no natural "parent netdev", so sometimes
> dummy netdevs are created or similar. IFLA_PARENT_DEV_NAME
> is a new attribute intended to contain a device (sysfs,
> struct device) name that can be used instead when creating
> a new netdev, if the rtnetlink family implements it.
>
> Suggested-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> v2:
>  * new patch
> ---
>  include/uapi/linux/if_link.h | 6 ++++++
>  net/core/rtnetlink.c         | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index f7c3beebb074..3455780193a4 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -341,6 +341,12 @@ enum {
>         IFLA_ALT_IFNAME, /* Alternative ifname */
>         IFLA_PERM_ADDRESS,
>         IFLA_PROTO_DOWN_REASON,
> +
> +       /* device (sysfs) name as parent, used instead
> +        * of IFLA_LINK where there's no parent netdev
> +        */
> +       IFLA_PARENT_DEV_NAME,
> +
>         __IFLA_MAX
>  };
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 4975dd91407d..49a27bf6e4a7 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1878,6 +1878,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>         [IFLA_PERM_ADDRESS]     = { .type = NLA_REJECT },
>         [IFLA_PROTO_DOWN_REASON] = { .type = NLA_NESTED },
>         [IFLA_NEW_IFINDEX]      = NLA_POLICY_MIN(NLA_S32, 1),
> +       [IFLA_PARENT_DEV_NAME]  = { .type = NLA_NUL_STRING },
>  };
>
>  static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {

I would like to clarify this proposal more broadly. There are some
devices that implement multiple data channels and require several
netdev for traffic separation. The most notable class of such devices
is WWAN modems.

At the moment, a common way to manage these multiple interfaces is to
create a master netdev per each HW instance and reuse IFLA_LINK
attribute to create all subsequent data channel netdevs as
subinterfaces of the master (see MBIM and RMNET drivers). But in fact
all these netdev do not have the master-subinterface relationship.
They are equal. The only reason to implement such a complex scheme is
the absenсe of an attribute that can indicate a parent device. So, in
some sense, driver developers were forced to abuse IFLA_LINK to avoid
a complete per-driver private API creation.

It was Johannes finding that we can greatly simplify the management
API for the WWAN subsystem by specifying a sysfs device name as a
parent for the data channel netdev creation procedure. The first RFC
introduces a private attribute to indicate a parent device. My
suggestion was only to make this attribute generic as it seems to be
generic. The parent device indication attribute should help us avoid
further IFLA_LINK abuse and perhaps even make it easier to rework
existing drivers. Moreover, we have the parent device pointer directly
in the netdev structure, so after applying this patch, we will be able
to export the parent device name using the netdev dump operation and
even filter its results.

ip-link(8) support for the generic parent device attribute will help
us avoid code duplication, so no other link type will require a custom
code to handle the parent name attribute. E.g. the WWAN interface
creation command will looks like this:

# ip link add wwan0-1 parent-dev wwan0 type wwan channel-id 1

So, some future subsystem (or driver) FOO will have an interface
creation command that looks like this:

# ip link add foo1-3 parent-dev foo1 type foo bar-id 3 baz-type Y

As suggested by Johannes, I will soon send a small series with
examples of using the suggested parent device name attribute.

-- 
Sergey