Re: [net-next,12/14] net: dsa: netc: add more basic functions support

Simon Horman
2026-03-18T14:53:24Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: netc: add more basic functions support

This patch expands the NETC switch driver with several foundational
features, including FDB and MDB management, STP state handling, MTU
configuration, port setup/teardown, and host flooding support.
At this stage, the driver operates only in standalone port mode.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 884ee899fc89..fd952b507cbd 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -427,24 +658,57 @@ static int netc_setup(struct dsa_switch *ds)
> if (err)
> goto free_mdio_bus;
>
> + INIT_HLIST_HEAD(&priv->fdb_list);
> + mutex_init(&priv->fdbt_lock);
> +
> netc_switch_fixed_config(priv);
>
> /* default setting for ports */
> dsa_switch_for_each_available_port(dp, ds)
> netc_port_default_config(priv->ports[dp->index]);
>
> + err = netc_add_standalone_vlan_entry(priv);
> + if (err)
> + goto free_ntmp_user;
> +
> + err = netc_add_standalone_fdb_bcast_entry(priv);
> + if (err)
> + goto free_ntmp_user;
> +
> return 0;
>
> +free_ntmp_user:
> + netc_free_ntmp_user(priv);
> free_mdio_bus:
> netc_free_mdio_bus(priv);
>
> return err;
> }

Does the error path handle the mutex properly?

In netc_setup(), the mutex is initialized with mutex_init(&priv->fdbt_lock).
If netc_add_standalone_vlan_entry() or netc_add_standalone_fdb_bcast_entry()
fails, the error path jumps to free_ntmp_user:

drivers/net/dsa/netc/netc_main.c:netc_setup() {
...
INIT_HLIST_HEAD(&priv->fdb_list);
mutex_init(&priv->fdbt_lock);
...
err = netc_add_standalone_vlan_entry(priv);
if (err)
goto free_ntmp_user;

err = netc_add_standalone_fdb_bcast_entry(priv);
if (err)
goto free_ntmp_user;

return 0;

free_ntmp_user:
netc_free_ntmp_user(priv);
free_mdio_bus:
netc_free_mdio_bus(priv);

return err;
}

But netc_free_ntmp_user() and netc_free_mdio_bus() don't call
mutex_destroy(&priv->fdbt_lock) to clean up the mutex.

Looking at the normal teardown path in netc_teardown(), it calls
netc_destroy_all_lists() which properly handles both the FDB list and
mutex cleanup:

drivers/net/dsa/netc/netc_main.c:netc_destroy_all_lists() {
netc_destroy_fdb_list(priv);
mutex_destroy(&priv->fdbt_lock);
}

drivers/net/dsa/netc/netc_main.c:netc_teardown() {
struct netc_switch *priv = ds->priv;

netc_destroy_all_lists(priv);
netc_free_host_flood_rules(priv);
netc_free_ntmp_user(priv);
netc_free_mdio_bus(priv);
}

Should the error path in netc_setup() call netc_destroy_all_lists() before
calling netc_free_ntmp_user()?

> @@ -595,6 +859,261 @@ static void netc_switch_get_ip_revision(struct netc_switch *priv)
> priv->revision = val & IPBRR0_IP_REV;
> }
>
> +static void netc_destroy_all_lists(struct netc_switch *priv)
> +{
> + netc_destroy_fdb_list(priv);
> + mutex_destroy(&priv->fdbt_lock);
> +}
> +
> +static void netc_free_host_flood_rules(struct netc_switch *priv)
> +{
> + struct dsa_port *dp;
> +
> + dsa_switch_for_each_user_port(dp, priv->ds) {
> + struct netc_port *np = priv->ports[dp->index];
> +
> + kfree(np->host_flood);
> + np->host_flood = NULL;
> + }
> +}

[Simon says: I think this is a false positive and the clean-up
suggestion is not needed in tear down]

Does this properly clean up the hardware state?

netc_free_host_flood_rules() just frees the host_flood memory and sets
the pointer to NULL:

drivers/net/dsa/netc/netc_main.c:netc_free_host_flood_rules() {
struct dsa_port *dp;

dsa_switch_for_each_user_port(dp, priv->ds) {
struct netc_port *np = priv->ports[dp->index];

kfree(np->host_flood);
np->host_flood = NULL;
}
}

But it doesn't delete the hardware IPFT entry or disable the PIPFCR register.

Compare with netc_port_remove_host_flood() which performs complete cleanup:

drivers/net/dsa/netc/netc_main.c:netc_port_remove_host_flood() {
struct netc_switch *priv = np->switch_priv;

if (!np->host_flood)
return;

ntmp_ipft_delete_entry(&priv->ntmp, np->host_flood->entry_id);
kfree(np->host_flood);
np->host_flood = NULL;
np->uc = false;
np->mc = false;
/* Disable ingress port filter table lookup */
netc_port_wr(np, NETC_PIPFCR, 0);
}

In netc_teardown(), netc_free_host_flood_rules() is called before
netc_free_ntmp_user(), so the NTMP command infrastructure is still
operational at that point.

Should netc_free_host_flood_rules() call netc_port_remove_host_flood(np)
for each user port instead of directly calling kfree()?