diff mbox series

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

Message ID 20210601100320.7d39e9c33a18.I0474861dad426152ac7e7afddfd7fe3ce70870e4@changeid
State New
Headers show
Series wwan framework netdev creation | expand

Commit Message

Johannes Berg June 1, 2021, 8:05 a.m. UTC
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

Comments

Johannes Berg June 2, 2021, 7:38 a.m. UTC | #1
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
Sergey Ryazanov June 2, 2021, 12:45 p.m. UTC | #2
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 Ryazanov June 2, 2021, 3:38 p.m. UTC | #3
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.
Loic Poulain June 3, 2021, 7 a.m. UTC | #4
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
Kumar, M Chetan June 3, 2021, 7:02 a.m. UTC | #5
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
diff mbox series

Patch

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_ */