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

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);
>

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

[Jens Emil Schulz Ostergaard]

On Tue, 2026-05-05 at 18:46 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

Yes I will add this deinit to the unwind in the error path.

> 
> > 
> >       /* 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.

No it does work, but it does makes more sense to not do this for the
npi port. I will change this.

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