[patch net 0/2] mlxsw: Couple of fixes

[patch net 0/2] mlxsw: Couple of fixes

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Please, queue-up both for stable. Thanks!

Arkadi Sharshevsky (1):
  mlxsw: spectrum_router: Correctly dump neighbour activity

Yotam Gigi (1):
  mlxsw: spectrum: Fix refcount bug on span entries

 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     | 11 ++++++-----
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 22 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.7.4

[patch net 1/2] mlxsw: spectrum: Fix refcount bug on span entries

[Jiri Pirko]

From: Yotam Gigi <yotamg@mellanox.com>

When binding port to a newly created span entry, its refcount is set 0
even though it has a bound port. That leeds to unexpected behaviour when
the user tries to delete that port from the span entry.

Change the binding process to increase the refcount of the bound entry
even if the entry is newly created, and add warning on the process of
removing bound port from entry when its refcount is 0.

Fixes: 763b4b70afcd3 ("mlxsw: spectrum: Add support in matchall mirror TC offloading")
Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 1ec0a4c..d75c1ff 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -269,17 +269,18 @@ static struct mlxsw_sp_span_entry
 	struct mlxsw_sp_span_entry *span_entry;
 
 	span_entry = mlxsw_sp_span_entry_find(port);
-	if (span_entry) {
-		span_entry->ref_count++;
-		return span_entry;
-	}
+	if (!span_entry)
+		span_entry = mlxsw_sp_span_entry_create(port);
 
-	return mlxsw_sp_span_entry_create(port);
+	span_entry->ref_count++;
+	return span_entry;
 }
 
 static int mlxsw_sp_span_entry_put(struct mlxsw_sp *mlxsw_sp,
 				   struct mlxsw_sp_span_entry *span_entry)
 {
+	WARN_ON(!span_entry->ref_count);
+
 	if (--span_entry->ref_count == 0)
 		mlxsw_sp_span_entry_destroy(mlxsw_sp, span_entry);
 	return 0;
-- 
2.7.4

Re: [patch net 1/2] mlxsw: spectrum: Fix refcount bug on span entries

[Ido Schimmel]

On Fri, Nov 11, 2016 at 11:20:41AM +0100, Jiri Pirko wrote:
> From: Yotam Gigi <yotamg@mellanox.com>
> 
> When binding port to a newly created span entry, its refcount is set 0
> even though it has a bound port. That leeds to unexpected behaviour when

s/leeds/leads/

> the user tries to delete that port from the span entry.
> 
> Change the binding process to increase the refcount of the bound entry
> even if the entry is newly created, and add warning on the process of
> removing bound port from entry when its refcount is 0.
> 
> Fixes: 763b4b70afcd3 ("mlxsw: spectrum: Add support in matchall mirror TC offloading")

You only need the first 12 characters.

> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> index 1ec0a4c..d75c1ff 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
> @@ -269,17 +269,18 @@ static struct mlxsw_sp_span_entry
>  	struct mlxsw_sp_span_entry *span_entry;
>  
>  	span_entry = mlxsw_sp_span_entry_find(port);
> -	if (span_entry) {
> -		span_entry->ref_count++;
> -		return span_entry;
> -	}
> +	if (!span_entry)
> +		span_entry = mlxsw_sp_span_entry_create(port);
>  
> -	return mlxsw_sp_span_entry_create(port);
> +	span_entry->ref_count++;

mlxsw_sp_span_entry_create() can return NULL. You can look at
mlxsw_sp_fib_entry_get() for reference.

> +	return span_entry;
>  }
>  
>  static int mlxsw_sp_span_entry_put(struct mlxsw_sp *mlxsw_sp,
>  				   struct mlxsw_sp_span_entry *span_entry)
>  {
> +	WARN_ON(!span_entry->ref_count);
> +
>  	if (--span_entry->ref_count == 0)
>  		mlxsw_sp_span_entry_destroy(mlxsw_sp, span_entry);
>  	return 0;
> -- 
> 2.7.4
> 

Re: [patch net 1/2] mlxsw: spectrum: Fix refcount bug on span entries

[Jiri Pirko]

Fri, Nov 11, 2016 at 01:49:28PM CET, idosch@idosch.org wrote:
>On Fri, Nov 11, 2016 at 11:20:41AM +0100, Jiri Pirko wrote:
>> From: Yotam Gigi <yotamg@mellanox.com>
>> 
>> When binding port to a newly created span entry, its refcount is set 0
>> even though it has a bound port. That leeds to unexpected behaviour when
>
>s/leeds/leads/
>
>> the user tries to delete that port from the span entry.
>> 
>> Change the binding process to increase the refcount of the bound entry
>> even if the entry is newly created, and add warning on the process of
>> removing bound port from entry when its refcount is 0.
>> 
>> Fixes: 763b4b70afcd3 ("mlxsw: spectrum: Add support in matchall mirror TC offloading")
>
>You only need the first 12 characters.
>
>> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> index 1ec0a4c..d75c1ff 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> @@ -269,17 +269,18 @@ static struct mlxsw_sp_span_entry
>>  	struct mlxsw_sp_span_entry *span_entry;
>>  
>>  	span_entry = mlxsw_sp_span_entry_find(port);
>> -	if (span_entry) {
>> -		span_entry->ref_count++;
>> -		return span_entry;
>> -	}
>> +	if (!span_entry)
>> +		span_entry = mlxsw_sp_span_entry_create(port);
>>  
>> -	return mlxsw_sp_span_entry_create(port);
>> +	span_entry->ref_count++;
>
>mlxsw_sp_span_entry_create() can return NULL. You can look at
>mlxsw_sp_fib_entry_get() for reference.

Right, missed that. Will fix. Thanks.

[patch net 2/2] mlxsw: spectrum_router: Correctly dump neighbour activity

[Jiri Pirko]

From: Arkadi Sharshevsky <arkadis@mellanox.com>

During neighbour activity check the device's table is dumped by multiple
query requests. The query session should end when the response carries
less records than requested or when a given record is not full. Current
code only stops the dumping process if the number of returned records is
zero, which can result in infinite loop in case of activity.

Fix this by stopping the dumping process according to the above logic.

Fixes: c723c735fa6b ("mlxsw: spectrum_router: Periodically update the kernel's neigh table")
Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 040737e..d437457 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -800,6 +800,26 @@ static void mlxsw_sp_router_neigh_rec_process(struct mlxsw_sp *mlxsw_sp,
 	}
 }
 
+static bool mlxsw_sp_router_rauhtd_is_full(char *rauhtd_pl)
+{
+	u8 num_rec, last_rec_index, num_entries;
+
+	num_rec = mlxsw_reg_rauhtd_num_rec_get(rauhtd_pl);
+	last_rec_index = num_rec - 1;
+
+	if (num_rec < MLXSW_REG_RAUHTD_REC_MAX_NUM)
+		return false;
+	if (mlxsw_reg_rauhtd_rec_type_get(rauhtd_pl, last_rec_index) ==
+	    MLXSW_REG_RAUHTD_TYPE_IPV6)
+		return true;
+
+	num_entries = mlxsw_reg_rauhtd_ipv4_rec_num_entries_get(rauhtd_pl,
+								last_rec_index);
+	if (++num_entries ==  MLXSW_REG_RAUHTD_IPV4_ENT_PER_REC)
+		return true;
+	return false;
+}
+
 static int mlxsw_sp_router_neighs_update_rauhtd(struct mlxsw_sp *mlxsw_sp)
 {
 	char *rauhtd_pl;
@@ -826,7 +846,7 @@ static int mlxsw_sp_router_neighs_update_rauhtd(struct mlxsw_sp *mlxsw_sp)
 		for (i = 0; i < num_rec; i++)
 			mlxsw_sp_router_neigh_rec_process(mlxsw_sp, rauhtd_pl,
 							  i);
-	} while (num_rec);
+	} while (mlxsw_sp_router_rauhtd_is_full(rauhtd_pl));
 	rtnl_unlock();
 
 	kfree(rauhtd_pl);
-- 
2.7.4

Re: [patch net 2/2] mlxsw: spectrum_router: Correctly dump neighbour activity

[Ido Schimmel]

On Fri, Nov 11, 2016 at 11:20:42AM +0100, Jiri Pirko wrote:
> From: Arkadi Sharshevsky <arkadis@mellanox.com>
> 
> During neighbour activity check the device's table is dumped by multiple
> query requests. The query session should end when the response carries
> less records than requested or when a given record is not full. Current
> code only stops the dumping process if the number of returned records is
> zero, which can result in infinite loop in case of activity.
> 
> Fix this by stopping the dumping process according to the above logic.
> 
> Fixes: c723c735fa6b ("mlxsw: spectrum_router: Periodically update the kernel's neigh table")
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> index 040737e..d437457 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> @@ -800,6 +800,26 @@ static void mlxsw_sp_router_neigh_rec_process(struct mlxsw_sp *mlxsw_sp,
>  	}
>  }
>  
> +static bool mlxsw_sp_router_rauhtd_is_full(char *rauhtd_pl)
> +{
> +	u8 num_rec, last_rec_index, num_entries;
> +
> +	num_rec = mlxsw_reg_rauhtd_num_rec_get(rauhtd_pl);
> +	last_rec_index = num_rec - 1;
> +
> +	if (num_rec < MLXSW_REG_RAUHTD_REC_MAX_NUM)
> +		return false;
> +	if (mlxsw_reg_rauhtd_rec_type_get(rauhtd_pl, last_rec_index) ==
> +	    MLXSW_REG_RAUHTD_TYPE_IPV6)
> +		return true;
> +
> +	num_entries = mlxsw_reg_rauhtd_ipv4_rec_num_entries_get(rauhtd_pl,
> +								last_rec_index);
> +	if (++num_entries ==  MLXSW_REG_RAUHTD_IPV4_ENT_PER_REC)

Jiri, I just noticed we have an extra space after the '=='. Can you
please remove it in v2? Sorry for not spotting this earlier.

> +		return true;
> +	return false;
> +}
> +
>  static int mlxsw_sp_router_neighs_update_rauhtd(struct mlxsw_sp *mlxsw_sp)
>  {
>  	char *rauhtd_pl;
> @@ -826,7 +846,7 @@ static int mlxsw_sp_router_neighs_update_rauhtd(struct mlxsw_sp *mlxsw_sp)
>  		for (i = 0; i < num_rec; i++)
>  			mlxsw_sp_router_neigh_rec_process(mlxsw_sp, rauhtd_pl,
>  							  i);
> -	} while (num_rec);
> +	} while (mlxsw_sp_router_rauhtd_is_full(rauhtd_pl));
>  	rtnl_unlock();
>  
>  	kfree(rauhtd_pl);
> -- 
> 2.7.4
> 

Re: [patch net 2/2] mlxsw: spectrum_router: Correctly dump neighbour activity

[Jiri Pirko]

Fri, Nov 11, 2016 at 01:54:05PM CET, idosch@idosch.org wrote:
>On Fri, Nov 11, 2016 at 11:20:42AM +0100, Jiri Pirko wrote:
>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>> 
>> During neighbour activity check the device's table is dumped by multiple
>> query requests. The query session should end when the response carries
>> less records than requested or when a given record is not full. Current
>> code only stops the dumping process if the number of returned records is
>> zero, which can result in infinite loop in case of activity.
>> 
>> Fix this by stopping the dumping process according to the above logic.
>> 
>> Fixes: c723c735fa6b ("mlxsw: spectrum_router: Periodically update the kernel's neigh table")
>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> index 040737e..d437457 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> @@ -800,6 +800,26 @@ static void mlxsw_sp_router_neigh_rec_process(struct mlxsw_sp *mlxsw_sp,
>>  	}
>>  }
>>  
>> +static bool mlxsw_sp_router_rauhtd_is_full(char *rauhtd_pl)
>> +{
>> +	u8 num_rec, last_rec_index, num_entries;
>> +
>> +	num_rec = mlxsw_reg_rauhtd_num_rec_get(rauhtd_pl);
>> +	last_rec_index = num_rec - 1;
>> +
>> +	if (num_rec < MLXSW_REG_RAUHTD_REC_MAX_NUM)
>> +		return false;
>> +	if (mlxsw_reg_rauhtd_rec_type_get(rauhtd_pl, last_rec_index) ==
>> +	    MLXSW_REG_RAUHTD_TYPE_IPV6)
>> +		return true;
>> +
>> +	num_entries = mlxsw_reg_rauhtd_ipv4_rec_num_entries_get(rauhtd_pl,
>> +								last_rec_index);
>> +	if (++num_entries ==  MLXSW_REG_RAUHTD_IPV4_ENT_PER_REC)
>
>Jiri, I just noticed we have an extra space after the '=='. Can you
>please remove it in v2? Sorry for not spotting this earlier.


Will do.

Re: [patch net 0/2] mlxsw: Couple of fixes

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 11 Nov 2016 11:20:40 +0100

> Please, queue-up both for stable. Thanks!

Series applied and queued up for -stable, thanks.