netdev - Re: [PATCH V2 net-next 5/5] net: ena: ethtool: support set

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: <20191002131132.7b81f339@cakuba.hsd1.ca.comcast.net>
Date:   Wed, 2 Oct 2019 13:11:32 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     <sameehj@...zon.com>
Cc:     <davem@...emloft.net>, <netdev@...r.kernel.org>, <dwmw@...zon.com>,
        <zorik@...zon.com>, <matua@...zon.com>, <saeedb@...zon.com>,
        <msw@...zon.com>, <aliguori@...zon.com>, <nafea@...zon.com>,
        <gtzalik@...zon.com>, <netanel@...zon.com>, <alisaidi@...zon.com>,
        <benh@...zon.com>, <akiyano@...zon.com>
Subject: Re: [PATCH V2 net-next 5/5] net: ena: ethtool: support set_channels
 callback

On Wed, 2 Oct 2019 11:20:52 +0300, sameehj@...zon.com wrote:
> From: Sameeh Jubran <sameehj@...zon.com>
> 
> Set channels callback enables the user to change the count of queues
> used by the driver using ethtool. We decided to currently support only
> equal number of rx and tx queues, this might change in the future.
> 
> Also rename dev_up to dev_was_up in ena_update_queue_count() to make
> it clearer.
> 
> Signed-off-by: Sameeh Jubran <sameehj@...zon.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_ethtool.c | 17 ++++++++++++++
>  drivers/net/ethernet/amazon/ena/ena_netdev.c  | 22 ++++++++++++++++---
>  drivers/net/ethernet/amazon/ena/ena_netdev.h  |  3 +++
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> index c9d760465..f58fc3c68 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> @@ -744,6 +744,22 @@ static void ena_get_channels(struct net_device *netdev,
>  	channels->combined_count = 0;
>  }
>  
> +static int ena_set_channels(struct net_device *netdev,
> +			    struct ethtool_channels *channels)
> +{
> +	struct ena_adapter *adapter = netdev_priv(netdev);
> +	u32 new_channel_count;
> +
> +	if (channels->rx_count != channels->tx_count ||

If you use the same IRQ and NAPI to service RX and TX it's a combined
channel, not rx and tx channels.

> +	    channels->max_tx != channels->max_rx)

Hm.. maxes are generally ignored on set 🤔

> +		return -EINVAL;
> +
> +	new_channel_count = clamp_val(channels->tx_count,
> +				      ENA_MIN_NUM_IO_QUEUES, channels->max_tx);

You should return an error if the value is not within bounds, rather
than guessing.

> +	return ena_update_queue_count(adapter, new_channel_count);
> +}
> +
>  static int ena_get_tunable(struct net_device *netdev,
>  			   const struct ethtool_tunable *tuna, void *data)
>  {
> @@ -807,6 +823,7 @@ static const struct ethtool_ops ena_ethtool_ops = {
>  	.get_rxfh		= ena_get_rxfh,
>  	.set_rxfh		= ena_set_rxfh,
>  	.get_channels		= ena_get_channels,
> +	.set_channels		= ena_set_channels,
>  	.get_tunable		= ena_get_tunable,
>  	.set_tunable		= ena_set_tunable,
>  };
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index e964783c4..7d44b3440 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -2044,14 +2044,30 @@ int ena_update_queue_sizes(struct ena_adapter *adapter,
>  			   u32 new_tx_size,
>  			   u32 new_rx_size)
>  {
> -	bool dev_up;
> +	bool dev_was_up;
>  
> -	dev_up = test_bit(ENA_FLAG_DEV_UP, &adapter->flags);
> +	dev_was_up = test_bit(ENA_FLAG_DEV_UP, &adapter->flags);
>  	ena_close(adapter->netdev);
>  	adapter->requested_tx_ring_size = new_tx_size;
>  	adapter->requested_rx_ring_size = new_rx_size;
>  	ena_init_io_rings(adapter);
> -	return dev_up ? ena_up(adapter) : 0;
> +	return dev_was_up ? ena_up(adapter) : 0;
> +}
> +
> +int ena_update_queue_count(struct ena_adapter *adapter, u32 new_channel_count)
> +{
> +	struct ena_com_dev *ena_dev = adapter->ena_dev;
> +	bool dev_was_up;
> +
> +	dev_was_up = test_bit(ENA_FLAG_DEV_UP, &adapter->flags);
> +	ena_close(adapter->netdev);
> +	adapter->num_io_queues = new_channel_count;
> +       /* We need to destroy the rss table so that the indirection
> +	* table will be reinitialized by ena_up()
> +	*/
> +	ena_com_rss_destroy(ena_dev);
> +	ena_init_io_rings(adapter);
> +	return dev_was_up ? ena_open(adapter->netdev) : 0;

You should try to prepare the resources for the new configuration
before you attempt the change. Otherwise if allocation of new rings
fails the open will leave the device in a broken state.

This is not always enforced upstream, but you can see mlx5 or nfp for
examples of drivers which do this right..

>  }
>  
>  static void ena_tx_csum(struct ena_com_tx_ctx *ena_tx_ctx, struct sk_buff *skb)