diff mbox series

[net-next,08/12] amd-xgbe: Add ethtool show/set channels support

Message ID 20180521215937.8135.63942.stgit@tlendack-t1.amdoffice.net
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series amd-xgbe: AMD XGBE driver updates 2018-05-21 | expand

Commit Message

Tom Lendacky May 21, 2018, 9:59 p.m. UTC 
Add ethtool support to show and set the device channel configuration.
Changing the channel configuration will result in a device restart.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c     |   25 +++++
 drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c |  131 ++++++++++++++++++++++++++
 drivers/net/ethernet/amd/xgbe/xgbe.h         |    4 +
 3 files changed, 160 insertions(+)

Comments

Jakub Kicinski May 22, 2018, 5:35 a.m. UTC | #1 
On Mon, 21 May 2018 16:59:37 -0500, Tom Lendacky wrote:
> +	rx = combined + channels->rx_count;
> +	tx = combined + channels->tx_count;
> +	netdev_notice(netdev, "final channel count assignment: combined=%u, rx-only=%u, tx-only=%u\n",
> +		      min(rx, tx), rx - min(rx, tx), tx - min(rx, tx));

If user requests combined 0 rx 8 tx 8 they will end up with combined 8
rx 0 tx 0.  Is that expected?

The man page clearly sayeth:

       -L --set-channels
              Changes the numbers of channels of the specified network device.

           rx N   Changes the number of channels with only receive queues.

           tx N   Changes the number of channels with only transmit queues.

           other N
                  Changes the number of channels used only for other  purposes
                  e.g. link interrupts or SR-IOV co-ordination.

           combined N
                  Changes the number of multi-purpose channels.

Note the use of word *only*.  There are drivers in tree which adhere to
this interpretation and dutifully allocate separate IRQs if RX and TX
channels are requested separately.

Which is not to claim that majority of existing drivers adhere to this
wording :)
Tom Lendacky May 22, 2018, 1:24 p.m. UTC | #2 
On 5/22/2018 12:35 AM, Jakub Kicinski wrote:
> On Mon, 21 May 2018 16:59:37 -0500, Tom Lendacky wrote:
>> +	rx = combined + channels->rx_count;
>> +	tx = combined + channels->tx_count;
>> +	netdev_notice(netdev, "final channel count assignment: combined=%u, rx-only=%u, tx-only=%u\n",
>> +		      min(rx, tx), rx - min(rx, tx), tx - min(rx, tx));
> 
> If user requests combined 0 rx 8 tx 8 they will end up with combined 8
> rx 0 tx 0.  Is that expected?

Yes, which is the reason that I issue the final channel count message. I
debated on how to do all this and looked at other drivers as well as the
ethtool man page and decided on this logic.

> 
> The man page clearly sayeth:
> 
>        -L --set-channels
>               Changes the numbers of channels of the specified network device.
> 
>            rx N   Changes the number of channels with only receive queues.
> 
>            tx N   Changes the number of channels with only transmit queues.
> 
>            other N
>                   Changes the number of channels used only for other  purposes
>                   e.g. link interrupts or SR-IOV co-ordination.
> 
>            combined N
>                   Changes the number of multi-purpose channels.
> 
> Note the use of word *only*.  There are drivers in tree which adhere to
> this interpretation and dutifully allocate separate IRQs if RX and TX
> channels are requested separately.

The amd-xgbe driver is not designed to allocate separate IRQs for Rx and
Tx.  In general, there is one IRQ for a channel of which Tx and Rx are
shared.  You can have more Tx channels than Rx channels or vice-versa, but
the min() of those numbers will be a combined Tx/Rx with the excess being
Tx or Rx only: e.g. combined 0 tx 8 rx 10 results in 8 combined channels
plus two Rx only channels.

I thought this was the most reasonable way to do this, please let me know
if there's a strong objection to this.

Thanks,
Tom

> 
> Which is not to claim that majority of existing drivers adhere to this
> wording :)
>
Edward Cree May 22, 2018, 1:29 p.m. UTC | #3 
On 22/05/18 14:24, Tom Lendacky wrote:
> The amd-xgbe driver is not designed to allocate separate IRQs for Rx and
> Tx.  In general, there is one IRQ for a channel of which Tx and Rx are
> shared.  You can have more Tx channels than Rx channels or vice-versa, but
> the min() of those numbers will be a combined Tx/Rx with the excess being
> Tx or Rx only: e.g. combined 0 tx 8 rx 10 results in 8 combined channels
> plus two Rx only channels.
If you cannot allocate the channels requested by the user, surely the correct
 thing is not to fudge it into something similar, but rather to return an
 error from the ethtool set_channels() op.

-Ed
Tom Lendacky May 22, 2018, 3:37 p.m. UTC | #4 
On 5/22/2018 8:29 AM, Edward Cree wrote:
> On 22/05/18 14:24, Tom Lendacky wrote:
>> The amd-xgbe driver is not designed to allocate separate IRQs for Rx and
>> Tx.  In general, there is one IRQ for a channel of which Tx and Rx are
>> shared.  You can have more Tx channels than Rx channels or vice-versa, but
>> the min() of those numbers will be a combined Tx/Rx with the excess being
>> Tx or Rx only: e.g. combined 0 tx 8 rx 10 results in 8 combined channels
>> plus two Rx only channels.
> If you cannot allocate the channels requested by the user, surely the correct
>  thing is not to fudge it into something similar, but rather to return an
>  error from the ethtool set_channels() op.

Ok, another vote on changing the logic.  I'll update it and submit a v2.

Thanks,
Tom

> 
> -Ed
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 397e3a0..24f1053 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -1329,6 +1329,17 @@  static int xgbe_alloc_memory(struct xgbe_prv_data *pdata)
 	struct net_device *netdev = pdata->netdev;
 	int ret;
 
+	if (pdata->new_tx_ring_count) {
+		pdata->tx_ring_count = pdata->new_tx_ring_count;
+		pdata->tx_q_count = pdata->tx_ring_count;
+		pdata->new_tx_ring_count = 0;
+	}
+
+	if (pdata->new_rx_ring_count) {
+		pdata->rx_ring_count = pdata->new_rx_ring_count;
+		pdata->new_rx_ring_count = 0;
+	}
+
 	/* Calculate the Rx buffer size before allocating rings */
 	pdata->rx_buf_size = xgbe_calc_rx_buf_size(netdev, netdev->mtu);
 
@@ -1482,6 +1493,20 @@  static void xgbe_stopdev(struct work_struct *work)
 	netdev_alert(pdata->netdev, "device stopped\n");
 }
 
+void xgbe_full_restart_dev(struct xgbe_prv_data *pdata)
+{
+	/* If not running, "restart" will happen on open */
+	if (!netif_running(pdata->netdev))
+		return;
+
+	xgbe_stop(pdata);
+
+	xgbe_free_memory(pdata);
+	xgbe_alloc_memory(pdata);
+
+	xgbe_start(pdata);
+}
+
 void xgbe_restart_dev(struct xgbe_prv_data *pdata)
 {
 	/* If not running, "restart" will happen on open */
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
index d12f982..d26fd95 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
@@ -705,6 +705,135 @@  static int xgbe_set_ringparam(struct net_device *netdev,
 	return 0;
 }
 
+static void xgbe_get_channels(struct net_device *netdev,
+			      struct ethtool_channels *channels)
+{
+	struct xgbe_prv_data *pdata = netdev_priv(netdev);
+	unsigned int rx, tx, combined;
+
+	/* Calculate maximums allowed:
+	 *   - Take into account the number of available IRQs
+	 *   - Do not take into account the number of online CPUs so that
+	 *     the user can over-subscribe if desired
+	 *   - Tx is additionally limited by the number of hardware queues
+	 */
+	rx = min(pdata->hw_feat.rx_ch_cnt, pdata->rx_max_channel_count);
+	rx = min(rx, pdata->channel_irq_count);
+	tx = min(pdata->hw_feat.tx_ch_cnt, pdata->tx_max_channel_count);
+	tx = min(tx, pdata->channel_irq_count);
+	tx = min(tx, pdata->tx_max_q_count);
+
+	combined = min(rx, tx);
+
+	channels->max_combined = combined;
+	channels->max_rx = rx;
+	channels->max_tx = tx;
+
+	/* Current running settings */
+	rx = pdata->rx_ring_count;
+	tx = pdata->tx_ring_count;
+
+	combined = min(rx, tx);
+	rx -= combined;
+	tx -= combined;
+
+	channels->combined_count = combined;
+	channels->rx_count = rx;
+	channels->tx_count = tx;
+}
+
+static void xgbe_print_set_channels_input(struct net_device *netdev,
+					  struct ethtool_channels *channels)
+{
+	netdev_err(netdev, "channel inputs: combined=%u, rx-only=%u, tx-only=%u\n",
+		   channels->combined_count, channels->rx_count,
+		   channels->tx_count);
+}
+
+static int xgbe_set_channels(struct net_device *netdev,
+			     struct ethtool_channels *channels)
+{
+	struct xgbe_prv_data *pdata = netdev_priv(netdev);
+	unsigned int rx, tx, combined;
+
+	/* Calculate maximums allowed:
+	 *   - Take into account the number of available IRQs
+	 *   - Do not take into account the number of online CPUs so that
+	 *     the user can over-subscribe if desired
+	 *   - Tx is additionally limited by the number of hardware queues
+	 */
+	rx = min(pdata->hw_feat.rx_ch_cnt, pdata->rx_max_channel_count);
+	rx = min(rx, pdata->channel_irq_count);
+	tx = min(pdata->hw_feat.tx_ch_cnt, pdata->tx_max_channel_count);
+	tx = min(tx, pdata->tx_max_q_count);
+	tx = min(tx, pdata->channel_irq_count);
+
+	combined = min(rx, tx);
+
+	/* Should not be setting other count */
+	if (channels->other_count) {
+		netdev_err(netdev,
+			   "other channel count must be zero\n");
+		return -EINVAL;
+	}
+
+	/* Require at least one Rx and Tx channel */
+	if (!channels->combined_count) {
+		if (!channels->rx_count || !channels->tx_count) {
+			netdev_err(netdev,
+				   "at least one Rx and one Tx channel is required\n");
+			xgbe_print_set_channels_input(netdev, channels);
+			return -EINVAL;
+		}
+	}
+
+	/* Check combined channels */
+	if (channels->combined_count > combined) {
+		netdev_err(netdev,
+			   "combined channel count cannot exceed %u\n",
+			   combined);
+		xgbe_print_set_channels_input(netdev, channels);
+		return -EINVAL;
+	}
+
+	/* Check for Rx/Tx specific channels */
+	combined = channels->combined_count;
+	tx -= combined;
+	rx -= combined;
+	if (channels->rx_count > rx) {
+		netdev_err(netdev,
+			   "Rx channel count cannot exceed %u when combined channel count is %u\n",
+			   rx, combined);
+		xgbe_print_set_channels_input(netdev, channels);
+		return -EINVAL;
+	}
+
+	if (channels->tx_count > tx) {
+		netdev_err(netdev,
+			   "Tx channel count cannot exceed %u when combined channel count is %u\n",
+			   tx, combined);
+		xgbe_print_set_channels_input(netdev, channels);
+		return -EINVAL;
+	}
+
+	rx = combined + channels->rx_count;
+	tx = combined + channels->tx_count;
+	netdev_notice(netdev, "final channel count assignment: combined=%u, rx-only=%u, tx-only=%u\n",
+		      min(rx, tx), rx - min(rx, tx), tx - min(rx, tx));
+
+	if ((rx == pdata->rx_ring_count) &&
+	    (tx == pdata->tx_ring_count))
+		goto out;
+
+	pdata->new_rx_ring_count = rx;
+	pdata->new_tx_ring_count = tx;
+
+	xgbe_full_restart_dev(pdata);
+
+out:
+	return 0;
+}
+
 static const struct ethtool_ops xgbe_ethtool_ops = {
 	.get_drvinfo = xgbe_get_drvinfo,
 	.get_msglevel = xgbe_get_msglevel,
@@ -729,6 +858,8 @@  static int xgbe_set_ringparam(struct net_device *netdev,
 	.get_module_eeprom = xgbe_get_module_eeprom,
 	.get_ringparam = xgbe_get_ringparam,
 	.set_ringparam = xgbe_set_ringparam,
+	.get_channels = xgbe_get_channels,
+	.set_channels = xgbe_set_channels,
 };
 
 const struct ethtool_ops *xgbe_get_ethtool_ops(void)
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
index 7dc0fac..7a412cf 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
@@ -1122,6 +1122,9 @@  struct xgbe_prv_data {
 	unsigned int rx_ring_count;
 	unsigned int rx_desc_count;
 
+	unsigned int new_tx_ring_count;
+	unsigned int new_rx_ring_count;
+
 	unsigned int tx_max_q_count;
 	unsigned int rx_max_q_count;
 	unsigned int tx_q_count;
@@ -1336,6 +1339,7 @@  void xgbe_dump_rx_desc(struct xgbe_prv_data *, struct xgbe_ring *,
 void xgbe_init_rx_coalesce(struct xgbe_prv_data *);
 void xgbe_init_tx_coalesce(struct xgbe_prv_data *);
 void xgbe_restart_dev(struct xgbe_prv_data *pdata);
+void xgbe_full_restart_dev(struct xgbe_prv_data *pdata);
 
 #ifdef CONFIG_DEBUG_FS
 void xgbe_debugfs_init(struct xgbe_prv_data *);