Skip to contentSkip to conversation

mlxsw: Preparations for out-of-order-operations patches

PATCH In review Petr Machata 3 participants 14 messages Jun 12, 2023 – Jun 14, 2023

Subsystems (3)
Networking Network drivers Mellanox Ethernet switch drivers
lore
Share & tools
lore

Petr Machata

author maintainer
#0 lore raw

cover mlxsw: Preparations for out-of-order-operations patches

The mlxsw driver currently makes the assumption that the user applies configuration in a bottom-up manner. Thus netdevices need to be added to the bridge before IP addresses are configured on that bridge or SVI added on top of it. Enslaving a netdevice to another netdevice that already has uppers is in fact forbidden by mlxsw for this reason. Despite this safety, it is rather easy to get into situations where the offloaded configuration is just plain wrong.

As an example, take a front panel port, configure an IP address: it gets a RIF. Now enslave the port to a bridge, and the RIF is gone. Remove the port from the bridge again, but the RIF never comes back. There is a number of similar situations, where changing the configuration there and back utterly breaks the offload.

Over the course of the following several patchsets, mlxsw code is going to be adjusted to diminish the space of wrongly offloaded configurations. Ideally the offload state will reflect the actual state, regardless of the sequence of operation used to construct that state.

No functional changes are intended in this patchset yet. Rather the patches prepare the codebase for easier introduction of functional changes in later patchsets.

- In patch #1, extract a helper to join a RIF of a given port, if there is one. In patch #2, use it in a newly-added helper to join a LAG interface.

- In patches #3, #4 and #5, add helpers that abstract away the rif->dev access. This will make it simpler in the future to change the way the deduction is done. In patch #6, do this for deduction from nexthop group info to RIF.

- In patch #7, add a helper to destroy a RIF. So far RIF was destroyed simply by kfree'ing it.

- In patch #8, add a helper to check if any IP addresses are configured on a netdevice. This helper will be useful later.

- In patch #9, add a helper to migrate a RIF. This will be a convenient place to put extensions later on.

- Patch #10 move IPIP initialization up to make ipip_ops_arr available earlier.

Petr Machata (10):
mlxsw: spectrum_router: Extract a helper from
mlxsw_sp_port_vlan_router_join()
mlxsw: spectrum_router: Add a helper specifically for joining a LAG
mlxsw: spectrum_router: Access rif->dev through a helper
mlxsw: spectrum_router: Access rif->dev from params in
mlxsw_sp_rif_create()
mlxsw: spectrum_router: Access nh->rif->dev through a helper
mlxsw: spectrum_router: Access nhgi->rif through a helper
mlxsw: spectrum_router: Extract a helper to free a RIF
mlxsw: spectrum_router: Add a helper to check if netdev has addresses
mlxsw: spectrum_router: Extract a helper for RIF migration
mlxsw: spectrum_router: Move IPIP init up

 .../net/ethernet/mellanox/mlxsw/spectrum.c    |   4 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   4 -
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 311 ++++++++++++------
 .../ethernet/mellanox/mlxsw/spectrum_router.h |   3 +
 4 files changed, 212 insertions(+), 110 deletions(-)
Signature · 1 line
2.40.1
author maintainer
#1 lore raw

PATCH 1/10 mlxsw: spectrum_router: Extract a helper from mlxsw_sp_port_vlan_router_join()

Split out of mlxsw_sp_port_vlan_router_join() the part that checks for RIF and dispatches to __mlxsw_sp_port_vlan_router_join(), leaving it as wrapper that just manages the router lock.

The new function, mlxsw_sp_port_vlan_router_join_existing(), will be useful as an atom in later patches.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 29 +++++++++++++------
 1 file changed, 20 insertions(+), 9 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +20 -9
index f9328e8410f5..0edda06e92bb 100644
@@ -8562,24 +8562,35 @@ __mlxsw_sp_port_vlan_router_leave(struct mlxsw_sp_port_vlan *mlxsw_sp_port_vlan)
 	mlxsw_sp_rif_subport_put(rif);
 }
 
+static int
+mlxsw_sp_port_vlan_router_join_existing(struct mlxsw_sp_port_vlan *mlxsw_sp_port_vlan,
+					struct net_device *l3_dev,
+					struct netlink_ext_ack *extack)
+{
+	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port_vlan->mlxsw_sp_port->mlxsw_sp;
+
+	lockdep_assert_held(&mlxsw_sp->router->lock);
+
+	if (!mlxsw_sp_rif_find_by_dev(mlxsw_sp, l3_dev))
+		return 0;
+
+	return __mlxsw_sp_port_vlan_router_join(mlxsw_sp_port_vlan, l3_dev,
+						extack);
+}
+
 int
 mlxsw_sp_port_vlan_router_join(struct mlxsw_sp_port_vlan *mlxsw_sp_port_vlan,
 			       struct net_device *l3_dev,
 			       struct netlink_ext_ack *extack)
 {
 	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port_vlan->mlxsw_sp_port->mlxsw_sp;
-	struct mlxsw_sp_rif *rif;
-	int err = 0;
+	int err;
 
 	mutex_lock(&mlxsw_sp->router->lock);
-	rif = mlxsw_sp_rif_find_by_dev(mlxsw_sp, l3_dev);
-	if (!rif)
-		goto out;
-
-	err = __mlxsw_sp_port_vlan_router_join(mlxsw_sp_port_vlan, l3_dev,
-					       extack);
-out:
+	err = mlxsw_sp_port_vlan_router_join_existing(mlxsw_sp_port_vlan,
+						      l3_dev, extack);
 	mutex_unlock(&mlxsw_sp->router->lock);
+
 	return err;
 }
Signature · 1 line
2.40.1
author maintainer
#2 lore raw

PATCH 2/10 mlxsw: spectrum_router: Add a helper specifically for joining a LAG

In reply to Petr Machata

Currently, joining a LAG very simply means that the LAG RIF should be joined by the subport representing untagged traffic. If the RIF does not exist, it does not have to be created: if the user wants there to be RIF for the LAG device, they are supposed to add an IP address, and they are supposed to do it after tha LAG becomes mlxsw upper.

We can also assume that the LAG has no uppers, otherwise the enslavement is not allowed.

In the future, these ordering dependencies should be removed. That means that joining LAG will be more complex operation, possibly involving a lazy RIF creation, and possibly joining / lazily creating RIFs for VLAN uppers of the LAG. It will be handy to have a dedicated function that handles all this. The new function mlxsw_sp_router_port_join_lag() is that.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  4 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  4 --
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 56 +++++++++++++------
 .../ethernet/mellanox/mlxsw/spectrum_router.h |  3 +
 4 files changed, 45 insertions(+), 22 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum.c +2 -2
index 4609b13bda02..25a01dafde1b 100644
@@ -4337,8 +4337,8 @@ static int mlxsw_sp_port_lag_join(struct mlxsw_sp_port *mlxsw_sp_port,
 		mlxsw_sp_port_vlan_router_leave(mlxsw_sp_port->default_vlan);
 
 	/* Join a router interface configured on the LAG, if exists */
-	err = mlxsw_sp_port_vlan_router_join(mlxsw_sp_port->default_vlan,
-					     lag_dev, extack);
+	err = mlxsw_sp_router_port_join_lag(mlxsw_sp_port, lag_dev,
+					    extack);
 	if (err)
 		goto err_router_join;
drivers/net/ethernet/mellanox/mlxsw/spectrum.h +0 -4
index 0b57c8d0cce0..231e364cbb7c 100644
@@ -755,10 +755,6 @@ int mlxsw_sp_router_init(struct mlxsw_sp *mlxsw_sp,
 void mlxsw_sp_router_fini(struct mlxsw_sp *mlxsw_sp);
 void mlxsw_sp_rif_macvlan_del(struct mlxsw_sp *mlxsw_sp,
 			      const struct net_device *macvlan_dev);
-int
-mlxsw_sp_port_vlan_router_join(struct mlxsw_sp_port_vlan *mlxsw_sp_port_vlan,
-			       struct net_device *l3_dev,
-			       struct netlink_ext_ack *extack);
 void
 mlxsw_sp_port_vlan_router_leave(struct mlxsw_sp_port_vlan *mlxsw_sp_port_vlan);
 void mlxsw_sp_rif_destroy_by_dev(struct mlxsw_sp *mlxsw_sp,
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +40 -16

68 lines not on this page · raw message

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h +3 -0
index 5ff443f27136..5a0babc614b4 100644
@@ -170,5 +170,8 @@ int mlxsw_sp_ipip_ecn_encap_init(struct mlxsw_sp *mlxsw_sp);
 int mlxsw_sp_ipip_ecn_decap_init(struct mlxsw_sp *mlxsw_sp);
 struct net_device *
 mlxsw_sp_ipip_netdev_ul_dev_get(const struct net_device *ol_dev);
+int mlxsw_sp_router_port_join_lag(struct mlxsw_sp_port *mlxsw_sp_port,
+				  struct net_device *lag_dev,
+				  struct netlink_ext_ack *extack);
 
 #endif /* _MLXSW_ROUTER_H_*/
Signature · 1 line
2.40.1
author maintainer
#3 lore raw

PATCH 3/10 mlxsw: spectrum_router: Access rif->dev through a helper

In reply to Petr Machata

In order to abstract away deduction of netdevice from the corresponding RIF, introduce a helper, mlxsw_sp_rif_dev(), and use it throughout. This will make it possible to change the deduction path easily later on.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 109 ++++++++++--------
 1 file changed, 64 insertions(+), 45 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +64 -45
index 2c3dcbc2f9a6..e9183c223575 100644
@@ -71,6 +71,11 @@ struct mlxsw_sp_rif {
 	bool counter_egress_valid;
 };
 
+static struct net_device *mlxsw_sp_rif_dev(const struct mlxsw_sp_rif *rif)
+{
+	return rif->dev;
+}
+
 struct mlxsw_sp_rif_params {
 	struct net_device *dev;
 	union {

@@ -1560,6 +1565,7 @@ mlxsw_sp_rif_ipip_lb_op(struct mlxsw_sp_rif_ipip_lb *lb_rif, u16 ul_vr_id,
 			u16 ul_rif_id, bool enable)
 {
 	struct mlxsw_sp_rif_ipip_lb_config lb_cf = lb_rif->lb_config;
+	struct net_device *dev = mlxsw_sp_rif_dev(&lb_rif->common);
 	enum mlxsw_reg_ritr_loopback_ipip_options ipip_options;
 	struct mlxsw_sp_rif *rif = &lb_rif->common;
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;

@@ -1572,7 +1578,7 @@ mlxsw_sp_rif_ipip_lb_op(struct mlxsw_sp_rif_ipip_lb *lb_rif, u16 ul_vr_id,
 	case MLXSW_SP_L3_PROTO_IPV4:
 		saddr4 = be32_to_cpu(lb_cf.saddr.addr4);
 		mlxsw_reg_ritr_pack(ritr_pl, enable, MLXSW_REG_RITR_LOOPBACK_IF,
-				    rif->rif_index, rif->vr_id, rif->dev->mtu);
+				    rif->rif_index, rif->vr_id, dev->mtu);
 		mlxsw_reg_ritr_loopback_ipip4_pack(ritr_pl, lb_cf.lb_ipipt,
 						   ipip_options, ul_vr_id,
 						   ul_rif_id, saddr4,

@@ -1582,7 +1588,7 @@ mlxsw_sp_rif_ipip_lb_op(struct mlxsw_sp_rif_ipip_lb *lb_rif, u16 ul_vr_id,
 	case MLXSW_SP_L3_PROTO_IPV6:
 		saddr6 = &lb_cf.saddr.addr6;
 		mlxsw_reg_ritr_pack(ritr_pl, enable, MLXSW_REG_RITR_LOOPBACK_IF,
-				    rif->rif_index, rif->vr_id, rif->dev->mtu);
+				    rif->rif_index, rif->vr_id, dev->mtu);
 		mlxsw_reg_ritr_loopback_ipip6_pack(ritr_pl, lb_cf.lb_ipipt,
 						   ipip_options, ul_vr_id,
 						   ul_rif_id, saddr6,

@@ -2332,7 +2338,7 @@ static void mlxsw_sp_router_neigh_ent_ipv4_process(struct mlxsw_sp *mlxsw_sp,
 	}
 
 	dipn = htonl(dip);
-	dev = mlxsw_sp->router->rifs[rif]->dev;
+	dev = mlxsw_sp_rif_dev(mlxsw_sp->router->rifs[rif]);
 	n = neigh_lookup(&arp_tbl, &dipn, dev);
 	if (!n)
 		return;

@@ -2360,7 +2366,7 @@ static void mlxsw_sp_router_neigh_ent_ipv6_process(struct mlxsw_sp *mlxsw_sp,
 		return;
 	}
 
-	dev = mlxsw_sp->router->rifs[rif]->dev;
+	dev = mlxsw_sp_rif_dev(mlxsw_sp->router->rifs[rif]);
 	n = neigh_lookup(&nd_tbl, &dip, dev);
 	if (!n)
 		return;

@@ -4368,6 +4374,7 @@ static void mlxsw_sp_nexthop4_event(struct mlxsw_sp *mlxsw_sp,
 static void mlxsw_sp_nexthop_rif_update(struct mlxsw_sp *mlxsw_sp,
 					struct mlxsw_sp_rif *rif)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct mlxsw_sp_nexthop *nh;
 	bool removing;
 

@@ -4377,7 +4384,7 @@ static void mlxsw_sp_nexthop_rif_update(struct mlxsw_sp *mlxsw_sp,
 			removing = false;
 			break;
 		case MLXSW_SP_NEXTHOP_TYPE_IPIP:
-			removing = !mlxsw_sp_ipip_netdev_ul_up(rif->dev);
+			removing = !mlxsw_sp_ipip_netdev_ul_up(dev);
 			break;
 		default:
 			WARN_ON(1);

@@ -7798,7 +7805,7 @@ mlxsw_sp_rif_should_config(struct mlxsw_sp_rif *rif, struct net_device *dev,
 			return true;
 
 		if (rif && addr_list_empty &&
-		    !netif_is_l3_slave(rif->dev))
+		    !netif_is_l3_slave(mlxsw_sp_rif_dev(rif)))
 			return true;
 		/* It is possible we already removed the RIF ourselves
 		 * if it was assigned to a netdev that is now a bridge

@@ -7895,7 +7902,8 @@ u16 mlxsw_sp_ipip_lb_rif_index(const struct mlxsw_sp_rif_ipip_lb *lb_rif)
 
 u16 mlxsw_sp_ipip_lb_ul_vr_id(const struct mlxsw_sp_rif_ipip_lb *lb_rif)
 {
-	u32 ul_tb_id = mlxsw_sp_ipip_dev_ul_tb_id(lb_rif->common.dev);
+	struct net_device *dev = mlxsw_sp_rif_dev(&lb_rif->common);
+	u32 ul_tb_id = mlxsw_sp_ipip_dev_ul_tb_id(dev);
 	struct mlxsw_sp_vr *ul_vr;
 
 	ul_vr = mlxsw_sp_vr_get(lb_rif->common.mlxsw_sp, ul_tb_id, NULL);

@@ -8072,12 +8080,7 @@ mlxsw_sp_router_hwstats_notify_schedule(struct net_device *dev)
 
 int mlxsw_sp_rif_dev_ifindex(const struct mlxsw_sp_rif *rif)
 {
-	return rif->dev->ifindex;
-}
-
-static const struct net_device *mlxsw_sp_rif_dev(const struct mlxsw_sp_rif *rif)
-{
-	return rif->dev;
+	return mlxsw_sp_rif_dev(rif)->ifindex;
 }
 
 bool mlxsw_sp_rif_has_dev(const struct mlxsw_sp_rif *rif)

@@ -8096,7 +8099,7 @@ static void mlxsw_sp_rif_push_l3_stats(struct mlxsw_sp_rif *rif)
 	struct rtnl_hw_stats64 stats = {};
 
 	if (!mlxsw_sp_router_port_l3_stats_fetch(rif, &stats))
-		netdev_offload_xstats_push_delta(rif->dev,
+		netdev_offload_xstats_push_delta(mlxsw_sp_rif_dev(rif),
 						 NETDEV_OFFLOAD_XSTATS_TYPE_L3,
 						 &stats);
 }

@@ -8198,6 +8201,7 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 
 static void mlxsw_sp_rif_destroy(struct mlxsw_sp_rif *rif)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	const struct mlxsw_sp_rif_ops *ops = rif->ops;
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	struct mlxsw_sp_fid *fid = rif->fid;

@@ -8210,11 +8214,10 @@ static void mlxsw_sp_rif_destroy(struct mlxsw_sp_rif *rif)
 	mlxsw_sp_router_rif_gone_sync(mlxsw_sp, rif);
 	vr = &mlxsw_sp->router->vrs[rif->vr_id];
 
-	if (netdev_offload_xstats_enabled(rif->dev,
-					  NETDEV_OFFLOAD_XSTATS_TYPE_L3)) {
+	if (netdev_offload_xstats_enabled(dev, NETDEV_OFFLOAD_XSTATS_TYPE_L3)) {
 		mlxsw_sp_rif_push_l3_stats(rif);
 		mlxsw_sp_router_port_l3_stats_disable(rif);
-		mlxsw_sp_router_hwstats_notify_schedule(rif->dev);
+		mlxsw_sp_router_hwstats_notify_schedule(dev);
 	} else {
 		mlxsw_sp_rif_counters_free(rif);
 	}

@@ -8226,7 +8229,7 @@ static void mlxsw_sp_rif_destroy(struct mlxsw_sp_rif *rif)
 		/* Loopback RIFs are not associated with a FID. */
 		mlxsw_sp_fid_put(fid);
 	mlxsw_sp->router->rifs[rif->rif_index] = NULL;
-	dev_put(rif->dev);
+	dev_put(dev);
 	kfree(rif);
 	mlxsw_sp_rif_index_free(mlxsw_sp, rif_index, rif_entries);
 	vr->rif_count--;

@@ -9012,7 +9015,7 @@ mlxsw_sp_router_port_change_event(struct mlxsw_sp *mlxsw_sp,
 				  struct mlxsw_sp_rif *rif,
 				  struct netlink_ext_ack *extack)
 {
-	struct net_device *dev = rif->dev;
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	u8 old_mac_profile;
 	u16 fid_index;
 	int err;

@@ -9348,15 +9351,16 @@ static int __mlxsw_sp_rif_macvlan_flush(struct net_device *dev,
 
 static int mlxsw_sp_rif_macvlan_flush(struct mlxsw_sp_rif *rif)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct netdev_nested_priv priv = {
 		.data = (void *)rif,
 	};
 
-	if (!netif_is_macvlan_port(rif->dev))
+	if (!netif_is_macvlan_port(dev))
 		return 0;
 
-	netdev_warn(rif->dev, "Router interface is deleted. Upper macvlans will not work\n");
-	return netdev_walk_all_upper_dev_rcu(rif->dev,
+	netdev_warn(dev, "Router interface is deleted. Upper macvlans will not work\n");
+	return netdev_walk_all_upper_dev_rcu(dev,
 					     __mlxsw_sp_rif_macvlan_flush, &priv);
 }
 

@@ -9377,6 +9381,7 @@ static void mlxsw_sp_rif_subport_setup(struct mlxsw_sp_rif *rif,
 
 static int mlxsw_sp_rif_subport_op(struct mlxsw_sp_rif *rif, bool enable)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	struct mlxsw_sp_rif_subport *rif_subport;
 	char ritr_pl[MLXSW_REG_RITR_LEN];

@@ -9384,8 +9389,8 @@ static int mlxsw_sp_rif_subport_op(struct mlxsw_sp_rif *rif, bool enable)
 
 	rif_subport = mlxsw_sp_rif_subport_rif(rif);
 	mlxsw_reg_ritr_pack(ritr_pl, enable, MLXSW_REG_RITR_SP_IF,
-			    rif->rif_index, rif->vr_id, rif->dev->mtu);
-	mlxsw_reg_ritr_mac_pack(ritr_pl, rif->dev->dev_addr);
+			    rif->rif_index, rif->vr_id, dev->mtu);
+	mlxsw_reg_ritr_mac_pack(ritr_pl, dev->dev_addr);
 	mlxsw_reg_ritr_if_mac_profile_id_set(ritr_pl, rif->mac_profile_id);
 	efid = mlxsw_sp_fid_index(rif->fid);
 	mlxsw_reg_ritr_sp_if_pack(ritr_pl, rif_subport->lag,

@@ -9398,6 +9403,7 @@ static int mlxsw_sp_rif_subport_op(struct mlxsw_sp_rif *rif, bool enable)
 static int mlxsw_sp_rif_subport_configure(struct mlxsw_sp_rif *rif,
 					  struct netlink_ext_ack *extack)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	u8 mac_profile;
 	int err;
 

@@ -9411,7 +9417,7 @@ static int mlxsw_sp_rif_subport_configure(struct mlxsw_sp_rif *rif,
 	if (err)
 		goto err_rif_subport_op;
 
-	err = mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	err = mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 				  mlxsw_sp_fid_index(rif->fid), true);
 	if (err)
 		goto err_rif_fdb_op;

@@ -9423,7 +9429,7 @@ static int mlxsw_sp_rif_subport_configure(struct mlxsw_sp_rif *rif,
 	return 0;
 
 err_fid_rif_set:
-	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 			    mlxsw_sp_fid_index(rif->fid), false);
 err_rif_fdb_op:
 	mlxsw_sp_rif_subport_op(rif, false);

@@ -9434,10 +9440,11 @@ static int mlxsw_sp_rif_subport_configure(struct mlxsw_sp_rif *rif,
 
 static void mlxsw_sp_rif_subport_deconfigure(struct mlxsw_sp_rif *rif)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct mlxsw_sp_fid *fid = rif->fid;
 
 	mlxsw_sp_fid_rif_unset(fid);
-	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 			    mlxsw_sp_fid_index(fid), false);
 	mlxsw_sp_rif_macvlan_flush(rif);
 	mlxsw_sp_rif_subport_op(rif, false);

@@ -9463,12 +9470,13 @@ static const struct mlxsw_sp_rif_ops mlxsw_sp_rif_subport_ops = {
 static int mlxsw_sp_rif_fid_op(struct mlxsw_sp_rif *rif, u16 fid, bool enable)
 {
 	enum mlxsw_reg_ritr_if_type type = MLXSW_REG_RITR_FID_IF;
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	char ritr_pl[MLXSW_REG_RITR_LEN];
 
 	mlxsw_reg_ritr_pack(ritr_pl, enable, type, rif->rif_index, rif->vr_id,
-			    rif->dev->mtu);
-	mlxsw_reg_ritr_mac_pack(ritr_pl, rif->dev->dev_addr);
+			    dev->mtu);
+	mlxsw_reg_ritr_mac_pack(ritr_pl, dev->dev_addr);
 	mlxsw_reg_ritr_if_mac_profile_id_set(ritr_pl, rif->mac_profile_id);
 	mlxsw_reg_ritr_fid_if_fid_set(ritr_pl, fid);
 

@@ -9483,6 +9491,7 @@ u16 mlxsw_sp_router_port(const struct mlxsw_sp *mlxsw_sp)
 static int mlxsw_sp_rif_fid_configure(struct mlxsw_sp_rif *rif,
 				      struct netlink_ext_ack *extack)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	u16 fid_index = mlxsw_sp_fid_index(rif->fid);
 	u8 mac_profile;

@@ -9508,7 +9517,7 @@ static int mlxsw_sp_rif_fid_configure(struct mlxsw_sp_rif *rif,
 	if (err)
 		goto err_fid_bc_flood_set;
 
-	err = mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	err = mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 				  mlxsw_sp_fid_index(rif->fid), true);
 	if (err)
 		goto err_rif_fdb_op;

@@ -9520,7 +9529,7 @@ static int mlxsw_sp_rif_fid_configure(struct mlxsw_sp_rif *rif,
 	return 0;
 
 err_fid_rif_set:
-	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 			    mlxsw_sp_fid_index(rif->fid), false);
 err_rif_fdb_op:
 	mlxsw_sp_fid_flood_set(rif->fid, MLXSW_SP_FLOOD_TYPE_BC,

@@ -9537,12 +9546,13 @@ static int mlxsw_sp_rif_fid_configure(struct mlxsw_sp_rif *rif,
 
 static void mlxsw_sp_rif_fid_deconfigure(struct mlxsw_sp_rif *rif)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	u16 fid_index = mlxsw_sp_fid_index(rif->fid);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	struct mlxsw_sp_fid *fid = rif->fid;
 
 	mlxsw_sp_fid_rif_unset(fid);
-	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 			    mlxsw_sp_fid_index(fid), false);
 	mlxsw_sp_rif_macvlan_flush(rif);
 	mlxsw_sp_fid_flood_set(rif->fid, MLXSW_SP_FLOOD_TYPE_BC,

@@ -9557,7 +9567,9 @@ static struct mlxsw_sp_fid *
 mlxsw_sp_rif_fid_fid_get(struct mlxsw_sp_rif *rif,
 			 struct netlink_ext_ack *extack)
 {
-	return mlxsw_sp_fid_8021d_get(rif->mlxsw_sp, rif->dev->ifindex);
+	int rif_ifindex = mlxsw_sp_rif_dev_ifindex(rif);
+
+	return mlxsw_sp_fid_8021d_get(rif->mlxsw_sp, rif_ifindex);
 }
 
 static void mlxsw_sp_rif_fid_fdb_del(struct mlxsw_sp_rif *rif, const char *mac)

@@ -9565,7 +9577,7 @@ static void mlxsw_sp_rif_fid_fdb_del(struct mlxsw_sp_rif *rif, const char *mac)
 	struct switchdev_notifier_fdb_info info = {};
 	struct net_device *dev;
 
-	dev = br_fdb_find_port(rif->dev, mac, 0);
+	dev = br_fdb_find_port(mlxsw_sp_rif_dev(rif), mac, 0);
 	if (!dev)
 		return;
 

@@ -9588,17 +9600,18 @@ static struct mlxsw_sp_fid *
 mlxsw_sp_rif_vlan_fid_get(struct mlxsw_sp_rif *rif,
 			  struct netlink_ext_ack *extack)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct net_device *br_dev;
 	u16 vid;
 	int err;
 
-	if (is_vlan_dev(rif->dev)) {
-		vid = vlan_dev_vlan_id(rif->dev);
-		br_dev = vlan_dev_real_dev(rif->dev);
+	if (is_vlan_dev(dev)) {
+		vid = vlan_dev_vlan_id(dev);
+		br_dev = vlan_dev_real_dev(dev);
 		if (WARN_ON(!netif_is_bridge_master(br_dev)))
 			return ERR_PTR(-EINVAL);
 	} else {
-		err = br_vlan_get_pvid(rif->dev, &vid);
+		err = br_vlan_get_pvid(dev, &vid);
 		if (err < 0 || !vid) {
 			NL_SET_ERR_MSG_MOD(extack, "Couldn't determine bridge PVID");
 			return ERR_PTR(-EINVAL);

@@ -9610,12 +9623,13 @@ mlxsw_sp_rif_vlan_fid_get(struct mlxsw_sp_rif *rif,
 
 static void mlxsw_sp_rif_vlan_fdb_del(struct mlxsw_sp_rif *rif, const char *mac)
 {
+	struct net_device *rif_dev = mlxsw_sp_rif_dev(rif);
 	struct switchdev_notifier_fdb_info info = {};
 	u16 vid = mlxsw_sp_fid_8021q_vid(rif->fid);
 	struct net_device *br_dev;
 	struct net_device *dev;
 
-	br_dev = is_vlan_dev(rif->dev) ? vlan_dev_real_dev(rif->dev) : rif->dev;
+	br_dev = is_vlan_dev(rif_dev) ? vlan_dev_real_dev(rif_dev) : rif_dev;
 	dev = br_fdb_find_port(br_dev, mac, vid);
 	if (!dev)
 		return;

@@ -9629,11 +9643,12 @@ static void mlxsw_sp_rif_vlan_fdb_del(struct mlxsw_sp_rif *rif, const char *mac)
 static int mlxsw_sp_rif_vlan_op(struct mlxsw_sp_rif *rif, u16 vid, u16 efid,
 				bool enable)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	char ritr_pl[MLXSW_REG_RITR_LEN];
 
 	mlxsw_reg_ritr_vlan_if_pack(ritr_pl, enable, rif->rif_index, rif->vr_id,
-				    rif->dev->mtu, rif->dev->dev_addr,
+				    dev->mtu, dev->dev_addr,
 				    rif->mac_profile_id, vid, efid);
 
 	return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(ritr), ritr_pl);

@@ -9642,6 +9657,7 @@ static int mlxsw_sp_rif_vlan_op(struct mlxsw_sp_rif *rif, u16 vid, u16 efid,
 static int mlxsw_sp_rif_vlan_configure(struct mlxsw_sp_rif *rif, u16 efid,
 				       struct netlink_ext_ack *extack)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	u16 vid = mlxsw_sp_fid_8021q_vid(rif->fid);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	u8 mac_profile;

@@ -9667,7 +9683,7 @@ static int mlxsw_sp_rif_vlan_configure(struct mlxsw_sp_rif *rif, u16 efid,
 	if (err)
 		goto err_fid_bc_flood_set;
 
-	err = mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	err = mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 				  mlxsw_sp_fid_index(rif->fid), true);
 	if (err)
 		goto err_rif_fdb_op;

@@ -9679,7 +9695,7 @@ static int mlxsw_sp_rif_vlan_configure(struct mlxsw_sp_rif *rif, u16 efid,
 	return 0;
 
 err_fid_rif_set:
-	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 			    mlxsw_sp_fid_index(rif->fid), false);
 err_rif_fdb_op:
 	mlxsw_sp_fid_flood_set(rif->fid, MLXSW_SP_FLOOD_TYPE_BC,

@@ -9696,11 +9712,12 @@ static int mlxsw_sp_rif_vlan_configure(struct mlxsw_sp_rif *rif, u16 efid,
 
 static void mlxsw_sp_rif_vlan_deconfigure(struct mlxsw_sp_rif *rif)
 {
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
 	u16 vid = mlxsw_sp_fid_8021q_vid(rif->fid);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 
 	mlxsw_sp_fid_rif_unset(rif->fid);
-	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, rif->dev->dev_addr,
+	mlxsw_sp_rif_fdb_op(rif->mlxsw_sp, dev->dev_addr,
 			    mlxsw_sp_fid_index(rif->fid), false);
 	mlxsw_sp_rif_macvlan_flush(rif);
 	mlxsw_sp_fid_flood_set(rif->fid, MLXSW_SP_FLOOD_TYPE_BC,

@@ -9767,7 +9784,8 @@ mlxsw_sp1_rif_ipip_lb_configure(struct mlxsw_sp_rif *rif,
 				struct netlink_ext_ack *extack)
 {
 	struct mlxsw_sp_rif_ipip_lb *lb_rif = mlxsw_sp_rif_ipip_lb_rif(rif);
-	u32 ul_tb_id = mlxsw_sp_ipip_dev_ul_tb_id(rif->dev);
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
+	u32 ul_tb_id = mlxsw_sp_ipip_dev_ul_tb_id(dev);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	struct mlxsw_sp_vr *ul_vr;
 	int err;

@@ -9966,7 +9984,8 @@ mlxsw_sp2_rif_ipip_lb_configure(struct mlxsw_sp_rif *rif,
 				struct netlink_ext_ack *extack)
 {
 	struct mlxsw_sp_rif_ipip_lb *lb_rif = mlxsw_sp_rif_ipip_lb_rif(rif);
-	u32 ul_tb_id = mlxsw_sp_ipip_dev_ul_tb_id(rif->dev);
+	struct net_device *dev = mlxsw_sp_rif_dev(rif);
+	u32 ul_tb_id = mlxsw_sp_ipip_dev_ul_tb_id(dev);
 	struct mlxsw_sp *mlxsw_sp = rif->mlxsw_sp;
 	struct mlxsw_sp_rif *ul_rif;
 	int err;
Signature · 1 line
2.40.1
author maintainer
#4 lore raw

PATCH 4/10 mlxsw: spectrum_router: Access rif->dev from params in mlxsw_sp_rif_create()

In reply to Petr Machata

The previous patch added a helper to access a netdevice given a RIF. Using this helper in mlxsw_sp_rif_create() is unreasonable: the netdevice was given in RIF creation parameters. Just take it there.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +4 -4
index e9183c223575..da582ef8efda 100644
@@ -8138,7 +8138,7 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 		err = -ENOMEM;
 		goto err_rif_alloc;
 	}
-	dev_hold(rif->dev);
+	dev_hold(params->dev);
 	mlxsw_sp->router->rifs[rif_index] = rif;
 	rif->mlxsw_sp = mlxsw_sp;
 	rif->ops = ops;

@@ -8166,12 +8166,12 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 			goto err_mr_rif_add;
 	}
 
-	if (netdev_offload_xstats_enabled(rif->dev,
+	if (netdev_offload_xstats_enabled(params->dev,
 					  NETDEV_OFFLOAD_XSTATS_TYPE_L3)) {
 		err = mlxsw_sp_router_port_l3_stats_enable(rif);
 		if (err)
 			goto err_stats_enable;
-		mlxsw_sp_router_hwstats_notify_schedule(rif->dev);
+		mlxsw_sp_router_hwstats_notify_schedule(params->dev);
 	} else {
 		mlxsw_sp_rif_counters_alloc(rif);
 	}

@@ -8189,7 +8189,7 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 		mlxsw_sp_fid_put(fid);
 err_fid_get:
 	mlxsw_sp->router->rifs[rif_index] = NULL;
-	dev_put(rif->dev);
+	dev_put(params->dev);
 	kfree(rif);
 err_rif_alloc:
 	mlxsw_sp_rif_index_free(mlxsw_sp, rif_index, rif_entries);
Signature · 1 line
2.40.1
maintainer
#5 lore raw

Re: [PATCH net-next 04/10] mlxsw: spectrum_router: Access rif->dev from params in mlxsw_sp_rif_create()

Quoted 39 lines · Petr Machata

The previous patch added a helper to access a netdevice given a RIF. Using this helper in mlxsw_sp_rif_create() is unreasonable: the netdevice was given in RIF creation parameters. Just take it there.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +3 -3
index e9183c223575..da582ef8efda 100644
@@ -8138,7 +8138,7 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 		err = -ENOMEM;
 		goto err_rif_alloc;
 	}
-	dev_hold(rif->dev);
+	dev_hold(params->dev);
 	mlxsw_sp->router->rifs[rif_index] = rif;
 	rif->mlxsw_sp = mlxsw_sp;
 	rif->ops = ops;

@@ -8166,12 +8166,12 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 			goto err_mr_rif_add;
 	}
 
-	if (netdev_offload_xstats_enabled(rif->dev,
+	if (netdev_offload_xstats_enabled(params->dev,
 					  NETDEV_OFFLOAD_XSTATS_TYPE_L3)) {
 		err = mlxsw_sp_router_port_l3_stats_enable(rif);
 		if (err)
 			goto err_stats_enable;
-		mlxsw_sp_router_hwstats_notify_schedule(rif->dev);
+		mlxsw_sp_router_hwstats_notify_schedule(params->dev);
 	} else {
 		mlxsw_sp_rif_counters_alloc(rif);
 	}

drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp, · line 8192

Quoted 5 lines
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +1 -1
@@ -8189,7 +8189,7 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 		mlxsw_sp_fid_put(fid);
 err_fid_get:
 	mlxsw_sp->router->rifs[rif_index] = NULL;
-	dev_put(rif->dev);
+	dev_put(params->dev);

Side note: since you are touching this, and dev_{put,hold} are now deprecated in favour of the tracker-enabled variants netdev_{put,hold}, what about a follow-up introducing the usage of the latter helpers?

Thanks!

Paolo

author maintainer
#6 lore raw

Re: [PATCH net-next 04/10] mlxsw: spectrum_router: Access rif->dev from params in mlxsw_sp_rif_create()

Quoted 3 lines · Paolo Abeni

Side note: since you are touching this, and dev_{put,hold} are now deprecated in favour of the tracker-enabled variants netdev_{put,hold}, what about a follow-up introducing the usage of the latter helpers?

Yeah, we should do that. I'll look into it.

author maintainer
#7 lore raw

PATCH 5/10 mlxsw: spectrum_router: Access nh->rif->dev through a helper

In reply to Petr Machata

In order to abstract away deduction of netdevice from the corresponding next hop, introduce a helper, mlxsw_sp_nexthop_dev(), and use it throughout. This will make it possible to change the deduction path easily later on.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 23 +++++++++++++++----
 1 file changed, 18 insertions(+), 5 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +18 -5
index da582ef8efda..d7013727da21 100644
@@ -2941,6 +2941,14 @@ struct mlxsw_sp_nexthop {
 	bool counter_valid;
 };
 
+static struct net_device *
+mlxsw_sp_nexthop_dev(const struct mlxsw_sp_nexthop *nh)
+{
+	if (nh->rif)
+		return mlxsw_sp_rif_dev(nh->rif);
+	return NULL;
+}
+
 enum mlxsw_sp_nexthop_group_type {
 	MLXSW_SP_NEXTHOP_GROUP_TYPE_IPV4,
 	MLXSW_SP_NEXTHOP_GROUP_TYPE_IPV6,

@@ -4014,16 +4022,18 @@ mlxsw_sp_nexthop_dead_neigh_replace(struct mlxsw_sp *mlxsw_sp,
 {
 	struct neighbour *n, *old_n = neigh_entry->key.n;
 	struct mlxsw_sp_nexthop *nh;
+	struct net_device *dev;
 	bool entry_connected;
 	u8 nud_state, dead;
 	int err;
 
 	nh = list_first_entry(&neigh_entry->nexthop_list,
 			      struct mlxsw_sp_nexthop, neigh_list_node);
+	dev = mlxsw_sp_nexthop_dev(nh);
 
-	n = neigh_lookup(nh->neigh_tbl, &nh->gw_addr, nh->rif->dev);
+	n = neigh_lookup(nh->neigh_tbl, &nh->gw_addr, dev);
 	if (!n) {
-		n = neigh_create(nh->neigh_tbl, &nh->gw_addr, nh->rif->dev);
+		n = neigh_create(nh->neigh_tbl, &nh->gw_addr, dev);
 		if (IS_ERR(n))
 			return PTR_ERR(n);
 		neigh_event_send(n, NULL);

@@ -4110,21 +4120,23 @@ static int mlxsw_sp_nexthop_neigh_init(struct mlxsw_sp *mlxsw_sp,
 				       struct mlxsw_sp_nexthop *nh)
 {
 	struct mlxsw_sp_neigh_entry *neigh_entry;
+	struct net_device *dev;
 	struct neighbour *n;
 	u8 nud_state, dead;
 	int err;
 
 	if (!nh->nhgi->gateway || nh->neigh_entry)
 		return 0;
+	dev = mlxsw_sp_nexthop_dev(nh);
 
 	/* Take a reference of neigh here ensuring that neigh would
 	 * not be destructed before the nexthop entry is finished.
 	 * The reference is taken either in neigh_lookup() or
 	 * in neigh_create() in case n is not found.
 	 */
-	n = neigh_lookup(nh->neigh_tbl, &nh->gw_addr, nh->rif->dev);
+	n = neigh_lookup(nh->neigh_tbl, &nh->gw_addr, dev);
 	if (!n) {
-		n = neigh_create(nh->neigh_tbl, &nh->gw_addr, nh->rif->dev);
+		n = neigh_create(nh->neigh_tbl, &nh->gw_addr, dev);
 		if (IS_ERR(n))
 			return PTR_ERR(n);
 		neigh_event_send(n, NULL);

@@ -5516,9 +5528,10 @@ mlxsw_sp_rt6_nexthop(struct mlxsw_sp_nexthop_group *nh_grp,
 
 	for (i = 0; i < nh_grp->nhgi->count; i++) {
 		struct mlxsw_sp_nexthop *nh = &nh_grp->nhgi->nexthops[i];
+		struct net_device *dev = mlxsw_sp_nexthop_dev(nh);
 		struct fib6_info *rt = mlxsw_sp_rt6->rt;
 
-		if (nh->rif && nh->rif->dev == rt->fib6_nh->fib_nh_dev &&
+		if (dev && dev == rt->fib6_nh->fib_nh_dev &&
 		    ipv6_addr_equal((const struct in6_addr *) &nh->gw_addr,
 				    &rt->fib6_nh->fib_nh_gw6))
 			return nh;
Signature · 1 line
2.40.1
author maintainer
#8 lore raw

PATCH 6/10 mlxsw: spectrum_router: Access nhgi->rif through a helper

In reply to Petr Machata

To abstract away deduction of RIF from the corresponding next hop group info (NHGI), mlxsw currently uses a macro. In its current form, that macro is impossible to extend to more general computation. Therefore introduce a helper, mlxsw_sp_nhgi_rif(), and use it throughout. This will make it possible to change the deduction path easily later on.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +10 -4
index d7013727da21..e05c47568ece 100644
@@ -2966,9 +2966,14 @@ struct mlxsw_sp_nexthop_group_info {
 	   is_resilient:1;
 	struct list_head list; /* member in nh_res_grp_list */
 	struct mlxsw_sp_nexthop nexthops[];
-#define nh_rif	nexthops[0].rif
 };
 
+static struct mlxsw_sp_rif *
+mlxsw_sp_nhgi_rif(const struct mlxsw_sp_nexthop_group_info *nhgi)
+{
+	return nhgi->nexthops[0].rif;
+}
+
 struct mlxsw_sp_nexthop_group_vr_key {
 	u16 vr_id;
 	enum mlxsw_sp_l3proto proto;

@@ -5510,7 +5515,7 @@ mlxsw_sp_fib_entry_should_offload(const struct mlxsw_sp_fib_entry *fib_entry)
 	case MLXSW_SP_FIB_ENTRY_TYPE_REMOTE:
 		return !!nh_group->nhgi->adj_index_valid;
 	case MLXSW_SP_FIB_ENTRY_TYPE_LOCAL:
-		return !!nh_group->nhgi->nh_rif;
+		return !!mlxsw_sp_nhgi_rif(nh_group->nhgi);
 	case MLXSW_SP_FIB_ENTRY_TYPE_BLACKHOLE:
 	case MLXSW_SP_FIB_ENTRY_TYPE_IPIP_DECAP:
 	case MLXSW_SP_FIB_ENTRY_TYPE_NVE_DECAP:

@@ -5772,7 +5777,8 @@ static int mlxsw_sp_fib_entry_op_remote(struct mlxsw_sp *mlxsw_sp,
 		trap_action = MLXSW_REG_RALUE_TRAP_ACTION_NOP;
 		adjacency_index = nhgi->adj_index;
 		ecmp_size = nhgi->ecmp_size;
-	} else if (!nhgi->adj_index_valid && nhgi->count && nhgi->nh_rif) {
+	} else if (!nhgi->adj_index_valid && nhgi->count &&
+		   mlxsw_sp_nhgi_rif(nhgi)) {
 		trap_action = MLXSW_REG_RALUE_TRAP_ACTION_NOP;
 		adjacency_index = mlxsw_sp->router->adj_trap_index;
 		ecmp_size = 1;

@@ -5791,7 +5797,7 @@ static int mlxsw_sp_fib_entry_op_local(struct mlxsw_sp *mlxsw_sp,
 				       struct mlxsw_sp_fib_entry *fib_entry,
 				       enum mlxsw_reg_ralue_op op)
 {
-	struct mlxsw_sp_rif *rif = fib_entry->nh_group->nhgi->nh_rif;
+	struct mlxsw_sp_rif *rif = mlxsw_sp_nhgi_rif(fib_entry->nh_group->nhgi);
 	enum mlxsw_reg_ralue_trap_action trap_action;
 	char ralue_pl[MLXSW_REG_RALUE_LEN];
 	u16 trap_id = 0;
Signature · 1 line
2.40.1
author maintainer
#9 lore raw

PATCH 7/10 mlxsw: spectrum_router: Extract a helper to free a RIF

In reply to Petr Machata

Right now freeing the object that mlxsw uses to keep track of a RIF is as simple as calling a kfree. But later on as CRIF abstraction is brought in, it will involve severing the link between CRIF and its RIF as well. Better to have the logic encapsulated in a helper.

Since a helper is being introduced, make it a full-fledged destructor and have it validate that the objects tracked at the RIF have been released.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +11 -4
index e05c47568ece..1e05ecd29c8d 100644
@@ -7903,6 +7903,13 @@ static struct mlxsw_sp_rif *mlxsw_sp_rif_alloc(size_t rif_size, u16 rif_index,
 	return rif;
 }
 
+static void mlxsw_sp_rif_free(struct mlxsw_sp_rif *rif)
+{
+	WARN_ON(!list_empty(&rif->neigh_list));
+	WARN_ON(!list_empty(&rif->nexthop_list));
+	kfree(rif);
+}
+
 struct mlxsw_sp_rif *mlxsw_sp_rif_by_index(const struct mlxsw_sp *mlxsw_sp,
 					   u16 rif_index)
 {

@@ -8209,7 +8216,7 @@ mlxsw_sp_rif_create(struct mlxsw_sp *mlxsw_sp,
 err_fid_get:
 	mlxsw_sp->router->rifs[rif_index] = NULL;
 	dev_put(params->dev);
-	kfree(rif);
+	mlxsw_sp_rif_free(rif);
 err_rif_alloc:
 	mlxsw_sp_rif_index_free(mlxsw_sp, rif_index, rif_entries);
 err_rif_index_alloc:

@@ -8249,7 +8256,7 @@ static void mlxsw_sp_rif_destroy(struct mlxsw_sp_rif *rif)
 		mlxsw_sp_fid_put(fid);
 	mlxsw_sp->router->rifs[rif->rif_index] = NULL;
 	dev_put(dev);
-	kfree(rif);
+	mlxsw_sp_rif_free(rif);
 	mlxsw_sp_rif_index_free(mlxsw_sp, rif_index, rif_entries);
 	vr->rif_count--;
 	mlxsw_sp_vr_put(mlxsw_sp, vr);

@@ -9902,7 +9909,7 @@ mlxsw_sp_ul_rif_create(struct mlxsw_sp *mlxsw_sp, struct mlxsw_sp_vr *vr,
 
 ul_rif_op_err:
 	mlxsw_sp->router->rifs[rif_index] = NULL;
-	kfree(ul_rif);
+	mlxsw_sp_rif_free(ul_rif);
 err_rif_alloc:
 	mlxsw_sp_rif_index_free(mlxsw_sp, rif_index, rif_entries);
 	return ERR_PTR(err);

@@ -9917,7 +9924,7 @@ static void mlxsw_sp_ul_rif_destroy(struct mlxsw_sp_rif *ul_rif)
 	atomic_sub(rif_entries, &mlxsw_sp->router->rifs_count);
 	mlxsw_sp_rif_ipip_lb_ul_rif_op(ul_rif, false);
 	mlxsw_sp->router->rifs[ul_rif->rif_index] = NULL;
-	kfree(ul_rif);
+	mlxsw_sp_rif_free(ul_rif);
 	mlxsw_sp_rif_index_free(mlxsw_sp, rif_index, rif_entries);
 }
Signature · 1 line
2.40.1
author maintainer
#10 lore raw

PATCH 8/10 mlxsw: spectrum_router: Add a helper to check if netdev has addresses

In reply to Petr Machata

This function will be useful later as the driver will need to retroactively create RIFs for new uppers with addresses.

Add another helper that assumes RCU lock, and restructure the code to skip the IPv6 branch not through conditioning on the addr_list_empty variable, but by directly returning the result value. This makes the skip more obvious than it previously was.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 44 +++++++++++++------
 1 file changed, 30 insertions(+), 14 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +30 -14
index 1e05ecd29c8d..25dbddabd91e 100644
@@ -7794,28 +7794,44 @@ static void mlxsw_sp_router_rif_gone_sync(struct mlxsw_sp *mlxsw_sp,
 	mlxsw_sp_neigh_rif_gone_sync(mlxsw_sp, rif);
 }
 
-static bool
-mlxsw_sp_rif_should_config(struct mlxsw_sp_rif *rif, struct net_device *dev,
-			   unsigned long event)
+static bool __mlxsw_sp_dev_addr_list_empty(const struct net_device *dev)
 {
 	struct inet6_dev *inet6_dev;
-	bool addr_list_empty = true;
 	struct in_device *idev;
 
+	idev = __in_dev_get_rcu(dev);
+	if (idev && idev->ifa_list)
+		return false;
+
+	inet6_dev = __in6_dev_get(dev);
+	if (inet6_dev && !list_empty(&inet6_dev->addr_list))
+		return false;
+
+	return true;
+}
+
+static bool mlxsw_sp_dev_addr_list_empty(const struct net_device *dev)
+{
+	bool addr_list_empty;
+
+	rcu_read_lock();
+	addr_list_empty = __mlxsw_sp_dev_addr_list_empty(dev);
+	rcu_read_unlock();
+
+	return addr_list_empty;
+}
+
+static bool
+mlxsw_sp_rif_should_config(struct mlxsw_sp_rif *rif, struct net_device *dev,
+			   unsigned long event)
+{
+	bool addr_list_empty;
+
 	switch (event) {
 	case NETDEV_UP:
 		return rif == NULL;
 	case NETDEV_DOWN:
-		rcu_read_lock();
-		idev = __in_dev_get_rcu(dev);
-		if (idev && idev->ifa_list)
-			addr_list_empty = false;
-
-		inet6_dev = __in6_dev_get(dev);
-		if (addr_list_empty && inet6_dev &&
-		    !list_empty(&inet6_dev->addr_list))
-			addr_list_empty = false;
-		rcu_read_unlock();
+		addr_list_empty = mlxsw_sp_dev_addr_list_empty(dev);
 
 		/* macvlans do not have a RIF, but rather piggy back on the
 		 * RIF of their lower device.
Signature · 1 line
2.40.1
author maintainer
#11 lore raw

PATCH 9/10 mlxsw: spectrum_router: Extract a helper for RIF migration

In reply to Petr Machata

RIF configuration contains a number of parameters that cannot be changed after the RIF is created. For the IPIP loopbacks, this is currently worked around by creating a new RIF with the desired configuration changes applied, and updating next hops to the new RIF, and then destroying the old RIF. This operation will be useful as a reusable atom, so extract a helper to that effect.

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../ethernet/mellanox/mlxsw/spectrum_router.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +13 -6
index 25dbddabd91e..fdb812152e71 100644
@@ -1651,6 +1651,17 @@ static void mlxsw_sp_netdevice_ipip_ol_down_event(struct mlxsw_sp *mlxsw_sp,
 static void mlxsw_sp_nexthop_rif_migrate(struct mlxsw_sp *mlxsw_sp,
 					 struct mlxsw_sp_rif *old_rif,
 					 struct mlxsw_sp_rif *new_rif);
+static void mlxsw_sp_rif_migrate_destroy(struct mlxsw_sp *mlxsw_sp,
+					 struct mlxsw_sp_rif *old_rif,
+					 struct mlxsw_sp_rif *new_rif,
+					 bool migrate_nhs)
+{
+	if (migrate_nhs)
+		mlxsw_sp_nexthop_rif_migrate(mlxsw_sp, old_rif, new_rif);
+
+	mlxsw_sp_rif_destroy(old_rif);
+}
+
 static int
 mlxsw_sp_ipip_entry_ol_lb_update(struct mlxsw_sp *mlxsw_sp,
 				 struct mlxsw_sp_ipip_entry *ipip_entry,

@@ -1668,12 +1679,8 @@ mlxsw_sp_ipip_entry_ol_lb_update(struct mlxsw_sp *mlxsw_sp,
 		return PTR_ERR(new_lb_rif);
 	ipip_entry->ol_lb = new_lb_rif;
 
-	if (keep_encap)
-		mlxsw_sp_nexthop_rif_migrate(mlxsw_sp, &old_lb_rif->common,
-					     &new_lb_rif->common);
-
-	mlxsw_sp_rif_destroy(&old_lb_rif->common);
-
+	mlxsw_sp_rif_migrate_destroy(mlxsw_sp, &old_lb_rif->common,
+				     &new_lb_rif->common, keep_encap);
 	return 0;
 }
Signature · 1 line
2.40.1
author maintainer
#12 lore raw

PATCH 10/10 mlxsw: spectrum_router: Move IPIP init up

In reply to Petr Machata

mlxsw will need to keep track of certain devices that are not related to any of its front panel ports. This includes IPIP netdevices. To be able to query the list of supported IPIP types, router->ipip_ops_arr needs to be initialized.

To that end, move the IPIP initialization up (and finalization correspondingly down).

Signed-off-by Petr Machata <[email protected]>
Reviewed-by Amit Cohen <[email protected]>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +7 -7
index fdb812152e71..43e8f19c7a0a 100644
@@ -10643,14 +10643,14 @@ int mlxsw_sp_router_init(struct mlxsw_sp *mlxsw_sp,
 	if (err)
 		goto err_router_init;
 
-	err = mlxsw_sp_rifs_init(mlxsw_sp);
-	if (err)
-		goto err_rifs_init;
-
 	err = mlxsw_sp->router_ops->ipips_init(mlxsw_sp);
 	if (err)
 		goto err_ipips_init;
 
+	err = mlxsw_sp_rifs_init(mlxsw_sp);
+	if (err)
+		goto err_rifs_init;
+
 	err = rhashtable_init(&mlxsw_sp->router->nexthop_ht,
 			      &mlxsw_sp_nexthop_ht_params);
 	if (err)

@@ -10776,10 +10776,10 @@ int mlxsw_sp_router_init(struct mlxsw_sp *mlxsw_sp,
 err_nexthop_group_ht_init:
 	rhashtable_destroy(&mlxsw_sp->router->nexthop_ht);
 err_nexthop_ht_init:
-	mlxsw_sp_ipips_fini(mlxsw_sp);
-err_ipips_init:
 	mlxsw_sp_rifs_fini(mlxsw_sp);
 err_rifs_init:
+	mlxsw_sp_ipips_fini(mlxsw_sp);
+err_ipips_init:
 	__mlxsw_sp_router_fini(mlxsw_sp);
 err_router_init:
 	cancel_delayed_work_sync(&mlxsw_sp->router->nh_grp_activity_dw);

@@ -10812,8 +10812,8 @@ void mlxsw_sp_router_fini(struct mlxsw_sp *mlxsw_sp)
 	mlxsw_sp_lpm_fini(mlxsw_sp);
 	rhashtable_destroy(&router->nexthop_group_ht);
 	rhashtable_destroy(&router->nexthop_ht);
-	mlxsw_sp_ipips_fini(mlxsw_sp);
 	mlxsw_sp_rifs_fini(mlxsw_sp);
+	mlxsw_sp_ipips_fini(mlxsw_sp);
 	__mlxsw_sp_router_fini(mlxsw_sp);
 	cancel_delayed_work_sync(&router->nh_grp_activity_dw);
 	mutex_destroy(&router->lock);
Signature · 1 line
2.40.1
CI
#13 lore raw
In reply to Petr Machata
This series was applied to netdev/net-next.git (main) by Paolo Abeni <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Paolo Abeni <[email protected]>:

Quoted 9 lines · On Mon, 12 Jun 2023 17:30:59 +0200 you wrote:

The mlxsw driver currently makes the assumption that the user applies configuration in a bottom-up manner. Thus netdevices need to be added to the bridge before IP addresses are configured on that bridge or SVI added on top of it. Enslaving a netdevice to another netdevice that already has uppers is in fact forbidden by mlxsw for this reason. Despite this safety, it is rather easy to get into situations where the offloaded configuration is just plain wrong.

[...]

Here is the summary with links:
- [net-next,01/10] mlxsw: spectrum_router: Extract a helper from mlxsw_sp_port_vlan_router_join()

    https://git.kernel.org/netdev/net-next/c/e0db883b6949
  - [net-next,02/10] mlxsw: spectrum_router: Add a helper specifically for joining a LAG
    https://git.kernel.org/netdev/net-next/c/76962b802efe
  - [net-next,03/10] mlxsw: spectrum_router: Access rif->dev through a helper
    https://git.kernel.org/netdev/net-next/c/fb6ac45e8666
  - [net-next,04/10] mlxsw: spectrum_router: Access rif->dev from params in mlxsw_sp_rif_create()
    https://git.kernel.org/netdev/net-next/c/2019b5eeae2a
  - [net-next,05/10] mlxsw: spectrum_router: Access nh->rif->dev through a helper
    https://git.kernel.org/netdev/net-next/c/69f4ba177d6b
  - [net-next,06/10] mlxsw: spectrum_router: Access nhgi->rif through a helper
    https://git.kernel.org/netdev/net-next/c/532b6e2bbc19
  - [net-next,07/10] mlxsw: spectrum_router: Extract a helper to free a RIF
    https://git.kernel.org/netdev/net-next/c/571c56911b45
  - [net-next,08/10] mlxsw: spectrum_router: Add a helper to check if netdev has addresses
    (no matching commit)
  - [net-next,09/10] mlxsw: spectrum_router: Extract a helper for RIF migration
    https://git.kernel.org/netdev/net-next/c/440273e763f5
  - [net-next,10/10] mlxsw: spectrum_router: Move IPIP init up
    https://git.kernel.org/netdev/net-next/c/d4a37bf0943d

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html