[PATCH] enic: add ethtool get_channel support

[PATCH] 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>
---
 drivers/net/ethernet/cisco/enic/enic_ethtool.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 241906697019..efbc0715b10e 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -608,6 +608,23 @@ 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);
+
+	channels->max_rx = ENIC_RQ_MAX;
+	channels->max_tx = ENIC_WQ_MAX;
+	channels->rx_count = enic->rq_count;
+	channels->tx_count = enic->wq_count;
+
+	/* enic doesn't use other channels or combined channels */
+	channels->combined_count = 0;
+	channels->max_combined = 0;
+	channels->max_other = 0;
+	channels->other_count = 0;
+}
+
 static const struct ethtool_ops enic_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
@@ -632,6 +649,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] enic: add ethtool get_channel support

[Przemek Kitszel]

On 6/18/24 18:01, Jon Kohler wrote:
> 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>
> ---
>   drivers/net/ethernet/cisco/enic/enic_ethtool.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> index 241906697019..efbc0715b10e 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> @@ -608,6 +608,23 @@ 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);
> +
> +	channels->max_rx = ENIC_RQ_MAX;
> +	channels->max_tx = ENIC_WQ_MAX;
> +	channels->rx_count = enic->rq_count;
> +	channels->tx_count = enic->wq_count;
> +
> +	/* enic doesn't use other channels or combined channels */
> +	channels->combined_count = 0;

my understanding is that effective Rx count is combined_count+rx_count,
analogous for Tx

and:
uapi/linux/ethtool.h:547: * @combined_count: Valid values are in the 
range 1 to the max_combined.

> +	channels->max_combined = 0;
> +	channels->max_other = 0;
> +	channels->other_count = 0;
> +}
> +
>   static const struct ethtool_ops enic_ethtool_ops = {
>   	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>   				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
> @@ -632,6 +649,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)

Re: [PATCH] enic: add ethtool get_channel support

[Jon Kohler]

> On Jun 18, 2024, at 12:20 PM, Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote:
> 
> !-------------------------------------------------------------------|
> CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> On 6/18/24 18:01, Jon Kohler wrote:
>> 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>
>> ---
>>  drivers/net/ethernet/cisco/enic/enic_ethtool.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> index 241906697019..efbc0715b10e 100644
>> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> @@ -608,6 +608,23 @@ 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);
>> +
>> + channels->max_rx = ENIC_RQ_MAX;
>> + channels->max_tx = ENIC_WQ_MAX;
>> + channels->rx_count = enic->rq_count;
>> + channels->tx_count = enic->wq_count;
>> +
>> + /* enic doesn't use other channels or combined channels */
>> + channels->combined_count = 0;
> 
> my understanding is that effective Rx count is combined_count+rx_count,
> analogous for Tx

Thanks for the quick review!

Looking through how other drivers do this, I didn’t get a sense that
any other drivers were stacking rx_count + combined_count together.

Also, enic and the underlying Cisco VIC hardware appears to be 
fairly specific that the queues they provision at the hardware level are
either RX or TX and not a unified ring or something to that effect.

I took that to mean that we would never call anything ‘combined’ in
the context of this driver.

> 
> and:
> uapi/linux/ethtool.h:547: * @combined_count: Valid values are in the range 1 to the max_combined.

I saw that, though I did see a few other drivers that also do combined_count = 0
broadcom/bnx2, google/gve, ibm/ibmvnic, and ti/cpsw

I also didn’t see anyone specifically setting it to 1 to comply with the uapi header
you pointed to.

I did check net/ethtool/channels.c -> channels_fill_reply and there is not
any handling there, or anywhere else in channels.c that would prevent the
usage of combined_count = 0.

With this patch, I see the following ethtool output when hardware RX/TX are both
set to 1

# ethtool -l eth0
Channel parameters for eth0:
Pre-set maximums:
RX: 8
TX: 8
Other: n/a
Combined: n/a
Current hardware settings:
RX: 1
TX: 1
Other: n/a
Combined: n/a

# dmesg|grep -i enic
[ 8.296127] enic 0000:35:00.0: vNIC MAC addr 00:25:b5:00:88:0e wq/rq 256/512 mtu 1500
[ 8.296131] enic 0000:35:00.0: vNIC csum tx/rx yes/yes tso/lro yes/yes rss no intr mode any type min timer 125 usec loopback tag 0x0000
[ 8.296134] enic 0000:35:00.0: vNIC resources avail: wq 1 rq 1 cq 2 intr 4
[ 8.296299] enic 0000:35:00.0: vNIC resources used: wq 1 rq 1 cq 2 intr 4 intr mode MSI-X
[ 8.300453] enic 0000:35:00.1: vNIC MAC addr 00:25:b5:00:88:1e wq/rq 256/512 mtu 1500
[ 8.300456] enic 0000:35:00.1: vNIC csum tx/rx yes/yes tso/lro yes/yes rss no intr mode any type min timer 125 usec loopback tag 0x0000
[ 8.300457] enic 0000:35:00.1: vNIC resources avail: wq 1 rq 1 cq 2 intr 4
[ 8.301444] enic 0000:35:00.1: vNIC resources used: wq 1 rq 1 cq 2 intr 4 intr mode MSI-X
[ 8.318699] enic 0000:35:00.0 eth0: renamed from ahv0
[ 8.359256] enic 0000:35:00.1 eth1: renamed from ahv1
[ 15.177655] enic 0000:35:00.0 eth0: Link UP
[ 15.192971] enic 0000:35:00.1 eth1: Link UP

Thanks again,
Jon

> 
>> + channels->max_combined = 0;
>> + channels->max_other = 0;
>> + channels->other_count = 0;
>> +}
>> +
>>  static const struct ethtool_ops enic_ethtool_ops = {
>>   .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>>       ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
>> @@ -632,6 +649,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)
> 

Re: [PATCH] enic: add ethtool get_channel support

[Jakub Kicinski]

On Tue, 18 Jun 2024 16:39:40 +0000 Jon Kohler wrote:
> Looking through how other drivers do this, I didn’t get a sense that
> any other drivers were stacking rx_count + combined_count together.
> 
> Also, enic and the underlying Cisco VIC hardware appears to be 
> fairly specific that the queues they provision at the hardware level are
> either RX or TX and not a unified ring or something to that effect.
> 
> I took that to mean that we would never call anything ‘combined’ in
> the context of this driver.

channel is a bit of an old term, think about interrupts more than
queues. ethtool man page has the most informative description.

Looking at this driver, specifically enic_dev_init() I'd venture
something along the lines of:

	switch (vnic_dev_get_intr_mode(enic->vdev)) { 
	default:
		channels->combined = 1;
		break;                
	case VNIC_DEV_INTR_MODE_MSIX: 
		channels->rx_count = enic->rq_count;
		channels->tx_count = enic->wq_count;
		break;
        }

Please not that you don't have to zero out unused fields, they come
zero-initilized.
-- 
pw-bot: cr

Re: [PATCH] enic: add ethtool get_channel support

[Jon Kohler]

> On Jun 19, 2024, at 8:04 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Tue, 18 Jun 2024 16:39:40 +0000 Jon Kohler wrote:
>> Looking through how other drivers do this, I didn’t get a sense that
>> any other drivers were stacking rx_count + combined_count together.
>> 
>> Also, enic and the underlying Cisco VIC hardware appears to be 
>> fairly specific that the queues they provision at the hardware level are
>> either RX or TX and not a unified ring or something to that effect.
>> 
>> I took that to mean that we would never call anything ‘combined’ in
>> the context of this driver.
> 
> channel is a bit of an old term, think about interrupts more than
> queues. ethtool man page has the most informative description.

Thanks for the pointer on man ethtool - one question, Przemek had
brought up a good point that ethtool uapi says that combined queues
valid values start at 1; however, I don’t see anything that enforces that
point in the code or the man page.

Should I just omit that completely from the change, since the fields
are zero initialized anyhow?

> 
> Looking at this driver, specifically enic_dev_init() I'd venture
> something along the lines of:
> 
> switch (vnic_dev_get_intr_mode(enic->vdev)) { 
> default:
> channels->combined = 1;
> break;                
> case VNIC_DEV_INTR_MODE_MSIX: 
> channels->rx_count = enic->rq_count;
> channels->tx_count = enic->wq_count;
> break;
>        }
> 
> Please not that you don't have to zero out unused fields, they come
> zero-initilized.

Ok thanks, I’ll see what I can cook up in a V2 patch. 

> -- 
> pw-bot: cr

Re: [PATCH] enic: add ethtool get_channel support

[Jakub Kicinski]

On Thu, 20 Jun 2024 19:49:45 +0000 Jon Kohler wrote:
> > channel is a bit of an old term, think about interrupts more than
> > queues. ethtool man page has the most informative description.  
> 
> Thanks for the pointer on man ethtool - one question, Przemek had
> brought up a good point that ethtool uapi says that combined queues
> valid values start at 1; however, I don’t see anything that enforces that
> point in the code or the man page.
> 
> Should I just omit that completely from the change, since the fields
> are zero initialized anyhow?

Not sure what the comment about 1 to max is intending to communicate.
But I'd guess it trying to convey that on SET driver doesn't have to
worry about the value being crazy, if it sets max correctly.

Re: [PATCH] enic: add ethtool get_channel support

[Jon Kohler]

> On Jun 20, 2024, at 3:58 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Thu, 20 Jun 2024 19:49:45 +0000 Jon Kohler wrote:
>>> channel is a bit of an old term, think about interrupts more than
>>> queues. ethtool man page has the most informative description.  
>> 
>> Thanks for the pointer on man ethtool - one question, Przemek had
>> brought up a good point that ethtool uapi says that combined queues
>> valid values start at 1; however, I don’t see anything that enforces that
>> point in the code or the man page.
>> 
>> Should I just omit that completely from the change, since the fields
>> are zero initialized anyhow?
> 
> Not sure what the comment about 1 to max is intending to communicate.
> But I'd guess it trying to convey that on SET driver doesn't have to
> worry about the value being crazy, if it sets max correctly.

Jakub - thanks for the pointer on the code suggestion. I’ll send out a v2
shortly. Note, check out enic_set_intr_mode. In both MSI and INTX cases
rq_count and wq_count will be 1, though in INTX the first INTR is
treated as combined. 

Re: [PATCH] enic: add ethtool get_channel support

[Simon Horman]

On Tue, Jun 18, 2024 at 09:01:46AM -0700, Jon Kohler wrote:
> Add .get_channel to enic_ethtool_ops to enable basic ethtool -l
> support to get the current channel configuration.

This is nice :)

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

That is a shame :(

> Signed-off-by: Jon Kohler <jon@nutanix.com>

Sad face aside, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH] enic: add ethtool get_channel support

[Simon Horman]

On Wed, Jun 19, 2024 at 06:59:15PM +0100, Simon Horman wrote:
> On Tue, Jun 18, 2024 at 09:01:46AM -0700, Jon Kohler wrote:
> > Add .get_channel to enic_ethtool_ops to enable basic ethtool -l
> > support to get the current channel configuration.
> 
> This is nice :)
> 
> > 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.
> 
> That is a shame :(
> 
> > Signed-off-by: Jon Kohler <jon@nutanix.com>
> 
> Sad face aside, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

Oops, I failed to notice that Przemek Kitszel has some questions about this
regarding issues that I overlooked. So I'll have to take my tag back for now.

Sorry for the noise.