[PATCH net 1/3] bonding: fix 802.3ad support for 5G and 50G speeds

[PATCH net 1/3] bonding: fix 802.3ad support for 5G and 50G speeds

[Nicolas Dichtel]

From: Thibaut Collet <thibaut.collet@6wind.com>

This patch adds [5|50] Gbps enum definition, and fixes
aggregated bandwidth calculation based on above slave links.

Fixes: c9a70d43461d ("net-next: ethtool: Added port speed macros.")
Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/bonding/bond_3ad.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index b44a6aeb346d..d1b09be63ba4 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -90,10 +90,12 @@ enum ad_link_speed_type {
 	AD_LINK_SPEED_100MBPS,
 	AD_LINK_SPEED_1000MBPS,
 	AD_LINK_SPEED_2500MBPS,
+	AD_LINK_SPEED_5000MBPS,
 	AD_LINK_SPEED_10000MBPS,
 	AD_LINK_SPEED_20000MBPS,
 	AD_LINK_SPEED_25000MBPS,
 	AD_LINK_SPEED_40000MBPS,
+	AD_LINK_SPEED_50000MBPS,
 	AD_LINK_SPEED_56000MBPS,
 	AD_LINK_SPEED_100000MBPS,
 };
@@ -259,10 +261,12 @@ static inline int __check_agg_selection_timer(struct port *port)
  *     %AD_LINK_SPEED_100MBPS,
  *     %AD_LINK_SPEED_1000MBPS,
  *     %AD_LINK_SPEED_2500MBPS,
+ *     %AD_LINK_SPEED_5000MBPS,
  *     %AD_LINK_SPEED_10000MBPS
  *     %AD_LINK_SPEED_20000MBPS
  *     %AD_LINK_SPEED_25000MBPS
  *     %AD_LINK_SPEED_40000MBPS
+ *     %AD_LINK_SPEED_50000MBPS
  *     %AD_LINK_SPEED_56000MBPS
  *     %AD_LINK_SPEED_100000MBPS
  */
@@ -296,6 +300,10 @@ static u16 __get_link_speed(struct port *port)
 			speed = AD_LINK_SPEED_2500MBPS;
 			break;
 
+		case SPEED_5000:
+			speed = AD_LINK_SPEED_5000MBPS;
+			break;
+
 		case SPEED_10000:
 			speed = AD_LINK_SPEED_10000MBPS;
 			break;
@@ -312,6 +320,10 @@ static u16 __get_link_speed(struct port *port)
 			speed = AD_LINK_SPEED_40000MBPS;
 			break;
 
+		case SPEED_50000:
+			speed = AD_LINK_SPEED_50000MBPS;
+			break;
+
 		case SPEED_56000:
 			speed = AD_LINK_SPEED_56000MBPS;
 			break;
@@ -707,6 +719,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
 		case AD_LINK_SPEED_2500MBPS:
 			bandwidth = nports * 2500;
 			break;
+		case AD_LINK_SPEED_5000MBPS:
+			bandwidth = nports * 5000;
+			break;
 		case AD_LINK_SPEED_10000MBPS:
 			bandwidth = nports * 10000;
 			break;
@@ -719,6 +734,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
 		case AD_LINK_SPEED_40000MBPS:
 			bandwidth = nports * 40000;
 			break;
+		case AD_LINK_SPEED_50000MBPS:
+			bandwidth = nports * 50000;
+			break;
 		case AD_LINK_SPEED_56000MBPS:
 			bandwidth = nports * 56000;
 			break;
-- 
2.8.1

[PATCH net 2/3] bonding: fix 802.3ad support for 14G speed

[Nicolas Dichtel]

This patch adds 14 Gbps enum definition, and fixes
aggregated bandwidth calculation based on above slave links.

Fixes: 0d7e2d2166f6 ("IB/ipoib: add get_link_ksettings in ethtool")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/bonding/bond_3ad.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index d1b09be63ba4..e5386ab706ec 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -92,6 +92,7 @@ enum ad_link_speed_type {
 	AD_LINK_SPEED_2500MBPS,
 	AD_LINK_SPEED_5000MBPS,
 	AD_LINK_SPEED_10000MBPS,
+	AD_LINK_SPEED_14000MBPS,
 	AD_LINK_SPEED_20000MBPS,
 	AD_LINK_SPEED_25000MBPS,
 	AD_LINK_SPEED_40000MBPS,
@@ -263,6 +264,7 @@ static inline int __check_agg_selection_timer(struct port *port)
  *     %AD_LINK_SPEED_2500MBPS,
  *     %AD_LINK_SPEED_5000MBPS,
  *     %AD_LINK_SPEED_10000MBPS
+ *     %AD_LINK_SPEED_14000MBPS,
  *     %AD_LINK_SPEED_20000MBPS
  *     %AD_LINK_SPEED_25000MBPS
  *     %AD_LINK_SPEED_40000MBPS
@@ -308,6 +310,10 @@ static u16 __get_link_speed(struct port *port)
 			speed = AD_LINK_SPEED_10000MBPS;
 			break;
 
+		case SPEED_14000:
+			speed = AD_LINK_SPEED_14000MBPS;
+			break;
+
 		case SPEED_20000:
 			speed = AD_LINK_SPEED_20000MBPS;
 			break;
@@ -725,6 +731,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
 		case AD_LINK_SPEED_10000MBPS:
 			bandwidth = nports * 10000;
 			break;
+		case AD_LINK_SPEED_14000MBPS:
+			bandwidth = nports * 14000;
+			break;
 		case AD_LINK_SPEED_20000MBPS:
 			bandwidth = nports * 20000;
 			break;
-- 
2.8.1

[PATCH net 3/3] ethtool.h: remind to update 802.3ad when adding new speeds

[Nicolas Dichtel]

Each time a new speed is added, the bonding 802.3ad isn't updated. Add a
comment to remind the developer to update this driver.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/uapi/linux/ethtool.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index d179d7767f51..7d4a594d5d58 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1486,8 +1486,10 @@ enum ethtool_link_mode_bit_indices {
  * it was forced up into this mode or autonegotiated.
  */
 
-/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. */
-/* Update drivers/net/phy/phy.c:phy_speed_to_str() when adding new values */
+/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
+ * Update drivers/net/phy/phy.c:phy_speed_to_str() and
+ * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.
+ */
 #define SPEED_10		10
 #define SPEED_100		100
 #define SPEED_1000		1000
-- 
2.8.1

Re: [PATCH net 1/3] bonding: fix 802.3ad support for 5G and 50G speeds

[Andy Gospodarek]

On Thu, Jun 08, 2017 at 11:18:11AM +0200, Nicolas Dichtel wrote:
> From: Thibaut Collet <thibaut.collet@6wind.com>
> 
> This patch adds [5|50] Gbps enum definition, and fixes
> aggregated bandwidth calculation based on above slave links.
> 
> Fixes: c9a70d43461d ("net-next: ethtool: Added port speed macros.")
> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Acked-by: Andy Gospodarek <andy@greyhouse.net>

> ---
>  drivers/net/bonding/bond_3ad.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index b44a6aeb346d..d1b09be63ba4 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -90,10 +90,12 @@ enum ad_link_speed_type {
>  	AD_LINK_SPEED_100MBPS,
>  	AD_LINK_SPEED_1000MBPS,
>  	AD_LINK_SPEED_2500MBPS,
> +	AD_LINK_SPEED_5000MBPS,
>  	AD_LINK_SPEED_10000MBPS,
>  	AD_LINK_SPEED_20000MBPS,
>  	AD_LINK_SPEED_25000MBPS,
>  	AD_LINK_SPEED_40000MBPS,
> +	AD_LINK_SPEED_50000MBPS,
>  	AD_LINK_SPEED_56000MBPS,
>  	AD_LINK_SPEED_100000MBPS,
>  };
> @@ -259,10 +261,12 @@ static inline int __check_agg_selection_timer(struct port *port)
>   *     %AD_LINK_SPEED_100MBPS,
>   *     %AD_LINK_SPEED_1000MBPS,
>   *     %AD_LINK_SPEED_2500MBPS,
> + *     %AD_LINK_SPEED_5000MBPS,
>   *     %AD_LINK_SPEED_10000MBPS
>   *     %AD_LINK_SPEED_20000MBPS
>   *     %AD_LINK_SPEED_25000MBPS
>   *     %AD_LINK_SPEED_40000MBPS
> + *     %AD_LINK_SPEED_50000MBPS
>   *     %AD_LINK_SPEED_56000MBPS
>   *     %AD_LINK_SPEED_100000MBPS
>   */
> @@ -296,6 +300,10 @@ static u16 __get_link_speed(struct port *port)
>  			speed = AD_LINK_SPEED_2500MBPS;
>  			break;
>  
> +		case SPEED_5000:
> +			speed = AD_LINK_SPEED_5000MBPS;
> +			break;
> +
>  		case SPEED_10000:
>  			speed = AD_LINK_SPEED_10000MBPS;
>  			break;
> @@ -312,6 +320,10 @@ static u16 __get_link_speed(struct port *port)
>  			speed = AD_LINK_SPEED_40000MBPS;
>  			break;
>  
> +		case SPEED_50000:
> +			speed = AD_LINK_SPEED_50000MBPS;
> +			break;
> +
>  		case SPEED_56000:
>  			speed = AD_LINK_SPEED_56000MBPS;
>  			break;
> @@ -707,6 +719,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
>  		case AD_LINK_SPEED_2500MBPS:
>  			bandwidth = nports * 2500;
>  			break;
> +		case AD_LINK_SPEED_5000MBPS:
> +			bandwidth = nports * 5000;
> +			break;
>  		case AD_LINK_SPEED_10000MBPS:
>  			bandwidth = nports * 10000;
>  			break;
> @@ -719,6 +734,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
>  		case AD_LINK_SPEED_40000MBPS:
>  			bandwidth = nports * 40000;
>  			break;
> +		case AD_LINK_SPEED_50000MBPS:
> +			bandwidth = nports * 50000;
> +			break;
>  		case AD_LINK_SPEED_56000MBPS:
>  			bandwidth = nports * 56000;
>  			break;
> -- 
> 2.8.1
> 

Re: [PATCH net 2/3] bonding: fix 802.3ad support for 14G speed

[Andy Gospodarek]

On Thu, Jun 08, 2017 at 11:18:12AM +0200, Nicolas Dichtel wrote:
> This patch adds 14 Gbps enum definition, and fixes
> aggregated bandwidth calculation based on above slave links.
> 
> Fixes: 0d7e2d2166f6 ("IB/ipoib: add get_link_ksettings in ethtool")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Acked-by: Andy Gospodarek <andy@greyhouse.net>

> ---
>  drivers/net/bonding/bond_3ad.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index d1b09be63ba4..e5386ab706ec 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -92,6 +92,7 @@ enum ad_link_speed_type {
>  	AD_LINK_SPEED_2500MBPS,
>  	AD_LINK_SPEED_5000MBPS,
>  	AD_LINK_SPEED_10000MBPS,
> +	AD_LINK_SPEED_14000MBPS,
>  	AD_LINK_SPEED_20000MBPS,
>  	AD_LINK_SPEED_25000MBPS,
>  	AD_LINK_SPEED_40000MBPS,
> @@ -263,6 +264,7 @@ static inline int __check_agg_selection_timer(struct port *port)
>   *     %AD_LINK_SPEED_2500MBPS,
>   *     %AD_LINK_SPEED_5000MBPS,
>   *     %AD_LINK_SPEED_10000MBPS
> + *     %AD_LINK_SPEED_14000MBPS,
>   *     %AD_LINK_SPEED_20000MBPS
>   *     %AD_LINK_SPEED_25000MBPS
>   *     %AD_LINK_SPEED_40000MBPS
> @@ -308,6 +310,10 @@ static u16 __get_link_speed(struct port *port)
>  			speed = AD_LINK_SPEED_10000MBPS;
>  			break;
>  
> +		case SPEED_14000:
> +			speed = AD_LINK_SPEED_14000MBPS;
> +			break;
> +
>  		case SPEED_20000:
>  			speed = AD_LINK_SPEED_20000MBPS;
>  			break;
> @@ -725,6 +731,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
>  		case AD_LINK_SPEED_10000MBPS:
>  			bandwidth = nports * 10000;
>  			break;
> +		case AD_LINK_SPEED_14000MBPS:
> +			bandwidth = nports * 14000;
> +			break;
>  		case AD_LINK_SPEED_20000MBPS:
>  			bandwidth = nports * 20000;
>  			break;
> -- 
> 2.8.1
> 

Re: [PATCH net 3/3] ethtool.h: remind to update 802.3ad when adding new speeds

[Andy Gospodarek]

On Thu, Jun 08, 2017 at 11:18:13AM +0200, Nicolas Dichtel wrote:
> Each time a new speed is added, the bonding 802.3ad isn't updated. Add a
> comment to remind the developer to update this driver.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Acked-by: Andy Gospodarek <andy@greyhouse.net>

> ---
>  include/uapi/linux/ethtool.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index d179d7767f51..7d4a594d5d58 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1486,8 +1486,10 @@ enum ethtool_link_mode_bit_indices {
>   * it was forced up into this mode or autonegotiated.
>   */
>  
> -/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. */
> -/* Update drivers/net/phy/phy.c:phy_speed_to_str() when adding new values */
> +/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
> + * Update drivers/net/phy/phy.c:phy_speed_to_str() and
> + * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.

A little wide, but not wider than it was before, so that's fine with me.

> + */
>  #define SPEED_10		10
>  #define SPEED_100		100
>  #define SPEED_1000		1000
> -- 
> 2.8.1
> 

Re: [PATCH net 2/3] bonding: fix 802.3ad support for 14G speed

[Andrew Lunn]

On Thu, Jun 08, 2017 at 11:18:12AM +0200, Nicolas Dichtel wrote:
> This patch adds 14 Gbps enum definition, and fixes
> aggregated bandwidth calculation based on above slave links.
> 
> Fixes: 0d7e2d2166f6 ("IB/ipoib: add get_link_ksettings in ethtool")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  drivers/net/bonding/bond_3ad.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index d1b09be63ba4..e5386ab706ec 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -92,6 +92,7 @@ enum ad_link_speed_type {
>  	AD_LINK_SPEED_2500MBPS,
>  	AD_LINK_SPEED_5000MBPS,
>  	AD_LINK_SPEED_10000MBPS,
> +	AD_LINK_SPEED_14000MBPS,
>  	AD_LINK_SPEED_20000MBPS,
>  	AD_LINK_SPEED_25000MBPS,
>  	AD_LINK_SPEED_40000MBPS,
> @@ -263,6 +264,7 @@ static inline int __check_agg_selection_timer(struct port *port)
>   *     %AD_LINK_SPEED_2500MBPS,
>   *     %AD_LINK_SPEED_5000MBPS,
>   *     %AD_LINK_SPEED_10000MBPS
> + *     %AD_LINK_SPEED_14000MBPS,
>   *     %AD_LINK_SPEED_20000MBPS
>   *     %AD_LINK_SPEED_25000MBPS
>   *     %AD_LINK_SPEED_40000MBPS
> @@ -308,6 +310,10 @@ static u16 __get_link_speed(struct port *port)
>  			speed = AD_LINK_SPEED_10000MBPS;
>  			break;
>  
> +		case SPEED_14000:
> +			speed = AD_LINK_SPEED_14000MBPS;
> +			break;
> +
>  		case SPEED_20000:
>  			speed = AD_LINK_SPEED_20000MBPS;
>  			break;
> @@ -725,6 +731,9 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
>  		case AD_LINK_SPEED_10000MBPS:
>  			bandwidth = nports * 10000;
>  			break;
> +		case AD_LINK_SPEED_14000MBPS:
> +			bandwidth = nports * 14000;
> +			break;
>  		case AD_LINK_SPEED_20000MBPS:
>  			bandwidth = nports * 20000;
>  			break;

How about adding a pr_warn() to the default clause?

    Andrew

Re: [PATCH net 1/3] bonding: fix 802.3ad support for 5G and 50G speeds

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu,  8 Jun 2017 11:18:11 +0200

> From: Thibaut Collet <thibaut.collet@6wind.com>
> 
> This patch adds [5|50] Gbps enum definition, and fixes
> aggregated bandwidth calculation based on above slave links.
> 
> Fixes: c9a70d43461d ("net-next: ethtool: Added port speed macros.")
> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied.

Re: [PATCH net 2/3] bonding: fix 802.3ad support for 14G speed

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu,  8 Jun 2017 11:18:12 +0200

> This patch adds 14 Gbps enum definition, and fixes
> aggregated bandwidth calculation based on above slave links.
> 
> Fixes: 0d7e2d2166f6 ("IB/ipoib: add get_link_ksettings in ethtool")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied.

Re: [PATCH net 3/3] ethtool.h: remind to update 802.3ad when adding new speeds

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu,  8 Jun 2017 11:18:13 +0200

> Each time a new speed is added, the bonding 802.3ad isn't updated. Add a
> comment to remind the developer to update this driver.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied.

Re: [PATCH net 2/3] bonding: fix 802.3ad support for 14G speed

[Joe Perches]

On Thu, 2017-06-08 at 11:18 +0200, Nicolas Dichtel wrote:
> This patch adds 14 Gbps enum definition, and fixes
> aggregated bandwidth calculation based on above slave links.

Doesn't 14G need to be added to phy.c?
---
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a4238cb56731..d1c33a8cbaf5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -54,6 +54,8 @@ static const char *phy_speed_to_str(int speed)
 		return "5Gbps";
 	case SPEED_10000:
 		return "10Gbps";
+	case SPEED_14000:
+		return "14Gbps";
 	case SPEED_20000:
 		return "20Gbps";
 	case SPEED_25000:

[PATCH net-next] bonding: warn user when 802.3ad speed is unknown

[Nicolas Dichtel]

Make it explicit in the log.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/bonding/bond_3ad.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index b44a6aeb346d..b15b177662b0 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -322,6 +322,9 @@ static u16 __get_link_speed(struct port *port)
 
 		default:
 			/* unknown speed value from ethtool. shouldn't happen */
+			pr_warn("%s: unknown speed (%d) for port %d (set it to 0)\n",
+				slave->bond->dev->name, slave->speed,
+				port->actor_port_number);
 			speed = 0;
 			break;
 		}
-- 
2.8.1

[PATCH net] net: phy: add missing SPEED_14000

[Nicolas Dichtel]

From: Joe Perches <joe@perches.com>

Fixes: 0d7e2d2166f6 ("IB/ipoib: add get_link_ksettings in ethtool")
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/phy/phy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7524caa0f29d..eebb0e1c70ff 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -54,6 +54,8 @@ static const char *phy_speed_to_str(int speed)
 		return "5Gbps";
 	case SPEED_10000:
 		return "10Gbps";
+	case SPEED_14000:
+		return "14Gbps";
 	case SPEED_20000:
 		return "20Gbps";
 	case SPEED_25000:
-- 
2.8.1

Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown

[Andrew Lunn]

On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote:
> Make it explicit in the log.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Hi Nicolas

How often is this called? It seems like it is used by
ad_port_selection_logic, which has the comment:

	is called in the inititalization (after all the handshkes),
	and after every lacpdu receive (if selected is off)

I just wonder if this should be rate limited?

  Andrew

Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown

[Nicolas Dichtel]

Le 09/06/2017 à 16:23, Andrew Lunn a écrit :
> On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote:
>> Make it explicit in the log.
>>
>> Suggested-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> 
> Hi Nicolas
Hi Andrew,

> 
> How often is this called? It seems like it is used by
Good question! ;-)

> ad_port_selection_logic, which has the comment:
> 
> 	is called in the inititalization (after all the handshkes),
> 	and after every lacpdu receive (if selected is off)
> 
> I just wonder if this should be rate limited?
So using net_warn_ratelimited()?

Displaying this message continuously in the log, even at a low rate, seems not
the best things to do. The first time seems enough, but it would require more
code for that. Is it not over-engineering?
The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that.

Regards,
Nicolas

Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown

[Andy Gospodarek]

On Fri, Jun 09, 2017 at 04:39:06PM +0200, Nicolas Dichtel wrote:
> Le 09/06/2017 à 16:23, Andrew Lunn a écrit :
> > On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote:
> >> Make it explicit in the log.
> >>
> >> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> > 
> > Hi Nicolas
> Hi Andrew,
> 
> > 
> > How often is this called? It seems like it is used by
> Good question! ;-)
> 
> > ad_port_selection_logic, which has the comment:
> > 
> > 	is called in the inititalization (after all the handshkes),
> > 	and after every lacpdu receive (if selected is off)
> > 
> > I just wonder if this should be rate limited?
> So using net_warn_ratelimited()?

Why not just use pr_warn_once()?

It may go unnoticed at first, but that seems like a decent option compared to
spamming log files.

> Displaying this message continuously in the log, even at a low rate, seems not
> the best things to do. The first time seems enough, but it would require more
> code for that. Is it not over-engineering?
> The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that.
> 

Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown

[Andrew Lunn]

> > I just wonder if this should be rate limited?
> So using net_warn_ratelimited()?
> 
> Displaying this message continuously in the log, even at a low rate, seems not
> the best things to do. The first time seems enough, but it would require more
> code for that. Is it not over-engineering?

Maybe look at the _once functions, dev_warn_once, printk_once.

> The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that.

Me neither.

   Andrew

[PATCH net-next v2] bonding: warn user when 802.3ad speed is unknown

[Nicolas Dichtel]

Goal is to advertise the user when ethtool speeds and 802.3ad speeds are
desynchronized.
When this case happens, the kernel needs to be patched.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

Here is another proposal. Comments are welcome.

v2: use pr_warn_once()
    warn only when speed != SPEED_UNKNOWN

 drivers/net/bonding/bond_3ad.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index b44a6aeb346d..165a8009c640 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -322,6 +322,11 @@ static u16 __get_link_speed(struct port *port)
 
 		default:
 			/* unknown speed value from ethtool. shouldn't happen */
+			if (slave->speed != SPEED_UNKNOWN)
+				pr_warn_once("%s: unknown ethtool speed (%d) for port %d (set it to 0)\n",
+					     slave->bond->dev->name,
+					     slave->speed,
+					     port->actor_port_number);
 			speed = 0;
 			break;
 		}
-- 
2.8.1

Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri,  9 Jun 2017 15:34:43 +0200

> Make it explicit in the log.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

I agree with others that we should rate limit this somehow given
the context in which it is invoked.

Re: [PATCH net-next v2] bonding: warn user when 802.3ad speed is unknown

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri,  9 Jun 2017 17:58:08 +0200

> Goal is to advertise the user when ethtool speeds and 802.3ad speeds are
> desynchronized.
> When this case happens, the kernel needs to be patched.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied, thanks Nicolas.