Re: [PATCH V1 net-next 5/6] net: ena: add ethtool function for changing io queue sizes

[Michal Kubecek <mkubecek@suse.cz>]

On Mon, Jun 10, 2019 at 07:46:08AM +0000, Jubran, Samih wrote:
> > -----Original Message-----
> > From: Michal Kubecek <mkubecek@suse.cz>
> > Sent: Thursday, June 6, 2019 5:48 PM
> > To: netdev@vger.kernel.org
> > Cc: Jubran, Samih <sameehj@amazon.com>; davem@davemloft.net;
> > Woodhouse, David <dwmw@amazon.co.uk>; Machulsky, Zorik
> > <zorik@amazon.com>; Matushevsky, Alexander <matua@amazon.com>;
> > Bshara, Saeed <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>;
> > Liguori, Anthony <aliguori@amazon.com>; Bshara, Nafea
> > <nafea@amazon.com>; Tzalik, Guy <gtzalik@amazon.com>; Belgazal,
> > Netanel <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>;
> > Herrenschmidt, Benjamin <benh@amazon.com>; Kiyanovski, Arthur
> > <akiyano@amazon.com>
> > Subject: Re: [PATCH V1 net-next 5/6] net: ena: add ethtool function for
> > changing io queue sizes
> > 
> > On Thu, Jun 06, 2019 at 02:55:19PM +0300, sameehj@amazon.com wrote:
> > > From: Sameeh Jubran <sameehj@amazon.com>
> > >
> > > Implement the set_ringparam() function of the ethtool interface to
> > > enable the changing of io queue sizes.
> > >
> > > Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> > > Signed-off-by: Sameeh Jubran <sameehj@amazon.com>
> > > ---
> > >  drivers/net/ethernet/amazon/ena/ena_ethtool.c | 25
> > > +++++++++++++++++++
> > drivers/net/ethernet/amazon/ena/ena_netdev.c  |
> > > 14 +++++++++++  drivers/net/ethernet/amazon/ena/ena_netdev.h  |  5
> > > +++-
> > >  3 files changed, 43 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > > b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > > index 101d93f16..33e28ad71 100644
> > > --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > > +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> > > @@ -495,6 +495,30 @@ static void ena_get_ringparam(struct net_device
> > *netdev,
> > >  	ring->rx_pending = adapter->rx_ring[0].ring_size;  }
> > >
> > > +static int ena_set_ringparam(struct net_device *netdev,
> > > +			     struct ethtool_ringparam *ring) {
> > > +	struct ena_adapter *adapter = netdev_priv(netdev);
> > > +	u32 new_tx_size, new_rx_size;
> > > +
> > > +	if (ring->rx_mini_pending || ring->rx_jumbo_pending)
> > > +		return -EINVAL;
> > 
> > This check is superfluous as ethtool_set_ringparam() checks supplied values
> > against maximum returned by ->get_ringparam() which will be 0 in this case.
> > 
> > > +
> > > +	new_tx_size = clamp_val(ring->tx_pending, ENA_MIN_RING_SIZE,
> > > +				adapter->max_tx_ring_size);
> > > +	new_tx_size = rounddown_pow_of_two(new_tx_size);
> > > +
> > > +	new_rx_size = clamp_val(ring->rx_pending, ENA_MIN_RING_SIZE,
> > > +				adapter->max_rx_ring_size);
> > > +	new_rx_size = rounddown_pow_of_two(new_rx_size);
> > 
> > For the same reason, clamping from below would suffice here.
> > 
> > Michal Kubecek
> > 
> > > +
> > > +	if (new_tx_size == adapter->requested_tx_ring_size &&
> > > +	    new_rx_size == adapter->requested_rx_ring_size)
> > > +		return 0;
> > > +
> > > +	return ena_update_queue_sizes(adapter, new_tx_size,
> > new_rx_size); }
> 
> You are right with both arguments the way the code is written now,
> however, in the future the code might change and we prefer to be extra
> cautious.

If we accept this logic, commit 37e2d99b59c4 ("ethtool: Ensure new ring
parameters are within bounds during SRINGPARAM") would be useless as
every driver would have to duplicate the checks anyway.

Michal Kubecek