diff mbox series

batman-adv: disable ethtool link speed detection when auto negotiation off

Message ID 20180512220223.3039-1-mareklindner@neomailbox.ch (mailing list archive)
State Superseded, archived
Delegated to: Simon Wunderlich
Headers
Series batman-adv: disable ethtool link speed detection when auto negotiation off |

Commit Message

Marek Lindner May 12, 2018, 10:02 p.m. UTC 
  If link auto-negotiation is disabled the exported link speed can
not be relied on.

Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
 net/batman-adv/bat_v_elp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli May 12, 2018, 10:04 p.m. UTC | #1 
On 13/05/18 06:02, Marek Lindner wrote:
> If link auto-negotiation is disabled the exported link speed can
> not be relied on.
> 
> Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>

Acked-by: Antonio Quartulli <a@unstable.cc>
Sven Eckelmann May 13, 2018, 6:56 a.m. UTC | #2 
On Sonntag, 13. Mai 2018 00:02:23 CEST Marek Lindner wrote:
> If link auto-negotiation is disabled the exported link speed can
> not be relied on.

Why is a manual set speed less worth than a auto-configured one?

Kind regards,
	Sven
Sven Eckelmann May 13, 2018, 7:49 a.m. UTC | #3 
Name of failed tests
====================

ecsv/pu
-------

 * sparse linux-3.18 cfg: BLA=n DAT=n DEBUGFS=y DEBUG=y NC=y MCAST=y BATMAN_V=y
 * sparse linux-3.18 cfg: BLA=y DAT=n DEBUGFS=y DEBUG=n NC=n MCAST=n BATMAN_V=y
 * sparse linux-4.2 cfg: BLA=y DAT=n DEBUGFS=n DEBUG=n NC=n MCAST=n BATMAN_V=y
 * sparse linux-4.4 cfg: BLA=n DAT=n DEBUGFS=n DEBUG=n NC=n MCAST=y BATMAN_V=y
 * sparse linux-4.4 cfg: BLA=y DAT=n DEBUGFS=n DEBUG=n NC=n MCAST=n BATMAN_V=y
 * sparse linux-4.4 cfg: BLA=y DAT=n DEBUGFS=y DEBUG=y NC=n MCAST=y BATMAN_V=y


Output of different failed tests
================================

ecsv/pu: sparse linux-4.2 cfg: BLA=y DAT=n DEBUGFS=n DEBUG=n NC=n MCAST=n BATMAN_V=y
---------------------------------------------------------------------------

    /home/build_test/build_env/tmp.Z3XNaFEJS1/build/net/batman-adv/bat_v_elp.c:134:43: error: no member 'autoneg' in struct <unnamed>
    /home/build_test/build_env/tmp.Z3XNaFEJS1/build/net/batman-adv/bat_v_elp.c:134:43: warning: unknown expression (8 46)
    /home/build_test/build_env/tmp.Z3XNaFEJS1/build/net/batman-adv/bat_v_elp.c: In function ‘batadv_v_elp_get_throughput’:
    /home/build_test/build_env/tmp.Z3XNaFEJS1/build/net/batman-adv/bat_v_elp.c:134:36: error: ‘struct <anonymous>’ has no member named ‘autoneg’
      if (ret == 0 && link_settings.base.autoneg == AUTONEG_ENABLE) {
                                        ^
    make[3]: *** [/home/build_test/build_env/tmp.Z3XNaFEJS1/build/net/batman-adv/bat_v_elp.o] Error 1
    make[2]: *** [/home/build_test/build_env/tmp.Z3XNaFEJS1/build/net/batman-adv] Error 2
    make[1]: *** [_module_/home/build_test/build_env/tmp.Z3XNaFEJS1/build] Error 2
    make: *** [all] Error 2
Antonio Quartulli May 13, 2018, 9:19 a.m. UTC | #4 
Hi Sven,

thanks for your comment.

On 13/05/18 14:56, Sven Eckelmann wrote:
> On Sonntag, 13. Mai 2018 00:02:23 CEST Marek Lindner wrote:
>> If link auto-negotiation is disabled the exported link speed can
>> not be relied on.
> 
> Why is a manual set speed less worth than a auto-configured one?

if the user wants to configure a "default throughput" for the interface,
he can always use the default_throughput knob in batman-adv.

The problem with autonegotiation disabled is that the advertised speed
is likely to be a random number set by default by the driver.

For example, some people in the freifunk community have tested this
patch because they wanted to see how the tp_meter would perform on VPN
links. Unfortunately the tun.ko module sets 10Mbps/AUTONEG_DISABLE as
default settings.

From there we thought that, after all, if the autonegotiation is
disabled, we should just assume that the speed is likely to be bogus and
it's better to measure it.


Does it make sense?


Cheers,
Sven Eckelmann May 13, 2018, 11:47 a.m. UTC | #5 
On Sonntag, 13. Mai 2018 11:19:52 CEST Antonio Quartulli wrote:
> Hi Sven,
> 
> thanks for your comment.
> 
> On 13/05/18 14:56, Sven Eckelmann wrote:
> > On Sonntag, 13. Mai 2018 00:02:23 CEST Marek Lindner wrote:
> >> If link auto-negotiation is disabled the exported link speed can
> >> not be relied on.
> > 
> > Why is a manual set speed less worth than a auto-configured one?
[...]
> For example, some people in the freifunk community have tested this
> patch because they wanted to see how the tp_meter would perform on VPN
> links. Unfortunately the tun.ko module sets 10Mbps/AUTONEG_DISABLE as
> default settings.
> 
> From there we thought that, after all, if the autonegotiation is
> disabled, we should just assume that the speed is likely to be bogus and
> it's better to measure it.
[...]

I saw the discussion with Matthias. But nothing about that wasn't mentioned in 
the commit message. Looks like Marek already addresses these things and just 
has to submit the patch again.

And he should not forget your Acked-by (in case it still stands).

Kind regards,
	Sven
diff mbox series

Patch

diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
index 28687493..fd6fe847 100644
--- a/net/batman-adv/bat_v_elp.c
+++ b/net/batman-adv/bat_v_elp.c
@@ -127,7 +127,11 @@  static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh)
 	rtnl_lock();
 	ret = __ethtool_get_link_ksettings(hard_iface->net_dev, &link_settings);
 	rtnl_unlock();
-	if (ret == 0) {
+
+	/* If link auto-negotiation is disabled the exported link speed can
+	 * not be relied on.
+	 */
+	if (ret == 0 && link_settings.base.autoneg == AUTONEG_ENABLE) {
 		/* link characteristics might change over time */
 		if (link_settings.base.duplex == DUPLEX_FULL)
 			hard_iface->bat_v.flags |= BATADV_FULL_DUPLEX;