[PATCH net-next] net: dsa: sja1105: reorganize probe, remove, setup and teardown ordering

[PATCH net-next] net: dsa: sja1105: reorganize probe, remove, setup and teardown ordering

[Vladimir Oltean]

The sja1105 driver's initialization and teardown sequence is a chaotic
mess that has gathered a lot of cruft over time. It works because there
is no strict dependency between the functions, but it could be improved.

The basic principle that teardown should be the exact reverse of setup
is obviously not held. We have initialization steps (sja1105_tas_setup,
sja1105_flower_setup) in the probe method that are torn down in the DSA
.teardown method instead of driver unbind time.

We also have code after the dsa_register_switch() call, which implicitly
means after the .setup() method has finished, which is pretty unusual.

Also, sja1105_teardown() has calls set up in a different order than the
error path of sja1105_setup(): see the reversed ordering between
sja1105_ptp_clock_unregister and sja1105_mdiobus_unregister.

Also, sja1105_static_config_load() is called towards the end of
sja1105_setup(), but sja1105_static_config_free() is also towards the
end of the error path and teardown path. The static_config_load() call
should be earlier.

Also, making and breaking the connections between struct sja1105_port
and struct dsa_port could be refactored into dedicated functions, makes
the code easier to follow.

We move some code from the DSA .setup() method into the probe method,
like the device tree parsing, and we move some code from the probe
method into the DSA .setup() method to be symmetric with its placement
in the DSA .teardown() method, which is nice because the unbind function
has a single call to dsa_unregister_switch(). Example of the latter type
of code movement are the connections between ports mentioned above, they
are now in the .setup() method.

Finally, due to fact that the kthread_init_worker() call is no longer
in sja1105_probe() - located towards the bottom of the file - but in
sja1105_setup() - located much higher - there is an inverse ordering
with the worker function declaration, sja1105_port_deferred_xmit. To
avoid that, the entire sja1105_setup() and sja1105_teardown() functions
are moved towards the bottom of the file.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 397 +++++++++++++------------
 1 file changed, 199 insertions(+), 198 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index ae7dd9fa70a1..fe894dc18335 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2515,149 +2515,6 @@ static int sja1105_prechangeupper(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-/* The programming model for the SJA1105 switch is "all-at-once" via static
- * configuration tables. Some of these can be dynamically modified at runtime,
- * but not the xMII mode parameters table.
- * Furthermode, some PHYs may not have crystals for generating their clocks
- * (e.g. RMII). Instead, their 50MHz clock is supplied via the SJA1105 port's
- * ref_clk pin. So port clocking needs to be initialized early, before
- * connecting to PHYs is attempted, otherwise they won't respond through MDIO.
- * Setting correct PHY link speed does not matter now.
- * But dsa_slave_phy_setup is called later than sja1105_setup, so the PHY
- * bindings are not yet parsed by DSA core. We need to parse early so that we
- * can populate the xMII mode parameters table.
- */
-static int sja1105_setup(struct dsa_switch *ds)
-{
-	struct sja1105_private *priv = ds->priv;
-	int rc;
-
-	rc = sja1105_parse_dt(priv);
-	if (rc < 0) {
-		dev_err(ds->dev, "Failed to parse DT: %d\n", rc);
-		return rc;
-	}
-
-	/* Error out early if internal delays are required through DT
-	 * and we can't apply them.
-	 */
-	rc = sja1105_parse_rgmii_delays(priv);
-	if (rc < 0) {
-		dev_err(ds->dev, "RGMII delay not supported\n");
-		return rc;
-	}
-
-	rc = sja1105_ptp_clock_register(ds);
-	if (rc < 0) {
-		dev_err(ds->dev, "Failed to register PTP clock: %d\n", rc);
-		return rc;
-	}
-
-	rc = sja1105_mdiobus_register(ds);
-	if (rc < 0) {
-		dev_err(ds->dev, "Failed to register MDIO bus: %pe\n",
-			ERR_PTR(rc));
-		goto out_ptp_clock_unregister;
-	}
-
-	if (priv->info->disable_microcontroller) {
-		rc = priv->info->disable_microcontroller(priv);
-		if (rc < 0) {
-			dev_err(ds->dev,
-				"Failed to disable microcontroller: %pe\n",
-				ERR_PTR(rc));
-			goto out_mdiobus_unregister;
-		}
-	}
-
-	/* Create and send configuration down to device */
-	rc = sja1105_static_config_load(priv);
-	if (rc < 0) {
-		dev_err(ds->dev, "Failed to load static config: %d\n", rc);
-		goto out_mdiobus_unregister;
-	}
-
-	/* Configure the CGU (PHY link modes and speeds) */
-	if (priv->info->clocking_setup) {
-		rc = priv->info->clocking_setup(priv);
-		if (rc < 0) {
-			dev_err(ds->dev,
-				"Failed to configure MII clocking: %pe\n",
-				ERR_PTR(rc));
-			goto out_static_config_free;
-		}
-	}
-
-	/* On SJA1105, VLAN filtering per se is always enabled in hardware.
-	 * The only thing we can do to disable it is lie about what the 802.1Q
-	 * EtherType is.
-	 * So it will still try to apply VLAN filtering, but all ingress
-	 * traffic (except frames received with EtherType of ETH_P_SJA1105)
-	 * will be internally tagged with a distorted VLAN header where the
-	 * TPID is ETH_P_SJA1105, and the VLAN ID is the port pvid.
-	 */
-	ds->vlan_filtering_is_global = true;
-	ds->untag_bridge_pvid = true;
-	/* tag_8021q has 3 bits for the VBID, and the value 0 is reserved */
-	ds->num_fwd_offloading_bridges = 7;
-
-	/* Advertise the 8 egress queues */
-	ds->num_tx_queues = SJA1105_NUM_TC;
-
-	ds->mtu_enforcement_ingress = true;
-	ds->assisted_learning_on_cpu_port = true;
-
-	rc = sja1105_devlink_setup(ds);
-	if (rc < 0)
-		goto out_static_config_free;
-
-	rtnl_lock();
-	rc = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
-	rtnl_unlock();
-	if (rc)
-		goto out_devlink_teardown;
-
-	return 0;
-
-out_devlink_teardown:
-	sja1105_devlink_teardown(ds);
-out_mdiobus_unregister:
-	sja1105_mdiobus_unregister(ds);
-out_ptp_clock_unregister:
-	sja1105_ptp_clock_unregister(ds);
-out_static_config_free:
-	sja1105_static_config_free(&priv->static_config);
-
-	return rc;
-}
-
-static void sja1105_teardown(struct dsa_switch *ds)
-{
-	struct sja1105_private *priv = ds->priv;
-	int port;
-
-	rtnl_lock();
-	dsa_tag_8021q_unregister(ds);
-	rtnl_unlock();
-
-	for (port = 0; port < ds->num_ports; port++) {
-		struct sja1105_port *sp = &priv->ports[port];
-
-		if (!dsa_is_user_port(ds, port))
-			continue;
-
-		if (sp->xmit_worker)
-			kthread_destroy_worker(sp->xmit_worker);
-	}
-
-	sja1105_devlink_teardown(ds);
-	sja1105_mdiobus_unregister(ds);
-	sja1105_flower_teardown(ds);
-	sja1105_tas_teardown(ds);
-	sja1105_ptp_clock_unregister(ds);
-	sja1105_static_config_free(&priv->static_config);
-}
-
 static void sja1105_port_disable(struct dsa_switch *ds, int port)
 {
 	struct sja1105_private *priv = ds->priv;
@@ -3060,6 +2917,189 @@ static int sja1105_port_bridge_flags(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void sja1105_teardown_ports(struct sja1105_private *priv)
+{
+	struct dsa_switch *ds = priv->ds;
+	int port;
+
+	for (port = 0; port < ds->num_ports; port++) {
+		struct sja1105_port *sp = &priv->ports[port];
+
+		if (sp->xmit_worker)
+			kthread_destroy_worker(sp->xmit_worker);
+	}
+}
+
+static int sja1105_setup_ports(struct sja1105_private *priv)
+{
+	struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
+	struct dsa_switch *ds = priv->ds;
+	int port, rc;
+
+	/* Connections between dsa_port and sja1105_port */
+	for (port = 0; port < ds->num_ports; port++) {
+		struct sja1105_port *sp = &priv->ports[port];
+		struct dsa_port *dp = dsa_to_port(ds, port);
+		struct kthread_worker *worker;
+		struct net_device *slave;
+
+		if (!dsa_port_is_user(dp))
+			continue;
+
+		dp->priv = sp;
+		sp->dp = dp;
+		sp->data = tagger_data;
+		slave = dp->slave;
+		kthread_init_work(&sp->xmit_work, sja1105_port_deferred_xmit);
+		worker = kthread_create_worker(0, "%s_xmit", slave->name);
+		if (IS_ERR(worker)) {
+			rc = PTR_ERR(worker);
+			dev_err(ds->dev,
+				"failed to create deferred xmit thread: %d\n",
+				rc);
+			goto out_destroy_workers;
+		}
+		sp->xmit_worker = worker;
+		skb_queue_head_init(&sp->xmit_queue);
+		sp->xmit_tpid = ETH_P_SJA1105;
+	}
+
+	return 0;
+
+out_destroy_workers:
+	sja1105_teardown_ports(priv);
+	return rc;
+}
+
+/* The programming model for the SJA1105 switch is "all-at-once" via static
+ * configuration tables. Some of these can be dynamically modified at runtime,
+ * but not the xMII mode parameters table.
+ * Furthermode, some PHYs may not have crystals for generating their clocks
+ * (e.g. RMII). Instead, their 50MHz clock is supplied via the SJA1105 port's
+ * ref_clk pin. So port clocking needs to be initialized early, before
+ * connecting to PHYs is attempted, otherwise they won't respond through MDIO.
+ * Setting correct PHY link speed does not matter now.
+ * But dsa_slave_phy_setup is called later than sja1105_setup, so the PHY
+ * bindings are not yet parsed by DSA core. We need to parse early so that we
+ * can populate the xMII mode parameters table.
+ */
+static int sja1105_setup(struct dsa_switch *ds)
+{
+	struct sja1105_private *priv = ds->priv;
+	int rc;
+
+	if (priv->info->disable_microcontroller) {
+		rc = priv->info->disable_microcontroller(priv);
+		if (rc < 0) {
+			dev_err(ds->dev,
+				"Failed to disable microcontroller: %pe\n",
+				ERR_PTR(rc));
+			return rc;
+		}
+	}
+
+	/* Create and send configuration down to device */
+	rc = sja1105_static_config_load(priv);
+	if (rc < 0) {
+		dev_err(ds->dev, "Failed to load static config: %d\n", rc);
+		return rc;
+	}
+
+	/* Configure the CGU (PHY link modes and speeds) */
+	if (priv->info->clocking_setup) {
+		rc = priv->info->clocking_setup(priv);
+		if (rc < 0) {
+			dev_err(ds->dev,
+				"Failed to configure MII clocking: %pe\n",
+				ERR_PTR(rc));
+			goto out_static_config_free;
+		}
+	}
+
+	rc = sja1105_setup_ports(priv);
+	if (rc)
+		goto out_static_config_free;
+
+	sja1105_tas_setup(ds);
+	sja1105_flower_setup(ds);
+
+	rc = sja1105_ptp_clock_register(ds);
+	if (rc < 0) {
+		dev_err(ds->dev, "Failed to register PTP clock: %d\n", rc);
+		goto out_flower_teardown;
+	}
+
+	rc = sja1105_mdiobus_register(ds);
+	if (rc < 0) {
+		dev_err(ds->dev, "Failed to register MDIO bus: %pe\n",
+			ERR_PTR(rc));
+		goto out_ptp_clock_unregister;
+	}
+
+	rc = sja1105_devlink_setup(ds);
+	if (rc < 0)
+		goto out_mdiobus_unregister;
+
+	rtnl_lock();
+	rc = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
+	rtnl_unlock();
+	if (rc)
+		goto out_devlink_teardown;
+
+	/* On SJA1105, VLAN filtering per se is always enabled in hardware.
+	 * The only thing we can do to disable it is lie about what the 802.1Q
+	 * EtherType is.
+	 * So it will still try to apply VLAN filtering, but all ingress
+	 * traffic (except frames received with EtherType of ETH_P_SJA1105)
+	 * will be internally tagged with a distorted VLAN header where the
+	 * TPID is ETH_P_SJA1105, and the VLAN ID is the port pvid.
+	 */
+	ds->vlan_filtering_is_global = true;
+	ds->untag_bridge_pvid = true;
+	/* tag_8021q has 3 bits for the VBID, and the value 0 is reserved */
+	ds->num_fwd_offloading_bridges = 7;
+
+	/* Advertise the 8 egress queues */
+	ds->num_tx_queues = SJA1105_NUM_TC;
+
+	ds->mtu_enforcement_ingress = true;
+	ds->assisted_learning_on_cpu_port = true;
+
+	return 0;
+
+out_devlink_teardown:
+	sja1105_devlink_teardown(ds);
+out_mdiobus_unregister:
+	sja1105_mdiobus_unregister(ds);
+out_ptp_clock_unregister:
+	sja1105_ptp_clock_unregister(ds);
+out_flower_teardown:
+	sja1105_flower_teardown(ds);
+	sja1105_tas_teardown(ds);
+	sja1105_teardown_ports(priv);
+out_static_config_free:
+	sja1105_static_config_free(&priv->static_config);
+
+	return rc;
+}
+
+static void sja1105_teardown(struct dsa_switch *ds)
+{
+	struct sja1105_private *priv = ds->priv;
+
+	rtnl_lock();
+	dsa_tag_8021q_unregister(ds);
+	rtnl_unlock();
+
+	sja1105_devlink_teardown(ds);
+	sja1105_mdiobus_unregister(ds);
+	sja1105_ptp_clock_unregister(ds);
+	sja1105_flower_teardown(ds);
+	sja1105_tas_teardown(ds);
+	sja1105_teardown_ports(priv);
+	sja1105_static_config_free(&priv->static_config);
+}
+
 static const struct dsa_switch_ops sja1105_switch_ops = {
 	.get_tag_protocol	= sja1105_get_tag_protocol,
 	.setup			= sja1105_setup,
@@ -3161,12 +3201,11 @@ static int sja1105_check_device_id(struct sja1105_private *priv)
 
 static int sja1105_probe(struct spi_device *spi)
 {
-	struct sja1105_tagger_data *tagger_data;
 	struct device *dev = &spi->dev;
 	struct sja1105_private *priv;
 	size_t max_xfer, max_msg;
 	struct dsa_switch *ds;
-	int rc, port;
+	int rc;
 
 	if (!dev->of_node) {
 		dev_err(dev, "No DTS bindings for SJA1105 driver\n");
@@ -3246,71 +3285,33 @@ static int sja1105_probe(struct spi_device *spi)
 	ds->priv = priv;
 	priv->ds = ds;
 
-	tagger_data = &priv->tagger_data;
-
 	mutex_init(&priv->ptp_data.lock);
 	mutex_init(&priv->mgmt_lock);
 
-	sja1105_tas_setup(ds);
-	sja1105_flower_setup(ds);
+	rc = sja1105_parse_dt(priv);
+	if (rc < 0) {
+		dev_err(ds->dev, "Failed to parse DT: %d\n", rc);
+		return rc;
+	}
 
-	rc = dsa_register_switch(priv->ds);
-	if (rc)
+	/* Error out early if internal delays are required through DT
+	 * and we can't apply them.
+	 */
+	rc = sja1105_parse_rgmii_delays(priv);
+	if (rc < 0) {
+		dev_err(ds->dev, "RGMII delay not supported\n");
 		return rc;
+	}
 
 	if (IS_ENABLED(CONFIG_NET_SCH_CBS)) {
 		priv->cbs = devm_kcalloc(dev, priv->info->num_cbs_shapers,
 					 sizeof(struct sja1105_cbs_entry),
 					 GFP_KERNEL);
-		if (!priv->cbs) {
-			rc = -ENOMEM;
-			goto out_unregister_switch;
-		}
+		if (!priv->cbs)
+			return -ENOMEM;
 	}
 
-	/* Connections between dsa_port and sja1105_port */
-	for (port = 0; port < ds->num_ports; port++) {
-		struct sja1105_port *sp = &priv->ports[port];
-		struct dsa_port *dp = dsa_to_port(ds, port);
-		struct net_device *slave;
-
-		if (!dsa_is_user_port(ds, port))
-			continue;
-
-		dp->priv = sp;
-		sp->dp = dp;
-		sp->data = tagger_data;
-		slave = dp->slave;
-		kthread_init_work(&sp->xmit_work, sja1105_port_deferred_xmit);
-		sp->xmit_worker = kthread_create_worker(0, "%s_xmit",
-							slave->name);
-		if (IS_ERR(sp->xmit_worker)) {
-			rc = PTR_ERR(sp->xmit_worker);
-			dev_err(ds->dev,
-				"failed to create deferred xmit thread: %d\n",
-				rc);
-			goto out_destroy_workers;
-		}
-		skb_queue_head_init(&sp->xmit_queue);
-		sp->xmit_tpid = ETH_P_SJA1105;
-	}
-
-	return 0;
-
-out_destroy_workers:
-	while (port-- > 0) {
-		struct sja1105_port *sp = &priv->ports[port];
-
-		if (!dsa_is_user_port(ds, port))
-			continue;
-
-		kthread_destroy_worker(sp->xmit_worker);
-	}
-
-out_unregister_switch:
-	dsa_unregister_switch(ds);
-
-	return rc;
+	return dsa_register_switch(priv->ds);
 }
 
 static int sja1105_remove(struct spi_device *spi)
-- 
2.25.1

Re: [PATCH net-next] net: dsa: sja1105: reorganize probe, remove, setup and teardown ordering

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Sun, 15 Aug 2021 15:00:35 +0300 you wrote:
> The sja1105 driver's initialization and teardown sequence is a chaotic
> mess that has gathered a lot of cruft over time. It works because there
> is no strict dependency between the functions, but it could be improved.
> 
> The basic principle that teardown should be the exact reverse of setup
> is obviously not held. We have initialization steps (sja1105_tas_setup,
> sja1105_flower_setup) in the probe method that are torn down in the DSA
> .teardown method instead of driver unbind time.
> 
> [...]

Here is the summary with links:
  - [net-next] net: dsa: sja1105: reorganize probe, remove, setup and teardown ordering
    https://git.kernel.org/netdev/net-next/c/022522aca430

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html