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

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.

[ ... ]

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()?

[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()?