[PATCH net-next 0/3] netdevsim: link and forward skbs between

[PATCH net-next 0/3] netdevsim: link and forward skbs between

[David Wei]

This patchset adds the ability to link two netdevsim ports together and
forward skbs between them, similar to veth. The goal is to use netdevsim
for testing features e.g. zero copy Rx using io_uring.

This feature was tested locally on QEMU, and a selftest is included.

David Wei (3):
  netdevsim: allow two netdevsim ports to be connected
  netdevsim: forward skbs from one connected port to another
  netdevsim: add selftest for forwarding skb between connected ports

 drivers/net/netdevsim/bus.c                   |  10 ++
 drivers/net/netdevsim/dev.c                   |  97 +++++++++++++++
 drivers/net/netdevsim/netdev.c                |  25 +++-
 drivers/net/netdevsim/netdevsim.h             |   3 +
 .../drivers/net/netdevsim/forward.sh          | 111 ++++++++++++++++++
 5 files changed, 241 insertions(+), 5 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/forward.sh

-- 
2.39.3

[PATCH net-next 1/3] netdevsim: allow two netdevsim ports to be

[David Wei]

Add a debugfs file in
/sys/kernel/debug/netdevsim/netdevsimN/ports/B/link

Writing "M B" to this file will link port A of netdevsim N with port B of
netdevsim M.

Reading this file will return the linked netdevsim id and port, if any.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/netdevsim/bus.c       | 10 ++++
 drivers/net/netdevsim/dev.c       | 97 +++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdev.c    |  5 ++
 drivers/net/netdevsim/netdevsim.h |  3 +
 4 files changed, 115 insertions(+)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index bcbc1e19edde..3e4378e9dbee 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -364,3 +364,13 @@ void nsim_bus_exit(void)
 	driver_unregister(&nsim_driver);
 	bus_unregister(&nsim_bus);
 }
+
+struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id)
+{
+	struct nsim_bus_dev *nsim_bus_dev;
+	list_for_each_entry(nsim_bus_dev, &nsim_bus_dev_list, list) {
+		if (nsim_bus_dev->dev.id == id)
+			return nsim_bus_dev;
+	}
+	return NULL;
+}
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index b4d3b9cde8bd..72ad61f141a2 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -25,6 +25,7 @@
 #include <linux/mutex.h>
 #include <linux/random.h>
 #include <linux/rtnetlink.h>
+#include <linux/string.h>
 #include <linux/workqueue.h>
 #include <net/devlink.h>
 #include <net/ip.h>
@@ -388,6 +389,99 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
 	.owner = THIS_MODULE,
 };
 
+static ssize_t nsim_dev_link_read(struct file *file, char __user *data,
+				  size_t count, loff_t *ppos)
+{
+	struct nsim_dev_port *nsim_dev_port;
+	struct netdevsim *peer;
+	unsigned int id, port;
+	char buf[11];
+	ssize_t len;
+
+	nsim_dev_port = file->private_data;
+	peer = nsim_dev_port->ns->peer;
+	if (!peer) {
+		len = scnprintf(buf, sizeof(buf), "\n");
+		goto out;
+	}
+
+	id = peer->nsim_bus_dev->dev.id;
+	port = peer->nsim_dev_port->port_index;
+	len = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
+
+out:
+	return simple_read_from_buffer(data, count, ppos, buf, len);
+}
+
+static ssize_t nsim_dev_link_write(struct file *file,
+					  const char __user *data,
+					  size_t count, loff_t *ppos)
+{
+	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
+	struct nsim_bus_dev *peer_bus_dev;
+	struct nsim_dev *peer_dev;
+	unsigned int id, port;
+	char *token, *cur;
+	char buf[10];
+	ssize_t ret;
+
+	if (count >= sizeof(buf))
+		return -ENOSPC;
+
+	ret = copy_from_user(buf, data, count);
+	if (ret)
+		return -EFAULT;
+	buf[count] = '\0';
+
+	cur = buf;
+	token = strsep(&cur, " ");
+	if (!token)
+		return -EINVAL;
+	ret = kstrtouint(token, 10, &id);
+	if (ret)
+		return ret;
+
+	token = strsep(&cur, " ");
+	if (!token)
+		return -EINVAL;
+	ret = kstrtouint(token, 10, &port);
+	if (ret)
+		return ret;
+
+	/* too many args */
+	if (strsep(&cur, " "))
+		return -E2BIG;
+
+	/* cannot link to self */
+	nsim_dev_port = file->private_data;
+	if (nsim_dev_port->ns->nsim_bus_dev->dev.id == id)
+		return -EINVAL;
+
+	/* invalid netdevsim id */
+	peer_bus_dev = nsim_bus_dev_get(id);
+	if (!peer_bus_dev)
+		return -EINVAL;
+
+	peer_dev = dev_get_drvdata(&peer_bus_dev->dev);
+	list_for_each_entry(peer_dev_port, &peer_dev->port_list, list) {
+		if (peer_dev_port->port_index == port) {
+			nsim_dev_port->ns->peer = peer_dev_port->ns;
+			peer_dev_port->ns->peer = nsim_dev_port->ns;
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static const struct file_operations nsim_dev_link_fops = {
+	.open = simple_open,
+	.read = nsim_dev_link_read,
+	.write = nsim_dev_link_write,
+	.llseek = generic_file_llseek,
+	.owner = THIS_MODULE,
+};
+
 static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
 				      struct nsim_dev_port *nsim_dev_port)
 {
@@ -418,6 +512,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
 	}
 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
 
+	debugfs_create_file("link", 0600, nsim_dev_port->ddir,
+			    nsim_dev_port, &nsim_dev_link_fops);
+
 	return 0;
 }
 
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index aecaf5f44374..1abdcd470f21 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 	ns->nsim_dev = nsim_dev;
 	ns->nsim_dev_port = nsim_dev_port;
 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
+	ns->peer = NULL;
 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
 	nsim_ethtool_init(ns);
@@ -409,6 +410,10 @@ void nsim_destroy(struct netdevsim *ns)
 	struct net_device *dev = ns->netdev;
 
 	rtnl_lock();
+	if (ns->peer) {
+		ns->peer->peer = NULL;
+		ns->peer = NULL;
+	}
 	unregister_netdevice(dev);
 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
 		nsim_macsec_teardown(ns);
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 028c825b86db..ac7b34a83585 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -125,6 +125,7 @@ struct netdevsim {
 	} udp_ports;
 
 	struct nsim_ethtool ethtool;
+	struct netdevsim *peer;
 };
 
 struct netdevsim *
@@ -417,3 +418,5 @@ struct nsim_bus_dev {
 
 int nsim_bus_init(void);
 void nsim_bus_exit(void);
+
+struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id);
-- 
2.39.3

[PATCH net-next 2/3] netdevsim: forward skbs from one connected

[David Wei]

Forward skbs sent from one netdevsim port to its connected netdevsim
port using dev_forward_skb, in a spirit similar to veth.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/netdevsim/netdev.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 1abdcd470f21..698819072c4f 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -29,19 +29,30 @@
 static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
+	struct netdevsim *peer_ns;
+	int ret = NETDEV_TX_OK;
 
 	if (!nsim_ipsec_tx(ns, skb))
-		goto out;
+		goto err;
 
 	u64_stats_update_begin(&ns->syncp);
 	ns->tx_packets++;
 	ns->tx_bytes += skb->len;
 	u64_stats_update_end(&ns->syncp);
 
-out:
-	dev_kfree_skb(skb);
+	peer_ns = ns->peer;
+	if (!peer_ns)
+		goto err;
+
+	skb_tx_timestamp(skb);
+	if (unlikely(dev_forward_skb(peer_ns->netdev, skb) == NET_RX_DROP))
+		ret = NET_XMIT_DROP;
 
-	return NETDEV_TX_OK;
+	return ret;
+
+err:
+	dev_kfree_skb(skb);
+	return ret;
 }
 
 static void nsim_set_rx_mode(struct net_device *dev)
@@ -302,7 +313,6 @@ static void nsim_setup(struct net_device *dev)
 	eth_hw_addr_random(dev);
 
 	dev->tx_queue_len = 0;
-	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
 			   IFF_NO_QUEUE;
-- 
2.39.3

[PATCH net-next 3/3] selftest: netdevsim: add selftest for

[David Wei]

Connect two netdevsim ports in different namespaces together, then send
packets between them using socat.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 .../drivers/net/netdevsim/forward.sh          | 111 ++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/forward.sh

diff --git a/tools/testing/selftests/drivers/net/netdevsim/forward.sh b/tools/testing/selftests/drivers/net/netdevsim/forward.sh
new file mode 100755
index 000000000000..7464fb42576d
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/netdevsim/forward.sh
@@ -0,0 +1,111 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+
+NSIM_DEV_SYS=/sys/bus/netdevsim
+NSIM_DEV_DFS=/sys/kernel/debug/netdevsim/netdevsim
+
+socat_check()
+{
+	if [ ! -x "$(command -v socat)" ]; then
+		echo "socat command not found. Skipping test"
+		return 1
+	fi
+
+	return 0
+}
+
+setup_ns()
+{
+	set -e
+	ip netns add nssv
+	ip netns add nscl
+
+	ip link set eni1np1 netns nssv
+	ip link set eni2np1 netns nscl
+
+	ip netns exec nssv ip addr add '192.168.1.1/24' dev eni1np1
+	ip netns exec nscl ip addr add '192.168.1.2/24' dev eni2np1
+
+	ip netns exec nssv ip link set dev eni1np1 up
+	ip netns exec nscl ip link set dev eni2np1 up
+	set +e
+}
+
+cleanup_ns()
+{
+	ip netns del nscl
+	ip netns del nssv
+}
+
+###
+### Code start
+###
+
+modprobe netdevsim
+
+# linking
+
+echo 1 > ${NSIM_DEV_SYS}/new_device
+
+echo "2 0" > ${NSIM_DEV_DFS}1/ports/0/link 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with non-existent netdevsim should fail"
+	return 1
+fi
+
+echo 2 > /sys/bus/netdevsim/new_device
+
+echo "2 0" > ${NSIM_DEV_DFS}1/ports/0/link
+if [ $? -ne 0 ]; then
+	echo "linking netdevsim1 port0 with netdevsim2 port0 should succeed"
+	return 1
+fi
+
+# argument error checking
+
+echo "2 1" > ${NSIM_DEV_DFS}1/ports/0/link 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with non-existent port in a netdevsim should fail"
+	return 1
+fi
+
+echo "2 a" > ${NSIM_DEV_DFS}1/ports/0/link 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "invalid arg should fail"
+	return 1
+fi
+
+echo "2 0 1" > ${NSIM_DEV_DFS}1/ports/0/link 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "invalid arg should fail"
+	return 1
+fi
+
+# send/recv packets
+
+socat_check || return 1
+
+setup_ns
+
+tmp_file=$(mktemp)
+ip netns exec nssv socat TCP-LISTEN:1234,fork $tmp_file &
+pid=$!
+
+echo "HI" | ip netns exec nscl socat STDIN TCP:192.168.1.1:1234
+
+count=$(cat $tmp_file | wc -c)
+if [[ $count -ne 3 ]]; then
+	echo "expected 3 bytes, got $count"
+	return 1
+fi
+
+echo 2 > ${NSIM_DEV_SYS}/del_device
+
+kill $pid
+echo 1 > ${NSIM_DEV_SYS}/del_device
+
+cleanup_ns
+
+modprobe -r netdevsim
+
+exit 0
-- 
2.39.3

Re: [PATCH net-next 0/3] netdevsim: link and forward skbs between

[Jiri Pirko]

"netdevsim: link and forward skbs between"
I think it there something wrong with your email client, cutting
subjects like this (all of them).

Thu, Dec 07, 2023 at 06:21:14PM CET, dw@davidwei.uk wrote:
>This patchset adds the ability to link two netdevsim ports together and
>forward skbs between them, similar to veth. The goal is to use netdevsim
>for testing features e.g. zero copy Rx using io_uring.
>
>This feature was tested locally on QEMU, and a selftest is included.
>
>David Wei (3):
>  netdevsim: allow two netdevsim ports to be connected
>  netdevsim: forward skbs from one connected port to another
>  netdevsim: add selftest for forwarding skb between connected ports
>
> drivers/net/netdevsim/bus.c                   |  10 ++
> drivers/net/netdevsim/dev.c                   |  97 +++++++++++++++
> drivers/net/netdevsim/netdev.c                |  25 +++-
> drivers/net/netdevsim/netdevsim.h             |   3 +
> .../drivers/net/netdevsim/forward.sh          | 111 ++++++++++++++++++
> 5 files changed, 241 insertions(+), 5 deletions(-)
> create mode 100755 tools/testing/selftests/drivers/net/netdevsim/forward.sh
>
>-- 
>2.39.3
>
>

Re: [PATCH net-next 1/3] netdevsim: allow two netdevsim ports to be

[Jiri Pirko]

Thu, Dec 07, 2023 at 06:21:15PM CET, dw@davidwei.uk wrote:
>Add a debugfs file in
>/sys/kernel/debug/netdevsim/netdevsimN/ports/B/link

"peer" perhaps?

>
>Writing "M B" to this file will link port A of netdevsim N with port B of
>netdevsim M.
>
>Reading this file will return the linked netdevsim id and port, if any.
>
>Signed-off-by: David Wei <dw@davidwei.uk>
>---
> drivers/net/netdevsim/bus.c       | 10 ++++
> drivers/net/netdevsim/dev.c       | 97 +++++++++++++++++++++++++++++++
> drivers/net/netdevsim/netdev.c    |  5 ++
> drivers/net/netdevsim/netdevsim.h |  3 +
> 4 files changed, 115 insertions(+)
>
>diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
>index bcbc1e19edde..3e4378e9dbee 100644
>--- a/drivers/net/netdevsim/bus.c
>+++ b/drivers/net/netdevsim/bus.c
>@@ -364,3 +364,13 @@ void nsim_bus_exit(void)
> 	driver_unregister(&nsim_driver);
> 	bus_unregister(&nsim_bus);
> }
>+
>+struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id)
>+{
>+	struct nsim_bus_dev *nsim_bus_dev;
>+	list_for_each_entry(nsim_bus_dev, &nsim_bus_dev_list, list) {
>+		if (nsim_bus_dev->dev.id == id)
>+			return nsim_bus_dev;
>+	}
>+	return NULL;
>+}
>diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>index b4d3b9cde8bd..72ad61f141a2 100644
>--- a/drivers/net/netdevsim/dev.c
>+++ b/drivers/net/netdevsim/dev.c
>@@ -25,6 +25,7 @@
> #include <linux/mutex.h>
> #include <linux/random.h>
> #include <linux/rtnetlink.h>
>+#include <linux/string.h>
> #include <linux/workqueue.h>
> #include <net/devlink.h>
> #include <net/ip.h>
>@@ -388,6 +389,99 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
> 	.owner = THIS_MODULE,
> };
> 
>+static ssize_t nsim_dev_link_read(struct file *file, char __user *data,
>+				  size_t count, loff_t *ppos)
>+{
>+	struct nsim_dev_port *nsim_dev_port;
>+	struct netdevsim *peer;
>+	unsigned int id, port;
>+	char buf[11];

See below.


>+	ssize_t len;
>+
>+	nsim_dev_port = file->private_data;
>+	peer = nsim_dev_port->ns->peer;
>+	if (!peer) {
>+		len = scnprintf(buf, sizeof(buf), "\n");
>+		goto out;
>+	}
>+
>+	id = peer->nsim_bus_dev->dev.id;
>+	port = peer->nsim_dev_port->port_index;
>+	len = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
>+
>+out:
>+	return simple_read_from_buffer(data, count, ppos, buf, len);
>+}
>+
>+static ssize_t nsim_dev_link_write(struct file *file,
>+					  const char __user *data,
>+					  size_t count, loff_t *ppos)
>+{
>+	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
>+	struct nsim_bus_dev *peer_bus_dev;
>+	struct nsim_dev *peer_dev;
>+	unsigned int id, port;
>+	char *token, *cur;
>+	char buf[10];

# echo "889879797" >/sys/bus/netdevsim/new_device
# devlink dev
netdevsim/netdevsim889879797

I don't think that 10/11 is enough.




>+	ssize_t ret;
>+
>+	if (count >= sizeof(buf))
>+		return -ENOSPC;
>+
>+	ret = copy_from_user(buf, data, count);
>+	if (ret)
>+		return -EFAULT;
>+	buf[count] = '\0';
>+
>+	cur = buf;
>+	token = strsep(&cur, " ");

Why you implement this differently, comparing to new_device_store()?
Just use sscanf(), no?


>+	if (!token)
>+		return -EINVAL;

In general, in case of user putting in invalid input, please hint him
the correct format. Again, see new_device_store().


>+	ret = kstrtouint(token, 10, &id);
>+	if (ret)
>+		return ret;
>+
>+	token = strsep(&cur, " ");
>+	if (!token)
>+		return -EINVAL;
>+	ret = kstrtouint(token, 10, &port);
>+	if (ret)
>+		return ret;
>+
>+	/* too many args */
>+	if (strsep(&cur, " "))
>+		return -E2BIG;
>+
>+	/* cannot link to self */
>+	nsim_dev_port = file->private_data;
>+	if (nsim_dev_port->ns->nsim_bus_dev->dev.id == id)

Why not? Loopback between 2 ports of same device seems like completely
valid scenario.


>+		return -EINVAL;
>+
>+	/* invalid netdevsim id */
>+	peer_bus_dev = nsim_bus_dev_get(id);
>+	if (!peer_bus_dev)
>+		return -EINVAL;
>+
>+	peer_dev = dev_get_drvdata(&peer_bus_dev->dev);
>+	list_for_each_entry(peer_dev_port, &peer_dev->port_list, list) {
>+		if (peer_dev_port->port_index == port) {
>+			nsim_dev_port->ns->peer = peer_dev_port->ns;
>+			peer_dev_port->ns->peer = nsim_dev_port->ns;
>+			return count;
>+		}
>+	}
>+
>+	return -EINVAL;
>+}
>+
>+static const struct file_operations nsim_dev_link_fops = {
>+	.open = simple_open,
>+	.read = nsim_dev_link_read,
>+	.write = nsim_dev_link_write,
>+	.llseek = generic_file_llseek,
>+	.owner = THIS_MODULE,
>+};
>+
> static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
> 				      struct nsim_dev_port *nsim_dev_port)
> {
>@@ -418,6 +512,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
> 	}
> 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
> 
>+	debugfs_create_file("link", 0600, nsim_dev_port->ddir,
>+			    nsim_dev_port, &nsim_dev_link_fops);
>+
> 	return 0;
> }
> 
>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>index aecaf5f44374..1abdcd470f21 100644
>--- a/drivers/net/netdevsim/netdev.c
>+++ b/drivers/net/netdevsim/netdev.c
>@@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
> 	ns->nsim_dev = nsim_dev;
> 	ns->nsim_dev_port = nsim_dev_port;
> 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
>+	ns->peer = NULL;
> 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
> 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
> 	nsim_ethtool_init(ns);
>@@ -409,6 +410,10 @@ void nsim_destroy(struct netdevsim *ns)
> 	struct net_device *dev = ns->netdev;
> 
> 	rtnl_lock();
>+	if (ns->peer) {
>+		ns->peer->peer = NULL;
>+		ns->peer = NULL;

What is this good for?


>+	}
> 	unregister_netdevice(dev);
> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
> 		nsim_macsec_teardown(ns);
>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>index 028c825b86db..ac7b34a83585 100644
>--- a/drivers/net/netdevsim/netdevsim.h
>+++ b/drivers/net/netdevsim/netdevsim.h
>@@ -125,6 +125,7 @@ struct netdevsim {
> 	} udp_ports;
> 
> 	struct nsim_ethtool ethtool;
>+	struct netdevsim *peer;
> };
> 
> struct netdevsim *
>@@ -417,3 +418,5 @@ struct nsim_bus_dev {
> 
> int nsim_bus_init(void);
> void nsim_bus_exit(void);
>+
>+struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id);
>-- 
>2.39.3
>
>

Re: [PATCH net-next 2/3] netdevsim: forward skbs from one connected

[Jiri Pirko]

Thu, Dec 07, 2023 at 06:21:16PM CET, dw@davidwei.uk wrote:
>Forward skbs sent from one netdevsim port to its connected netdevsim
>port using dev_forward_skb, in a spirit similar to veth.
>
>Signed-off-by: David Wei <dw@davidwei.uk>
>---
> drivers/net/netdevsim/netdev.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>index 1abdcd470f21..698819072c4f 100644
>--- a/drivers/net/netdevsim/netdev.c
>+++ b/drivers/net/netdevsim/netdev.c
>@@ -29,19 +29,30 @@
> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> 	struct netdevsim *ns = netdev_priv(dev);
>+	struct netdevsim *peer_ns;
>+	int ret = NETDEV_TX_OK;
> 
> 	if (!nsim_ipsec_tx(ns, skb))
>-		goto out;
>+		goto err;
> 
> 	u64_stats_update_begin(&ns->syncp);
> 	ns->tx_packets++;
> 	ns->tx_bytes += skb->len;
> 	u64_stats_update_end(&ns->syncp);
> 
>-out:
>-	dev_kfree_skb(skb);
>+	peer_ns = ns->peer;

What is stopping the peer to be removed and freed right now?


>+	if (!peer_ns)
>+		goto err;
>+
>+	skb_tx_timestamp(skb);
>+	if (unlikely(dev_forward_skb(peer_ns->netdev, skb) == NET_RX_DROP))
>+		ret = NET_XMIT_DROP;
> 
>-	return NETDEV_TX_OK;
>+	return ret;
>+
>+err:
>+	dev_kfree_skb(skb);
>+	return ret;
> }
> 
> static void nsim_set_rx_mode(struct net_device *dev)
>@@ -302,7 +313,6 @@ static void nsim_setup(struct net_device *dev)
> 	eth_hw_addr_random(dev);
> 
> 	dev->tx_queue_len = 0;
>-	dev->flags |= IFF_NOARP;
> 	dev->flags &= ~IFF_MULTICAST;
> 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
> 			   IFF_NO_QUEUE;
>-- 
>2.39.3
>
>

Re: [PATCH net-next 1/3] netdevsim: allow two netdevsim ports to be

[Jakub Kicinski]

On Thu,  7 Dec 2023 09:21:15 -0800 David Wei wrote:
> +	ret = copy_from_user(buf, data, count);
> +	if (ret)
> +		return -EFAULT;
> +	buf[count] = '\0';
> +
> +	cur = buf;
> +	token = strsep(&cur, " ");
> +	if (!token)
> +		return -EINVAL;
> +	ret = kstrtouint(token, 10, &id);
> +	if (ret)
> +		return ret;
> +
> +	token = strsep(&cur, " ");
> +	if (!token)
> +		return -EINVAL;
> +	ret = kstrtouint(token, 10, &port);
> +	if (ret)
> +		return ret;
> +
> +	/* too many args */
> +	if (strsep(&cur, " "))
> +		return -E2BIG;

What's wrong with scanf?

Re: [PATCH net-next 0/3] netdevsim: link and forward skbs between

[David Wei]

On 2023-12-08 02:13, Jiri Pirko wrote:
> "netdevsim: link and forward skbs between"
> I think it there something wrong with your email client, cutting
> subjects like this (all of them).

Apologies, I autoformatted my patch files before git send-email which
wrapped all lines including the subject.

> 
> Thu, Dec 07, 2023 at 06:21:14PM CET, dw@davidwei.uk wrote:
>> This patchset adds the ability to link two netdevsim ports together and
>> forward skbs between them, similar to veth. The goal is to use netdevsim
>> for testing features e.g. zero copy Rx using io_uring.
>>
>> This feature was tested locally on QEMU, and a selftest is included.
>>
>> David Wei (3):
>>  netdevsim: allow two netdevsim ports to be connected
>>  netdevsim: forward skbs from one connected port to another
>>  netdevsim: add selftest for forwarding skb between connected ports
>>
>> drivers/net/netdevsim/bus.c                   |  10 ++
>> drivers/net/netdevsim/dev.c                   |  97 +++++++++++++++
>> drivers/net/netdevsim/netdev.c                |  25 +++-
>> drivers/net/netdevsim/netdevsim.h             |   3 +
>> .../drivers/net/netdevsim/forward.sh          | 111 ++++++++++++++++++
>> 5 files changed, 241 insertions(+), 5 deletions(-)
>> create mode 100755 tools/testing/selftests/drivers/net/netdevsim/forward.sh
>>
>> -- 
>> 2.39.3
>>
>>

Re: [PATCH net-next 1/3] netdevsim: allow two netdevsim ports to be

[David Wei]

On 2023-12-08 02:59, Jiri Pirko wrote:
> Thu, Dec 07, 2023 at 06:21:15PM CET, dw@davidwei.uk wrote:
>> Add a debugfs file in
>> /sys/kernel/debug/netdevsim/netdevsimN/ports/B/link
> 
> "peer" perhaps?

Sounds good.

> 
>>
>> Writing "M B" to this file will link port A of netdevsim N with port B of
>> netdevsim M.
>>
>> Reading this file will return the linked netdevsim id and port, if any.
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>> drivers/net/netdevsim/bus.c       | 10 ++++
>> drivers/net/netdevsim/dev.c       | 97 +++++++++++++++++++++++++++++++
>> drivers/net/netdevsim/netdev.c    |  5 ++
>> drivers/net/netdevsim/netdevsim.h |  3 +
>> 4 files changed, 115 insertions(+)
>>
>> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
>> index bcbc1e19edde..3e4378e9dbee 100644
>> --- a/drivers/net/netdevsim/bus.c
>> +++ b/drivers/net/netdevsim/bus.c
>> @@ -364,3 +364,13 @@ void nsim_bus_exit(void)
>> 	driver_unregister(&nsim_driver);
>> 	bus_unregister(&nsim_bus);
>> }
>> +
>> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id)
>> +{
>> +	struct nsim_bus_dev *nsim_bus_dev;
>> +	list_for_each_entry(nsim_bus_dev, &nsim_bus_dev_list, list) {
>> +		if (nsim_bus_dev->dev.id == id)
>> +			return nsim_bus_dev;
>> +	}
>> +	return NULL;
>> +}
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index b4d3b9cde8bd..72ad61f141a2 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -25,6 +25,7 @@
>> #include <linux/mutex.h>
>> #include <linux/random.h>
>> #include <linux/rtnetlink.h>
>> +#include <linux/string.h>
>> #include <linux/workqueue.h>
>> #include <net/devlink.h>
>> #include <net/ip.h>
>> @@ -388,6 +389,99 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
>> 	.owner = THIS_MODULE,
>> };
>>
>> +static ssize_t nsim_dev_link_read(struct file *file, char __user *data,
>> +				  size_t count, loff_t *ppos)
>> +{
>> +	struct nsim_dev_port *nsim_dev_port;
>> +	struct netdevsim *peer;
>> +	unsigned int id, port;
>> +	char buf[11];
> 
> See below.
> 
> 
>> +	ssize_t len;
>> +
>> +	nsim_dev_port = file->private_data;
>> +	peer = nsim_dev_port->ns->peer;
>> +	if (!peer) {
>> +		len = scnprintf(buf, sizeof(buf), "\n");
>> +		goto out;
>> +	}
>> +
>> +	id = peer->nsim_bus_dev->dev.id;
>> +	port = peer->nsim_dev_port->port_index;
>> +	len = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
>> +
>> +out:
>> +	return simple_read_from_buffer(data, count, ppos, buf, len);
>> +}
>> +
>> +static ssize_t nsim_dev_link_write(struct file *file,
>> +					  const char __user *data,
>> +					  size_t count, loff_t *ppos)
>> +{
>> +	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
>> +	struct nsim_bus_dev *peer_bus_dev;
>> +	struct nsim_dev *peer_dev;
>> +	unsigned int id, port;
>> +	char *token, *cur;
>> +	char buf[10];
> 
> # echo "889879797" >/sys/bus/netdevsim/new_device
> # devlink dev
> netdevsim/netdevsim889879797
> 
> I don't think that 10/11 is enough.

I took char[10] from nsim_bus_dev_max_vfs_write() which seemed like a
reasonable amount for 4 digit id and ports. How much space is okay to
allocate on the stack for this? Can you please point me to where
new_device_store() is called from? I couldn't find it so I don't know
how big its char *buf arg is.

> 
> 
> 
> 
>> +	ssize_t ret;
>> +
>> +	if (count >= sizeof(buf))
>> +		return -ENOSPC;
>> +
>> +	ret = copy_from_user(buf, data, count);
>> +	if (ret)
>> +		return -EFAULT;
>> +	buf[count] = '\0';
>> +
>> +	cur = buf;
>> +	token = strsep(&cur, " ");
> 
> Why you implement this differently, comparing to new_device_store()?
> Just use sscanf(), no?

I went with strstep() instead of sscanf() because sscanf("%u %u", ...)
does not fail with echo "1 2 3 4". I'm happy to use sscanf() though if
this is not an issue.

> 
> 
>> +	if (!token)
>> +		return -EINVAL;
> 
> In general, in case of user putting in invalid input, please hint him
> the correct format. Again, see new_device_store().

Got it, I'll add an error message.

> 
> 
>> +	ret = kstrtouint(token, 10, &id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	token = strsep(&cur, " ");
>> +	if (!token)
>> +		return -EINVAL;
>> +	ret = kstrtouint(token, 10, &port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* too many args */
>> +	if (strsep(&cur, " "))
>> +		return -E2BIG;
>> +
>> +	/* cannot link to self */
>> +	nsim_dev_port = file->private_data;
>> +	if (nsim_dev_port->ns->nsim_bus_dev->dev.id == id)
> 
> Why not? Loopback between 2 ports of same device seems like completely
> valid scenario.

I'm imagining physical ports which cannot be connected back to itself. When
would this physical loopback be valid?

> 
> 
>> +		return -EINVAL;
>> +
>> +	/* invalid netdevsim id */
>> +	peer_bus_dev = nsim_bus_dev_get(id);
>> +	if (!peer_bus_dev)
>> +		return -EINVAL;
>> +
>> +	peer_dev = dev_get_drvdata(&peer_bus_dev->dev);
>> +	list_for_each_entry(peer_dev_port, &peer_dev->port_list, list) {
>> +		if (peer_dev_port->port_index == port) {
>> +			nsim_dev_port->ns->peer = peer_dev_port->ns;
>> +			peer_dev_port->ns->peer = nsim_dev_port->ns;
>> +			return count;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct file_operations nsim_dev_link_fops = {
>> +	.open = simple_open,
>> +	.read = nsim_dev_link_read,
>> +	.write = nsim_dev_link_write,
>> +	.llseek = generic_file_llseek,
>> +	.owner = THIS_MODULE,
>> +};
>> +
>> static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>> 				      struct nsim_dev_port *nsim_dev_port)
>> {
>> @@ -418,6 +512,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>> 	}
>> 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
>>
>> +	debugfs_create_file("link", 0600, nsim_dev_port->ddir,
>> +			    nsim_dev_port, &nsim_dev_link_fops);
>> +
>> 	return 0;
>> }
>>
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index aecaf5f44374..1abdcd470f21 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
>> @@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
>> 	ns->nsim_dev = nsim_dev;
>> 	ns->nsim_dev_port = nsim_dev_port;
>> 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
>> +	ns->peer = NULL;
>> 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
>> 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
>> 	nsim_ethtool_init(ns);
>> @@ -409,6 +410,10 @@ void nsim_destroy(struct netdevsim *ns)
>> 	struct net_device *dev = ns->netdev;
>>
>> 	rtnl_lock();
>> +	if (ns->peer) {
>> +		ns->peer->peer = NULL;
>> +		ns->peer = NULL;
> 
> What is this good for?

I want to make sure when a netdevsim is removed, its peer does not
forward skbs anymore.

> 
> 
>> +	}
>> 	unregister_netdevice(dev);
>> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
>> 		nsim_macsec_teardown(ns);
>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>> index 028c825b86db..ac7b34a83585 100644
>> --- a/drivers/net/netdevsim/netdevsim.h
>> +++ b/drivers/net/netdevsim/netdevsim.h
>> @@ -125,6 +125,7 @@ struct netdevsim {
>> 	} udp_ports;
>>
>> 	struct nsim_ethtool ethtool;
>> +	struct netdevsim *peer;
>> };
>>
>> struct netdevsim *
>> @@ -417,3 +418,5 @@ struct nsim_bus_dev {
>>
>> int nsim_bus_init(void);
>> void nsim_bus_exit(void);
>> +
>> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id);
>> -- 
>> 2.39.3
>>
>>

Re: [PATCH net-next 2/3] netdevsim: forward skbs from one connected

[David Wei]

On 2023-12-08 03:01, Jiri Pirko wrote:
> Thu, Dec 07, 2023 at 06:21:16PM CET, dw@davidwei.uk wrote:
>> Forward skbs sent from one netdevsim port to its connected netdevsim
>> port using dev_forward_skb, in a spirit similar to veth.
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>> drivers/net/netdevsim/netdev.c | 20 +++++++++++++++-----
>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index 1abdcd470f21..698819072c4f 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
>> @@ -29,19 +29,30 @@
>> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> {
>> 	struct netdevsim *ns = netdev_priv(dev);
>> +	struct netdevsim *peer_ns;
>> +	int ret = NETDEV_TX_OK;
>>
>> 	if (!nsim_ipsec_tx(ns, skb))
>> -		goto out;
>> +		goto err;
>>
>> 	u64_stats_update_begin(&ns->syncp);
>> 	ns->tx_packets++;
>> 	ns->tx_bytes += skb->len;
>> 	u64_stats_update_end(&ns->syncp);
>>
>> -out:
>> -	dev_kfree_skb(skb);
>> +	peer_ns = ns->peer;
> 
> What is stopping the peer to be removed and freed right now?

Thanks for pointing this out, you're right, nothing is stopping it. I'll
add some synchronisation here.

> 
> 
>> +	if (!peer_ns)
>> +		goto err;
>> +
>> +	skb_tx_timestamp(skb);
>> +	if (unlikely(dev_forward_skb(peer_ns->netdev, skb) == NET_RX_DROP))
>> +		ret = NET_XMIT_DROP;
>>
>> -	return NETDEV_TX_OK;
>> +	return ret;
>> +
>> +err:
>> +	dev_kfree_skb(skb);
>> +	return ret;
>> }
>>
>> static void nsim_set_rx_mode(struct net_device *dev)
>> @@ -302,7 +313,6 @@ static void nsim_setup(struct net_device *dev)
>> 	eth_hw_addr_random(dev);
>>
>> 	dev->tx_queue_len = 0;
>> -	dev->flags |= IFF_NOARP;
>> 	dev->flags &= ~IFF_MULTICAST;
>> 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
>> 			   IFF_NO_QUEUE;
>> -- 
>> 2.39.3
>>
>>

Re: [PATCH net-next 1/3] netdevsim: allow two netdevsim ports to be

[David Wei]

On 2023-12-08 09:58, Jakub Kicinski wrote:
> On Thu,  7 Dec 2023 09:21:15 -0800 David Wei wrote:
>> +	ret = copy_from_user(buf, data, count);
>> +	if (ret)
>> +		return -EFAULT;
>> +	buf[count] = '\0';
>> +
>> +	cur = buf;
>> +	token = strsep(&cur, " ");
>> +	if (!token)
>> +		return -EINVAL;
>> +	ret = kstrtouint(token, 10, &id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	token = strsep(&cur, " ");
>> +	if (!token)
>> +		return -EINVAL;
>> +	ret = kstrtouint(token, 10, &port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* too many args */
>> +	if (strsep(&cur, " "))
>> +		return -E2BIG;
> 
> What's wrong with scanf?

Also responded to Jiri:

I went with strstep() instead of sscanf() because sscanf("%u %u", ...)
does not fail with echo "1 2 3 4". I'm happy to use sscanf() though if
this is not an issue.

Re: [PATCH net-next 1/3] netdevsim: allow two netdevsim ports to be

[Jiri Pirko]

Fri, Dec 08, 2023 at 10:57:04PM CET, dw@davidwei.uk wrote:
>On 2023-12-08 02:59, Jiri Pirko wrote:
>> Thu, Dec 07, 2023 at 06:21:15PM CET, dw@davidwei.uk wrote:
>>> Add a debugfs file in
>>> /sys/kernel/debug/netdevsim/netdevsimN/ports/B/link
>> 
>> "peer" perhaps?
>
>Sounds good.
>
>> 
>>>
>>> Writing "M B" to this file will link port A of netdevsim N with port B of
>>> netdevsim M.
>>>
>>> Reading this file will return the linked netdevsim id and port, if any.
>>>
>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>> ---
>>> drivers/net/netdevsim/bus.c       | 10 ++++
>>> drivers/net/netdevsim/dev.c       | 97 +++++++++++++++++++++++++++++++
>>> drivers/net/netdevsim/netdev.c    |  5 ++
>>> drivers/net/netdevsim/netdevsim.h |  3 +
>>> 4 files changed, 115 insertions(+)
>>>
>>> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
>>> index bcbc1e19edde..3e4378e9dbee 100644
>>> --- a/drivers/net/netdevsim/bus.c
>>> +++ b/drivers/net/netdevsim/bus.c
>>> @@ -364,3 +364,13 @@ void nsim_bus_exit(void)
>>> 	driver_unregister(&nsim_driver);
>>> 	bus_unregister(&nsim_bus);
>>> }
>>> +
>>> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id)
>>> +{
>>> +	struct nsim_bus_dev *nsim_bus_dev;
>>> +	list_for_each_entry(nsim_bus_dev, &nsim_bus_dev_list, list) {
>>> +		if (nsim_bus_dev->dev.id == id)
>>> +			return nsim_bus_dev;
>>> +	}
>>> +	return NULL;
>>> +}
>>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>>> index b4d3b9cde8bd..72ad61f141a2 100644
>>> --- a/drivers/net/netdevsim/dev.c
>>> +++ b/drivers/net/netdevsim/dev.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/mutex.h>
>>> #include <linux/random.h>
>>> #include <linux/rtnetlink.h>
>>> +#include <linux/string.h>
>>> #include <linux/workqueue.h>
>>> #include <net/devlink.h>
>>> #include <net/ip.h>
>>> @@ -388,6 +389,99 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
>>> 	.owner = THIS_MODULE,
>>> };
>>>
>>> +static ssize_t nsim_dev_link_read(struct file *file, char __user *data,
>>> +				  size_t count, loff_t *ppos)
>>> +{
>>> +	struct nsim_dev_port *nsim_dev_port;
>>> +	struct netdevsim *peer;
>>> +	unsigned int id, port;
>>> +	char buf[11];
>> 
>> See below.
>> 
>> 
>>> +	ssize_t len;
>>> +
>>> +	nsim_dev_port = file->private_data;
>>> +	peer = nsim_dev_port->ns->peer;
>>> +	if (!peer) {
>>> +		len = scnprintf(buf, sizeof(buf), "\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	id = peer->nsim_bus_dev->dev.id;
>>> +	port = peer->nsim_dev_port->port_index;
>>> +	len = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
>>> +
>>> +out:
>>> +	return simple_read_from_buffer(data, count, ppos, buf, len);
>>> +}
>>> +
>>> +static ssize_t nsim_dev_link_write(struct file *file,
>>> +					  const char __user *data,
>>> +					  size_t count, loff_t *ppos)
>>> +{
>>> +	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
>>> +	struct nsim_bus_dev *peer_bus_dev;
>>> +	struct nsim_dev *peer_dev;
>>> +	unsigned int id, port;
>>> +	char *token, *cur;
>>> +	char buf[10];
>> 
>> # echo "889879797" >/sys/bus/netdevsim/new_device
>> # devlink dev
>> netdevsim/netdevsim889879797
>> 
>> I don't think that 10/11 is enough.
>
>I took char[10] from nsim_bus_dev_max_vfs_write() which seemed like a
>reasonable amount for 4 digit id and ports. How much space is okay to
>allocate on the stack for this? Can you please point me to where
>new_device_store() is called from? I couldn't find it so I don't know
>how big its char *buf arg is.

sysfs:
static BUS_ATTR_WO(new_device);

I see no problem in allocate this buffer using count size


>
>> 
>> 
>> 
>> 
>>> +	ssize_t ret;
>>> +
>>> +	if (count >= sizeof(buf))
>>> +		return -ENOSPC;
>>> +
>>> +	ret = copy_from_user(buf, data, count);
>>> +	if (ret)
>>> +		return -EFAULT;
>>> +	buf[count] = '\0';
>>> +
>>> +	cur = buf;
>>> +	token = strsep(&cur, " ");
>> 
>> Why you implement this differently, comparing to new_device_store()?
>> Just use sscanf(), no?
>
>I went with strstep() instead of sscanf() because sscanf("%u %u", ...)
>does not fail with echo "1 2 3 4". I'm happy to use sscanf() though if
>this is not an issue.

Up to you, but please maintain some consistency with existing code. If
you want to do this differently, please adjust the existing code first.


>
>> 
>> 
>>> +	if (!token)
>>> +		return -EINVAL;
>> 
>> In general, in case of user putting in invalid input, please hint him
>> the correct format. Again, see new_device_store().
>
>Got it, I'll add an error message.
>
>> 
>> 
>>> +	ret = kstrtouint(token, 10, &id);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	token = strsep(&cur, " ");
>>> +	if (!token)
>>> +		return -EINVAL;
>>> +	ret = kstrtouint(token, 10, &port);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* too many args */
>>> +	if (strsep(&cur, " "))
>>> +		return -E2BIG;
>>> +
>>> +	/* cannot link to self */
>>> +	nsim_dev_port = file->private_data;
>>> +	if (nsim_dev_port->ns->nsim_bus_dev->dev.id == id)
>> 
>> Why not? Loopback between 2 ports of same device seems like completely
>> valid scenario.
>
>I'm imagining physical ports which cannot be connected back to itself. When
>would this physical loopback be valid?

I'm talking about 2 ports of the same netdevsim instance. The instance
can have multiple ports.


>
>> 
>> 
>>> +		return -EINVAL;
>>> +
>>> +	/* invalid netdevsim id */
>>> +	peer_bus_dev = nsim_bus_dev_get(id);
>>> +	if (!peer_bus_dev)
>>> +		return -EINVAL;
>>> +
>>> +	peer_dev = dev_get_drvdata(&peer_bus_dev->dev);
>>> +	list_for_each_entry(peer_dev_port, &peer_dev->port_list, list) {
>>> +		if (peer_dev_port->port_index == port) {
>>> +			nsim_dev_port->ns->peer = peer_dev_port->ns;
>>> +			peer_dev_port->ns->peer = nsim_dev_port->ns;
>>> +			return count;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static const struct file_operations nsim_dev_link_fops = {
>>> +	.open = simple_open,
>>> +	.read = nsim_dev_link_read,
>>> +	.write = nsim_dev_link_write,
>>> +	.llseek = generic_file_llseek,
>>> +	.owner = THIS_MODULE,
>>> +};
>>> +
>>> static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>>> 				      struct nsim_dev_port *nsim_dev_port)
>>> {
>>> @@ -418,6 +512,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>>> 	}
>>> 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
>>>
>>> +	debugfs_create_file("link", 0600, nsim_dev_port->ddir,
>>> +			    nsim_dev_port, &nsim_dev_link_fops);
>>> +
>>> 	return 0;
>>> }
>>>
>>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>>> index aecaf5f44374..1abdcd470f21 100644
>>> --- a/drivers/net/netdevsim/netdev.c
>>> +++ b/drivers/net/netdevsim/netdev.c
>>> @@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
>>> 	ns->nsim_dev = nsim_dev;
>>> 	ns->nsim_dev_port = nsim_dev_port;
>>> 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
>>> +	ns->peer = NULL;
>>> 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
>>> 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
>>> 	nsim_ethtool_init(ns);
>>> @@ -409,6 +410,10 @@ void nsim_destroy(struct netdevsim *ns)
>>> 	struct net_device *dev = ns->netdev;
>>>
>>> 	rtnl_lock();
>>> +	if (ns->peer) {
>>> +		ns->peer->peer = NULL;
>>> +		ns->peer = NULL;
>> 
>> What is this good for?
>
>I want to make sure when a netdevsim is removed, its peer does not
>forward skbs anymore.

I mean "ns->peer = NULL;"


>
>> 
>> 
>>> +	}
>>> 	unregister_netdevice(dev);
>>> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
>>> 		nsim_macsec_teardown(ns);
>>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>> index 028c825b86db..ac7b34a83585 100644
>>> --- a/drivers/net/netdevsim/netdevsim.h
>>> +++ b/drivers/net/netdevsim/netdevsim.h
>>> @@ -125,6 +125,7 @@ struct netdevsim {
>>> 	} udp_ports;
>>>
>>> 	struct nsim_ethtool ethtool;
>>> +	struct netdevsim *peer;
>>> };
>>>
>>> struct netdevsim *
>>> @@ -417,3 +418,5 @@ struct nsim_bus_dev {
>>>
>>> int nsim_bus_init(void);
>>> void nsim_bus_exit(void);
>>> +
>>> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id);
>>> -- 
>>> 2.39.3
>>>
>>>