netdev - Re: [PATCH net-next 1/1] netdevsim: support ethtool ring and coalesce settings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201113141507.z4gehzlddi6eyggm@yoda.fritz.box>
Date:   Fri, 13 Nov 2020 15:15:07 +0100
From:   Antonio Cardace <acardace@...hat.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/1] netdevsim: support ethtool ring and
 coalesce settings

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@...hat.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