Re: [PATCH net-next v4 8/9] net: dsa: lan9645x: add mdb management

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: lan9645x: add mdb management

This change adds DSA mdb callbacks for lan9645x, with distinct handling
for L2 multicast (PGID-backed) and IP multicast (port mask inline in
the mac table).

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index b21e1bf25b0c..15a493c1f215 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> @@ -70,6 +70,7 @@ static void lan9645x_teardown(struct dsa_switch *ds)
>  	destroy_workqueue(lan9645x->owq);
>  	lan9645x_npi_port_deinit(lan9645x, lan9645x->npi);
>  	lan9645x_mac_deinit(lan9645x);
> +	lan9645x_mdb_deinit(lan9645x);
>  	mutex_destroy(&lan9645x->fwd_domain_lock);
>  }
>  
> @@ -141,6 +142,7 @@ static int lan9645x_setup(struct dsa_switch *ds)
>  	if (err)
>  		return err;
>  	lan9645x_mac_init(lan9645x);
> +	lan9645x_mdb_init(lan9645x);

Is lan9645x_mdb_deinit() reachable when lan9645x_setup() fails after
this point?

lan9645x_setup() continues after lan9645x_mdb_init() with operations
that can fail, including alloc_ordered_workqueue() near the bottom of
the function which returns -ENOMEM. On error from ds->ops->setup(),
dsa_switch_setup() in net/dsa/dsa.c jumps directly to the notifier
cleanup path and does not invoke ds->ops->teardown:

	err = ds->ops->setup(ds);
	if (err < 0)
		goto unregister_notifier;

Since lan9645x_teardown() is where lan9645x_mdb_deinit() runs,
the freshly initialized mdb_lock (and its lockdep map) is never
torn down if setup fails after lan9645x_mdb_init().

Could lan9645x_mdb_init() be moved later in setup (after the steps
that can still fail), or could setup-time error unwinding be added
for mdb_lock, mact_lock and fwd_domain_lock? The same pattern exists
for mact_lock (from lan9645x_mac_init()) and fwd_domain_lock, and this
change adds mdb_lock to the same class of resources.

>  
>  	/* Link Aggregation Mode: NETDEV_LAG_HASH_L2 */
>  	lan_wr(ANA_AGGR_CFG_AC_SMAC_ENA |

[ ... ]

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> index 661cd00465e2..5dee92940f89 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> @@ -189,6 +189,14 @@ int lan9645x_port_setup(struct dsa_switch *ds, int port)
>  		ANA_PORT_CFG_PORTID_VAL,
>  		lan9645x, ANA_PORT_CFG(p->chip_port));
>  
> +	lan_rmw(ANA_CPU_FWD_CFG_IGMP_REDIR_ENA_SET(true) |
> +		ANA_CPU_FWD_CFG_MLD_REDIR_ENA_SET(true) |
> +		ANA_CPU_FWD_CFG_IPMC_CTRL_COPY_ENA_SET(true),
> +		ANA_CPU_FWD_CFG_IGMP_REDIR_ENA |
> +		ANA_CPU_FWD_CFG_MLD_REDIR_ENA |
> +		ANA_CPU_FWD_CFG_IPMC_CTRL_COPY_ENA,
> +		lan9645x, ANA_CPU_FWD_CFG(p->chip_port));
> +

Should this block be guarded for the NPI (CPU-facing) port?

The IGMP/MLD redirect and IPMC control-copy bits are enabled here for
every chip_port that lan9645x_port_setup() is called on, with no
check for whether the port is the NPI. The VLAN host-mode call a few
lines below explicitly excludes the NPI:

	if (p->chip_port != lan9645x->npi)
		lan9645x_vlan_set_hostmode(p);

In the related driver for the same family, drivers/net/ethernet/mscc/
ocelot_net.c, ocelot_port_attr_mc_set() toggles these same bits only
on user ports in response to SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED.

Can CPU-injected IGMP/MLD/IPMC-control frames end up being redirected
or copied back to the CPU with this configuration, or does IFH_BYPASS=1
fully suppress these classifiers on NPI ingress? If the latter, the
bits on the NPI are at least wasteful; if the former, duplicate
delivery or a trap loop seems possible.

>  	if (p->chip_port != lan9645x->npi)
>  		lan9645x_vlan_set_hostmode(p);
>