Re: [net-next 13/15] ice: Implement ethtool ops for channels

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Wed, 20 Nov 2019 23:46:10 -0800, Jeff Kirsher wrote:
> +	curr_combined = ice_get_combined_cnt(vsi);
> +
> +	/* these checks are for cases where user didn't specify a particular
> +	 * value on cmd line but we get non-zero value anyway via
> +	 * get_channels(); look at ethtool.c in ethtool repository (the user
> +	 * space part), particularly, do_schannels() routine
> +	 */
> +	if (ch->rx_count == vsi->num_rxq - curr_combined)
> +		ch->rx_count = 0;
> +	if (ch->tx_count == vsi->num_txq - curr_combined)
> +		ch->tx_count = 0;
> +	if (ch->combined_count == curr_combined)
> +		ch->combined_count = 0;
> +
> +	if (!(ch->combined_count || (ch->rx_count && ch->tx_count))) {
> +		netdev_err(dev, "Please specify at least 1 Rx and 1 Tx channel\n");
> +		return -EINVAL;
> +	}
> +
> +	new_rx = ch->combined_count + ch->rx_count;
> +	new_tx = ch->combined_count + ch->tx_count;

The combined vs individual count logic looks correct to me which is not
common, so nice to see that!

> +	if (new_rx > ice_get_max_rxq(pf)) {
> +		netdev_err(dev, "Maximum allowed Rx channels is %d\n",
> +			   ice_get_max_rxq(pf));
> +		return -EINVAL;
> +	}
> +	if (new_tx > ice_get_max_txq(pf)) {
> +		netdev_err(dev, "Maximum allowed Tx channels is %d\n",
> +			   ice_get_max_txq(pf));
> +		return -EINVAL;
> +	}
> +
> +	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
> +
> +	if (new_rx)
> +		return ice_vsi_set_dflt_rss_lut(vsi, new_rx);

But I don't see you doing a netif_is_rxfh_configured() check, which is
supposed to prevent reconfiguring the RSS indirection table if it was
set up manually by the user.

This is doubly sad, because I believe that part of RSS infrastructure
was added by Jake Keller from Intel, so it should be caught by your
internal review, not to say tests.. :(

> +	return 0;
> +}