Re: [PATCH net-next v14 8/9] net: dsa: lan9645x: add bridge support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When the general-purpose PGIDs run out, L2 multicast groups
  fall back to PGID_MC.
- [Medium] Setting ds->ageing_time_min/max makes a bridge join fail late,
  and the failed join leaves DSA's dp->vlan_filtering out of step with…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the new
  lan9645x_set_ageing_time() is affected by it.

--

net: dsa: lan9645x: add bridge support

This adds hardware bridge offload to the lan9645x DSA driver for a single
bridge. It adds ageing time configuration with min/max limits enforced by
the DSA core, plus bridge port flags and STP state handling. Host flooding
is done by adding the CPU port module to the global flood masks.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index a0f48f3ad9aff..cefe643fce182 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c

[ ... ]

> @@ -306,6 +302,19 @@ static int lan9645x_setup(struct dsa_switch *ds)
>  	ds->fdb_isolation = true;
>  	ds->max_num_bridges = 1;
>  
> +	/* ANA_AUTOAGE_AGE_PERIOD is a seconds-based field and entries are
> +	 * aged after 2 * AGE_PERIOD, giving (2 * FIELD_MAX) seconds of
> +	 * maximum aging and 2 seconds of minimum aging.
> +	 *
> +	 * An AGE_PERIOD of 0 stops the analyzer from aging at all, which is
> +	 * the opposite of what a bridge ageing_time of 0 asks for. The bridge
> +	 * expires every dynamic entry immediately in that case and floods, so
> +	 * reject it rather than keep learned addresses forever.
> +	 */
> +	ds->ageing_time_min = 2U * MSEC_PER_SEC;
> +	ds->ageing_time_max = 2U * MSEC_PER_SEC *
> +			      FIELD_MAX(ANA_AUTOAGE_AGE_PERIOD);

[Severity: Medium]
Can a bridge join that fails on these limits leave the port VLAN-unaware
in hardware after the next successful join?

With ageing_time_min set, dsa_switch_ageing_time() returns -ERANGE for any
bridge ageing_time below 2s, including 0. For example:

  ip link add br0 type bridge vlan_filtering 1 ageing_time 0

dsa_port_switchdev_sync_attrs() applies the ageing time last. By then,
dsa_port_vlan_filtering(dp, br_vlan_enabled(br)) has already called
lan9645x_port_vlan_filtering(true) and set dp->vlan_filtering = true.

The rollback in dsa_port_bridge_join() then does:

net/dsa/port.c:dsa_port_bridge_join() {
    ...
out_rollback_unoffload:
	switchdev_bridge_port_unoffload(...);
	dsa_flush_workqueue();
out_rollback_unbridge:
	dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
out_rollback:
	dsa_port_bridge_destroy(dp, br);
    ...
}

This path never calls dsa_port_switchdev_unsync_attrs() or
dsa_port_reset_vlan_filtering(), so dp->vlan_filtering stays true.

Meanwhile the BRIDGE_LEAVE notifier reaches lan9645x_port_bridge_leave().
That function calls lan9645x_vlan_set_hostmode(), which sets
p->vlan_aware = false.

Now suppose the ageing time is fixed and the port is added to a
vlan_filtering=1 bridge again. dsa_port_vlan_filtering() returns early:

	if (dsa_port_is_vlan_filtering(dp) == vlan_filtering)
		return 0;

So .port_vlan_filtering is never called. Neither
lan9645x_port_bridge_join() nor lan9645x_vlan_clear_hostmode() restores
p->vlan_aware.

Would the port then stay at VLAN_AWARE_ENA=0 in ANA_VLAN_CFG while it is
in a VLAN-aware bridge? That would give wrong ingress VLAN classification
and filtering.

The incomplete rollback is in the DSA core. This patch makes it reachable
through the new ageing limits, and the new bridge_leave creates the
mismatch on the driver side. The issue still seems to be there at the end
of the series.

[ ... ]

> @@ -324,6 +333,246 @@ static void lan9645x_port_phylink_get_caps(struct dsa_switch *ds, int port,
>  	lan9645x_phylink_get_caps(ds->priv, port, config);
>  }
>  
> +static int lan9645x_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +
> +	/* Entry must suffer two aging scans before it is removed, so it is
> +	 * aged after 2*AGE_PERIOD, and the unit is in seconds.
> +	 */
> +	lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(msecs / (2 * MSEC_PER_SEC)),
> +		ANA_AUTOAGE_AGE_PERIOD,
> +		lan9645x, ANA_AUTOAGE);
> +
> +	return 0;
> +}

[Severity: Medium]
This is a pre-existing issue in the DSA core and was not introduced by
this patch, but it affects the new lan9645x_set_ageing_time(). Does
raising the bridge ageing time ever reach the hardware?

dsa_port_ageing_time() sends the notifier before it updates the cached
value:

net/dsa/port.c:dsa_port_ageing_time() {
    ...
	err = dsa_port_notify(dp, DSA_NOTIFIER_AGEING_TIME, &info);
	if (err)
		return err;

	dp->ageing_time = ageing_time;
    ...
}

dsa_switch_fastest_ageing_time() then takes the minimum over all ports,
including the old value of the requesting port:

	dsa_switch_for_each_port(dp, ds)
		if (dp->ageing_time && dp->ageing_time < ageing_time)
			ageing_time = dp->ageing_time;

So a change from 300000 ms to 600000 ms programs 300000 ms through
lan9645x_set_ageing_time() and returns success. 600000 ms is then cached
in dp->ageing_time, with no further hardware write.

The fix probably belongs in the DSA core rather than in this driver: the
requester's cached value could be updated, or left out, before the
minimum is computed.

[ ... ]

> +static int lan9645x_port_bridge_flags(struct dsa_switch *ds, int port,
> +				      struct switchdev_brport_flags f,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +

[ ... ]

> +	if (f.mask & BR_MCAST_FLOOD) {
> +		bool ena = !!(f.val & BR_MCAST_FLOOD);
> +
> +		lan9645x_port_pgid_set(lan9645x, PGID_MC, port, ena);
> +		lan9645x_port_pgid_set(lan9645x, PGID_MCIPV4, port, ena);
> +		lan9645x_port_pgid_set(lan9645x, PGID_MCIPV6, port, ena);
> +	}

[Severity: Medium]
Does this break the PGID_MC fallback that the mdb code uses when the
general-purpose PGIDs run out?

When a group's PGID is shared or no PGID is free, __lan9645x_mdb_del()
points the L2 group at PGID_MC:

drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c:__lan9645x_mdb_del() {
    ...
	/* Shared PGID or on flood mask. Point the entry at the L2 MC flood mask
	 * instead. This way our mac table entry survives and MAC_CPU_COPY
	 * keeps host membership working.
	 */
    ...
	lan9645x_mdb_narrow_dest(lan9645x, mdb_entry, type, NULL, PGID_MC,
				 new_ports);
    ...
}

__lan9645x_mdb_add() also keeps a group that is already on the flood mask
pointed at PGID_MC:

		/* For a L2 MC group already on the flood mask, we keep it there
		 * so that the host can still join/leave a group we had to give
		 * up offloading. A front port that joins forwards according to
		 * the flood masks, like the other members.
		 */
		if (lan9645x_mdb_on_flood_mask(mdb_entry, type))
			return lan9645x_mdb_widen_dest(lan9645x, mdb_entry,
						       type, NULL, PGID_MC,
						       new_ports);

This fallback depended on PGID_MC holding every front port, as
lan9645x_setup() sets it. With this change, a port configured with:

  bridge link set dev swpN mcast_flood off

is removed from PGID_MC. If that port is also a registered member of an
L2 group in the fallback state, will it stop receiving that group's
traffic?

The software bridge still delivers registered groups to their members
whatever their mcast_flood setting, so hardware forwarding would no
longer match it. Also, when such a port joins through
__lan9645x_mdb_add(), the join reports success but the port gets no
traffic. The issue still seems to be there at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213%40microchip.com