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