[PATCH v2] enic: add ethtool get_channel support

[PATCH v2] enic: add ethtool get_channel support

[Jon Kohler]

Add .get_channel to enic_ethtool_ops to enable basic ethtool -l
support to get the current channel configuration.

Note that the driver does not support dynamically changing queue
configuration, so .set_channel is intentionally unused. Instead, users
should use Cisco's hardware management tools (UCSM/IMC) to modify
virtual interface card configuration out of band.

Signed-off-by: Jon Kohler <jon@nutanix.com>
---
v1
- https://lore.kernel.org/netdev/20240618160146.3900470-1-jon@nutanix.com/T/#u
v1 -> v2:
- Addressed comments from Przemek and Jakub
---
 .../net/ethernet/cisco/enic/enic_ethtool.c    | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 241906697019..54f542238b4e 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -608,6 +608,32 @@ static int enic_get_ts_info(struct net_device *netdev,
 	return 0;
 }
 
+static void enic_get_channels(struct net_device *netdev,
+			      struct ethtool_channels *channels)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	switch (vnic_dev_get_intr_mode(enic->vdev)) {
+	case VNIC_DEV_INTR_MODE_MSIX:
+		channels->max_rx = ENIC_RQ_MAX;
+		channels->max_tx = ENIC_WQ_MAX;
+		channels->rx_count = enic->rq_count;
+		channels->tx_count = enic->wq_count;
+		break;
+	case VNIC_DEV_INTR_MODE_MSI:
+		channels->max_rx = 1;
+		channels->max_tx = 1;
+		channels->rx_count = 1;
+		channels->tx_count = 1;
+		break;
+	case VNIC_DEV_INTR_MODE_INTX:
+		channels->max_combined = 1;
+		channels->combined_count = 1;
+	default:
+		break;
+	}
+}
+
 static const struct ethtool_ops enic_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
@@ -632,6 +658,7 @@ static const struct ethtool_ops enic_ethtool_ops = {
 	.set_rxfh = enic_set_rxfh,
 	.get_link_ksettings = enic_get_ksettings,
 	.get_ts_info = enic_get_ts_info,
+	.get_channels = enic_get_channels,
 };
 
 void enic_set_ethtool_ops(struct net_device *netdev)
-- 
2.43.0

RE: [PATCH v2] enic: add ethtool get_channel support

[Sai Krishna Gajula]

> -----Original Message-----
> From: Jon Kohler <jon@nutanix.com>
> Sent: Tuesday, June 25, 2024 12:19 AM
> To: Christian Benvenuti <benve@cisco.com>; Satish Kharat
> <satishkh@cisco.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: Jon Kohler <jon@nutanix.com>
> Subject: [PATCH v2] enic: add ethtool get_channel support

It is better to indicate the patch is for NET or NET-NEXT ex: [ PATCH net-next v2]. 

> 
> Add .get_channel to enic_ethtool_ops to enable basic ethtool -l support to
> get the current channel configuration. Note that the driver does not support
> dynamically changing queue configuration, so .set_channel is intentionally
> unused. Instead, 
> Add .get_channel to enic_ethtool_ops to enable basic ethtool -l support to
> get the current channel configuration.
> 
> Note that the driver does not support dynamically changing queue
> configuration, so .set_channel is intentionally unused. Instead, users should
> use Cisco's hardware management tools (UCSM/IMC) to modify virtual
> interface card configuration out of band.
> 
> Signed-off-by: Jon Kohler <jon@nutanix.com>
> ---
> v1
> - https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lore.kernel.org_netdev_20240618160146.3900470-2D1-2Djon-
> 40nutanix.com_T_-
> 23u&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=c3MsgrR-U-
> HFhmFd6R4MWRZG-8QeikJn5PkjqMTpBSg&m=IIWcqk3E-
> aJ8g4EtpjIC9Jg_2T37OpnceY8mwAIuWEBmyGG9tHKaQp1rAgD5__2K&s=hTRY
> fTAQB9Tli-3DbKoTQkrJ2OxTBab-RqcKIJUjQTc&e=
> v1 -> v2:
> - Addressed comments from Przemek and Jakub
> ---
>  .../net/ethernet/cisco/enic/enic_ethtool.c    | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> index 241906697019..54f542238b4e 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> @@ -608,6 +608,32 @@ static int enic_get_ts_info(struct net_device *netdev,
>  	return 0;
>  }
> 
> +static void enic_get_channels(struct net_device *netdev,
> +			      struct ethtool_channels *channels) {
> +	struct enic *enic = netdev_priv(netdev);
> +
> +	switch (vnic_dev_get_intr_mode(enic->vdev)) {
> +	case VNIC_DEV_INTR_MODE_MSIX:
> +		channels->max_rx = ENIC_RQ_MAX;
> +		channels->max_tx = ENIC_WQ_MAX;
> +		channels->rx_count = enic->rq_count;
> +		channels->tx_count = enic->wq_count;
> +		break;
> +	case VNIC_DEV_INTR_MODE_MSI:
> +		channels->max_rx = 1;
> +		channels->max_tx = 1;
> +		channels->rx_count = 1;
> +		channels->tx_count = 1;
> +		break;
> +	case VNIC_DEV_INTR_MODE_INTX:
> +		channels->max_combined = 1;
> +		channels->combined_count = 1;
> +	default:
> +		break;
> +	}
> +}
> +
>  static const struct ethtool_ops enic_ethtool_ops = {
>  	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>  				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
> @@ -632,6 +658,7 @@ static const struct ethtool_ops enic_ethtool_ops = {
>  	.set_rxfh = enic_set_rxfh,
>  	.get_link_ksettings = enic_get_ksettings,
>  	.get_ts_info = enic_get_ts_info,
> +	.get_channels = enic_get_channels,
>  };
> 
>  void enic_set_ethtool_ops(struct net_device *netdev)
> --
> 2.43.0
> 
Reviewed-by: Sai Krishna <saikrishnag@marvell.com>

Re: [PATCH v2] enic: add ethtool get_channel support

[Jon Kohler]

> On Jun 25, 2024, at 1:44 PM, Sai Krishna Gajula <saikrishnag@marvell.com> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
>> -----Original Message-----
>> From: Jon Kohler <jon@nutanix.com>
>> Sent: Tuesday, June 25, 2024 12:19 AM
>> To: Christian Benvenuti <benve@cisco.com>; Satish Kharat
>> <satishkh@cisco.com>; David S. Miller <davem@davemloft.net>; Eric
>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
>> Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Cc: Jon Kohler <jon@nutanix.com>
>> Subject: [PATCH v2] enic: add ethtool get_channel support
> 
> It is better to indicate the patch is for NET or NET-NEXT ex: [ PATCH net-next v2]. 

Thanks for the Reviewed-By and the tip here. Do you suggest I send out a v3
with an updated tag? My apologies for the confusion.

> 
>> 
>> Add .get_channel to enic_ethtool_ops to enable basic ethtool -l support to
>> get the current channel configuration. Note that the driver does not support
>> dynamically changing queue configuration, so .set_channel is intentionally
>> unused. Instead, 
>> Add .get_channel to enic_ethtool_ops to enable basic ethtool -l support to
>> get the current channel configuration.
>> 
>> Note that the driver does not support dynamically changing queue
>> configuration, so .set_channel is intentionally unused. Instead, users should
>> use Cisco's hardware management tools (UCSM/IMC) to modify virtual
>> interface card configuration out of band.
>> 
>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>> ---
>> v1
>> - https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__lore.kernel.org_netdev_20240618160146.3900470-2D1-2Djon-
>> 40nutanix.com_T_-
>> 23u&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=c3MsgrR-U-
>> HFhmFd6R4MWRZG-8QeikJn5PkjqMTpBSg&m=IIWcqk3E-
>> aJ8g4EtpjIC9Jg_2T37OpnceY8mwAIuWEBmyGG9tHKaQp1rAgD5__2K&s=hTRY
>> fTAQB9Tli-3DbKoTQkrJ2OxTBab-RqcKIJUjQTc&e=
>> v1 -> v2:
>> - Addressed comments from Przemek and Jakub
>> ---
>> .../net/ethernet/cisco/enic/enic_ethtool.c    | 27 +++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> index 241906697019..54f542238b4e 100644
>> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> @@ -608,6 +608,32 @@ static int enic_get_ts_info(struct net_device *netdev,
>> return 0;
>> }
>> 
>> +static void enic_get_channels(struct net_device *netdev,
>> +       struct ethtool_channels *channels) {
>> + struct enic *enic = netdev_priv(netdev);
>> +
>> + switch (vnic_dev_get_intr_mode(enic->vdev)) {
>> + case VNIC_DEV_INTR_MODE_MSIX:
>> + channels->max_rx = ENIC_RQ_MAX;
>> + channels->max_tx = ENIC_WQ_MAX;
>> + channels->rx_count = enic->rq_count;
>> + channels->tx_count = enic->wq_count;
>> + break;
>> + case VNIC_DEV_INTR_MODE_MSI:
>> + channels->max_rx = 1;
>> + channels->max_tx = 1;
>> + channels->rx_count = 1;
>> + channels->tx_count = 1;
>> + break;
>> + case VNIC_DEV_INTR_MODE_INTX:
>> + channels->max_combined = 1;
>> + channels->combined_count = 1;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> static const struct ethtool_ops enic_ethtool_ops = {
>> .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>>      ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
>> @@ -632,6 +658,7 @@ static const struct ethtool_ops enic_ethtool_ops = {
>> .set_rxfh = enic_set_rxfh,
>> .get_link_ksettings = enic_get_ksettings,
>> .get_ts_info = enic_get_ts_info,
>> + .get_channels = enic_get_channels,
>> };
>> 
>> void enic_set_ethtool_ops(struct net_device *netdev)
>> --
>> 2.43.0
>> 
> Reviewed-by: Sai Krishna <saikrishnag@marvell.com>

Re: [PATCH v2] enic: add ethtool get_channel support

[Jakub Kicinski]

On Mon, 24 Jun 2024 11:49:00 -0700 Jon Kohler wrote:
> +	switch (vnic_dev_get_intr_mode(enic->vdev)) {
> +	case VNIC_DEV_INTR_MODE_MSIX:
> +		channels->max_rx = ENIC_RQ_MAX;
> +		channels->max_tx = ENIC_WQ_MAX;
> +		channels->rx_count = enic->rq_count;
> +		channels->tx_count = enic->wq_count;
> +		break;
> +	case VNIC_DEV_INTR_MODE_MSI:
> +		channels->max_rx = 1;
> +		channels->max_tx = 1;
> +		channels->rx_count = 1;
> +		channels->tx_count = 1;
> +		break;
> +	case VNIC_DEV_INTR_MODE_INTX:
> +		channels->max_combined = 1;
> +		channels->combined_count = 1;
> +	default:
> +		break;
> +	}

sorry for not responding properly to your earlier email, but I think
MSI should also be combined. What matters is whether the IRQ serves
just one of {Rx, Tx} or both.

For MSI, I see:

1 . enic_dev_init() does:
	netif_napi_add(netdev, &enic->napi[0], enic_poll);
                                               ^^^^^^^^^

2. enic_request_intr() does
	request_irq(enic->pdev->irq, enic_isr_msi, ...
                                     ^^^^^^^^^^^^

3. enic_isr_msi() does 
	napi_schedule_irqoff(&enic->napi[0]); 
thus matching the NAPI from step #1.

4. enic_poll() calls both enic_wq_service, and enic_rq_service

So it's combined, AFAICT, similar to INTX in the relevant parts.
-- 
pw-bot: cr

Re: [PATCH v2] enic: add ethtool get_channel support

[Jon Kohler]

> On Jun 25, 2024, at 8:03 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Mon, 24 Jun 2024 11:49:00 -0700 Jon Kohler wrote:
>> + switch (vnic_dev_get_intr_mode(enic->vdev)) {
>> + case VNIC_DEV_INTR_MODE_MSIX:
>> + channels->max_rx = ENIC_RQ_MAX;
>> + channels->max_tx = ENIC_WQ_MAX;
>> + channels->rx_count = enic->rq_count;
>> + channels->tx_count = enic->wq_count;
>> + break;
>> + case VNIC_DEV_INTR_MODE_MSI:
>> + channels->max_rx = 1;
>> + channels->max_tx = 1;
>> + channels->rx_count = 1;
>> + channels->tx_count = 1;
>> + break;
>> + case VNIC_DEV_INTR_MODE_INTX:
>> + channels->max_combined = 1;
>> + channels->combined_count = 1;
>> + default:
>> + break;
>> + }
> 
> sorry for not responding properly to your earlier email, but I think
> MSI should also be combined. What matters is whether the IRQ serves
> just one of {Rx, Tx} or both.
> 
> For MSI, I see:
> 
> 1 . enic_dev_init() does:
> netif_napi_add(netdev, &enic->napi[0], enic_poll);
>                                               ^^^^^^^^^
> 
> 2. enic_request_intr() does
> request_irq(enic->pdev->irq, enic_isr_msi, ...
>                                     ^^^^^^^^^^^^
> 
> 3. enic_isr_msi() does 
> napi_schedule_irqoff(&enic->napi[0]); 
> thus matching the NAPI from step #1.
> 
> 4. enic_poll() calls both enic_wq_service, and enic_rq_service
> 
> So it's combined, AFAICT, similar to INTX in the relevant parts.

Ok, thanks for the tip, I appreciate it. I’ll send out a v3 shortly

> -- 
> pw-bot: cr