netdev - Re: [PATCH] enic: add ethtool get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 18 Jun 2024 16:39:40 +0000
From: Jon Kohler <jon@...anix.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: Christian Benvenuti <benve@...co.com>, Satish Kharat <satishkh@...co.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Larysa Zaremba
	<larysa.zaremba@...el.com>
Subject: Re: [PATCH] enic: add ethtool get_channel support



> On Jun 18, 2024, at 12:20 PM, Przemek Kitszel <przemyslaw.kitszel@...el.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@...anix.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)
>