[PATCH net-next 1/4] rtnetlink: add alloc() method to rtnl_link_ops

[PATCH net-next 1/4] rtnetlink: add alloc() method to rtnl_link_ops

[Loic Poulain]

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>
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 include/net/rtnetlink.h |  8 ++++++++
 net/core/rtnetlink.c    | 19 ++++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 479f60e..384e800 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 714d5fa..c0c8dec 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -376,12 +376,12 @@ int __rtnl_link_register(struct rtnl_link_ops *ops)
 	if (rtnl_link_ops_get(ops->kind))
 		return -EEXIST;
 
-	/* The check for setup is here because if ops
+	/* The check for alloc/setup is here because if ops
 	 * does not have that filled up, it is not possible
 	 * to use the ops for creating device. So do not
 	 * fill up dellink as well. That disables rtnl_dellink.
 	 */
-	if (ops->setup && !ops->dellink)
+	if ((ops->alloc || ops->setup) && !ops->dellink)
 		ops->dellink = unregister_netdevice_queue;
 
 	list_add_tail(&ops->list, &link_ops);
@@ -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);
 
@@ -3411,7 +3420,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EOPNOTSUPP;
 	}
 
-	if (!ops->setup)
+	if (!ops->alloc && !ops->setup)
 		return -EOPNOTSUPP;
 
 	if (!ifname[0]) {
-- 
2.7.4

[PATCH net-next 2/4] rtnetlink: add IFLA_PARENT_DEV_NAME

[Loic Poulain]

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>
---
 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 cd5b382..0ac1f6a 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 c0c8dec..56ac16a 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.7.4

[PATCH net-next 3/4] rtnetlink: fill IFLA_PARENT_DEV_NAME on link dump

[Loic Poulain]

From: Sergey Ryazanov <ryazanov.s.a@gmail.com>

Return a parent device using the FLA_PARENT_DEV_NAME attribute during
links dump. This should help a user figure out which links belong to a
particular HW device. E.g. what data channels exists on a specific WWAN
modem.

Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
---
 net/core/rtnetlink.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 56ac16a..120887c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 	if (rtnl_fill_prop_list(skb, dev))
 		goto nla_put_failure;
 
+	if (dev->dev.parent &&
+	    nla_put_string(skb, IFLA_PARENT_DEV_NAME,
+			   dev_name(dev->dev.parent)))
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 	return 0;
 
-- 
2.7.4

[PATCH net-next 4/4] wwan: add interface creation support

[Loic Poulain]

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>
Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/net/wwan/wwan_core.c | 229 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/wwan.h         |  38 +++++++
 include/uapi/linux/wwan.h    |  16 +++
 3 files changed, 276 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 cff04e5..627bf7c 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;
 };
 
 /**
@@ -78,7 +84,8 @@ static const struct device_type wwan_dev_type = {
 
 static int wwan_dev_parent_match(struct device *dev, const void *parent)
 {
-	return (dev->type == &wwan_dev_type && dev->parent == parent);
+	return (dev->type == &wwan_dev_type &&
+		(dev->parent == parent || dev == parent));
 }
 
 static struct wwan_device *wwan_dev_get_by_parent(struct device *parent)
@@ -92,6 +99,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 +180,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 +553,210 @@ 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;
+	}
+
+	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);
+
+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, "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)
 {
+	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 aa05a25..1116b52 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,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 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 0000000..32a2720
--- /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.7.4

RE: [PATCH net-next 3/4] rtnetlink: fill IFLA_PARENT_DEV_NAME on link dump

[Parav Pandit]

> From: Loic Poulain <loic.poulain@linaro.org>
> Sent: Tuesday, June 8, 2021 7:37 PM
> 
> From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> 
> Return a parent device using the FLA_PARENT_DEV_NAME attribute during
> links dump. This should help a user figure out which links belong to a
> particular HW device. E.g. what data channels exists on a specific WWAN
> modem.
> 
Please add the output sample in the commit message, for this additional field possibly for a more common netdevice of a pci device or some other one.

> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> ---
>  net/core/rtnetlink.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 56ac16a..120887c
> 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>  	if (rtnl_fill_prop_list(skb, dev))
>  		goto nla_put_failure;
> 
> +	if (dev->dev.parent &&
> +	    nla_put_string(skb, IFLA_PARENT_DEV_NAME,
> +			   dev_name(dev->dev.parent)))
> +		goto nla_put_failure;
> +
A device name along with device bus establishes a unique identity in the system.
Hence you should add IFLA_PARENT_DEV_BUS_NAME and return it optionally if the device is on a bus.
If (dev->dev.parent->bus)
 return parent->bus->name string.

>  	nlmsg_end(skb, nlh);
>  	return 0;
> 
> --
> 2.7.4

Re: [PATCH net-next 3/4] rtnetlink: fill IFLA_PARENT_DEV_NAME on link dump

[Sergey Ryazanov]

Hello Parav,

On Tue, Jun 8, 2021 at 7:54 PM Parav Pandit <parav@nvidia.com> wrote:
>> From: Loic Poulain <loic.poulain@linaro.org>
>> Sent: Tuesday, June 8, 2021 7:37 PM
>>
>> From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>>
>> Return a parent device using the FLA_PARENT_DEV_NAME attribute during
>> links dump. This should help a user figure out which links belong to a
>> particular HW device. E.g. what data channels exists on a specific WWAN
>> modem.
>>
> Please add the output sample in the commit message, for this additional field possibly for a more common netdevice of a pci device or some other one.
>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> ---
>>  net/core/rtnetlink.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 56ac16a..120887c
>> 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>>       if (rtnl_fill_prop_list(skb, dev))
>>               goto nla_put_failure;
>>
>> +     if (dev->dev.parent &&
>> +         nla_put_string(skb, IFLA_PARENT_DEV_NAME,
>> +                        dev_name(dev->dev.parent)))
>> +             goto nla_put_failure;
>> +
> A device name along with device bus establishes a unique identity in the system.

Sure. To uniquely identify an abstract device we need a full path,
including a device parent. In sysfs it will be a device bus. But
IFLA_PARENT_DEV_NAME was introduced to identify the parent device
within a scope of a specific subsystem, which is specified by the
IFLA_INFO_KIND attribute. IFLA_PARENT_DEV_NAME should become a sane
alternative for the IFLA_LINK usage when the link parent is not a
netdev themself.

You can find more details in the description of IFLA_PARENT_DEV_NAME
introduction patch [1], my explanation, why we need to make the
attribute common [2] and see a usage example in the wwan interface
creation patch [3].

1. https://lore.kernel.org/netdev/1623161227-29930-2-git-send-email-loic.poulain@linaro.org/
2. https://lore.kernel.org/netdev/CAHNKnsTKfFF9EckwSnLrwaPdH_tkjvdB3PVraMD-OLqFdLmp_Q@mail.gmail.com/
3. https://lore.kernel.org/netdev/1623161227-29930-4-git-send-email-loic.poulain@linaro.org/

> Hence you should add IFLA_PARENT_DEV_BUS_NAME and return it optionally if the device is on a bus.
> If (dev->dev.parent->bus)
>  return parent->bus->name string.

Looks like we are able to export the device bus name. Do you have a
use case for this attribute? And even so, should we bloat this simple
patch with auxiliary attributes?

I would even prefer that this patch was merged with the attribute
introduction patch into a single patch. This way the purpose of the
attribute will become more clear.

-- 
Sergey

RE: [PATCH net-next 3/4] rtnetlink: fill IFLA_PARENT_DEV_NAME on link dump

[Parav Pandit]

Hi Sergey,

> From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Sent: Wednesday, June 9, 2021 5:00 AM
> 
> Hello Parav,
> 
> On Tue, Jun 8, 2021 at 7:54 PM Parav Pandit <parav@nvidia.com> wrote:
> >> From: Loic Poulain <loic.poulain@linaro.org>
> >> Sent: Tuesday, June 8, 2021 7:37 PM
> >>
> >> From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> >>
> >> Return a parent device using the FLA_PARENT_DEV_NAME attribute
> during
> >> links dump. This should help a user figure out which links belong to
> >> a particular HW device. E.g. what data channels exists on a specific
> >> WWAN modem.
> >>
> > Please add the output sample in the commit message, for this additional
> field possibly for a more common netdevice of a pci device or some other
> one.
> >
> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> >> ---
> >>  net/core/rtnetlink.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index
> >> 56ac16a..120887c
> >> 100644
> >> --- a/net/core/rtnetlink.c
> >> +++ b/net/core/rtnetlink.c
> >> @@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
> >>       if (rtnl_fill_prop_list(skb, dev))
> >>               goto nla_put_failure;
> >>
> >> +     if (dev->dev.parent &&
> >> +         nla_put_string(skb, IFLA_PARENT_DEV_NAME,
> >> +                        dev_name(dev->dev.parent)))
> >> +             goto nla_put_failure;
> >> +
> > A device name along with device bus establishes a unique identity in the
> system.
> 
> Sure. To uniquely identify an abstract device we need a full path, including a
> device parent. In sysfs it will be a device bus. But IFLA_PARENT_DEV_NAME
> was introduced to identify the parent device within a scope of a specific
> subsystem, which is specified by the IFLA_INFO_KIND attribute.

IFLA_INFO_KIND is not set for many types of netdevices which are not created by ip link add command.
Such as pci devices, auxiliary bus pci sf devices and possibly others.
IFLA_PARENT_DEV_NAME is returned for all netdevices whichever has it valid.

For example, for a netdevice with PCI parent, it will return as 0000:03:00.0.
This number string is useless without telling that it is pci device name.
So if you prefer to add PARENT_DEV_NAME, it needs to accompany along with its BUS_NAME too.
IFLA_INF_KIND for pci and other device is null.
So only PARENT_DEV is not sufficient.

> IFLA_PARENT_DEV_NAME should become a sane alternative for the
> IFLA_LINK usage when the link parent is not a netdev themself.
>
Ok. but not enough. It needs to accompany with the bus name too.
 
> You can find more details in the description of IFLA_PARENT_DEV_NAME
> introduction patch [1], my explanation, why we need to make the attribute
> common [2] and see a usage example in the wwan interface creation patch
> [3].
> 
I am not against making it common. I just say that if you prefer to expose this duplicate (and useful) info, please accompany with device bus name too.

Btw: parent device info is available via ethool such as

ethtool -i enp1s0f0 | grep bus-info
bus-info: 0000:01:00.0

This is left to the individual driver to fill up.
Compare to that generic way like this in this patch is desired with bus name.

> 1. https://lore.kernel.org/netdev/1623161227-29930-2-git-send-email-
> loic.poulain@linaro.org/
> 2.
> https://lore.kernel.org/netdev/CAHNKnsTKfFF9EckwSnLrwaPdH_tkjvdB3PVra
> MD-OLqFdLmp_Q@mail.gmail.com/
> 3. https://lore.kernel.org/netdev/1623161227-29930-4-git-send-email-
> loic.poulain@linaro.org/
> 
> > Hence you should add IFLA_PARENT_DEV_BUS_NAME and return it
> optionally if the device is on a bus.
> > If (dev->dev.parent->bus)
> >  return parent->bus->name string.
> 
> Looks like we are able to export the device bus name. Do you have a use case
> for this attribute? 
The one I explained above.

> And even so, should we bloat this simple patch with
> auxiliary attributes?
>
Not sure which one. I don’t see any more to add other that bus name and parent device name.

A similar patch [4] to include parent device for rdma networking devices didn't make through because its readily available in sysfs.
 
[4] https://lore.kernel.org/linux-rdma/BY5PR12MB432246F3155BC1D440857629DCEB0@BY5PR12MB4322.namprd12.prod.outlook.com/