[PATCH net 0/5] mlxsw: Various fixes

[PATCH net 0/5] mlxsw: Various fixes

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

This patch set contains various fixes for the mlxsw driver.

Patch #1 fixes an issue introduced in 5.6 in which a route in the main
table can replace an identical route in the local table despite the
local table having an higher precedence.

Patch #2 contains a test case for the bug fixed in patch #1.

Patch #3 also fixes an issue introduced in 5.6 in which the driver
failed to clear the offload indication from IPv6 nexthops upon abort.

Patch #4 fixes an issue that prevents the driver from loading on
Spectrum-3 systems. The problem and solution are explained in detail in
the commit message.

Patch #5 adds a missing error path. Discovered using smatch.

Ido Schimmel (4):
  mlxsw: spectrum_router: Prevent incorrect replacement of local table
    routes
  selftests: mlxsw: Add test cases for local table route replacement
  mlxsw: spectrum_router: Clear offload indication from IPv6 nexthops on
    abort
  mlxsw: spectrum_dpipe: Add missing error path

Vadim Pasternak (1):
  mlxsw: core: Add validation of hardware device types for MGPIR
    register

 .../net/ethernet/mellanox/mlxsw/core_hwmon.c  |  6 +-
 .../ethernet/mellanox/mlxsw/core_thermal.c    |  8 +-
 .../ethernet/mellanox/mlxsw/spectrum_dpipe.c  |  3 +-
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 55 +++++++++++++-
 .../selftests/drivers/net/mlxsw/fib.sh        | 76 +++++++++++++++++++
 5 files changed, 142 insertions(+), 6 deletions(-)

-- 
2.24.1

[PATCH net 1/5] mlxsw: spectrum_router: Prevent incorrect replacement of local table routes

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

The driver uses the same table to represent both the main and local
routing tables. Prevent routes in the main table from replacing routes
in the local table to reflect the fact that the local table is consulted
first during lookup.

Fixes: b6a1d871d37a ("mlxsw: spectrum_router: Start using new IPv4 route notifications")
Fixes: dacad7b34b59 ("mlxsw: spectrum_router: Start using new IPv6 route notifications")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 52 ++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index ce707723f8cf..f8b3869672c3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -4844,6 +4844,23 @@ mlxsw_sp_fib_node_entry_unlink(struct mlxsw_sp *mlxsw_sp,
 	fib_node->fib_entry = NULL;
 }
 
+static bool mlxsw_sp_fib4_allow_replace(struct mlxsw_sp_fib4_entry *fib4_entry)
+{
+	struct mlxsw_sp_fib_node *fib_node = fib4_entry->common.fib_node;
+	struct mlxsw_sp_fib4_entry *fib4_replaced;
+
+	if (!fib_node->fib_entry)
+		return true;
+
+	fib4_replaced = container_of(fib_node->fib_entry,
+				     struct mlxsw_sp_fib4_entry, common);
+	if (fib4_entry->tb_id == RT_TABLE_MAIN &&
+	    fib4_replaced->tb_id == RT_TABLE_LOCAL)
+		return false;
+
+	return true;
+}
+
 static int
 mlxsw_sp_router_fib4_replace(struct mlxsw_sp *mlxsw_sp,
 			     const struct fib_entry_notifier_info *fen_info)
@@ -4872,6 +4889,12 @@ mlxsw_sp_router_fib4_replace(struct mlxsw_sp *mlxsw_sp,
 		goto err_fib4_entry_create;
 	}
 
+	if (!mlxsw_sp_fib4_allow_replace(fib4_entry)) {
+		mlxsw_sp_fib4_entry_destroy(mlxsw_sp, fib4_entry);
+		mlxsw_sp_fib_node_put(mlxsw_sp, fib_node);
+		return 0;
+	}
+
 	replaced = fib_node->fib_entry;
 	err = mlxsw_sp_fib_node_entry_link(mlxsw_sp, &fib4_entry->common);
 	if (err) {
@@ -4908,7 +4931,7 @@ static void mlxsw_sp_router_fib4_del(struct mlxsw_sp *mlxsw_sp,
 		return;
 
 	fib4_entry = mlxsw_sp_fib4_entry_lookup(mlxsw_sp, fen_info);
-	if (WARN_ON(!fib4_entry))
+	if (!fib4_entry)
 		return;
 	fib_node = fib4_entry->common.fib_node;
 
@@ -5408,6 +5431,27 @@ mlxsw_sp_fib6_entry_lookup(struct mlxsw_sp *mlxsw_sp,
 	return NULL;
 }
 
+static bool mlxsw_sp_fib6_allow_replace(struct mlxsw_sp_fib6_entry *fib6_entry)
+{
+	struct mlxsw_sp_fib_node *fib_node = fib6_entry->common.fib_node;
+	struct mlxsw_sp_fib6_entry *fib6_replaced;
+	struct fib6_info *rt, *rt_replaced;
+
+	if (!fib_node->fib_entry)
+		return true;
+
+	fib6_replaced = container_of(fib_node->fib_entry,
+				     struct mlxsw_sp_fib6_entry,
+				     common);
+	rt = mlxsw_sp_fib6_entry_rt(fib6_entry);
+	rt_replaced = mlxsw_sp_fib6_entry_rt(fib6_replaced);
+	if (rt->fib6_table->tb6_id == RT_TABLE_MAIN &&
+	    rt_replaced->fib6_table->tb6_id == RT_TABLE_LOCAL)
+		return false;
+
+	return true;
+}
+
 static int mlxsw_sp_router_fib6_replace(struct mlxsw_sp *mlxsw_sp,
 					struct fib6_info **rt_arr,
 					unsigned int nrt6)
@@ -5442,6 +5486,12 @@ static int mlxsw_sp_router_fib6_replace(struct mlxsw_sp *mlxsw_sp,
 		goto err_fib6_entry_create;
 	}
 
+	if (!mlxsw_sp_fib6_allow_replace(fib6_entry)) {
+		mlxsw_sp_fib6_entry_destroy(mlxsw_sp, fib6_entry);
+		mlxsw_sp_fib_node_put(mlxsw_sp, fib_node);
+		return 0;
+	}
+
 	replaced = fib_node->fib_entry;
 	err = mlxsw_sp_fib_node_entry_link(mlxsw_sp, &fib6_entry->common);
 	if (err)
-- 
2.24.1

[PATCH net 2/5] selftests: mlxsw: Add test cases for local table route replacement

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

Test that routes in the main table do not replace identical routes in
the local table and that routes in the local table do replace identical
routes in the main table.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 .../selftests/drivers/net/mlxsw/fib.sh        | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/fib.sh b/tools/testing/selftests/drivers/net/mlxsw/fib.sh
index 45115f81c2b1..eab79b9e58cd 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/fib.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/fib.sh
@@ -14,6 +14,7 @@ ALL_TESTS="
 	ipv4_plen
 	ipv4_replay
 	ipv4_flush
+	ipv4_local_replace
 	ipv6_add
 	ipv6_metric
 	ipv6_append_single
@@ -26,6 +27,7 @@ ALL_TESTS="
 	ipv6_delete_multipath
 	ipv6_replay_single
 	ipv6_replay_multipath
+	ipv6_local_replace
 "
 NUM_NETIFS=0
 source $lib_dir/lib.sh
@@ -89,6 +91,43 @@ ipv4_flush()
 	fib_ipv4_flush_test "testns1"
 }
 
+ipv4_local_replace()
+{
+	local ns="testns1"
+
+	RET=0
+
+	ip -n $ns link add name dummy1 type dummy
+	ip -n $ns link set dev dummy1 up
+
+	ip -n $ns route add table local 192.0.2.1/32 dev dummy1
+	fib4_trap_check $ns "table local 192.0.2.1/32 dev dummy1" false
+	check_err $? "Local table route not in hardware when should"
+
+	ip -n $ns route add table main 192.0.2.1/32 dev dummy1
+	fib4_trap_check $ns "table main 192.0.2.1/32 dev dummy1" true
+	check_err $? "Main table route in hardware when should not"
+
+	fib4_trap_check $ns "table local 192.0.2.1/32 dev dummy1" false
+	check_err $? "Local table route was replaced when should not"
+
+	# Test that local routes can replace routes in main table.
+	ip -n $ns route add table main 192.0.2.2/32 dev dummy1
+	fib4_trap_check $ns "table main 192.0.2.2/32 dev dummy1" false
+	check_err $? "Main table route not in hardware when should"
+
+	ip -n $ns route add table local 192.0.2.2/32 dev dummy1
+	fib4_trap_check $ns "table local 192.0.2.2/32 dev dummy1" false
+	check_err $? "Local table route did not replace route in main table when should"
+
+	fib4_trap_check $ns "table main 192.0.2.2/32 dev dummy1" true
+	check_err $? "Main table route was not replaced when should"
+
+	log_test "IPv4 local table route replacement"
+
+	ip -n $ns link del dev dummy1
+}
+
 ipv6_add()
 {
 	fib_ipv6_add_test "testns1"
@@ -149,6 +188,43 @@ ipv6_replay_multipath()
 	fib_ipv6_replay_multipath_test "testns1" "$DEVLINK_DEV"
 }
 
+ipv6_local_replace()
+{
+	local ns="testns1"
+
+	RET=0
+
+	ip -n $ns link add name dummy1 type dummy
+	ip -n $ns link set dev dummy1 up
+
+	ip -n $ns route add table local 2001:db8:1::1/128 dev dummy1
+	fib6_trap_check $ns "table local 2001:db8:1::1/128 dev dummy1" false
+	check_err $? "Local table route not in hardware when should"
+
+	ip -n $ns route add table main 2001:db8:1::1/128 dev dummy1
+	fib6_trap_check $ns "table main 2001:db8:1::1/128 dev dummy1" true
+	check_err $? "Main table route in hardware when should not"
+
+	fib6_trap_check $ns "table local 2001:db8:1::1/128 dev dummy1" false
+	check_err $? "Local table route was replaced when should not"
+
+	# Test that local routes can replace routes in main table.
+	ip -n $ns route add table main 2001:db8:1::2/128 dev dummy1
+	fib6_trap_check $ns "table main 2001:db8:1::2/128 dev dummy1" false
+	check_err $? "Main table route not in hardware when should"
+
+	ip -n $ns route add table local 2001:db8:1::2/128 dev dummy1
+	fib6_trap_check $ns "table local 2001:db8:1::2/128 dev dummy1" false
+	check_err $? "Local route route did not replace route in main table when should"
+
+	fib6_trap_check $ns "table main 2001:db8:1::2/128 dev dummy1" true
+	check_err $? "Main table route was not replaced when should"
+
+	log_test "IPv6 local table route replacement"
+
+	ip -n $ns link del dev dummy1
+}
+
 setup_prepare()
 {
 	ip netns add testns1
-- 
2.24.1

[PATCH net 3/5] mlxsw: spectrum_router: Clear offload indication from IPv6 nexthops on abort

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

Unlike IPv4, in IPv6 there is no unique structure to represent the
nexthop and both the route and nexthop information are squashed to the
same structure ('struct fib6_info'). In order to improve resource
utilization the driver consolidates identical nexthop groups to the same
internal representation of a nexthop group.

Therefore, when the offload indication of a nexthop changes, the driver
needs to iterate over all the linked fib6_info and toggle their offload
flag accordingly.

During abort, all the routes are removed from the device and unlinked
from their nexthop group. The offload indication is cleared just before
the group is destroyed, but by that time no fib6_info is linked to the
group and the offload indication remains set.

Fix this by clearing the offload indication just before dropping the
reference from the nexthop.

Fixes: ee5a0448e72b ("mlxsw: spectrum_router: Set hardware flags for routes")
Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Tested-by: Alex Kushnarov <alexanderk@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index f8b3869672c3..4a77b511ead2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -4993,6 +4993,9 @@ static void mlxsw_sp_rt6_release(struct fib6_info *rt)
 
 static void mlxsw_sp_rt6_destroy(struct mlxsw_sp_rt6 *mlxsw_sp_rt6)
 {
+	struct fib6_nh *fib6_nh = mlxsw_sp_rt6->rt->fib6_nh;
+
+	fib6_nh->fib_nh_flags &= ~RTNH_F_OFFLOAD;
 	mlxsw_sp_rt6_release(mlxsw_sp_rt6->rt);
 	kfree(mlxsw_sp_rt6);
 }
-- 
2.24.1

[PATCH net 4/5] mlxsw: core: Add validation of hardware device types for MGPIR register

[Ido Schimmel]

From: Vadim Pasternak <vadimp@mellanox.com>

When reading the number of gearboxes from the hardware, the driver does
not validate the returned 'device type' field. The driver can therefore
wrongly assume that the queried devices are gearboxes.

On Spectrum-3 systems that support different types of devices, this can
prevent the driver from loading, as it will try to query the
temperature sensors from devices which it assumes are gearboxes and in
fact are not.

For example:
[  218.129230] mlxsw_minimal 2-0048: Reg cmd access status failed (status=7(bad parameter))
[  218.138282] mlxsw_minimal 2-0048: Reg cmd access failed (reg_id=900a(mtmp),type=write)
[  218.147131] mlxsw_minimal 2-0048: Failed to setup temp sensor number 256
[  218.534480] mlxsw_minimal 2-0048: Fail to register core bus
[  218.540714] mlxsw_minimal: probe of 2-0048 failed with error -5

Fix this by validating the 'device type' field.

Fixes: 2e265a8b6c094 ("mlxsw: core: Extend hwmon interface with inter-connect temperature attributes")
Fixes: f14f4e621b1b4 ("mlxsw: core: Extend thermal core with per inter-connect device thermal zones")
Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core_hwmon.c   | 6 ++++--
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 8 ++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_hwmon.c b/drivers/net/ethernet/mellanox/mlxsw/core_hwmon.c
index 9bf8da5f6daf..3fe878d7c94c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_hwmon.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_hwmon.c
@@ -573,6 +573,7 @@ static int mlxsw_hwmon_module_init(struct mlxsw_hwmon *mlxsw_hwmon)
 
 static int mlxsw_hwmon_gearbox_init(struct mlxsw_hwmon *mlxsw_hwmon)
 {
+	enum mlxsw_reg_mgpir_device_type device_type;
 	int index, max_index, sensor_index;
 	char mgpir_pl[MLXSW_REG_MGPIR_LEN];
 	char mtmp_pl[MLXSW_REG_MTMP_LEN];
@@ -584,8 +585,9 @@ static int mlxsw_hwmon_gearbox_init(struct mlxsw_hwmon *mlxsw_hwmon)
 	if (err)
 		return err;
 
-	mlxsw_reg_mgpir_unpack(mgpir_pl, &gbox_num, NULL, NULL, NULL);
-	if (!gbox_num)
+	mlxsw_reg_mgpir_unpack(mgpir_pl, &gbox_num, &device_type, NULL, NULL);
+	if (device_type != MLXSW_REG_MGPIR_DEVICE_TYPE_GEARBOX_DIE ||
+	    !gbox_num)
 		return 0;
 
 	index = mlxsw_hwmon->module_sensor_max;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
index c721b171bd8d..ce0a6837daa3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
@@ -895,8 +895,10 @@ static int
 mlxsw_thermal_gearboxes_init(struct device *dev, struct mlxsw_core *core,
 			     struct mlxsw_thermal *thermal)
 {
+	enum mlxsw_reg_mgpir_device_type device_type;
 	struct mlxsw_thermal_module *gearbox_tz;
 	char mgpir_pl[MLXSW_REG_MGPIR_LEN];
+	u8 gbox_num;
 	int i;
 	int err;
 
@@ -908,11 +910,13 @@ mlxsw_thermal_gearboxes_init(struct device *dev, struct mlxsw_core *core,
 	if (err)
 		return err;
 
-	mlxsw_reg_mgpir_unpack(mgpir_pl, &thermal->tz_gearbox_num, NULL, NULL,
+	mlxsw_reg_mgpir_unpack(mgpir_pl, &gbox_num, &device_type, NULL,
 			       NULL);
-	if (!thermal->tz_gearbox_num)
+	if (device_type != MLXSW_REG_MGPIR_DEVICE_TYPE_GEARBOX_DIE ||
+	    !gbox_num)
 		return 0;
 
+	thermal->tz_gearbox_num = gbox_num;
 	thermal->tz_gearbox_arr = kcalloc(thermal->tz_gearbox_num,
 					  sizeof(*thermal->tz_gearbox_arr),
 					  GFP_KERNEL);
-- 
2.24.1

[PATCH net 5/5] mlxsw: spectrum_dpipe: Add missing error path

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

In case devlink_dpipe_entry_ctx_prepare() failed, release RTNL that was
previously taken and free the memory allocated by
mlxsw_sp_erif_entry_prepare().

Fixes: 2ba5999f009d ("mlxsw: spectrum: Add Support for erif table entries access")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c
index 49933818c6f5..2dc0978428e6 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c
@@ -215,7 +215,7 @@ mlxsw_sp_dpipe_table_erif_entries_dump(void *priv, bool counters_enabled,
 start_again:
 	err = devlink_dpipe_entry_ctx_prepare(dump_ctx);
 	if (err)
-		return err;
+		goto err_ctx_prepare;
 	j = 0;
 	for (; i < rif_count; i++) {
 		struct mlxsw_sp_rif *rif = mlxsw_sp_rif_by_index(mlxsw_sp, i);
@@ -247,6 +247,7 @@ mlxsw_sp_dpipe_table_erif_entries_dump(void *priv, bool counters_enabled,
 	return 0;
 err_entry_append:
 err_entry_get:
+err_ctx_prepare:
 	rtnl_unlock();
 	devlink_dpipe_entry_clear(&entry);
 	return err;
-- 
2.24.1

Re: [PATCH net 0/5] mlxsw: Various fixes

[David Miller]

From: Ido Schimmel <idosch@idosch.org>
Date: Fri,  7 Feb 2020 19:26:23 +0200

> From: Ido Schimmel <idosch@mellanox.com>
> 
> This patch set contains various fixes for the mlxsw driver.
> 
> Patch #1 fixes an issue introduced in 5.6 in which a route in the main
> table can replace an identical route in the local table despite the
> local table having an higher precedence.
> 
> Patch #2 contains a test case for the bug fixed in patch #1.
> 
> Patch #3 also fixes an issue introduced in 5.6 in which the driver
> failed to clear the offload indication from IPv6 nexthops upon abort.
> 
> Patch #4 fixes an issue that prevents the driver from loading on
> Spectrum-3 systems. The problem and solution are explained in detail in
> the commit message.
> 
> Patch #5 adds a missing error path. Discovered using smatch.

Series applied, thank you.