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

On Tue, Jan 14, 2020 at 3:38 PM Michal Kubecek [off-list ref] wrote:

On Tue, Jan 14, 2020 at 12:32:17PM +0530, sunil.kovvuri@gmail.com wrote:

quoted

From: Christina Jacob <redacted>

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 <redacted>
Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
---

[...]

quoted

+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?

Will submit VF driver as a follow up, which will result in calling
either otx2_stop or otx2vf_stop.

[...]

quoted

+/* 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).

Will fix.

quoted

+     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.

Will fix.

quoted

+
+     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?

quoted

+     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

Will fix.
Thanks for the feedback.

Sunil.