| Message ID | 20231207172117.3671183-2-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdevsim: link and forward skbs between | expand |
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 > >
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?
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 >> >>
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.
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 >>> >>>
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);
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(+)