Re: [B.A.T.M.A.N.] [RFC 18/23] batman-adv: ELP - use phydev to determine link characteristics

Antonio Quartulli
2014-02-13T11:02:17Z

[-- Attachment #1: Type: text/plain, Size: 4991 bytes --]

On 13/02/14 11:52, Andrew Lunn wrote:
> On Tue, Feb 11, 2014 at 01:48:18PM +0100, Antonio Quartulli wrote:
>> From: Antonio Quartulli <antonio@open-mesh.com>
>>
>> The phydev member of a net_device can be used to get
>> information about an ethernet link like HALF/FULL_DUPLEX
>> and advertised bandwidth (e.g. 100/10Mbps).
>>
>> This information are then stored in the hard_iface object
>> to be used during the metric computation routine.
>>
>> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
>> ---
>> bat_v_elp.c | 8 ++++++++
>> bat_v_ogm.c | 4 ++--
>> hard-interface.c | 19 +++++++++++++++++++
>> types.h | 12 ++++++++++++
>> 4 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/bat_v_elp.c b/bat_v_elp.c
>> index 763113e..958843c 100644
>> --- a/bat_v_elp.c
>> +++ b/bat_v_elp.c
>> @@ -78,6 +78,14 @@ batadv_v_elp_get_throughput(struct batadv_elp_neigh_node *neigh)
>> if (throughput != 0)
>> return throughput;
>>
>> + /* In case of Ethernet interface, the throughput has already been
>> + * obtained from the phydev object in the net_device struct (see
>> + * batadv_hardif_activate_interface()). So return this value.
>> + */
>> + throughput = hard_iface->bat_v.eth_throughput;
>> + if (throughput != 0)
>> + return throughput;
>> +
>> /* if this is a wireless device, then ask its throughput through
>> * cfg80211 API
>> */
>> diff --git a/bat_v_ogm.c b/bat_v_ogm.c
>> index fa3d1a1..060ce80 100644
>> --- a/bat_v_ogm.c
>> +++ b/bat_v_ogm.c
>> @@ -393,8 +393,8 @@ static uint32_t batadv_v_penalty(struct batadv_priv *bat_priv,
>> /* proportion to use the same value used in batman iv (x * 128 / 256) */
>> hop_penalty = hop_penalty * 100 / 255;
>>
>> - if (batadv_is_wifi_netdev(if_incoming->net_dev) &&
>> - metric > link_metric / 10)
>> + if ((if_incoming->bat_v.flags & BATADV_FULL_DUPLEX) &&
>> + (metric > link_metric / 10))
>> return metric / 2;
>>
>> return metric * (100 - hop_penalty) / 100;
>> diff --git a/hard-interface.c b/hard-interface.c
>> index 2a04130..58c8669 100644
>> --- a/hard-interface.c
>> +++ b/hard-interface.c
>> @@ -31,6 +31,8 @@
>>
>> #include <linux/if_arp.h>
>> #include <linux/if_ether.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/phy.h>
>>
>> void batadv_hardif_free_rcu(struct rcu_head *rcu)
>> {
>> @@ -297,6 +299,7 @@ void batadv_update_min_mtu(struct net_device *soft_iface)
>> static void
>> batadv_hardif_activate_interface(struct batadv_hard_iface *hard_iface)
>> {
>> + struct net_device *dev = hard_iface->net_dev;
>> struct batadv_priv *bat_priv;
>> struct batadv_hard_iface *primary_if = NULL;
>>
>> @@ -315,6 +318,22 @@ batadv_hardif_activate_interface(struct batadv_hard_iface *hard_iface)
>> if (!primary_if)
>> batadv_primary_if_select(bat_priv, hard_iface);
>>
>> + /* set the default values */
>> + hard_iface->bat_v.flags &= ~BATADV_FULL_DUPLEX;
>> + hard_iface->bat_v.eth_throughput = 0;
>> + if (dev->phydev) {
>> + if (dev->phydev->duplex == DUPLEX_FULL)
>> + hard_iface->bat_v.flags |= BATADV_FULL_DUPLEX;
>> +
>> + /* set the speed according to the phydev setting. Store the
>> + * value in Kbps (as done for the other throughput variables)
>> + */
>> + if (dev->phydev->speed != SPEED_UNKNOWN) {
>> + hard_iface->bat_v.eth_throughput = dev->phydev->speed;
>> + hard_iface->bat_v.eth_throughput *= 10;
>> + }
>> + }
>> +
>
> Hi Antonio
>
> If i'm reading this correctly, you get the bandwidth once when the
> interface is added to bat0?

it is invoked every time the interface is brought up. So if you
disconnect and then reconnect the cable (e.g. to connect the NIC to a
faster switch) this function is invoked again and the bandwidth is updated.

> Are we sure that auto-negotiation has
> finished? It can take a few seconds to complete after the interface is
> brought up. It would not be good to have batman use 10Mbps/Half Duplex
> on my gigabit links...

So it can take up to "few" seconds? I did not expect such a delay!

However this function if invoked when the NETDEV_UP event is issued by
the underlying system. I expect the event to be thrown when the
interface is ready to be used, not when the auto-negotiatin is still
going on. I will double check.

>
> Is there a notification mechanism which could be used to keep these
> values up to date? Or is that in a patch i have not yet got to.

No there is no notification mechanism.

I see only two ways of changing this value at runtime:

1) NIC disconnected and re-connected to a different "thing" -> NETDEV_UP
is issued, we re-read the values

2) the user manually changes the bandwidth (e.g. using ethtool) -> not
handled right now (I would also not consider it a high priority item),
but maybe the NETDEV_CHANGE event is fired up in this case?

Cheers,

--
Antonio Quartulli

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]