Re: [PATCH v2 14/17] octeontx2-pf: Add basic ethtool support

[Michal Kubecek <mkubecek@suse.cz>]

On Tue, Jan 14, 2020 at 12:32:17PM +0530, sunil.kovvuri@gmail.com wrote:
> From: Christina Jacob <cjacob@marvell.com>
> 
> This patch adds ethtool support for
>  - Driver stats, Tx/Rx perqueue and CGX LMAC stats
>  - Set/show Rx/Tx queue count
>  - Set/show Rx/Tx ring sizes
>  - Set/show IRQ coalescing parameters
> 
> Signed-off-by: Christina Jacob <cjacob@marvell.com>
> Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
> ---
[...]
> +static void otx2_dev_open(struct net_device *netdev)
> +{
> +	otx2_open(netdev);
> +}
> +
> +static void otx2_dev_stop(struct net_device *netdev)
> +{
> +	otx2_stop(netdev);
> +}

Why don't you call these directly?

[...]
> +/* Get no of queues device supports and current queue count */
> +static void otx2_get_channels(struct net_device *dev,
> +			      struct ethtool_channels *channel)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(dev);
> +
> +	memset(channel, 0, sizeof(*channel));

The structure is already zero initialized in ethtool_get_channels()
(except for cmd).

> +	channel->max_rx = pfvf->hw.max_queues;
> +	channel->max_tx = pfvf->hw.max_queues;
> +
> +	channel->rx_count = pfvf->hw.rx_queues;
> +	channel->tx_count = pfvf->hw.tx_queues;
> +}
> +
> +/* Set no of Tx, Rx queues to be used */
> +static int otx2_set_channels(struct net_device *dev,
> +			     struct ethtool_channels *channel)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(dev);
> +	bool if_up = netif_running(dev);
> +	int err = 0;
> +
> +	if (!channel->rx_count || !channel->tx_count)
> +		return -EINVAL;
> +	if (channel->rx_count > pfvf->hw.max_queues)
> +		return -EINVAL;
> +	if (channel->tx_count > pfvf->hw.max_queues)
> +		return -EINVAL;

The upper bounds are checked in ethtool_set_channels() so that you don't
get here if requested counts are too high.

> +
> +	if (if_up)
> +		otx2_dev_stop(dev);
> +
> +	pfvf->hw.rx_queues = channel->rx_count;
> +	pfvf->hw.tx_queues = channel->tx_count;
> +	err = otx2_set_real_num_queues(dev, pfvf->hw.tx_queues,
> +				       pfvf->hw.rx_queues);
> +	pfvf->qset.cq_cnt = pfvf->hw.tx_queues +  pfvf->hw.rx_queues;
> +	if (err)
> +		return err;
> +
> +	if (if_up)
> +		otx2_dev_open(dev);

Is it intentional that you leave the device down when the change fails?

> +	netdev_info(dev, "Setting num Tx rings to %d, Rx rings to %d success\n",
> +		    pfvf->hw.tx_queues, pfvf->hw.rx_queues);
> +
> +	return err;
> +}
> +
> +static void otx2_get_ringparam(struct net_device *netdev,
> +			       struct ethtool_ringparam *ring)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(netdev);
> +	struct otx2_qset *qs = &pfvf->qset;
> +
> +	ring->rx_max_pending = Q_COUNT(Q_SIZE_MAX);
> +	ring->rx_pending = qs->rqe_cnt ? qs->rqe_cnt : Q_COUNT(Q_SIZE_256);
> +	ring->tx_max_pending = Q_COUNT(Q_SIZE_MAX);
> +	ring->tx_pending = qs->sqe_cnt ? qs->sqe_cnt : Q_COUNT(Q_SIZE_4K);
> +}
> +
> +static int otx2_set_ringparam(struct net_device *netdev,
> +			      struct ethtool_ringparam *ring)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(netdev);
> +	bool if_up = netif_running(netdev);
> +	struct otx2_qset *qs = &pfvf->qset;
> +	u32 rx_count, tx_count;
> +
> +	if (ring->rx_mini_pending || ring->rx_jumbo_pending)
> +		return -EINVAL;
> +
> +	/* Permitted lengths are 16 64 256 1K 4K 16K 64K 256K 1M  */
> +	rx_count = clamp_t(u32, ring->rx_pending,
> +			   Q_COUNT(Q_SIZE_MIN), Q_COUNT(Q_SIZE_MAX));

The upper bound is checked in ethtool_set_ringparam().

Michal Kubecek

> +	/* On some silicon variants a skid or reserved CQEs are
> +	 * needed to avoid CQ overflow.
> +	 */
> +	if (rx_count < pfvf->hw.rq_skid)
> +		rx_count =  pfvf->hw.rq_skid;
> +	rx_count = Q_COUNT(Q_SIZE(rx_count, 3));
> +
> +	/* Due pipelining impact minimum 2000 unused SQ CQE's
> +	 * need to maintain to avoid CQ overflow, hence the
> +	 * minimum 4K size.
> +	 */
> +	tx_count = clamp_t(u32, ring->tx_pending,
> +			   Q_COUNT(Q_SIZE_4K), Q_COUNT(Q_SIZE_MAX));
> +	tx_count = Q_COUNT(Q_SIZE(tx_count, 3));
> +
> +	if (tx_count == qs->sqe_cnt && rx_count == qs->rqe_cnt)
> +		return 0;
> +
> +	if (if_up)
> +		otx2_dev_stop(netdev);
> +
> +	/* Assigned to the nearest possible exponent. */
> +	qs->sqe_cnt = tx_count;
> +	qs->rqe_cnt = rx_count;
> +
> +	if (if_up)
> +		otx2_dev_open(netdev);
> +	return 0;
> +}