[PATCH net-next 0/4] net: ethernet: ti: am65-cpsw: streamline RX/TX queue creation and cleanup

[PATCH net-next 0/4] net: ethernet: ti: am65-cpsw: streamline RX/TX queue creation and cleanup

[Roger Quadros]

In this series we first cleanup probe error path and get rid of
devm_add/remove_action(). Split code into symmetric init and cleanup
functions and use them.

Then we streamline RX and TX queue creation and cleanup. The queues
can now be created or destroyed by calling the appropriate
functions am65_cpsw_create_rxqs/txqs() and am65_cpsw_destroy_rxq/txqs().

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
Roger Quadros (4):
      net: ethernet: am65-cpsw: call netif_carrier_on/off() when appropriate
      net: ethernet: ti: am65-cpsw: streamline .probe() error handling
      net: ethernet: ti: am65-cpsw: streamline RX queue creation and cleanup
      net: ethernet: ti: am65-cpsw: streamline TX queue creation and cleanup

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 643 ++++++++++++++++---------------
 1 file changed, 324 insertions(+), 319 deletions(-)
---
base-commit: 9c7ad35632297edc08d0f2c7b599137e9fb5f9ff
change-id: 20250111-am65-cpsw-streamline-33587ae6e9e5

Best regards,
-- 
Roger Quadros <rogerq@kernel.org>

[PATCH net-next 1/4] net: ethernet: am65-cpsw: call netif_carrier_on/off() when appropriate

[Roger Quadros]

Call netif_carrier_on/off when link is up/down.
When link is up only wake TX netif queue if network device is
running.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index dcb6662b473d..36c29d3db329 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2155,6 +2155,7 @@ static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned
 	cpsw_sl_ctl_clr(port->slave.mac_sl, mac_control);
 
 	am65_cpsw_qos_link_down(ndev);
+	netif_carrier_off(ndev);
 	netif_tx_stop_all_queues(ndev);
 }
 
@@ -2196,7 +2197,9 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
 	cpsw_ale_control_set(common->ale, port->port_id, ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);
 
 	am65_cpsw_qos_link_up(ndev, speed);
-	netif_tx_wake_all_queues(ndev);
+	netif_carrier_on(ndev);
+	if (netif_running(ndev))
+		netif_tx_wake_all_queues(ndev);
 }
 
 static const struct phylink_mac_ops am65_cpsw_phylink_mac_ops = {

-- 
2.34.1

[PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: streamline .probe() error handling

[Roger Quadros]

Keep things simple by explicitly cleaning up on .probe() error
path or .remove(). Get rid of devm_add/remove_action() usage.

Rename am65_cpsw_disable_serdes_phy() to
am65_cpsw_nuss_cleanup_slave_ports() and move it right before
am65_cpsw_nuss_init_slave_ports().

Get rid of am65_cpsw_nuss_phylink_cleanup() and introduce
am65_cpsw_nuss_cleanup_ndevs() right before am65_cpsw_nuss_init_ndevs()

Move channel initiailzation code out of am65_cpsw_nuss_register_ndevs()
into new function am65_cpsw_nuss_init_chns().
Add am65_cpsw_nuss_remove_chns() to do reverse of
am65_cpsw_nuss_init_chns().

Add am65_cpsw_nuss_unregister_ndev() to do reverse of
am65_cpsw_nuss_register_ndevs().

Use the introduced helpers in probe/remove.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 276 ++++++++++++++-----------------
 1 file changed, 126 insertions(+), 150 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 36c29d3db329..783ec461dbdc 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2056,20 +2056,6 @@ static int am65_cpsw_enable_phy(struct phy *phy)
 	return 0;
 }
 
-static void am65_cpsw_disable_serdes_phy(struct am65_cpsw_common *common)
-{
-	struct am65_cpsw_port *port;
-	struct phy *phy;
-	int i;
-
-	for (i = 0; i < common->port_num; i++) {
-		port = &common->ports[i];
-		phy = port->slave.serdes_phy;
-		if (phy)
-			am65_cpsw_disable_phy(phy);
-	}
-}
-
 static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
 				     struct am65_cpsw_port *port)
 {
@@ -2222,14 +2208,20 @@ static void am65_cpsw_nuss_slave_disable_unused(struct am65_cpsw_port *port)
 	cpsw_sl_ctl_reset(port->slave.mac_sl);
 }
 
-static void am65_cpsw_nuss_free_tx_chns(void *data)
+static void am65_cpsw_nuss_cleanup_tx_chns(struct am65_cpsw_common *common)
 {
-	struct am65_cpsw_common *common = data;
+	struct device *dev = common->dev;
 	int i;
 
+	common->tx_ch_rate_msk = 0;
 	for (i = 0; i < common->tx_ch_num; i++) {
 		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
 
+		if (tx_chn->irq)
+			devm_free_irq(dev, tx_chn->irq, tx_chn);
+
+		netif_napi_del(&tx_chn->napi_tx);
+
 		if (!IS_ERR_OR_NULL(tx_chn->desc_pool))
 			k3_cppi_desc_pool_destroy(tx_chn->desc_pool);
 
@@ -2240,26 +2232,6 @@ static void am65_cpsw_nuss_free_tx_chns(void *data)
 	}
 }
 
-static void am65_cpsw_nuss_remove_tx_chns(struct am65_cpsw_common *common)
-{
-	struct device *dev = common->dev;
-	int i;
-
-	devm_remove_action(dev, am65_cpsw_nuss_free_tx_chns, common);
-
-	common->tx_ch_rate_msk = 0;
-	for (i = 0; i < common->tx_ch_num; i++) {
-		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
-
-		if (tx_chn->irq)
-			devm_free_irq(dev, tx_chn->irq, tx_chn);
-
-		netif_napi_del(&tx_chn->napi_tx);
-	}
-
-	am65_cpsw_nuss_free_tx_chns(common);
-}
-
 static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
 {
 	struct device *dev = common->dev;
@@ -2360,36 +2332,14 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
 	}
 
 	ret = am65_cpsw_nuss_ndev_add_tx_napi(common);
-	if (ret) {
+	if (ret)
 		dev_err(dev, "Failed to add tx NAPI %d\n", ret);
-		goto err;
-	}
 
 err:
-	i = devm_add_action(dev, am65_cpsw_nuss_free_tx_chns, common);
-	if (i) {
-		dev_err(dev, "Failed to add free_tx_chns action %d\n", i);
-		return i;
-	}
-
 	return ret;
 }
 
-static void am65_cpsw_nuss_free_rx_chns(void *data)
-{
-	struct am65_cpsw_common *common = data;
-	struct am65_cpsw_rx_chn *rx_chn;
-
-	rx_chn = &common->rx_chns;
-
-	if (!IS_ERR_OR_NULL(rx_chn->desc_pool))
-		k3_cppi_desc_pool_destroy(rx_chn->desc_pool);
-
-	if (!IS_ERR_OR_NULL(rx_chn->rx_chn))
-		k3_udma_glue_release_rx_chn(rx_chn->rx_chn);
-}
-
-static void am65_cpsw_nuss_remove_rx_chns(struct am65_cpsw_common *common)
+static void am65_cpsw_nuss_cleanup_rx_chns(struct am65_cpsw_common *common)
 {
 	struct device *dev = common->dev;
 	struct am65_cpsw_rx_chn *rx_chn;
@@ -2398,7 +2348,6 @@ static void am65_cpsw_nuss_remove_rx_chns(struct am65_cpsw_common *common)
 
 	rx_chn = &common->rx_chns;
 	flows = rx_chn->flows;
-	devm_remove_action(dev, am65_cpsw_nuss_free_rx_chns, common);
 
 	for (i = 0; i < common->rx_ch_num_flows; i++) {
 		if (!(flows[i].irq < 0))
@@ -2406,7 +2355,11 @@ static void am65_cpsw_nuss_remove_rx_chns(struct am65_cpsw_common *common)
 		netif_napi_del(&flows[i].napi_rx);
 	}
 
-	am65_cpsw_nuss_free_rx_chns(common);
+	if (!IS_ERR_OR_NULL(rx_chn->desc_pool))
+		k3_cppi_desc_pool_destroy(rx_chn->desc_pool);
+
+	if (!IS_ERR_OR_NULL(rx_chn->rx_chn))
+		k3_udma_glue_release_rx_chn(rx_chn->rx_chn);
 
 	common->rx_flow_id_base = -1;
 }
@@ -2535,14 +2488,7 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
 
 	/* setup classifier to route priorities to flows */
 	cpsw_ale_classifier_setup_default(common->ale, common->rx_ch_num_flows);
-
 err:
-	i = devm_add_action(dev, am65_cpsw_nuss_free_rx_chns, common);
-	if (i) {
-		dev_err(dev, "Failed to add free_rx_chns action %d\n", i);
-		return i;
-	}
-
 	return ret;
 }
 
@@ -2626,6 +2572,22 @@ static int am65_cpsw_init_cpts(struct am65_cpsw_common *common)
 	return 0;
 }
 
+static void am65_cpsw_nuss_cleanup_slave_ports(struct am65_cpsw_common *common)
+{
+	struct am65_cpsw_port *port;
+	struct phy *phy;
+	int i;
+
+	for (i = 0; i < common->port_num; i++) {
+		port = &common->ports[i];
+		phy = port->slave.serdes_phy;
+		if (phy) {
+			am65_cpsw_disable_phy(phy);
+			port->slave.serdes_phy = NULL;
+		}
+	}
+}
+
 static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 {
 	struct device_node *node, *port_np;
@@ -2743,18 +2705,6 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 	return ret;
 }
 
-static void am65_cpsw_nuss_phylink_cleanup(struct am65_cpsw_common *common)
-{
-	struct am65_cpsw_port *port;
-	int i;
-
-	for (i = 0; i < common->port_num; i++) {
-		port = &common->ports[i];
-		if (port->slave.phylink)
-			phylink_destroy(port->slave.phylink);
-	}
-}
-
 static int
 am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 {
@@ -2863,34 +2813,42 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 	return 0;
 }
 
-static int am65_cpsw_nuss_init_ndevs(struct am65_cpsw_common *common)
+static void am65_cpsw_nuss_cleanup_ndevs(struct am65_cpsw_common *common)
 {
-	int ret;
+	struct am65_cpsw_port *port;
 	int i;
 
 	for (i = 0; i < common->port_num; i++) {
-		ret = am65_cpsw_nuss_init_port_ndev(common, i);
-		if (ret)
-			return ret;
+		port = &common->ports[i];
+		if (port->disabled)
+			continue;
+
+		if (port->slave.phylink) {
+			phylink_destroy(port->slave.phylink);
+			port->slave.phylink = NULL;
+		}
+
+		if (port->ndev) {
+			free_netdev(port->ndev);
+			port->ndev = NULL;
+		}
 	}
 
-	return ret;
+	common->dma_ndev = NULL;
 }
 
-static void am65_cpsw_nuss_cleanup_ndev(struct am65_cpsw_common *common)
+static int am65_cpsw_nuss_init_ndevs(struct am65_cpsw_common *common)
 {
-	struct am65_cpsw_port *port;
+	int ret;
 	int i;
 
 	for (i = 0; i < common->port_num; i++) {
-		port = &common->ports[i];
-		if (!port->ndev)
-			continue;
-		if (port->ndev->reg_state == NETREG_REGISTERED)
-			unregister_netdev(port->ndev);
-		free_netdev(port->ndev);
-		port->ndev = NULL;
+		ret = am65_cpsw_nuss_init_port_ndev(common, i);
+		if (ret)
+			return ret;
 	}
+
+	return ret;
 }
 
 static void am65_cpsw_port_offload_fwd_mark_update(struct am65_cpsw_common *common)
@@ -3338,21 +3296,29 @@ static void am65_cpsw_unregister_devlink(struct am65_cpsw_common *common)
 	devlink_free(common->devlink);
 }
 
-static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
+static void am65_cpsw_nuss_cleanup_chns(struct am65_cpsw_common *common)
+{
+	am65_cpsw_nuss_cleanup_rx_chns(common);
+	am65_cpsw_nuss_cleanup_tx_chns(common);
+}
+
+static int am65_cpsw_nuss_init_chns(struct am65_cpsw_common *common)
 {
 	struct am65_cpsw_rx_chn *rx_chan = &common->rx_chns;
 	struct am65_cpsw_tx_chn *tx_chan = common->tx_chns;
-	struct device *dev = common->dev;
-	struct am65_cpsw_port *port;
-	int ret = 0, i;
+	int ret, i;
 
 	/* init tx channels */
 	ret = am65_cpsw_nuss_init_tx_chns(common);
 	if (ret)
 		return ret;
+
+	/* init rx channels */
 	ret = am65_cpsw_nuss_init_rx_chns(common);
-	if (ret)
+	if (ret) {
+		am65_cpsw_nuss_cleanup_tx_chns(common);
 		return ret;
+	}
 
 	/* The DMA Channels are not guaranteed to be in a clean state.
 	 * Reset and disable them to ensure that they are back to the
@@ -3371,13 +3337,32 @@ static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
 
 	k3_udma_glue_disable_rx_chn(rx_chan->rx_chn);
 
-	ret = am65_cpsw_nuss_register_devlink(common);
-	if (ret)
-		return ret;
+	return 0;
+}
+
+static void am65_cpsw_nuss_unregister_ndev(struct am65_cpsw_common *common)
+{
+	struct am65_cpsw_port *port;
+	int i;
 
 	for (i = 0; i < common->port_num; i++) {
 		port = &common->ports[i];
+		if (!port->ndev)
+			continue;
+
+		if (port->ndev->reg_state == NETREG_REGISTERED)
+			unregister_netdev(port->ndev);
+	}
+}
+
+static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
+{
+	struct device *dev = common->dev;
+	struct am65_cpsw_port *port;
+	int ret = 0, i;
 
+	for (i = 0; i < common->port_num; i++) {
+		port = &common->ports[i];
 		if (!port->ndev)
 			continue;
 
@@ -3387,25 +3372,11 @@ static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
 		if (ret) {
 			dev_err(dev, "error registering slave net device%i %d\n",
 				i, ret);
-			goto err_cleanup_ndev;
+			return ret;
 		}
 	}
 
-	ret = am65_cpsw_register_notifiers(common);
-	if (ret)
-		goto err_cleanup_ndev;
-
-	/* can't auto unregister ndev using devm_add_action() due to
-	 * devres release sequence in DD core for DMA
-	 */
-
 	return 0;
-
-err_cleanup_ndev:
-	am65_cpsw_nuss_cleanup_ndev(common);
-	am65_cpsw_unregister_devlink(common);
-
-	return ret;
 }
 
 int am65_cpsw_nuss_update_tx_rx_chns(struct am65_cpsw_common *common,
@@ -3413,17 +3384,10 @@ int am65_cpsw_nuss_update_tx_rx_chns(struct am65_cpsw_common *common,
 {
 	int ret;
 
-	am65_cpsw_nuss_remove_tx_chns(common);
-	am65_cpsw_nuss_remove_rx_chns(common);
-
+	am65_cpsw_nuss_cleanup_chns(common);
 	common->tx_ch_num = num_tx;
 	common->rx_ch_num_flows = num_rx;
-	ret = am65_cpsw_nuss_init_tx_chns(common);
-	if (ret)
-		return ret;
-
-	ret = am65_cpsw_nuss_init_rx_chns(common);
-
+	ret = am65_cpsw_nuss_init_chns(common);
 	return ret;
 }
 
@@ -3599,7 +3563,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 
 	ret = am65_cpsw_nuss_init_slave_ports(common);
 	if (ret)
-		goto err_of_clear;
+		goto err_ports_clear;
 
 	/* init common data */
 	ale_params.dev = dev;
@@ -3613,7 +3577,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 	if (IS_ERR(common->ale)) {
 		dev_err(dev, "error initializing ale engine\n");
 		ret = PTR_ERR(common->ale);
-		goto err_of_clear;
+		goto err_ports_clear;
 	}
 
 	ale_entries = common->ale->params.ale_entries;
@@ -3622,7 +3586,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 					   GFP_KERNEL);
 	ret = am65_cpsw_init_cpts(common);
 	if (ret)
-		goto err_of_clear;
+		goto err_ports_clear;
 
 	/* init ports */
 	for (i = 0; i < common->port_num; i++)
@@ -3634,19 +3598,39 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 
 	ret = am65_cpsw_nuss_init_ndevs(common);
 	if (ret)
-		goto err_ndevs_clear;
+		goto err_cpts_release;
+
+	ret = am65_cpsw_nuss_init_chns(common);
+	if (ret)
+		goto err_cleanup_ndevs;
+
+	ret = am65_cpsw_nuss_register_devlink(common);
+	if (ret)
+		goto err_cleanup_chns;
 
 	ret = am65_cpsw_nuss_register_ndevs(common);
 	if (ret)
-		goto err_ndevs_clear;
+		goto err_unregister_devlink;
+
+	ret = am65_cpsw_register_notifiers(common);
+	if (ret)
+		goto err_unregister_ndev;
 
 	pm_runtime_put(dev);
 	return 0;
 
-err_ndevs_clear:
-	am65_cpsw_nuss_cleanup_ndev(common);
-	am65_cpsw_nuss_phylink_cleanup(common);
+err_unregister_ndev:
+	am65_cpsw_nuss_unregister_ndev(common);
+err_unregister_devlink:
+	am65_cpsw_unregister_devlink(common);
+err_cleanup_chns:
+	am65_cpsw_nuss_cleanup_chns(common);
+err_cleanup_ndevs:
+	am65_cpsw_nuss_cleanup_ndevs(common);
+err_cpts_release:
 	am65_cpts_release(common->cpts);
+err_ports_clear:
+	am65_cpsw_nuss_cleanup_slave_ports(common);
 err_of_clear:
 	if (common->mdio_dev)
 		of_platform_device_destroy(common->mdio_dev, NULL);
@@ -3675,15 +3659,12 @@ static void am65_cpsw_nuss_remove(struct platform_device *pdev)
 	}
 
 	am65_cpsw_unregister_notifiers(common);
-
-	/* must unregister ndevs here because DD release_driver routine calls
-	 * dma_deconfigure(dev) before devres_release_all(dev)
-	 */
-	am65_cpsw_nuss_cleanup_ndev(common);
+	am65_cpsw_nuss_unregister_ndev(common);
 	am65_cpsw_unregister_devlink(common);
-	am65_cpsw_nuss_phylink_cleanup(common);
+	am65_cpsw_nuss_cleanup_chns(common);
+	am65_cpsw_nuss_cleanup_ndevs(common);
 	am65_cpts_release(common->cpts);
-	am65_cpsw_disable_serdes_phy(common);
+	am65_cpsw_nuss_cleanup_slave_ports(common);
 
 	if (common->mdio_dev)
 		of_platform_device_destroy(common->mdio_dev, NULL);
@@ -3723,9 +3704,7 @@ static int am65_cpsw_nuss_suspend(struct device *dev)
 	}
 
 	am65_cpts_suspend(common->cpts);
-
-	am65_cpsw_nuss_remove_rx_chns(common);
-	am65_cpsw_nuss_remove_tx_chns(common);
+	am65_cpsw_nuss_cleanup_chns(common);
 
 	return 0;
 }
@@ -3738,10 +3717,7 @@ static int am65_cpsw_nuss_resume(struct device *dev)
 	struct net_device *ndev;
 	int i, ret;
 
-	ret = am65_cpsw_nuss_init_tx_chns(common);
-	if (ret)
-		return ret;
-	ret = am65_cpsw_nuss_init_rx_chns(common);
+	ret = am65_cpsw_nuss_init_chns(common);
 	if (ret)
 		return ret;
 

-- 
2.34.1

[PATCH net-next 3/4] net: ethernet: ti: am65-cpsw: streamline RX queue creation and cleanup

[Roger Quadros]

Introduce am65_cpsw_create_rxqs() and am65_cpsw_destroy_rxqs()
and use them.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 243 +++++++++++++++----------------
 1 file changed, 119 insertions(+), 124 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 783ec461dbdc..b5e679bb3f3c 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -497,35 +497,61 @@ static void am65_cpsw_init_host_port_switch(struct am65_cpsw_common *common);
 static void am65_cpsw_init_host_port_emac(struct am65_cpsw_common *common);
 static void am65_cpsw_init_port_switch_ale(struct am65_cpsw_port *port);
 static void am65_cpsw_init_port_emac_ale(struct am65_cpsw_port *port);
+static inline void am65_cpsw_put_page(struct am65_cpsw_rx_flow *flow,
+				      struct page *page,
+				      bool allow_direct);
+static void am65_cpsw_nuss_rx_cleanup(void *data, dma_addr_t desc_dma);
 
-static void am65_cpsw_destroy_xdp_rxqs(struct am65_cpsw_common *common)
+static void am65_cpsw_destroy_rxq(struct am65_cpsw_common *common, int id)
 {
 	struct am65_cpsw_rx_chn *rx_chn = &common->rx_chns;
 	struct am65_cpsw_rx_flow *flow;
 	struct xdp_rxq_info *rxq;
-	int id, port;
+	int port;
 
-	for (id = 0; id < common->rx_ch_num_flows; id++) {
-		flow = &rx_chn->flows[id];
+	flow = &rx_chn->flows[id];
+	napi_disable(&flow->napi_rx);
+	hrtimer_cancel(&flow->rx_hrtimer);
+	k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, id, rx_chn,
+				  am65_cpsw_nuss_rx_cleanup, !!id);
 
-		for (port = 0; port < common->port_num; port++) {
-			if (!common->ports[port].ndev)
-				continue;
+	for (port = 0; port < common->port_num; port++) {
+		if (!common->ports[port].ndev)
+			continue;
 
-			rxq = &common->ports[port].xdp_rxq[id];
+		rxq = &common->ports[port].xdp_rxq[id];
 
-			if (xdp_rxq_info_is_reg(rxq))
-				xdp_rxq_info_unreg(rxq);
-		}
+		if (xdp_rxq_info_is_reg(rxq))
+			xdp_rxq_info_unreg(rxq);
+	}
 
-		if (flow->page_pool) {
-			page_pool_destroy(flow->page_pool);
-			flow->page_pool = NULL;
-		}
+	if (flow->page_pool) {
+		page_pool_destroy(flow->page_pool);
+		flow->page_pool = NULL;
 	}
 }
 
-static int am65_cpsw_create_xdp_rxqs(struct am65_cpsw_common *common)
+static void am65_cpsw_destroy_rxqs(struct am65_cpsw_common *common)
+{
+	struct am65_cpsw_rx_chn *rx_chn = &common->rx_chns;
+	int id;
+
+	reinit_completion(&common->tdown_complete);
+	k3_udma_glue_tdown_rx_chn(rx_chn->rx_chn, true);
+
+	if (common->pdata.quirks & AM64_CPSW_QUIRK_DMA_RX_TDOWN_IRQ) {
+		id = wait_for_completion_timeout(&common->tdown_complete, msecs_to_jiffies(1000));
+		if (!id)
+			dev_err(common->dev, "rx teardown timeout\n");
+	}
+
+	for (id = common->rx_ch_num_flows - 1; id >= 0; id--)
+		am65_cpsw_destroy_rxq(common, id);
+
+	k3_udma_glue_disable_rx_chn(common->rx_chns.rx_chn);
+}
+
+static int am65_cpsw_create_rxq(struct am65_cpsw_common *common, int id)
 {
 	struct am65_cpsw_rx_chn *rx_chn = &common->rx_chns;
 	struct page_pool_params pp_params = {
@@ -540,45 +566,92 @@ static int am65_cpsw_create_xdp_rxqs(struct am65_cpsw_common *common)
 	struct am65_cpsw_rx_flow *flow;
 	struct xdp_rxq_info *rxq;
 	struct page_pool *pool;
-	int id, port, ret;
+	struct page *page;
+	int port, ret, i;
 
-	for (id = 0; id < common->rx_ch_num_flows; id++) {
-		flow = &rx_chn->flows[id];
-		pp_params.napi = &flow->napi_rx;
-		pool = page_pool_create(&pp_params);
-		if (IS_ERR(pool)) {
-			ret = PTR_ERR(pool);
+	flow = &rx_chn->flows[id];
+	pp_params.napi = &flow->napi_rx;
+	pool = page_pool_create(&pp_params);
+	if (IS_ERR(pool)) {
+		ret = PTR_ERR(pool);
+		return ret;
+	}
+
+	flow->page_pool = pool;
+
+	/* using same page pool is allowed as no running rx handlers
+	 * simultaneously for both ndevs
+	 */
+	for (port = 0; port < common->port_num; port++) {
+		if (!common->ports[port].ndev)
+		/* FIXME should we BUG here? */
+			continue;
+
+		rxq = &common->ports[port].xdp_rxq[id];
+		ret = xdp_rxq_info_reg(rxq, common->ports[port].ndev,
+				       id, flow->napi_rx.napi_id);
+		if (ret)
+			goto err;
+
+		ret = xdp_rxq_info_reg_mem_model(rxq,
+						 MEM_TYPE_PAGE_POOL,
+						 pool);
+		if (ret)
+			goto err;
+	}
+
+	for (i = 0; i < AM65_CPSW_MAX_RX_DESC; i++) {
+		page = page_pool_dev_alloc_pages(flow->page_pool);
+		if (!page) {
+			dev_err(common->dev, "cannot allocate page in flow %d\n",
+				id);
+			ret = -ENOMEM;
 			goto err;
 		}
 
-		flow->page_pool = pool;
+		ret = am65_cpsw_nuss_rx_push(common, page, id);
+		if (ret < 0) {
+			dev_err(common->dev,
+				"cannot submit page to rx channel flow %d, error %d\n",
+				id, ret);
+			am65_cpsw_put_page(flow, page, false);
+			goto err;
+		}
+	}
 
-		/* using same page pool is allowed as no running rx handlers
-		 * simultaneously for both ndevs
-		 */
-		for (port = 0; port < common->port_num; port++) {
-			if (!common->ports[port].ndev)
-				continue;
+	napi_enable(&flow->napi_rx);
+	return 0;
 
-			rxq = &common->ports[port].xdp_rxq[id];
+err:
+	am65_cpsw_destroy_rxq(common, id);
+	return ret;
+}
 
-			ret = xdp_rxq_info_reg(rxq, common->ports[port].ndev,
-					       id, flow->napi_rx.napi_id);
-			if (ret)
-				goto err;
+static int am65_cpsw_create_rxqs(struct am65_cpsw_common *common)
+{
+	int id, ret;
 
-			ret = xdp_rxq_info_reg_mem_model(rxq,
-							 MEM_TYPE_PAGE_POOL,
-							 pool);
-			if (ret)
-				goto err;
+	for (id = 0; id < common->rx_ch_num_flows; id++) {
+		ret = am65_cpsw_create_rxq(common, id);
+		if (ret) {
+			dev_err(common->dev, "couldn't create rxq %d: %d\n",
+				id, ret);
+			goto err;
 		}
 	}
 
+	ret = k3_udma_glue_enable_rx_chn(common->rx_chns.rx_chn);
+	if (ret) {
+		dev_err(common->dev, "couldn't enable rx chn: %d\n", ret);
+		goto err;
+	}
+
 	return 0;
 
 err:
-	am65_cpsw_destroy_xdp_rxqs(common);
+	for (--id; id >= 0; id--)
+		am65_cpsw_destroy_rxq(common, id);
+
 	return ret;
 }
 
@@ -642,7 +715,6 @@ static void am65_cpsw_nuss_rx_cleanup(void *data, dma_addr_t desc_dma)
 	k3_udma_glue_rx_cppi5_to_dma_addr(rx_chn->rx_chn, &buf_dma);
 	dma_unmap_single(rx_chn->dma_dev, buf_dma, buf_dma_len, DMA_FROM_DEVICE);
 	k3_cppi_desc_pool_free(rx_chn->desc_pool, desc_rx);
-
 	am65_cpsw_put_page(&rx_chn->flows[flow_id], page, false);
 }
 
@@ -717,12 +789,9 @@ static struct sk_buff *am65_cpsw_build_skb(void *page_addr,
 static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 {
 	struct am65_cpsw_host *host_p = am65_common_get_host(common);
-	struct am65_cpsw_rx_chn *rx_chn = &common->rx_chns;
 	struct am65_cpsw_tx_chn *tx_chn = common->tx_chns;
-	int port_idx, i, ret, tx, flow_idx;
-	struct am65_cpsw_rx_flow *flow;
+	int port_idx, ret, tx;
 	u32 val, port_mask;
-	struct page *page;
 
 	if (common->usage_count)
 		return 0;
@@ -782,47 +851,9 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 
 	am65_cpsw_qos_tx_p0_rate_init(common);
 
-	ret = am65_cpsw_create_xdp_rxqs(common);
-	if (ret) {
-		dev_err(common->dev, "Failed to create XDP rx queues\n");
+	ret = am65_cpsw_create_rxqs(common);
+	if (ret)
 		return ret;
-	}
-
-	for (flow_idx = 0; flow_idx < common->rx_ch_num_flows; flow_idx++) {
-		flow = &rx_chn->flows[flow_idx];
-		for (i = 0; i < AM65_CPSW_MAX_RX_DESC; i++) {
-			page = page_pool_dev_alloc_pages(flow->page_pool);
-			if (!page) {
-				dev_err(common->dev, "cannot allocate page in flow %d\n",
-					flow_idx);
-				ret = -ENOMEM;
-				goto fail_rx;
-			}
-
-			ret = am65_cpsw_nuss_rx_push(common, page, flow_idx);
-			if (ret < 0) {
-				dev_err(common->dev,
-					"cannot submit page to rx channel flow %d, error %d\n",
-					flow_idx, ret);
-				am65_cpsw_put_page(flow, page, false);
-				goto fail_rx;
-			}
-		}
-	}
-
-	ret = k3_udma_glue_enable_rx_chn(rx_chn->rx_chn);
-	if (ret) {
-		dev_err(common->dev, "couldn't enable rx chn: %d\n", ret);
-		goto fail_rx;
-	}
-
-	for (i = 0; i < common->rx_ch_num_flows ; i++) {
-		napi_enable(&rx_chn->flows[i].napi_rx);
-		if (rx_chn->flows[i].irq_disabled) {
-			rx_chn->flows[i].irq_disabled = false;
-			enable_irq(rx_chn->flows[i].irq);
-		}
-	}
 
 	for (tx = 0; tx < common->tx_ch_num; tx++) {
 		ret = k3_udma_glue_enable_tx_chn(tx_chn[tx].tx_chn);
@@ -845,30 +876,13 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 		tx--;
 	}
 
-	for (flow_idx = 0; i < common->rx_ch_num_flows; flow_idx++) {
-		flow = &rx_chn->flows[flow_idx];
-		if (!flow->irq_disabled) {
-			disable_irq(flow->irq);
-			flow->irq_disabled = true;
-		}
-		napi_disable(&flow->napi_rx);
-	}
-
-	k3_udma_glue_disable_rx_chn(rx_chn->rx_chn);
-
-fail_rx:
-	for (i = 0; i < common->rx_ch_num_flows; i++)
-		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
-					  am65_cpsw_nuss_rx_cleanup, !!i);
-
-	am65_cpsw_destroy_xdp_rxqs(common);
+	am65_cpsw_destroy_rxqs(common);
 
 	return ret;
 }
 
 static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
 {
-	struct am65_cpsw_rx_chn *rx_chn = &common->rx_chns;
 	struct am65_cpsw_tx_chn *tx_chn = common->tx_chns;
 	int i;
 
@@ -902,31 +916,12 @@ static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
 		k3_udma_glue_disable_tx_chn(tx_chn[i].tx_chn);
 	}
 
-	reinit_completion(&common->tdown_complete);
-	k3_udma_glue_tdown_rx_chn(rx_chn->rx_chn, true);
-
-	if (common->pdata.quirks & AM64_CPSW_QUIRK_DMA_RX_TDOWN_IRQ) {
-		i = wait_for_completion_timeout(&common->tdown_complete, msecs_to_jiffies(1000));
-		if (!i)
-			dev_err(common->dev, "rx teardown timeout\n");
-	}
-
-	for (i = common->rx_ch_num_flows - 1; i >= 0; i--) {
-		napi_disable(&rx_chn->flows[i].napi_rx);
-		hrtimer_cancel(&rx_chn->flows[i].rx_hrtimer);
-		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
-					  am65_cpsw_nuss_rx_cleanup, !!i);
-	}
-
-	k3_udma_glue_disable_rx_chn(rx_chn->rx_chn);
-
+	am65_cpsw_destroy_rxqs(common);
 	cpsw_ale_stop(common->ale);
 
 	writel(0, common->cpsw_base + AM65_CPSW_REG_CTL);
 	writel(0, common->cpsw_base + AM65_CPSW_REG_STAT_PORT_EN);
 
-	am65_cpsw_destroy_xdp_rxqs(common);
-
 	dev_dbg(common->dev, "cpsw_nuss stopped\n");
 	return 0;
 }

-- 
2.34.1

[PATCH net-next 4/4] net: ethernet: ti: am65-cpsw: streamline TX queue creation and cleanup

[Roger Quadros]

Introduce am65_cpsw_create_txqs() and am65_cpsw_destroy_txqs()
and use them.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 123 +++++++++++++++++++------------
 1 file changed, 77 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index b5e679bb3f3c..55c50fefe4b5 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -501,6 +501,7 @@ static inline void am65_cpsw_put_page(struct am65_cpsw_rx_flow *flow,
 				      struct page *page,
 				      bool allow_direct);
 static void am65_cpsw_nuss_rx_cleanup(void *data, dma_addr_t desc_dma);
+static void am65_cpsw_nuss_tx_cleanup(void *data, dma_addr_t desc_dma);
 
 static void am65_cpsw_destroy_rxq(struct am65_cpsw_common *common, int id)
 {
@@ -655,6 +656,76 @@ static int am65_cpsw_create_rxqs(struct am65_cpsw_common *common)
 	return ret;
 }
 
+static void am65_cpsw_destroy_txq(struct am65_cpsw_common *common, int id)
+{
+	struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[id];
+
+	napi_disable(&tx_chn->napi_tx);
+	hrtimer_cancel(&tx_chn->tx_hrtimer);
+	k3_udma_glue_reset_tx_chn(tx_chn->tx_chn, tx_chn,
+				  am65_cpsw_nuss_tx_cleanup);
+	k3_udma_glue_disable_tx_chn(tx_chn->tx_chn);
+}
+
+static void am65_cpsw_destroy_txqs(struct am65_cpsw_common *common)
+{
+	struct am65_cpsw_tx_chn *tx_chn = common->tx_chns;
+	int id;
+
+	/* shutdown tx channels */
+	atomic_set(&common->tdown_cnt, common->tx_ch_num);
+	/* ensure new tdown_cnt value is visible */
+	smp_mb__after_atomic();
+	reinit_completion(&common->tdown_complete);
+
+	for (id = 0; id < common->tx_ch_num; id++)
+		k3_udma_glue_tdown_tx_chn(tx_chn[id].tx_chn, false);
+
+	id = wait_for_completion_timeout(&common->tdown_complete,
+					 msecs_to_jiffies(1000));
+	if (!id)
+		dev_err(common->dev, "tx teardown timeout\n");
+
+	for (id = common->tx_ch_num - 1; id >= 0; id--)
+		am65_cpsw_destroy_txq(common, id);
+}
+
+static int am65_cpsw_create_txq(struct am65_cpsw_common *common, int id)
+{
+	struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[id];
+	int ret;
+
+	ret = k3_udma_glue_enable_tx_chn(tx_chn->tx_chn);
+	if (ret)
+		return ret;
+
+	napi_enable(&tx_chn->napi_tx);
+
+	return 0;
+}
+
+static int am65_cpsw_create_txqs(struct am65_cpsw_common *common)
+{
+	int id, ret;
+
+	for (id = 0; id < common->tx_ch_num; id++) {
+		ret = am65_cpsw_create_txq(common, id);
+		if (ret) {
+			dev_err(common->dev, "couldn't create txq %d: %d\n",
+				id, ret);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	for (--id; id >= 0; id--)
+		am65_cpsw_destroy_txq(common, id);
+
+	return ret;
+}
+
 static int am65_cpsw_nuss_desc_idx(struct k3_cppi_desc_pool *desc_pool,
 				   void *desc,
 				   unsigned char dsize_log2)
@@ -789,9 +860,8 @@ static struct sk_buff *am65_cpsw_build_skb(void *page_addr,
 static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 {
 	struct am65_cpsw_host *host_p = am65_common_get_host(common);
-	struct am65_cpsw_tx_chn *tx_chn = common->tx_chns;
-	int port_idx, ret, tx;
 	u32 val, port_mask;
+	int port_idx, ret;
 
 	if (common->usage_count)
 		return 0;
@@ -855,27 +925,14 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 	if (ret)
 		return ret;
 
-	for (tx = 0; tx < common->tx_ch_num; tx++) {
-		ret = k3_udma_glue_enable_tx_chn(tx_chn[tx].tx_chn);
-		if (ret) {
-			dev_err(common->dev, "couldn't enable tx chn %d: %d\n",
-				tx, ret);
-			tx--;
-			goto fail_tx;
-		}
-		napi_enable(&tx_chn[tx].napi_tx);
-	}
+	ret = am65_cpsw_create_txqs(common);
+	if (ret)
+		goto cleanup_rx;
 
 	dev_dbg(common->dev, "cpsw_nuss started\n");
 	return 0;
 
-fail_tx:
-	while (tx >= 0) {
-		napi_disable(&tx_chn[tx].napi_tx);
-		k3_udma_glue_disable_tx_chn(tx_chn[tx].tx_chn);
-		tx--;
-	}
-
+cleanup_rx:
 	am65_cpsw_destroy_rxqs(common);
 
 	return ret;
@@ -883,39 +940,13 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 
 static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
 {
-	struct am65_cpsw_tx_chn *tx_chn = common->tx_chns;
-	int i;
-
 	if (common->usage_count != 1)
 		return 0;
 
 	cpsw_ale_control_set(common->ale, HOST_PORT_NUM,
 			     ALE_PORT_STATE, ALE_PORT_STATE_DISABLE);
 
-	/* shutdown tx channels */
-	atomic_set(&common->tdown_cnt, common->tx_ch_num);
-	/* ensure new tdown_cnt value is visible */
-	smp_mb__after_atomic();
-	reinit_completion(&common->tdown_complete);
-
-	for (i = 0; i < common->tx_ch_num; i++)
-		k3_udma_glue_tdown_tx_chn(tx_chn[i].tx_chn, false);
-
-	i = wait_for_completion_timeout(&common->tdown_complete,
-					msecs_to_jiffies(1000));
-	if (!i)
-		dev_err(common->dev, "tx timeout\n");
-	for (i = 0; i < common->tx_ch_num; i++) {
-		napi_disable(&tx_chn[i].napi_tx);
-		hrtimer_cancel(&tx_chn[i].tx_hrtimer);
-	}
-
-	for (i = 0; i < common->tx_ch_num; i++) {
-		k3_udma_glue_reset_tx_chn(tx_chn[i].tx_chn, &tx_chn[i],
-					  am65_cpsw_nuss_tx_cleanup);
-		k3_udma_glue_disable_tx_chn(tx_chn[i].tx_chn);
-	}
-
+	am65_cpsw_destroy_txqs(common);
 	am65_cpsw_destroy_rxqs(common);
 	cpsw_ale_stop(common->ale);
 

-- 
2.34.1

Re: [PATCH net-next 1/4] net: ethernet: am65-cpsw: call netif_carrier_on/off() when appropriate

[Maxime Chevallier]

Hello Roger,

On Wed, 15 Jan 2025 18:43:00 +0200
Roger Quadros <rogerq@kernel.org> wrote:

> Call netif_carrier_on/off when link is up/down.
> When link is up only wake TX netif queue if network device is
> running.
> 
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index dcb6662b473d..36c29d3db329 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2155,6 +2155,7 @@ static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned
>  	cpsw_sl_ctl_clr(port->slave.mac_sl, mac_control);
>  
>  	am65_cpsw_qos_link_down(ndev);
> +	netif_carrier_off(ndev);

You shouldn't need to do that, phylink does that for you :
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/net/phy/phylink.c#L1434

Are you facing any specific problem that motivates that patch ?

>  	netif_tx_stop_all_queues(ndev);
>  }
>  
> @@ -2196,7 +2197,9 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>  	cpsw_ale_control_set(common->ale, port->port_id, ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);
>  
>  	am65_cpsw_qos_link_up(ndev, speed);
> -	netif_tx_wake_all_queues(ndev);
> +	netif_carrier_on(ndev);

Same here, phylink will set the carrier on by itself.

Thanks,

Maxime

Re: [PATCH net-next 1/4] net: ethernet: am65-cpsw: call netif_carrier_on/off() when appropriate

[Russell King (Oracle)]

On Wed, Jan 15, 2025 at 06:43:00PM +0200, Roger Quadros wrote:
> Call netif_carrier_on/off when link is up/down.
> When link is up only wake TX netif queue if network device is
> running.

Sorry, but no, this is wrong.

Documentation/networking/sfp-phylink.rst:

16. Verify that the driver does not call::

        netif_carrier_on()
        netif_carrier_off()

    as these will interfere with phylink's tracking of the link state,
    and cause phylink to omit calls via the :c:func:`mac_link_up` and
    :c:func:`mac_link_down` methods.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: streamline .probe() error handling

[Russell King (Oracle)]

On Wed, Jan 15, 2025 at 06:43:01PM +0200, Roger Quadros wrote:
> Keep things simple by explicitly cleaning up on .probe() error
> path or .remove(). Get rid of devm_add/remove_action() usage.
> 
> Rename am65_cpsw_disable_serdes_phy() to
> am65_cpsw_nuss_cleanup_slave_ports() and move it right before
> am65_cpsw_nuss_init_slave_ports().
> 
> Get rid of am65_cpsw_nuss_phylink_cleanup() and introduce
> am65_cpsw_nuss_cleanup_ndevs() right before am65_cpsw_nuss_init_ndevs()
> 
> Move channel initiailzation code out of am65_cpsw_nuss_register_ndevs()
> into new function am65_cpsw_nuss_init_chns().
> Add am65_cpsw_nuss_remove_chns() to do reverse of
> am65_cpsw_nuss_init_chns().
> 
> Add am65_cpsw_nuss_unregister_ndev() to do reverse of
> am65_cpsw_nuss_register_ndevs().
> 
> Use the introduced helpers in probe/remove.

Wow, so we're now saying that devm shouldn't be used? Given that patch 1
is wrong, I'm not sure I'd trust this patch to be correct either as it
goes against what I understand is preferred - to avoid explicit cleanups
that can get in the wrong order or be missed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] net: ethernet: am65-cpsw: call netif_carrier_on/off() when appropriate

[Roger Quadros]

On 15/01/2025 19:13, Maxime Chevallier wrote:
> Hello Roger,
> 
> On Wed, 15 Jan 2025 18:43:00 +0200
> Roger Quadros <rogerq@kernel.org> wrote:
> 
>> Call netif_carrier_on/off when link is up/down.
>> When link is up only wake TX netif queue if network device is
>> running.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index dcb6662b473d..36c29d3db329 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -2155,6 +2155,7 @@ static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned
>>  	cpsw_sl_ctl_clr(port->slave.mac_sl, mac_control);
>>  
>>  	am65_cpsw_qos_link_down(ndev);
>> +	netif_carrier_off(ndev);
> 
> You shouldn't need to do that, phylink does that for you :
> https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/net/phy/phylink.c#L1434
> 
> Are you facing any specific problem that motivates that patch ?

No. I overlooked it.

> 
>>  	netif_tx_stop_all_queues(ndev);
>>  }
>>  
>> @@ -2196,7 +2197,9 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>>  	cpsw_ale_control_set(common->ale, port->port_id, ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);
>>  
>>  	am65_cpsw_qos_link_up(ndev, speed);
>> -	netif_tx_wake_all_queues(ndev);
>> +	netif_carrier_on(ndev);
> 
> Same here, phylink will set the carrier on by itself.

Thanks for catching this. I'll drop this patch on next spin.

> 
> Thanks,
> 
> Maxime

-- 
cheers,
-roger

Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: streamline .probe() error handling

[Roger Quadros]

On 15/01/2025 22:30, Russell King (Oracle) wrote:
> On Wed, Jan 15, 2025 at 06:43:01PM +0200, Roger Quadros wrote:
>> Keep things simple by explicitly cleaning up on .probe() error
>> path or .remove(). Get rid of devm_add/remove_action() usage.
>>
>> Rename am65_cpsw_disable_serdes_phy() to
>> am65_cpsw_nuss_cleanup_slave_ports() and move it right before
>> am65_cpsw_nuss_init_slave_ports().
>>
>> Get rid of am65_cpsw_nuss_phylink_cleanup() and introduce
>> am65_cpsw_nuss_cleanup_ndevs() right before am65_cpsw_nuss_init_ndevs()
>>
>> Move channel initiailzation code out of am65_cpsw_nuss_register_ndevs()
>> into new function am65_cpsw_nuss_init_chns().
>> Add am65_cpsw_nuss_remove_chns() to do reverse of
>> am65_cpsw_nuss_init_chns().
>>
>> Add am65_cpsw_nuss_unregister_ndev() to do reverse of
>> am65_cpsw_nuss_register_ndevs().
>>
>> Use the introduced helpers in probe/remove.
> 
> Wow, so we're now saying that devm shouldn't be used? Given that patch 1

No. We're not getting rid of all devm_ users in the driver, just the case
for am65_cpsw_nuss_free_rx/tx_chns(). I'll explain below.

> is wrong, I'm not sure I'd trust this patch to be correct either as it
> goes against what I understand is preferred - to avoid explicit cleanups
> that can get in the wrong order or be missed.
> 

In this particular case, we had to repeatedly call devm_remove/add_action()
on am65_cpsw_nuss_free_rx/tx_chns() at multiple places as number of channel
can be changed at runtime.

I thought it would be easier to keep track of the cleanup than to keep track
of devm_remove/add_action().

Another motivation for doing so was this paragraph found in
Documentation/process/maintainer-netdev.rst:

|Netdev remains skeptical about promises of all "auto-cleanup" APIs,
|including even ``devm_`` helpers, historically. They are not the preferred
|style of implementation, merely an acceptable one.

-- 
cheers,
-roger

Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: streamline .probe() error handling

[Jakub Kicinski]

On Thu, 16 Jan 2025 14:07:40 +0200 Roger Quadros wrote:
> Another motivation for doing so was this paragraph found in
> Documentation/process/maintainer-netdev.rst:
> 
> |Netdev remains skeptical about promises of all "auto-cleanup" APIs,
> |including even ``devm_`` helpers, historically. They are not the preferred
> |style of implementation, merely an acceptable one.

FWIW, the devm_ part of this is mostly to stop people from posting
"devm_ conversion" patches for some ancient drivers. Most devm_
uses are perfectly fine.

But as you pointed out, if you have to free the resources manually 
as well the devm_ scheme becomes a distraction.