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

[Wei Fang]

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

Yes, I will fix it.

> 
> > @@ -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?

No need to clear the hardware IPFT entry in tear down. Because PCIe FLR will
be performed when the switch is registered, it will reset hardware state. The
driver only needs to free the memory to avoid memory leak.

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