Date:   Fri, 11 Jun 2021 12:47:01 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Loic Poulain <loic.poulain@...aro.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "johannes.berg@...el.com" <johannes.berg@...el.com>,
        "leon@...nel.org" <leon@...nel.org>,
        "m.chetan.kumar@...el.com" <m.chetan.kumar@...el.com>,
        Sergey Ryazanov <ryazanov.s.a@...il.com>
Subject: RE: [PATCH net-next v2 3/3] wwan: add interface creation support


> From: Loic Poulain <loic.poulain@...aro.org>
> Sent: Thursday, June 10, 2021 11:15 PM
> 
> From: Johannes Berg <johannes.berg@...el.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.
The responsible driver must be in same series.

> 
> +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;
> +	}
> +
> +	if (!try_module_get(ops->owner)) {
> +		wwan_remove_dev(wwandev);
> +		return -ENODEV;
> +	}
> +
> +	wwandev->ops = ops;
> +	wwandev->ops_ctxt = ctxt;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(wwan_register_ops);
> +
I am not familiar with wwan so cannot comment much on code correctness.
But I know for sure that this series is in complete.
Above wwan_register_ops() and below symbols are exported and never used in this kernel.
This practice is no longer in use to have dead code.
Please remove this dead code or have the user of these APIs in same series along with cover letter.

> +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;
> +
> +	module_put(wwandev->ops->owner);
> +
> +	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, 0, WWAN_MAX_MINORS,
> "wwan_port",
>  				       &wwan_port_fops);
>  	if (wwan_major < 0) {
> -		class_destroy(wwan_class);
> -		return wwan_major;
> +		err = wwan_major;
> +		goto destroy;
>  	}
> 
>  	return 0;
> +
> +destroy:
> +	class_destroy(wwan_class);
> +unregister:
> +	rtnl_link_unregister(&wwan_rtnl_link_ops);
> +	return err;
>  }
> 
>  static void __exit wwan_exit(void)
>  {
>  	__unregister_chrdev(wwan_major, 0, WWAN_MAX_MINORS,
> "wwan_port");
> +	rtnl_link_unregister(&wwan_rtnl_link_ops);
>  	class_destroy(wwan_class);
>  }
> 
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h index
> fa33cc1..7071f95 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 @@ -116,4 +117,41 @@ void
> wwan_port_txon(struct wwan_port *port);
>   */
>  void *wwan_port_get_drvdata(struct wwan_port *port);
> 
> +/**
> + * struct wwan_ops - WWAN device ops
> + * @owner: module owner of the WWAN 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 {
> +	struct module *owner;
> +	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
Kdoc comment section goes in the .c file.

Powered by blists - more mailing lists