netdev - Re: [PATCH net-next] team: add ethtool get_link

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, 28 May 2019 13:24:31 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net-next] team: add ethtool get_link_ksettings

Tue, May 28, 2019 at 12:02:11PM CEST, liuhangbin@...il.com wrote:
>On Tue, May 28, 2019 at 11:08:23AM +0200, Jiri Pirko wrote:
>> >+static int team_ethtool_get_link_ksettings(struct net_device *dev,
>> >+					   struct ethtool_link_ksettings *cmd)
>> >+{
>> >+	struct team *team= netdev_priv(dev);
>> >+	unsigned long speed = 0;
>> >+	struct team_port *port;
>> >+
>> >+	cmd->base.duplex = DUPLEX_UNKNOWN;
>> >+	cmd->base.port = PORT_OTHER;
>> >+
>> >+	list_for_each_entry(port, &team->port_list, list) {
>> >+		if (team_port_txable(port)) {
>> >+			if (port->state.speed != SPEED_UNKNOWN)
>> >+				speed += port->state.speed;
>> >+			if (cmd->base.duplex == DUPLEX_UNKNOWN &&
>> >+			    port->state.duplex != DUPLEX_UNKNOWN)
>> >+				cmd->base.duplex = port->state.duplex;
>> 
>> What is exactly the point of this patch? Why do you need such
>> information. This is hw-related info. If you simply sum-up all txable
>> ports, the value is always highly misleading.
>> 
>> For example for hash-based port selection with 2 100Mbit ports,
>> you will get 200Mbit, but it is not true. It is up to the traffic and
>> hash function what is the actual TX speed you can get.
>> On the RX side, this is even more misleading as the actual speed depends
>> on the other side of the wire.
>
>The number is the maximum speed in theory. I added it because someone

"in theory" is not what this value should return in my opinion.


>said bond interface could show total speed while team could not...
>The usage is customer could get team link-speed and throughput via SNMP.

Has no meaning though :/


>
>Thanks
>Hangbin
>> 
>> 
>> >+		}
>> >+	}
>> >+	cmd->base.speed = speed ? : SPEED_UNKNOWN;
>> >+
>> >+	return 0;
>> >+}
>> >+
>> > static const struct ethtool_ops team_ethtool_ops = {
>> > 	.get_drvinfo		= team_ethtool_get_drvinfo,
>> > 	.get_link		= ethtool_op_get_link,
>> >+	.get_link_ksettings	= team_ethtool_get_link_ksettings,
>> > };
>> > 
>> > /***********************
>> >-- 
>> >2.19.2
>> >