Message ID | 20210601100320.7d39e9c33a18.I0474861dad426152ac7e7afddfd7fe3ce70870e4@changeid |
---|---|
State | New |
Headers | show |
Series | wwan framework netdev creation | expand |
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
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.
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.
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
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 --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(®->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_ */