realtek: l3: rtl839x: do not invent a next-hop L2 entry

On RTL839x a route through a gateway the switch holds no L2 entry for
gets one invented by otto_l2_nexthop_add(): static, with the don't-care
port. The gateway is then cut off from the switch, and what the route
should forward goes to the CPU instead. Measured on an HPE 1920-48G
(RTL8393), main f29dfe25ba, gateway G on a bridge VLAN behind lan48,
with a permanent neighbour and a MAC the switch had not seen, 8 echo
requests to a destination behind it, forwarding off:
- G's L2 entry was written static, port 63
- G got 0 of 3 replies from the switch; the 8 echo requests reached
  the CPU, none reached G
- with the neighbour made dynamic the entry stayed as it was
- with the route removed G got 3 of 3 replies, and the switch learnt
  it on port 47
Setting lan48's static move action to forward did not restore it, so
that alone does not explain it; the mechanism is not known.

RTL930x stopped inventing that entry in 138fabb79f ("realtek: l3: do
not invent an L2 entry for a next hop") wherever a route can trap, and
since #25805 an RTL839x route can. Require an entry the switch already
holds on every family: the route traps until the switch learns the
gateway from the traffic the CPU routes, and a later neighbour update
programs it. Nothing asks otto_l2_nexthop_add() to invent an entry any
more, so the flag and the invented entry go. A route trapped for want
of one is now logged on RTL839x as on RTL930x. On RTL839x an L2 entry
that is a next hop already is now pointed at its gateway's ROUTING row
as on RTL930x, one row per gateway MAC since c282dd1f4d.

The same case with this change: no entry written, the route trapped,
G got 3 of 3 replies and was learnt on port 47; the 8 echo requests
reached the CPU and, with forwarding on, G through the kernel, TTL 63.
With the neighbour made dynamic the route forwarded on the learnt
entry, 8 of 8 at G, none at the CPU. The cases from #25805 rerun (two
prefixes in both orders, neighbour loss): same results. Since that run
only the log line and comments changed.

As 138fabb79f says, a route through a permanent neighbour the switch
had not learnt stays with the CPU, since such a neighbour sees no
update: measured here too, it stayed trapped after G was learnt, until
the neighbour was made dynamic. Also as on RTL930x, a route that claims
an entry the switch learnt again after a flush counts itself on top of
the count the flushed entry left, so that entry can stay a next hop
once the route is gone; not tested.

RTL930x: no change, it already required such an entry. RTL838x and
RTL931x have no setup, so their FIB and neighbour events are ignored and
none of this runs there. Compile-tested on rtl930x.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Gennaro Cimmino <gcimmino@rayonra.net>
Link: https://github.com/openwrt/openwrt/pull/25824
Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de>

target/linux/realtek/files-6.18/drivers/net/dsa/rtl83xx/l2.c

@@ -253,13 +253,12 @@ static void otto_l2_uc_put_row(struct rtl838x_switch_priv *priv,
 }
 
 /* Add an L2 nexthop entry for the L3 routing system / PIE forwarding in the SoC
- * Use VID and MAC in rtl838x_l2_entry to identify either a free slot in the L2 hash table
- * or mark an existing entry as a nexthop by setting it's nexthop bit
+ * Use VID and MAC in rtl838x_l2_entry to find the entry the switch holds for the
+ * gateway and mark it as a nexthop by setting its nexthop bit
  * Called from the L3 layer
  * The index in the L2 hash table is filled into nh->l2_id;
  */
-int otto_l2_nexthop_add(struct rtl838x_switch_priv *priv, struct otto_l3_nexthop *nh,
-			bool require_existing)
+int otto_l2_nexthop_add(struct rtl838x_switch_priv *priv, struct otto_l3_nexthop *nh)
 {
 	struct rtl838x_l2_entry e = {};
 	u64 seed = priv->r->l2_hash_seed(nh->mac, nh->rvid);
@@ -280,63 +279,43 @@ int otto_l2_nexthop_add(struct rtl838x_switch_priv *priv, struct otto_l3_nexthop
 	}
 
 	/* With a next hop that has no port, an RTL930x delivers every forwarded
-	 * frame twice, once from the switch and once from the Linux stack. A
-	 * row invented here would be static, and nothing would replace it:
-	 * dsa_user_fdb_event() drops an address the software bridge learns on
-	 * an offloaded port before the driver sees it. A caller that can trap
-	 * the route leaves the slot to the address, and lets the switch learn
-	 * it.
+	 * frame twice, once from the switch and once from the Linux stack, and
+	 * an RTL839x cuts the gateway off from the switch. A row invented here
+	 * would be static, and nothing would replace it: dsa_user_fdb_event()
+	 * drops an address the software bridge learns on an offloaded port
+	 * before the driver sees it. The caller traps the route, leaves the
+	 * slot to the address, and lets the switch learn it.
 	 */
-	if (require_existing && !e.valid) {
+	if (!e.valid) {
 		if (nh->l2_installed)
 			otto_l2_uc_put_row(priv, nh);
 		nh->l2_installed = false;
 		return -ENOENT;
 	}
 
-	/* Found an existing (e->valid is true) or empty entry, make it a nexthop entry */
+	/* Found an existing entry, make it a nexthop entry */
 	if (nh->l2_installed && nh->l2_id != idx)
 		otto_l2_uc_put_row(priv, nh);
 
-	if (!nh->l2_installed || nh->l2_id != idx) {
-		struct rtldsa_l2_uc *m = otto_l2_uc_lookup(priv, idx);
-
-		/* An entry nobody had claimed carries whatever its last owner
-		 * left behind, including a count for a route long gone.
-		 */
-		if (m && !e.valid)
-			*m = (struct rtldsa_l2_uc){};
-
+	if (!nh->l2_installed || nh->l2_id != idx)
 		otto_l2_uc_get(priv, idx);
-	}
 
 	nh->l2_id = idx;
 	nh->l2_seed = seed;
-	if (e.valid) {
-		nh->port = e.port;
-		nh->rvid = e.rvid;
-		nh->dev_id = e.stack_dev;
-		/* If the entry is already a valid next hop entry, don't change it,
-		 * but for the DMAC entry it names where the L3 tables hold one
-		 * per gateway MAC, the families a route traps on: every route
-		 * through the entry holds that one, and the entry may still name
-		 * one given up while it stayed a next hop.
-		 */
-		if (e.next_hop) {
-			if (!require_existing || e.nh_route_id == nh->dmac_id)
-				return 0;
-			e.nh_route_id = nh->dmac_id;
-			priv->r->write_l2_entry_using_hash(idx >> 2, idx & 0x3, &e);
+	nh->port = e.port;
+	nh->rvid = e.rvid;
+	nh->dev_id = e.stack_dev;
+	/* If the entry is already a valid next hop entry, don't change it, but
+	 * for the DMAC entry it names, one per gateway MAC: every route through
+	 * the entry holds that one, and the entry may still name one given up
+	 * while it stayed a next hop.
+	 */
+	if (e.next_hop) {
+		if (e.nh_route_id == nh->dmac_id)
 			return 0;
-		}
-	} else {
-		memset(&e, 0, sizeof(e));
-		e.type = L2_UNICAST;
-		e.valid = true;
-		e.is_static = true;
-		e.rvid = nh->rvid;
-		e.port = priv->r->port_ignore;
-		u64_to_ether_addr(nh->mac, &e.mac[0]);
+		e.nh_route_id = nh->dmac_id;
+		priv->r->write_l2_entry_using_hash(idx >> 2, idx & 0x3, &e);
+		return 0;
 	}
 	e.next_hop = true;
 	e.nh_route_id = nh->dmac_id;		/* DMAC entry or route ID takes place of VID */

target/linux/realtek/files-6.18/drivers/net/dsa/rtl83xx/l2.h

@@ -67,8 +67,7 @@ int otto_l2_find_cam_entry(struct rtl838x_switch_priv *priv, u64 seed,
 			   bool must_exist, struct rtl838x_l2_entry *e);
 struct rtldsa_l2_uc *otto_l2_uc_lookup(struct rtl838x_switch_priv *priv, int idx);
 
-int otto_l2_nexthop_add(struct rtl838x_switch_priv *priv, struct otto_l3_nexthop *nh,
-			bool require_existing);
+int otto_l2_nexthop_add(struct rtl838x_switch_priv *priv, struct otto_l3_nexthop *nh);
 int otto_l2_nexthop_del(struct rtl838x_switch_priv *priv, struct otto_l3_nexthop *nh);
 
 u64 otto_l2_838x_hash_seed(u64 mac, u32 vid);

target/linux/realtek/files-6.18/drivers/net/dsa/rtl83xx/l3.c

@@ -43,11 +43,11 @@
  *
  * Driver logic on RTL839x. The PIE rules L3 keeps for itself are the prefix
  * rows, and every route takes one, in the same order and with the same traps.
- * A route forwards once the gateway's neighbour is resolved, whether its MAC
- * was learnt or not. A trapping rule only takes frames addressed to one of the
- * switch's MACs, so a frame for another station goes its way. A destination no
- * rule matches needs no catch-all: its frames are addressed to the switch,
- * which traps them.
+ * A route forwards once the gateway's neighbour is resolved and the switch
+ * holds an L2 entry for its MAC. A trapping rule only takes frames addressed
+ * to one of the switch's MACs, so a frame for another station goes its way. A
+ * destination no rule matches needs no catch-all: its frames are addressed to
+ * the switch, which traps them.
  *
  * State per family:
  * - RTL930x: IPv4 and IPv6 unicast routes through a gateway are forwarded.
@@ -1732,7 +1732,6 @@ static void otto_l3_route_update_hw(struct otto_l3_ctrl *ctrl, struct otto_l3_ro
 				    u64 mac)
 {
 	struct rtl838x_switch_priv *priv = ctrl->priv;
-	bool require_existing = ctrl->cfg->use_l3_tables;
 	char dst[INET6_ADDRSTRLEN + sizeof("/128")];
 	bool trapped = r->attr.action == ROUTE_ACT_TRAP2CPU;
 	int old_dmac = r->nh.dmac_id;
@@ -1774,7 +1773,7 @@ static void otto_l3_route_update_hw(struct otto_l3_ctrl *ctrl, struct otto_l3_ro
 	r->nh.dmac_id = dmac;
 
 	/* Update ROUTING table: map gateway-mac and switch-mac id to route id */
-	no_l2 = otto_l2_nexthop_add(priv, &r->nh, require_existing);
+	no_l2 = otto_l2_nexthop_add(priv, &r->nh);
 	if (!no_l2)
 		r->nh.l2_installed = true;
 
@@ -1789,7 +1788,7 @@ static void otto_l3_route_update_hw(struct otto_l3_ctrl *ctrl, struct otto_l3_ro
 	 */
 	no_port = ctrl->cfg->use_l3_tables &&
 		  r->nh.port == priv->r->port_ignore;
-	if (no_port && (first || !trapped)) {
+	if ((no_port || no_l2) && (first || !trapped)) {
 		if (r->attr.type == ROUTE_TYPE_IP4UC)
 			dev_info(ctrl->dev, "no port for %pI4, routing %s in software\n",
 				 &r->gw_ip.s6_addr32[3], otto_l3_route_dst(r, dst, sizeof(dst)));