| Message ID | 20201112151229.1288504-1-acardace@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,1/1] netdevsim: support ethtool ring and coalesce settings | expand |
On Thu, 12 Nov 2020 16:12:29 +0100 Antonio Cardace wrote: > Add ethtool ring and coalesce settings support for testing. > > Signed-off-by: Antonio Cardace <acardace@redhat.com> Please add a test to tools/testing/.../netdevsim/ We don't add functionality to netdevsim unless there is a in-tree test that exercises it.
On Thu, Nov 12, 2020 at 04:12:29PM +0100, Antonio Cardace wrote: > Add ethtool ring and coalesce settings support for testing. > > Signed-off-by: Antonio Cardace <acardace@redhat.com> > --- > drivers/net/netdevsim/ethtool.c | 65 +++++++++++++++++++++++++++---- > drivers/net/netdevsim/netdevsim.h | 9 ++++- > 2 files changed, 65 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c > index f1884d90a876..25acd3bc1781 100644 > --- a/drivers/net/netdevsim/ethtool.c > +++ b/drivers/net/netdevsim/ethtool.c > @@ -7,15 +7,18 @@ > > #include "netdevsim.h" > > +#define UINT32_MAX 0xFFFFFFFFU We already have U32_MAX in <linux/limits.h> > +#define ETHTOOL_COALESCE_ALL_PARAMS UINT32_MAX I would rather prefer this constant to include only bits corresponding to parameters which actually exist, i.e. either GENMASK(21, 0) or combination of existing ETHTOOL_COALESCE_* macros. It should probably be defined in include/linux/ethtool.h then. [...] > +static void nsim_get_ringparam(struct net_device *dev, struct ethtool_ringparam *ring) > +{ > + struct netdevsim *ns = netdev_priv(dev); > + > + memcpy(ring, &ns->ethtool.ring, sizeof(ns->ethtool.ring)); > +} > + > +static int nsim_set_ringparam(struct net_device *dev, struct ethtool_ringparam *ring) > +{ > + struct netdevsim *ns = netdev_priv(dev); > + > + memcpy(&ns->ethtool.ring, ring, sizeof(ns->ethtool.ring)); > return 0; > } [...] > > +static void nsim_ethtool_coalesce_init(struct netdevsim *ns) > +{ > + ns->ethtool.ring.rx_max_pending = UINT32_MAX; > + ns->ethtool.ring.rx_jumbo_max_pending = UINT32_MAX; > + ns->ethtool.ring.rx_mini_max_pending = UINT32_MAX; > + ns->ethtool.ring.tx_max_pending = UINT32_MAX; > +} This way an ETHTOOL_MSG_RINGS_SET request would never fail. It would be more useful for selftests if the max values were more realistic and ideally also configurable via debugfs. Michal
On Fri, Nov 13, 2020 at 12:45:22PM +0100, Michal Kubecek wrote: > On Thu, Nov 12, 2020 at 04:12:29PM +0100, Antonio Cardace wrote: > > Add ethtool ring and coalesce settings support for testing. > > > > Signed-off-by: Antonio Cardace <acardace@redhat.com> > > --- > > drivers/net/netdevsim/ethtool.c | 65 +++++++++++++++++++++++++++---- > > drivers/net/netdevsim/netdevsim.h | 9 ++++- > > 2 files changed, 65 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c > > index f1884d90a876..25acd3bc1781 100644 > > --- a/drivers/net/netdevsim/ethtool.c > > +++ b/drivers/net/netdevsim/ethtool.c > > @@ -7,15 +7,18 @@ > > > > #include "netdevsim.h" > > > > +#define UINT32_MAX 0xFFFFFFFFU > > We already have U32_MAX in <linux/limits.h> > > > +#define ETHTOOL_COALESCE_ALL_PARAMS UINT32_MAX > > I would rather prefer this constant to include only bits corresponding > to parameters which actually exist, i.e. either GENMASK(21, 0) or > combination of existing ETHTOOL_COALESCE_* macros. It should probably > be defined in include/linux/ethtool.h then. > > [...] > > +static void nsim_get_ringparam(struct net_device *dev, struct ethtool_ringparam *ring) > > +{ > > + struct netdevsim *ns = netdev_priv(dev); > > + > > + memcpy(ring, &ns->ethtool.ring, sizeof(ns->ethtool.ring)); > > +} > > + > > +static int nsim_set_ringparam(struct net_device *dev, struct ethtool_ringparam *ring) > > +{ > > + struct netdevsim *ns = netdev_priv(dev); > > + > > + memcpy(&ns->ethtool.ring, ring, sizeof(ns->ethtool.ring)); > > return 0; > > } > [...] > > > > +static void nsim_ethtool_coalesce_init(struct netdevsim *ns) > > +{ > > + ns->ethtool.ring.rx_max_pending = UINT32_MAX; > > + ns->ethtool.ring.rx_jumbo_max_pending = UINT32_MAX; > > + ns->ethtool.ring.rx_mini_max_pending = UINT32_MAX; > > + ns->ethtool.ring.tx_max_pending = UINT32_MAX; > > +} > > This way an ETHTOOL_MSG_RINGS_SET request would never fail. It would be > more useful for selftests if the max values were more realistic and > ideally also configurable via debugfs. > > Michal > Thanks Michal and Jakub, I will send a v2 patch that addresses the comments you made. Antonio
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c index f1884d90a876..25acd3bc1781 100644 --- a/drivers/net/netdevsim/ethtool.c +++ b/drivers/net/netdevsim/ethtool.c @@ -7,15 +7,18 @@ #include "netdevsim.h" +#define UINT32_MAX 0xFFFFFFFFU +#define ETHTOOL_COALESCE_ALL_PARAMS UINT32_MAX + static void nsim_get_pause_stats(struct net_device *dev, struct ethtool_pause_stats *pause_stats) { struct netdevsim *ns = netdev_priv(dev); - if (ns->ethtool.report_stats_rx) + if (ns->ethtool.pauseparam.report_stats_rx) pause_stats->rx_pause_frames = 1; - if (ns->ethtool.report_stats_tx) + if (ns->ethtool.pauseparam.report_stats_tx) pause_stats->tx_pause_frames = 2; } @@ -25,8 +28,8 @@ nsim_get_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause) struct netdevsim *ns = netdev_priv(dev); pause->autoneg = 0; /* We don't support ksettings, so can't pretend */ - pause->rx_pause = ns->ethtool.rx; - pause->tx_pause = ns->ethtool.tx; + pause->rx_pause = ns->ethtool.pauseparam.rx; + pause->tx_pause = ns->ethtool.pauseparam.tx; } static int @@ -37,8 +40,39 @@ nsim_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause) if (pause->autoneg) return -EINVAL; - ns->ethtool.rx = pause->rx_pause; - ns->ethtool.tx = pause->tx_pause; + ns->ethtool.pauseparam.rx = pause->rx_pause; + ns->ethtool.pauseparam.tx = pause->tx_pause; + return 0; +} + +static int nsim_get_coalesce(struct net_device *dev, struct ethtool_coalesce *coal) +{ + struct netdevsim *ns = netdev_priv(dev); + + memcpy(coal, &ns->ethtool.coalesce, sizeof(ns->ethtool.coalesce)); + return 0; +} + +static int nsim_set_coalesce(struct net_device *dev, struct ethtool_coalesce *coal) +{ + struct netdevsim *ns = netdev_priv(dev); + + memcpy(&ns->ethtool.coalesce, coal, sizeof(ns->ethtool.coalesce)); + return 0; +} + +static void nsim_get_ringparam(struct net_device *dev, struct ethtool_ringparam *ring) +{ + struct netdevsim *ns = netdev_priv(dev); + + memcpy(ring, &ns->ethtool.ring, sizeof(ns->ethtool.ring)); +} + +static int nsim_set_ringparam(struct net_device *dev, struct ethtool_ringparam *ring) +{ + struct netdevsim *ns = netdev_priv(dev); + + memcpy(&ns->ethtool.ring, ring, sizeof(ns->ethtool.ring)); return 0; } @@ -46,19 +80,34 @@ static const struct ethtool_ops nsim_ethtool_ops = { .get_pause_stats = nsim_get_pause_stats, .get_pauseparam = nsim_get_pauseparam, .set_pauseparam = nsim_set_pauseparam, + .supported_coalesce_params = ETHTOOL_COALESCE_ALL_PARAMS, + .set_coalesce = nsim_set_coalesce, + .get_coalesce = nsim_get_coalesce, + .get_ringparam = nsim_get_ringparam, + .set_ringparam = nsim_set_ringparam, }; +static void nsim_ethtool_coalesce_init(struct netdevsim *ns) +{ + ns->ethtool.ring.rx_max_pending = UINT32_MAX; + ns->ethtool.ring.rx_jumbo_max_pending = UINT32_MAX; + ns->ethtool.ring.rx_mini_max_pending = UINT32_MAX; + ns->ethtool.ring.tx_max_pending = UINT32_MAX; +} + void nsim_ethtool_init(struct netdevsim *ns) { struct dentry *ethtool, *dir; ns->netdev->ethtool_ops = &nsim_ethtool_ops; + nsim_ethtool_coalesce_init(ns); + ethtool = debugfs_create_dir("ethtool", ns->nsim_dev_port->ddir); dir = debugfs_create_dir("pause", ethtool); debugfs_create_bool("report_stats_rx", 0600, dir, - &ns->ethtool.report_stats_rx); + &ns->ethtool.pauseparam.report_stats_rx); debugfs_create_bool("report_stats_tx", 0600, dir, - &ns->ethtool.report_stats_tx); + &ns->ethtool.pauseparam.report_stats_tx); } diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h index 827fc80f50a0..b023dc0a4259 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -15,6 +15,7 @@ #include <linux/debugfs.h> #include <linux/device.h> +#include <linux/ethtool.h> #include <linux/kernel.h> #include <linux/list.h> #include <linux/netdevice.h> @@ -51,13 +52,19 @@ struct nsim_ipsec { u32 ok; }; -struct nsim_ethtool { +struct nsim_ethtool_pauseparam { bool rx; bool tx; bool report_stats_rx; bool report_stats_tx; }; +struct nsim_ethtool { + struct nsim_ethtool_pauseparam pauseparam; + struct ethtool_coalesce coalesce; + struct ethtool_ringparam ring; +}; + struct netdevsim { struct net_device *netdev; struct nsim_dev *nsim_dev;
Add ethtool ring and coalesce settings support for testing. Signed-off-by: Antonio Cardace <acardace@redhat.com> --- drivers/net/netdevsim/ethtool.c | 65 +++++++++++++++++++++++++++---- drivers/net/netdevsim/netdevsim.h | 9 ++++- 2 files changed, 65 insertions(+), 9 deletions(-)