netdev - Re: [PATCH net-next v3 2/4] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201116164715.163197b7@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 16 Nov 2020 16:47:15 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Antonio Cardace <acardace@...hat.com>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Michal Kubecek <mkubecek@...e.cz>
Subject: Re: [PATCH net-next v3 2/4] netdevsim: support ethtool ring and
 coalesce settings

On Sat, 14 Nov 2020 00:16:53 +0100 Antonio Cardace wrote:
> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
> index f1884d90a876..169154dba0cc 100644
> --- a/drivers/net/netdevsim/ethtool.c
> +++ b/drivers/net/netdevsim/ethtool.c
> @@ -13,9 +13,9 @@ nsim_get_pause_stats(struct net_device *dev,
>  {
>  	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 +25,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,28 +37,88 @@ 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;
> +}

Please separate the refactoring / moving pauseparam into another struct 
out to its own patch. This makes review easier.

> +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;
>  }
>  
>  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,
> +	.get_pause_stats	        = nsim_get_pause_stats,
> +	.get_pauseparam		        = nsim_get_pauseparam,
> +	.set_pauseparam		        = nsim_set_pauseparam,
> +	.supported_coalesce_params	= ETHTOOL_COALESCE_ALL_PARAMS,

Please make this member first. I think that's what all drivers do.

> +	.set_coalesce			= nsim_set_coalesce,
> +	.get_coalesce			= nsim_get_coalesce,
> +	.get_ringparam			= nsim_get_ringparam,
> +	.set_ringparam			= nsim_set_ringparam,
>  };