[PATCH v2 0/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[PATCH v2 0/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[Roger Quadros]

Hi,

This contains a critical bug fix for the recently merged suspend/resume
support that broke set channel operation. (ethtool -L eth0 tx <n>)

Remaining patches are optimizations.

cheers,
-roger

Changelog:
v2:
-Fix build warning
 drivers/net/ethernet/ti/am65-cpsw-nuss.c:562:13: warning: variable 'tmo' set but not used [-Wunused-but-set-variable]

Roger Quadros (4):
  net: ethernet: ti: am65-cpsw: Fix set channel operation
  net: ethernet: ti: am65-cpsw-nuss: Remove redundant ALE_CLEAR
  net: ethernet: ti: am65-cpsw: Restore ALE only if any interface was up
  net: ethernet: ti: cpsw_ale: optimize cpsw_ale_restore()

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 167 +++++++++++++----------
 drivers/net/ethernet/ti/cpsw_ale.c       |   7 +-
 2 files changed, 99 insertions(+), 75 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[Roger Quadros]

The set channel operation "ethtool -L tx <n>" broke with
the recent suspend/resume changes.

Revert back to original driver behaviour of not freeing
the TX/RX IRQs at am65_cpsw_nuss_common_stop(). We will
now free them only on .suspend() as we need to release
the DMA channels (as DMA looses context) and re-acquiring
them on .resume() may not necessarily give us the same
IRQs.

Introduce am65_cpsw_nuss_remove_rx_chns() which is similar
to am65_cpsw_nuss_remove_tx_chns() and invoke them both in
.suspend().

At .resume() call am65_cpsw_nuss_init_rx/tx_chns() to
acquire the DMA channels.

To as IRQs need to be requested after knowing the IRQ
numbers, move am65_cpsw_nuss_ndev_add_tx_napi() call to
am65_cpsw_nuss_init_tx_chns().

Also fixes the below warning during suspend/resume on multi
CPU system.

[   67.347684] ------------[ cut here ]------------
[   67.347700] Unbalanced enable for IRQ 119
[   67.347726] WARNING: CPU: 0 PID: 1080 at kernel/irq/manage.c:781 __enable_irq+0x4c/0x80
[   67.347754] Modules linked in: wlcore_sdio wl18xx wlcore mac80211 libarc4 cfg80211 rfkill crct10dif_ce sch_fq_codel ipv6
[   67.347803] CPU: 0 PID: 1080 Comm: rtcwake Not tainted 6.1.0-rc4-00023-gc826e5480732-dirty #203
[   67.347812] Hardware name: Texas Instruments AM625 (DT)
[   67.347818] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   67.347829] pc : __enable_irq+0x4c/0x80
[   67.347838] lr : __enable_irq+0x4c/0x80
[   67.347846] sp : ffff80000999ba00
[   67.347850] x29: ffff80000999ba00 x28: ffff0000011c1c80 x27: 0000000000000000
[   67.347863] x26: 00000000000001f4 x25: ffff000001058358 x24: ffff000001059080
[   67.347876] x23: ffff000001058080 x22: ffff000001060000 x21: 0000000000000077
[   67.347888] x20: ffff0000011c1c80 x19: ffff000001429600 x18: 0000000000000001
[   67.347900] x17: 0000000000000080 x16: fffffc000176e008 x15: ffff0000011c21b0
[   67.347913] x14: 0000000000000000 x13: 3931312051524920 x12: 726f6620656c6261
[   67.347925] x11: 656820747563205b x10: 000000000000000a x9 : ffff80000999ba00
[   67.347938] x8 : ffff800009121068 x7 : ffff80000999b810 x6 : 00000000fffff17f
[   67.347950] x5 : ffff00007fb99b18 x4 : 0000000000000000 x3 : 0000000000000027
[   67.347962] x2 : ffff00007fb99b20 x1 : 50dd48f7f19deb00 x0 : 0000000000000000
[   67.347975] Call trace:
[   67.347980]  __enable_irq+0x4c/0x80
[   67.347989]  enable_irq+0x4c/0xa0
[   67.347999]  am65_cpsw_nuss_ndo_slave_open+0x4b0/0x568
[   67.348015]  am65_cpsw_nuss_resume+0x68/0x160
[   67.348025]  dpm_run_callback.isra.0+0x28/0x88
[   67.348040]  device_resume+0x78/0x160
[   67.348050]  dpm_resume+0xc0/0x1f8
[   67.348057]  dpm_resume_end+0x18/0x30
[   67.348063]  suspend_devices_and_enter+0x1cc/0x4e0
[   67.348075]  pm_suspend+0x1f8/0x268
[   67.348084]  state_store+0x8c/0x118
[   67.348092]  kobj_attr_store+0x18/0x30
[   67.348104]  sysfs_kf_write+0x44/0x58
[   67.348117]  kernfs_fop_write_iter+0x118/0x1a8
[   67.348127]  vfs_write+0x31c/0x418
[   67.348140]  ksys_write+0x6c/0xf8
[   67.348150]  __arm64_sys_write+0x1c/0x28
[   67.348160]  invoke_syscall+0x44/0x108
[   67.348172]  el0_svc_common.constprop.0+0x44/0xf0
[   67.348182]  do_el0_svc+0x2c/0xc8
[   67.348191]  el0_svc+0x2c/0x88
[   67.348201]  el0t_64_sync_handler+0xb8/0xc0
[   67.348209]  el0t_64_sync+0x18c/0x190
[   67.348218] ---[ end trace 0000000000000000 ]---

Fixes: fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 161 +++++++++++++----------
 1 file changed, 90 insertions(+), 71 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index f2e377524088..505c9edf98ff 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -133,10 +133,7 @@
 			 NETIF_MSG_IFUP	| NETIF_MSG_PROBE | NETIF_MSG_IFDOWN | \
 			 NETIF_MSG_RX_ERR | NETIF_MSG_TX_ERR)
 
-static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common);
-static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common);
-static void am65_cpsw_nuss_free_tx_chns(struct am65_cpsw_common *common);
-static void am65_cpsw_nuss_free_rx_chns(struct am65_cpsw_common *common);
+static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common);
 
 static void am65_cpsw_port_set_sl_mac(struct am65_cpsw_port *slave,
 				      const u8 *dev_addr)
@@ -379,20 +376,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 	if (common->usage_count)
 		return 0;
 
-	/* init tx/rx channels */
-	ret = am65_cpsw_nuss_init_tx_chns(common);
-	if (ret) {
-		dev_err(common->dev, "init_tx_chns failed\n");
-		return ret;
-	}
-
-	ret = am65_cpsw_nuss_init_rx_chns(common);
-	if (ret) {
-		dev_err(common->dev, "init_rx_chns failed\n");
-		am65_cpsw_nuss_free_tx_chns(common);
-		return ret;
-	}
-
 	/* Control register */
 	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
 	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
@@ -453,8 +436,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 						  GFP_KERNEL);
 		if (!skb) {
 			dev_err(common->dev, "cannot allocate skb\n");
-			ret = -ENOMEM;
-			goto err;
+			return -ENOMEM;
 		}
 
 		ret = am65_cpsw_nuss_rx_push(common, skb);
@@ -463,7 +445,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 				"cannot submit skb to channel rx, error %d\n",
 				ret);
 			kfree_skb(skb);
-			goto err;
+			return ret;
 		}
 		kmemleak_not_leak(skb);
 	}
@@ -472,7 +454,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 	for (i = 0; i < common->tx_ch_num; i++) {
 		ret = k3_udma_glue_enable_tx_chn(common->tx_chns[i].tx_chn);
 		if (ret)
-			goto err;
+			return ret;
 		napi_enable(&common->tx_chns[i].napi_tx);
 	}
 
@@ -484,12 +466,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 
 	dev_dbg(common->dev, "cpsw_nuss started\n");
 	return 0;
-
-err:
-	am65_cpsw_nuss_free_tx_chns(common);
-	am65_cpsw_nuss_free_rx_chns(common);
-
-	return ret;
 }
 
 static void am65_cpsw_nuss_tx_cleanup(void *data, dma_addr_t desc_dma);
@@ -543,9 +519,6 @@ static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
 	writel(0, common->cpsw_base + AM65_CPSW_REG_CTL);
 	writel(0, common->cpsw_base + AM65_CPSW_REG_STAT_PORT_EN);
 
-	am65_cpsw_nuss_free_tx_chns(common);
-	am65_cpsw_nuss_free_rx_chns(common);
-
 	dev_dbg(common->dev, "cpsw_nuss stopped\n");
 	return 0;
 }
@@ -597,8 +570,8 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
 	cpsw_sl_ctl_set(port->slave.mac_sl, CPSW_SL_CTL_CMD_IDLE);
 
 	tmo = cpsw_sl_wait_for_idle(port->slave.mac_sl, 100);
-	dev_info(common->dev, "down msc_sl %08x tmo %d\n",
-		 cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_MACSTATUS), tmo);
+	dev_dbg(common->dev, "down msc_sl %08x tmo %d\n",
+		cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_MACSTATUS), tmo);
 
 	cpsw_sl_ctl_reset(port->slave.mac_sl);
 
@@ -1548,9 +1521,9 @@ 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(struct am65_cpsw_common *common)
+static void am65_cpsw_nuss_free_tx_chns(void *data)
 {
-	struct device *dev = common->dev;
+	struct am65_cpsw_common *common = data;
 	int i;
 
 	for (i = 0; i < common->tx_ch_num; i++) {
@@ -1562,11 +1535,7 @@ static void am65_cpsw_nuss_free_tx_chns(struct am65_cpsw_common *common)
 		if (!IS_ERR_OR_NULL(tx_chn->tx_chn))
 			k3_udma_glue_release_tx_chn(tx_chn->tx_chn);
 
-		/* Don't clear tx_chn memory as we need to preserve
-		 * data between suspend/resume
-		 */
-		if (!(tx_chn->irq < 0))
-			devm_free_irq(dev, tx_chn->irq, tx_chn);
+		memset(tx_chn, 0, sizeof(*tx_chn));
 	}
 }
 
@@ -1575,10 +1544,12 @@ 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);
+
 	for (i = 0; i < common->tx_ch_num; i++) {
 		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
 
-		if (!(tx_chn->irq < 0))
+		if (tx_chn->irq)
 			devm_free_irq(dev, tx_chn->irq, tx_chn);
 
 		netif_napi_del(&tx_chn->napi_tx);
@@ -1648,7 +1619,7 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
 		}
 
 		tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn);
-		if (tx_chn->irq < 0) {
+		if (tx_chn->irq <= 0) {
 			dev_err(dev, "Failed to get tx dma irq %d\n",
 				tx_chn->irq);
 			goto err;
@@ -1657,41 +1628,59 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
 		snprintf(tx_chn->tx_chn_name,
 			 sizeof(tx_chn->tx_chn_name), "%s-tx%d",
 			 dev_name(dev), tx_chn->id);
-
-		ret = devm_request_irq(dev, tx_chn->irq,
-				       am65_cpsw_nuss_tx_irq,
-				       IRQF_TRIGGER_HIGH,
-				       tx_chn->tx_chn_name, tx_chn);
-		if (ret) {
-			dev_err(dev, "failure requesting tx%u irq %u, %d\n",
-				tx_chn->id, tx_chn->irq, ret);
-			tx_chn->irq = -EINVAL;
-			goto err;
-		}
 	}
 
-	return 0;
+	ret = am65_cpsw_nuss_ndev_add_tx_napi(common);
+	if (ret) {
+		dev_err(dev, "Failed to add tx NAPI %d\n", ret);
+		goto err;
+	}
 
 err:
-	am65_cpsw_nuss_free_tx_chns(common);
+	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(struct am65_cpsw_common *common)
+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(void *data)
 {
+	struct am65_cpsw_common *common = data;
 	struct am65_cpsw_rx_chn *rx_chn;
+	struct device *dev = common->dev;
 
 	rx_chn = &common->rx_chns;
+	devm_remove_action(dev, am65_cpsw_nuss_free_rx_chns, common);
 
 	if (!(rx_chn->irq < 0))
-		devm_free_irq(common->dev, rx_chn->irq, common);
+		devm_free_irq(dev, rx_chn->irq, common);
+
+	netif_napi_del(&common->napi_rx);
 
 	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;
 }
 
 static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
@@ -1709,7 +1698,7 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
 
 	rx_cfg.swdata_size = AM65_CPSW_NAV_SW_DATA_SIZE;
 	rx_cfg.flow_id_num = AM65_CPSW_MAX_RX_FLOWS;
-	rx_cfg.flow_id_base = -1;
+	rx_cfg.flow_id_base = common->rx_flow_id_base;
 
 	/* init all flows */
 	rx_chn->dev = dev;
@@ -1781,20 +1770,24 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
 		}
 	}
 
+	netif_napi_add(common->dma_ndev, &common->napi_rx,
+		       am65_cpsw_nuss_rx_poll);
+
 	ret = devm_request_irq(dev, rx_chn->irq,
 			       am65_cpsw_nuss_rx_irq,
 			       IRQF_TRIGGER_HIGH, dev_name(dev), common);
 	if (ret) {
 		dev_err(dev, "failure requesting rx irq %u, %d\n",
 			rx_chn->irq, ret);
-		rx_chn->irq = -EINVAL;
 		goto err;
 	}
 
-	return 0;
-
 err:
-	am65_cpsw_nuss_free_rx_chns(common);
+	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;
 }
@@ -2114,24 +2107,33 @@ static int am65_cpsw_nuss_init_ndevs(struct am65_cpsw_common *common)
 			return ret;
 	}
 
-	netif_napi_add(common->dma_ndev, &common->napi_rx,
-		       am65_cpsw_nuss_rx_poll);
-
 	return ret;
 }
 
 static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
 {
-	int i;
+	struct device *dev = common->dev;
+	int i, ret = 0;
 
 	for (i = 0; i < common->tx_ch_num; i++) {
 		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
 
 		netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
 				  am65_cpsw_nuss_tx_poll);
+
+		ret = devm_request_irq(dev, tx_chn->irq,
+				       am65_cpsw_nuss_tx_irq,
+				       IRQF_TRIGGER_HIGH,
+				       tx_chn->tx_chn_name, tx_chn);
+		if (ret) {
+			dev_err(dev, "failure requesting tx%u irq %u, %d\n",
+				tx_chn->id, tx_chn->irq, ret);
+			goto err;
+		}
 	}
 
-	return 0;
+err:
+	return ret;
 }
 
 static void am65_cpsw_nuss_cleanup_ndev(struct am65_cpsw_common *common)
@@ -2597,7 +2599,11 @@ static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
 	struct am65_cpsw_port *port;
 	int ret = 0, i;
 
-	ret = am65_cpsw_nuss_ndev_add_tx_napi(common);
+	/* init tx channels */
+	ret = am65_cpsw_nuss_init_tx_chns(common);
+	if (ret)
+		return ret;
+	ret = am65_cpsw_nuss_init_rx_chns(common);
 	if (ret)
 		return ret;
 
@@ -2645,10 +2651,8 @@ int am65_cpsw_nuss_update_tx_chns(struct am65_cpsw_common *common, int num_tx)
 
 	common->tx_ch_num = num_tx;
 	ret = am65_cpsw_nuss_init_tx_chns(common);
-	if (ret)
-		return ret;
 
-	return am65_cpsw_nuss_ndev_add_tx_napi(common);
+	return ret;
 }
 
 struct am65_cpsw_soc_pdata {
@@ -2756,6 +2760,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 	if (common->port_num < 1 || common->port_num > AM65_CPSW_MAX_PORTS)
 		return -ENOENT;
 
+	common->rx_flow_id_base = -1;
 	init_completion(&common->tdown_complete);
 	common->tx_ch_num = 1;
 	common->pf_p0_rx_ptype_rrobin = false;
@@ -2918,6 +2923,9 @@ 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);
+
 	return 0;
 }
 
@@ -2929,6 +2937,17 @@ static int am65_cpsw_nuss_resume(struct device *dev)
 	int i, ret;
 	struct am65_cpsw_host *host_p = am65_common_get_host(common);
 
+	ret = am65_cpsw_nuss_init_tx_chns(common);
+	if (ret)
+		return ret;
+	ret = am65_cpsw_nuss_init_rx_chns(common);
+	if (ret)
+		return ret;
+
+	/* If RX IRQ was disabled before suspend, keep it disabled */
+	if (common->rx_irq_disabled)
+		disable_irq(common->rx_chns.irq);
+
 	am65_cpts_resume(common->cpts);
 
 	for (i = 0; i < common->port_num; i++) {
-- 
2.17.1

[PATCH v2 2/4] net: ethernet: ti: am65-cpsw-nuss: Remove redundant ALE_CLEAR

[Roger Quadros]

ALE_CLEAR command is issued in cpsw_ale_start() so no need
to issue it before the call to cpsw_ale_start().

Fixes: fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 505c9edf98ff..2acde5b14516 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -404,7 +404,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 	/* disable priority elevation */
 	writel(0, common->cpsw_base + AM65_CPSW_REG_PTYPE);
 
-	cpsw_ale_control_set(common->ale, 0, ALE_CLEAR, 1);
 	cpsw_ale_start(common->ale);
 
 	/* limit to one RX flow only */
-- 
2.17.1

[PATCH v2 3/4] net: ethernet: ti: am65-cpsw: Restore ALE only if any interface was up

[Roger Quadros]

There is no point in restoring ALE if all interfaces were down
prior to suspend as ALE will be cleared when any interface is
brought up.

So restore ALE only if any interface was up before system suspended.

Fixes: 1af3cb3702d0 ("net: ethernet: ti: am65-cpsw: Fix hardware switch mode on suspend/resume")
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 2acde5b14516..dda9afe5410c 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2935,6 +2935,7 @@ static int am65_cpsw_nuss_resume(struct device *dev)
 	struct net_device *ndev;
 	int i, ret;
 	struct am65_cpsw_host *host_p = am65_common_get_host(common);
+	bool need_ale_restore = false;
 
 	ret = am65_cpsw_nuss_init_tx_chns(common);
 	if (ret)
@@ -2957,6 +2958,7 @@ static int am65_cpsw_nuss_resume(struct device *dev)
 			continue;
 
 		if (netif_running(ndev)) {
+			need_ale_restore = true;
 			rtnl_lock();
 			ret = am65_cpsw_nuss_ndo_slave_open(ndev);
 			rtnl_unlock();
@@ -2971,7 +2973,8 @@ static int am65_cpsw_nuss_resume(struct device *dev)
 	}
 
 	writel(host_p->vid_context, host_p->port_base + AM65_CPSW_PORT_VLAN_REG_OFFSET);
-	cpsw_ale_restore(common->ale, common->ale_context);
+	if (need_ale_restore)
+		cpsw_ale_restore(common->ale, common->ale_context);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 4/4] net: ethernet: ti: cpsw_ale: optimize cpsw_ale_restore()

[Roger Quadros]

If an entry was FREE then we don't have to restore it.
This will make the restore faster in most cases.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/cpsw_ale.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
index 0c5e783e574c..41bcf34a22f8 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -1452,12 +1452,15 @@ void cpsw_ale_dump(struct cpsw_ale *ale, u32 *data)
 	}
 }
 
+/* ALE table should be cleared (ALE_CLEAR) before cpsw_ale_restore() */
 void cpsw_ale_restore(struct cpsw_ale *ale, u32 *data)
 {
-	int i;
+	int i, type;
 
 	for (i = 0; i < ale->params.ale_entries; i++) {
-		cpsw_ale_write(ale, i, data);
+		type = cpsw_ale_get_entry_type(data);
+		if (type != ALE_TYPE_FREE)
+			cpsw_ale_write(ale, i, data);
 		data += ALE_ENTRY_WORDS;
 	}
 }
-- 
2.17.1

Re: [PATCH v2 0/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[Maciej Fijalkowski]

On Mon, Nov 21, 2022 at 03:22:56PM +0100, Roger Quadros wrote:
> Hi,

Hi,

> 
> This contains a critical bug fix for the recently merged suspend/resume
> support that broke set channel operation. (ethtool -L eth0 tx <n>)
> 
> Remaining patches are optimizations.

So I think you should resend and specify which trees should these patches
be routed to. I'd say first one goes to net with fixes tag and rest should
go to net-next.

Let me take a look at the contents of this series.

> 
> cheers,
> -roger
> 
> Changelog:
> v2:
> -Fix build warning
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c:562:13: warning: variable 'tmo' set but not used [-Wunused-but-set-variable]
> 
> Roger Quadros (4):
>   net: ethernet: ti: am65-cpsw: Fix set channel operation
>   net: ethernet: ti: am65-cpsw-nuss: Remove redundant ALE_CLEAR
>   net: ethernet: ti: am65-cpsw: Restore ALE only if any interface was up
>   net: ethernet: ti: cpsw_ale: optimize cpsw_ale_restore()
> 
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 167 +++++++++++++----------
>  drivers/net/ethernet/ti/cpsw_ale.c       |   7 +-
>  2 files changed, 99 insertions(+), 75 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[Maciej Fijalkowski]

On Mon, Nov 21, 2022 at 03:22:57PM +0100, Roger Quadros wrote:
> The set channel operation "ethtool -L tx <n>" broke with
> the recent suspend/resume changes.

Would be worth just dropping here the SHA-1 of offending commit, I deduce
that it is the one that fixes tag points to.

> 
> Revert back to original driver behaviour of not freeing
> the TX/RX IRQs at am65_cpsw_nuss_common_stop(). We will
> now free them only on .suspend() as we need to release
> the DMA channels (as DMA looses context) and re-acquiring
> them on .resume() may not necessarily give us the same
> IRQs.
> 
> Introduce am65_cpsw_nuss_remove_rx_chns() which is similar
> to am65_cpsw_nuss_remove_tx_chns() and invoke them both in
> .suspend().
> 
> At .resume() call am65_cpsw_nuss_init_rx/tx_chns() to
> acquire the DMA channels.
> 
> To as IRQs need to be requested after knowing the IRQ
> numbers, move am65_cpsw_nuss_ndev_add_tx_napi() call to
> am65_cpsw_nuss_init_tx_chns().
> 
> Also fixes the below warning during suspend/resume on multi

s/fixes/fix ?

> CPU system.
> 
> [   67.347684] ------------[ cut here ]------------
> [   67.347700] Unbalanced enable for IRQ 119
> [   67.347726] WARNING: CPU: 0 PID: 1080 at kernel/irq/manage.c:781 __enable_irq+0x4c/0x80
> [   67.347754] Modules linked in: wlcore_sdio wl18xx wlcore mac80211 libarc4 cfg80211 rfkill crct10dif_ce sch_fq_codel ipv6
> [   67.347803] CPU: 0 PID: 1080 Comm: rtcwake Not tainted 6.1.0-rc4-00023-gc826e5480732-dirty #203
> [   67.347812] Hardware name: Texas Instruments AM625 (DT)
> [   67.347818] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   67.347829] pc : __enable_irq+0x4c/0x80
> [   67.347838] lr : __enable_irq+0x4c/0x80
> [   67.347846] sp : ffff80000999ba00
> [   67.347850] x29: ffff80000999ba00 x28: ffff0000011c1c80 x27: 0000000000000000
> [   67.347863] x26: 00000000000001f4 x25: ffff000001058358 x24: ffff000001059080
> [   67.347876] x23: ffff000001058080 x22: ffff000001060000 x21: 0000000000000077
> [   67.347888] x20: ffff0000011c1c80 x19: ffff000001429600 x18: 0000000000000001
> [   67.347900] x17: 0000000000000080 x16: fffffc000176e008 x15: ffff0000011c21b0
> [   67.347913] x14: 0000000000000000 x13: 3931312051524920 x12: 726f6620656c6261
> [   67.347925] x11: 656820747563205b x10: 000000000000000a x9 : ffff80000999ba00
> [   67.347938] x8 : ffff800009121068 x7 : ffff80000999b810 x6 : 00000000fffff17f
> [   67.347950] x5 : ffff00007fb99b18 x4 : 0000000000000000 x3 : 0000000000000027
> [   67.347962] x2 : ffff00007fb99b20 x1 : 50dd48f7f19deb00 x0 : 0000000000000000
> [   67.347975] Call trace:
> [   67.347980]  __enable_irq+0x4c/0x80
> [   67.347989]  enable_irq+0x4c/0xa0
> [   67.347999]  am65_cpsw_nuss_ndo_slave_open+0x4b0/0x568
> [   67.348015]  am65_cpsw_nuss_resume+0x68/0x160
> [   67.348025]  dpm_run_callback.isra.0+0x28/0x88
> [   67.348040]  device_resume+0x78/0x160
> [   67.348050]  dpm_resume+0xc0/0x1f8
> [   67.348057]  dpm_resume_end+0x18/0x30
> [   67.348063]  suspend_devices_and_enter+0x1cc/0x4e0
> [   67.348075]  pm_suspend+0x1f8/0x268
> [   67.348084]  state_store+0x8c/0x118
> [   67.348092]  kobj_attr_store+0x18/0x30
> [   67.348104]  sysfs_kf_write+0x44/0x58
> [   67.348117]  kernfs_fop_write_iter+0x118/0x1a8
> [   67.348127]  vfs_write+0x31c/0x418
> [   67.348140]  ksys_write+0x6c/0xf8
> [   67.348150]  __arm64_sys_write+0x1c/0x28
> [   67.348160]  invoke_syscall+0x44/0x108
> [   67.348172]  el0_svc_common.constprop.0+0x44/0xf0
> [   67.348182]  do_el0_svc+0x2c/0xc8
> [   67.348191]  el0_svc+0x2c/0x88
> [   67.348201]  el0t_64_sync_handler+0xb8/0xc0
> [   67.348209]  el0t_64_sync+0x18c/0x190
> [   67.348218] ---[ end trace 0000000000000000 ]---
> 
> Fixes: fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 161 +++++++++++++----------
>  1 file changed, 90 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index f2e377524088..505c9edf98ff 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -133,10 +133,7 @@
>  			 NETIF_MSG_IFUP	| NETIF_MSG_PROBE | NETIF_MSG_IFDOWN | \
>  			 NETIF_MSG_RX_ERR | NETIF_MSG_TX_ERR)
>  
> -static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common);
> -static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common);
> -static void am65_cpsw_nuss_free_tx_chns(struct am65_cpsw_common *common);
> -static void am65_cpsw_nuss_free_rx_chns(struct am65_cpsw_common *common);
> +static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common);
>  
>  static void am65_cpsw_port_set_sl_mac(struct am65_cpsw_port *slave,
>  				      const u8 *dev_addr)
> @@ -379,20 +376,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>  	if (common->usage_count)
>  		return 0;
>  
> -	/* init tx/rx channels */
> -	ret = am65_cpsw_nuss_init_tx_chns(common);
> -	if (ret) {
> -		dev_err(common->dev, "init_tx_chns failed\n");
> -		return ret;
> -	}
> -
> -	ret = am65_cpsw_nuss_init_rx_chns(common);
> -	if (ret) {
> -		dev_err(common->dev, "init_rx_chns failed\n");
> -		am65_cpsw_nuss_free_tx_chns(common);
> -		return ret;
> -	}
> -
>  	/* Control register */
>  	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
>  	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
> @@ -453,8 +436,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>  						  GFP_KERNEL);
>  		if (!skb) {
>  			dev_err(common->dev, "cannot allocate skb\n");
> -			ret = -ENOMEM;
> -			goto err;
> +			return -ENOMEM;
>  		}
>  
>  		ret = am65_cpsw_nuss_rx_push(common, skb);
> @@ -463,7 +445,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>  				"cannot submit skb to channel rx, error %d\n",
>  				ret);
>  			kfree_skb(skb);
> -			goto err;
> +			return ret;
>  		}
>  		kmemleak_not_leak(skb);
>  	}
> @@ -472,7 +454,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>  	for (i = 0; i < common->tx_ch_num; i++) {
>  		ret = k3_udma_glue_enable_tx_chn(common->tx_chns[i].tx_chn);
>  		if (ret)
> -			goto err;
> +			return ret;
>  		napi_enable(&common->tx_chns[i].napi_tx);
>  	}
>  
> @@ -484,12 +466,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>  
>  	dev_dbg(common->dev, "cpsw_nuss started\n");
>  	return 0;
> -
> -err:
> -	am65_cpsw_nuss_free_tx_chns(common);
> -	am65_cpsw_nuss_free_rx_chns(common);
> -
> -	return ret;
>  }
>  
>  static void am65_cpsw_nuss_tx_cleanup(void *data, dma_addr_t desc_dma);
> @@ -543,9 +519,6 @@ static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
>  	writel(0, common->cpsw_base + AM65_CPSW_REG_CTL);
>  	writel(0, common->cpsw_base + AM65_CPSW_REG_STAT_PORT_EN);
>  
> -	am65_cpsw_nuss_free_tx_chns(common);
> -	am65_cpsw_nuss_free_rx_chns(common);
> -
>  	dev_dbg(common->dev, "cpsw_nuss stopped\n");
>  	return 0;
>  }
> @@ -597,8 +570,8 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
>  	cpsw_sl_ctl_set(port->slave.mac_sl, CPSW_SL_CTL_CMD_IDLE);
>  
>  	tmo = cpsw_sl_wait_for_idle(port->slave.mac_sl, 100);
> -	dev_info(common->dev, "down msc_sl %08x tmo %d\n",
> -		 cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_MACSTATUS), tmo);
> +	dev_dbg(common->dev, "down msc_sl %08x tmo %d\n",
> +		cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_MACSTATUS), tmo);

Looks like unncecessary noise?

>  
>  	cpsw_sl_ctl_reset(port->slave.mac_sl);
>  
> @@ -1548,9 +1521,9 @@ 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(struct am65_cpsw_common *common)
> +static void am65_cpsw_nuss_free_tx_chns(void *data)
>  {
> -	struct device *dev = common->dev;
> +	struct am65_cpsw_common *common = data;
>  	int i;
>  
>  	for (i = 0; i < common->tx_ch_num; i++) {
> @@ -1562,11 +1535,7 @@ static void am65_cpsw_nuss_free_tx_chns(struct am65_cpsw_common *common)
>  		if (!IS_ERR_OR_NULL(tx_chn->tx_chn))
>  			k3_udma_glue_release_tx_chn(tx_chn->tx_chn);
>  
> -		/* Don't clear tx_chn memory as we need to preserve
> -		 * data between suspend/resume
> -		 */
> -		if (!(tx_chn->irq < 0))
> -			devm_free_irq(dev, tx_chn->irq, tx_chn);
> +		memset(tx_chn, 0, sizeof(*tx_chn));
>  	}
>  }
>  
> @@ -1575,10 +1544,12 @@ 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);
> +
>  	for (i = 0; i < common->tx_ch_num; i++) {
>  		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
>  
> -		if (!(tx_chn->irq < 0))
> +		if (tx_chn->irq)
>  			devm_free_irq(dev, tx_chn->irq, tx_chn);
>  
>  		netif_napi_del(&tx_chn->napi_tx);
> @@ -1648,7 +1619,7 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
>  		}
>  
>  		tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn);
> -		if (tx_chn->irq < 0) {
> +		if (tx_chn->irq <= 0) {
>  			dev_err(dev, "Failed to get tx dma irq %d\n",
>  				tx_chn->irq);
>  			goto err;
> @@ -1657,41 +1628,59 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
>  		snprintf(tx_chn->tx_chn_name,
>  			 sizeof(tx_chn->tx_chn_name), "%s-tx%d",
>  			 dev_name(dev), tx_chn->id);
> -
> -		ret = devm_request_irq(dev, tx_chn->irq,
> -				       am65_cpsw_nuss_tx_irq,
> -				       IRQF_TRIGGER_HIGH,
> -				       tx_chn->tx_chn_name, tx_chn);
> -		if (ret) {
> -			dev_err(dev, "failure requesting tx%u irq %u, %d\n",
> -				tx_chn->id, tx_chn->irq, ret);
> -			tx_chn->irq = -EINVAL;
> -			goto err;
> -		}
>  	}
>  
> -	return 0;
> +	ret = am65_cpsw_nuss_ndev_add_tx_napi(common);
> +	if (ret) {
> +		dev_err(dev, "Failed to add tx NAPI %d\n", ret);
> +		goto err;
> +	}
>  
>  err:
> -	am65_cpsw_nuss_free_tx_chns(common);
> +	i = devm_add_action(dev, am65_cpsw_nuss_free_tx_chns, common);

Can you explain why you're using devm_ variant instead of a direct call in
the commit message? Couldn't these (devm_{add,remove}_action) be pulled
out the separate commit on top of this one?

> +	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(struct am65_cpsw_common *common)
> +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(void *data)
>  {
> +	struct am65_cpsw_common *common = data;
>  	struct am65_cpsw_rx_chn *rx_chn;
> +	struct device *dev = common->dev;
>  
>  	rx_chn = &common->rx_chns;
> +	devm_remove_action(dev, am65_cpsw_nuss_free_rx_chns, common);
>  
>  	if (!(rx_chn->irq < 0))
> -		devm_free_irq(common->dev, rx_chn->irq, common);
> +		devm_free_irq(dev, rx_chn->irq, common);
> +
> +	netif_napi_del(&common->napi_rx);
>  
>  	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;
>  }
>  
>  static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
> @@ -1709,7 +1698,7 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
>  
>  	rx_cfg.swdata_size = AM65_CPSW_NAV_SW_DATA_SIZE;
>  	rx_cfg.flow_id_num = AM65_CPSW_MAX_RX_FLOWS;
> -	rx_cfg.flow_id_base = -1;
> +	rx_cfg.flow_id_base = common->rx_flow_id_base;
>  
>  	/* init all flows */
>  	rx_chn->dev = dev;
> @@ -1781,20 +1770,24 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
>  		}
>  	}
>  
> +	netif_napi_add(common->dma_ndev, &common->napi_rx,
> +		       am65_cpsw_nuss_rx_poll);
> +
>  	ret = devm_request_irq(dev, rx_chn->irq,
>  			       am65_cpsw_nuss_rx_irq,
>  			       IRQF_TRIGGER_HIGH, dev_name(dev), common);
>  	if (ret) {
>  		dev_err(dev, "failure requesting rx irq %u, %d\n",
>  			rx_chn->irq, ret);
> -		rx_chn->irq = -EINVAL;
>  		goto err;
>  	}
>  
> -	return 0;
> -
>  err:
> -	am65_cpsw_nuss_free_rx_chns(common);
> +	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;
>  }
> @@ -2114,24 +2107,33 @@ static int am65_cpsw_nuss_init_ndevs(struct am65_cpsw_common *common)
>  			return ret;
>  	}
>  
> -	netif_napi_add(common->dma_ndev, &common->napi_rx,
> -		       am65_cpsw_nuss_rx_poll);
> -
>  	return ret;
>  }
>  
>  static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
>  {
> -	int i;
> +	struct device *dev = common->dev;
> +	int i, ret = 0;
>  
>  	for (i = 0; i < common->tx_ch_num; i++) {
>  		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
>  
>  		netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
>  				  am65_cpsw_nuss_tx_poll);
> +
> +		ret = devm_request_irq(dev, tx_chn->irq,
> +				       am65_cpsw_nuss_tx_irq,
> +				       IRQF_TRIGGER_HIGH,
> +				       tx_chn->tx_chn_name, tx_chn);
> +		if (ret) {
> +			dev_err(dev, "failure requesting tx%u irq %u, %d\n",
> +				tx_chn->id, tx_chn->irq, ret);
> +			goto err;

Shouldn't you rewind all of the successful irq requests on error path?

> +		}
>  	}
>  
> -	return 0;
> +err:
> +	return ret;
>  }
>  
>  static void am65_cpsw_nuss_cleanup_ndev(struct am65_cpsw_common *common)
> @@ -2597,7 +2599,11 @@ static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
>  	struct am65_cpsw_port *port;
>  	int ret = 0, i;
>  
> -	ret = am65_cpsw_nuss_ndev_add_tx_napi(common);
> +	/* init tx channels */
> +	ret = am65_cpsw_nuss_init_tx_chns(common);
> +	if (ret)
> +		return ret;
> +	ret = am65_cpsw_nuss_init_rx_chns(common);
>  	if (ret)
>  		return ret;
>  
> @@ -2645,10 +2651,8 @@ int am65_cpsw_nuss_update_tx_chns(struct am65_cpsw_common *common, int num_tx)
>  
>  	common->tx_ch_num = num_tx;
>  	ret = am65_cpsw_nuss_init_tx_chns(common);
> -	if (ret)
> -		return ret;
>  
> -	return am65_cpsw_nuss_ndev_add_tx_napi(common);
> +	return ret;
>  }
>  
>  struct am65_cpsw_soc_pdata {
> @@ -2756,6 +2760,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
>  	if (common->port_num < 1 || common->port_num > AM65_CPSW_MAX_PORTS)
>  		return -ENOENT;
>  
> +	common->rx_flow_id_base = -1;
>  	init_completion(&common->tdown_complete);
>  	common->tx_ch_num = 1;
>  	common->pf_p0_rx_ptype_rrobin = false;
> @@ -2918,6 +2923,9 @@ 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);
> +
>  	return 0;
>  }
>  
> @@ -2929,6 +2937,17 @@ static int am65_cpsw_nuss_resume(struct device *dev)
>  	int i, ret;
>  	struct am65_cpsw_host *host_p = am65_common_get_host(common);
>  
> +	ret = am65_cpsw_nuss_init_tx_chns(common);
> +	if (ret)
> +		return ret;
> +	ret = am65_cpsw_nuss_init_rx_chns(common);
> +	if (ret)
> +		return ret;
> +
> +	/* If RX IRQ was disabled before suspend, keep it disabled */
> +	if (common->rx_irq_disabled)
> +		disable_irq(common->rx_chns.irq);
> +
>  	am65_cpts_resume(common->cpts);
>  
>  	for (i = 0; i < common->port_num; i++) {
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/4] net: ethernet: ti: am65-cpsw-nuss: Remove redundant ALE_CLEAR

[Maciej Fijalkowski]

On Mon, Nov 21, 2022 at 03:22:58PM +0100, Roger Quadros wrote:
> ALE_CLEAR command is issued in cpsw_ale_start() so no need
> to issue it before the call to cpsw_ale_start().
> 
> Fixes: fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")

Not a fix to me, can you send it to -next tree? As you said, it's an
optimization.

> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 505c9edf98ff..2acde5b14516 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -404,7 +404,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>  	/* disable priority elevation */
>  	writel(0, common->cpsw_base + AM65_CPSW_REG_PTYPE);
>  
> -	cpsw_ale_control_set(common->ale, 0, ALE_CLEAR, 1);
>  	cpsw_ale_start(common->ale);
>  
>  	/* limit to one RX flow only */
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[Andrew Lunn]

On Mon, Nov 21, 2022 at 04:22:57PM +0200, Roger Quadros wrote:
> The set channel operation "ethtool -L tx <n>" broke with
> the recent suspend/resume changes.
> 
> Revert back to original driver behaviour of not freeing
> the TX/RX IRQs at am65_cpsw_nuss_common_stop(). We will
> now free them only on .suspend() as we need to release
> the DMA channels (as DMA looses context) and re-acquiring
> them on .resume() may not necessarily give us the same
> IRQs.
> 
> Introduce am65_cpsw_nuss_remove_rx_chns() which is similar
> to am65_cpsw_nuss_remove_tx_chns() and invoke them both in
> .suspend().
> 
> At .resume() call am65_cpsw_nuss_init_rx/tx_chns() to
> acquire the DMA channels.
> 
> To as IRQs need to be requested after knowing the IRQ
> numbers, move am65_cpsw_nuss_ndev_add_tx_napi() call to
> am65_cpsw_nuss_init_tx_chns().

It is probably easier to review if you first do a revert and then add
the new code to make suspend/resume work.

    Andrew

Re: [PATCH v2 2/4] net: ethernet: ti: am65-cpsw-nuss: Remove redundant ALE_CLEAR

[Andrew Lunn]

On Mon, Nov 21, 2022 at 06:51:05PM +0100, Maciej Fijalkowski wrote:
> On Mon, Nov 21, 2022 at 03:22:58PM +0100, Roger Quadros wrote:
> > ALE_CLEAR command is issued in cpsw_ale_start() so no need
> > to issue it before the call to cpsw_ale_start().
> > 
> > Fixes: fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")
> 
> Not a fix to me, can you send it to -next tree? As you said, it's an
> optimization.

commit fd23df72f2be317d38d9fde0a8996b8e7454fd2a
Author: Roger Quadros <rogerq@kernel.org>
Date:   Fri Nov 4 15:23:07 2022 +0200

The change being fixed is in net-next.

Roger, please take a look at the netdev FAQ and fix your Subject line.

       Andrew

Re: [PATCH v2 2/4] net: ethernet: ti: am65-cpsw-nuss: Remove redundant ALE_CLEAR

[Maciej Fijalkowski]

On Tue, Nov 22, 2022 at 02:02:29AM +0100, Andrew Lunn wrote:
> On Mon, Nov 21, 2022 at 06:51:05PM +0100, Maciej Fijalkowski wrote:
> > On Mon, Nov 21, 2022 at 03:22:58PM +0100, Roger Quadros wrote:
> > > ALE_CLEAR command is issued in cpsw_ale_start() so no need
> > > to issue it before the call to cpsw_ale_start().
> > > 
> > > Fixes: fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")
> > 
> > Not a fix to me, can you send it to -next tree? As you said, it's an
> > optimization.
> 
> commit fd23df72f2be317d38d9fde0a8996b8e7454fd2a
> Author: Roger Quadros <rogerq@kernel.org>
> Date:   Fri Nov 4 15:23:07 2022 +0200
> 
> The change being fixed is in net-next.

Ah right, nevertheless I had some comments there.

> 
> Roger, please take a look at the netdev FAQ and fix your Subject line.
> 
>        Andrew

Re: [PATCH v2 1/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[Roger Quadros]

Hi Maciej,

On 21/11/2022 19:50, Maciej Fijalkowski wrote:
> On Mon, Nov 21, 2022 at 03:22:57PM +0100, Roger Quadros wrote:
>> The set channel operation "ethtool -L tx <n>" broke with
>> the recent suspend/resume changes.
> 
> Would be worth just dropping here the SHA-1 of offending commit, I deduce
> that it is the one that fixes tag points to.

OK. I'll follow Andrew's suggestion and first revert the offending commit.

It should also address the other issues you pointed out with this patch.

cheers,
-roger

> 
>>
>> Revert back to original driver behaviour of not freeing
>> the TX/RX IRQs at am65_cpsw_nuss_common_stop(). We will
>> now free them only on .suspend() as we need to release
>> the DMA channels (as DMA looses context) and re-acquiring
>> them on .resume() may not necessarily give us the same
>> IRQs.
>>
>> Introduce am65_cpsw_nuss_remove_rx_chns() which is similar
>> to am65_cpsw_nuss_remove_tx_chns() and invoke them both in
>> .suspend().
>>
>> At .resume() call am65_cpsw_nuss_init_rx/tx_chns() to
>> acquire the DMA channels.
>>
>> To as IRQs need to be requested after knowing the IRQ
>> numbers, move am65_cpsw_nuss_ndev_add_tx_napi() call to
>> am65_cpsw_nuss_init_tx_chns().
>>
>> Also fixes the below warning during suspend/resume on multi
> 
> s/fixes/fix ?
> 
>> CPU system.
>>
>> [   67.347684] ------------[ cut here ]------------
>> [   67.347700] Unbalanced enable for IRQ 119
>> [   67.347726] WARNING: CPU: 0 PID: 1080 at kernel/irq/manage.c:781 __enable_irq+0x4c/0x80
>> [   67.347754] Modules linked in: wlcore_sdio wl18xx wlcore mac80211 libarc4 cfg80211 rfkill crct10dif_ce sch_fq_codel ipv6
>> [   67.347803] CPU: 0 PID: 1080 Comm: rtcwake Not tainted 6.1.0-rc4-00023-gc826e5480732-dirty #203
>> [   67.347812] Hardware name: Texas Instruments AM625 (DT)
>> [   67.347818] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [   67.347829] pc : __enable_irq+0x4c/0x80
>> [   67.347838] lr : __enable_irq+0x4c/0x80
>> [   67.347846] sp : ffff80000999ba00
>> [   67.347850] x29: ffff80000999ba00 x28: ffff0000011c1c80 x27: 0000000000000000
>> [   67.347863] x26: 00000000000001f4 x25: ffff000001058358 x24: ffff000001059080
>> [   67.347876] x23: ffff000001058080 x22: ffff000001060000 x21: 0000000000000077
>> [   67.347888] x20: ffff0000011c1c80 x19: ffff000001429600 x18: 0000000000000001
>> [   67.347900] x17: 0000000000000080 x16: fffffc000176e008 x15: ffff0000011c21b0
>> [   67.347913] x14: 0000000000000000 x13: 3931312051524920 x12: 726f6620656c6261
>> [   67.347925] x11: 656820747563205b x10: 000000000000000a x9 : ffff80000999ba00
>> [   67.347938] x8 : ffff800009121068 x7 : ffff80000999b810 x6 : 00000000fffff17f
>> [   67.347950] x5 : ffff00007fb99b18 x4 : 0000000000000000 x3 : 0000000000000027
>> [   67.347962] x2 : ffff00007fb99b20 x1 : 50dd48f7f19deb00 x0 : 0000000000000000
>> [   67.347975] Call trace:
>> [   67.347980]  __enable_irq+0x4c/0x80
>> [   67.347989]  enable_irq+0x4c/0xa0
>> [   67.347999]  am65_cpsw_nuss_ndo_slave_open+0x4b0/0x568
>> [   67.348015]  am65_cpsw_nuss_resume+0x68/0x160
>> [   67.348025]  dpm_run_callback.isra.0+0x28/0x88
>> [   67.348040]  device_resume+0x78/0x160
>> [   67.348050]  dpm_resume+0xc0/0x1f8
>> [   67.348057]  dpm_resume_end+0x18/0x30
>> [   67.348063]  suspend_devices_and_enter+0x1cc/0x4e0
>> [   67.348075]  pm_suspend+0x1f8/0x268
>> [   67.348084]  state_store+0x8c/0x118
>> [   67.348092]  kobj_attr_store+0x18/0x30
>> [   67.348104]  sysfs_kf_write+0x44/0x58
>> [   67.348117]  kernfs_fop_write_iter+0x118/0x1a8
>> [   67.348127]  vfs_write+0x31c/0x418
>> [   67.348140]  ksys_write+0x6c/0xf8
>> [   67.348150]  __arm64_sys_write+0x1c/0x28
>> [   67.348160]  invoke_syscall+0x44/0x108
>> [   67.348172]  el0_svc_common.constprop.0+0x44/0xf0
>> [   67.348182]  do_el0_svc+0x2c/0xc8
>> [   67.348191]  el0_svc+0x2c/0x88
>> [   67.348201]  el0t_64_sync_handler+0xb8/0xc0
>> [   67.348209]  el0t_64_sync+0x18c/0x190
>> [   67.348218] ---[ end trace 0000000000000000 ]---
>>
>> Fixes: fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 161 +++++++++++++----------
>>  1 file changed, 90 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index f2e377524088..505c9edf98ff 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -133,10 +133,7 @@
>>  			 NETIF_MSG_IFUP	| NETIF_MSG_PROBE | NETIF_MSG_IFDOWN | \
>>  			 NETIF_MSG_RX_ERR | NETIF_MSG_TX_ERR)
>>  
>> -static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common);
>> -static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common);
>> -static void am65_cpsw_nuss_free_tx_chns(struct am65_cpsw_common *common);
>> -static void am65_cpsw_nuss_free_rx_chns(struct am65_cpsw_common *common);
>> +static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common);
>>  
>>  static void am65_cpsw_port_set_sl_mac(struct am65_cpsw_port *slave,
>>  				      const u8 *dev_addr)
>> @@ -379,20 +376,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  	if (common->usage_count)
>>  		return 0;
>>  
>> -	/* init tx/rx channels */
>> -	ret = am65_cpsw_nuss_init_tx_chns(common);
>> -	if (ret) {
>> -		dev_err(common->dev, "init_tx_chns failed\n");
>> -		return ret;
>> -	}
>> -
>> -	ret = am65_cpsw_nuss_init_rx_chns(common);
>> -	if (ret) {
>> -		dev_err(common->dev, "init_rx_chns failed\n");
>> -		am65_cpsw_nuss_free_tx_chns(common);
>> -		return ret;
>> -	}
>> -
>>  	/* Control register */
>>  	writel(AM65_CPSW_CTL_P0_ENABLE | AM65_CPSW_CTL_P0_TX_CRC_REMOVE |
>>  	       AM65_CPSW_CTL_VLAN_AWARE | AM65_CPSW_CTL_P0_RX_PAD,
>> @@ -453,8 +436,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  						  GFP_KERNEL);
>>  		if (!skb) {
>>  			dev_err(common->dev, "cannot allocate skb\n");
>> -			ret = -ENOMEM;
>> -			goto err;
>> +			return -ENOMEM;
>>  		}
>>  
>>  		ret = am65_cpsw_nuss_rx_push(common, skb);
>> @@ -463,7 +445,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  				"cannot submit skb to channel rx, error %d\n",
>>  				ret);
>>  			kfree_skb(skb);
>> -			goto err;
>> +			return ret;
>>  		}
>>  		kmemleak_not_leak(skb);
>>  	}
>> @@ -472,7 +454,7 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  	for (i = 0; i < common->tx_ch_num; i++) {
>>  		ret = k3_udma_glue_enable_tx_chn(common->tx_chns[i].tx_chn);
>>  		if (ret)
>> -			goto err;
>> +			return ret;
>>  		napi_enable(&common->tx_chns[i].napi_tx);
>>  	}
>>  
>> @@ -484,12 +466,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  
>>  	dev_dbg(common->dev, "cpsw_nuss started\n");
>>  	return 0;
>> -
>> -err:
>> -	am65_cpsw_nuss_free_tx_chns(common);
>> -	am65_cpsw_nuss_free_rx_chns(common);
>> -
>> -	return ret;
>>  }
>>  
>>  static void am65_cpsw_nuss_tx_cleanup(void *data, dma_addr_t desc_dma);
>> @@ -543,9 +519,6 @@ static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
>>  	writel(0, common->cpsw_base + AM65_CPSW_REG_CTL);
>>  	writel(0, common->cpsw_base + AM65_CPSW_REG_STAT_PORT_EN);
>>  
>> -	am65_cpsw_nuss_free_tx_chns(common);
>> -	am65_cpsw_nuss_free_rx_chns(common);
>> -
>>  	dev_dbg(common->dev, "cpsw_nuss stopped\n");
>>  	return 0;
>>  }
>> @@ -597,8 +570,8 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
>>  	cpsw_sl_ctl_set(port->slave.mac_sl, CPSW_SL_CTL_CMD_IDLE);
>>  
>>  	tmo = cpsw_sl_wait_for_idle(port->slave.mac_sl, 100);
>> -	dev_info(common->dev, "down msc_sl %08x tmo %d\n",
>> -		 cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_MACSTATUS), tmo);
>> +	dev_dbg(common->dev, "down msc_sl %08x tmo %d\n",
>> +		cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_MACSTATUS), tmo);
> 
> Looks like unncecessary noise?
> 
>>  
>>  	cpsw_sl_ctl_reset(port->slave.mac_sl);
>>  
>> @@ -1548,9 +1521,9 @@ 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(struct am65_cpsw_common *common)
>> +static void am65_cpsw_nuss_free_tx_chns(void *data)
>>  {
>> -	struct device *dev = common->dev;
>> +	struct am65_cpsw_common *common = data;
>>  	int i;
>>  
>>  	for (i = 0; i < common->tx_ch_num; i++) {
>> @@ -1562,11 +1535,7 @@ static void am65_cpsw_nuss_free_tx_chns(struct am65_cpsw_common *common)
>>  		if (!IS_ERR_OR_NULL(tx_chn->tx_chn))
>>  			k3_udma_glue_release_tx_chn(tx_chn->tx_chn);
>>  
>> -		/* Don't clear tx_chn memory as we need to preserve
>> -		 * data between suspend/resume
>> -		 */
>> -		if (!(tx_chn->irq < 0))
>> -			devm_free_irq(dev, tx_chn->irq, tx_chn);
>> +		memset(tx_chn, 0, sizeof(*tx_chn));
>>  	}
>>  }
>>  
>> @@ -1575,10 +1544,12 @@ 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);
>> +
>>  	for (i = 0; i < common->tx_ch_num; i++) {
>>  		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
>>  
>> -		if (!(tx_chn->irq < 0))
>> +		if (tx_chn->irq)
>>  			devm_free_irq(dev, tx_chn->irq, tx_chn);
>>  
>>  		netif_napi_del(&tx_chn->napi_tx);
>> @@ -1648,7 +1619,7 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
>>  		}
>>  
>>  		tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn);
>> -		if (tx_chn->irq < 0) {
>> +		if (tx_chn->irq <= 0) {
>>  			dev_err(dev, "Failed to get tx dma irq %d\n",
>>  				tx_chn->irq);
>>  			goto err;
>> @@ -1657,41 +1628,59 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
>>  		snprintf(tx_chn->tx_chn_name,
>>  			 sizeof(tx_chn->tx_chn_name), "%s-tx%d",
>>  			 dev_name(dev), tx_chn->id);
>> -
>> -		ret = devm_request_irq(dev, tx_chn->irq,
>> -				       am65_cpsw_nuss_tx_irq,
>> -				       IRQF_TRIGGER_HIGH,
>> -				       tx_chn->tx_chn_name, tx_chn);
>> -		if (ret) {
>> -			dev_err(dev, "failure requesting tx%u irq %u, %d\n",
>> -				tx_chn->id, tx_chn->irq, ret);
>> -			tx_chn->irq = -EINVAL;
>> -			goto err;
>> -		}
>>  	}
>>  
>> -	return 0;
>> +	ret = am65_cpsw_nuss_ndev_add_tx_napi(common);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to add tx NAPI %d\n", ret);
>> +		goto err;
>> +	}
>>  
>>  err:
>> -	am65_cpsw_nuss_free_tx_chns(common);
>> +	i = devm_add_action(dev, am65_cpsw_nuss_free_tx_chns, common);
> 
> Can you explain why you're using devm_ variant instead of a direct call in
> the commit message? Couldn't these (devm_{add,remove}_action) be pulled
> out the separate commit on top of this one?
> 
>> +	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(struct am65_cpsw_common *common)
>> +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(void *data)
>>  {
>> +	struct am65_cpsw_common *common = data;
>>  	struct am65_cpsw_rx_chn *rx_chn;
>> +	struct device *dev = common->dev;
>>  
>>  	rx_chn = &common->rx_chns;
>> +	devm_remove_action(dev, am65_cpsw_nuss_free_rx_chns, common);
>>  
>>  	if (!(rx_chn->irq < 0))
>> -		devm_free_irq(common->dev, rx_chn->irq, common);
>> +		devm_free_irq(dev, rx_chn->irq, common);
>> +
>> +	netif_napi_del(&common->napi_rx);
>>  
>>  	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;
>>  }
>>  
>>  static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
>> @@ -1709,7 +1698,7 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
>>  
>>  	rx_cfg.swdata_size = AM65_CPSW_NAV_SW_DATA_SIZE;
>>  	rx_cfg.flow_id_num = AM65_CPSW_MAX_RX_FLOWS;
>> -	rx_cfg.flow_id_base = -1;
>> +	rx_cfg.flow_id_base = common->rx_flow_id_base;
>>  
>>  	/* init all flows */
>>  	rx_chn->dev = dev;
>> @@ -1781,20 +1770,24 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
>>  		}
>>  	}
>>  
>> +	netif_napi_add(common->dma_ndev, &common->napi_rx,
>> +		       am65_cpsw_nuss_rx_poll);
>> +
>>  	ret = devm_request_irq(dev, rx_chn->irq,
>>  			       am65_cpsw_nuss_rx_irq,
>>  			       IRQF_TRIGGER_HIGH, dev_name(dev), common);
>>  	if (ret) {
>>  		dev_err(dev, "failure requesting rx irq %u, %d\n",
>>  			rx_chn->irq, ret);
>> -		rx_chn->irq = -EINVAL;
>>  		goto err;
>>  	}
>>  
>> -	return 0;
>> -
>>  err:
>> -	am65_cpsw_nuss_free_rx_chns(common);
>> +	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;
>>  }
>> @@ -2114,24 +2107,33 @@ static int am65_cpsw_nuss_init_ndevs(struct am65_cpsw_common *common)
>>  			return ret;
>>  	}
>>  
>> -	netif_napi_add(common->dma_ndev, &common->napi_rx,
>> -		       am65_cpsw_nuss_rx_poll);
>> -
>>  	return ret;
>>  }
>>  
>>  static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
>>  {
>> -	int i;
>> +	struct device *dev = common->dev;
>> +	int i, ret = 0;
>>  
>>  	for (i = 0; i < common->tx_ch_num; i++) {
>>  		struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
>>  
>>  		netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
>>  				  am65_cpsw_nuss_tx_poll);
>> +
>> +		ret = devm_request_irq(dev, tx_chn->irq,
>> +				       am65_cpsw_nuss_tx_irq,
>> +				       IRQF_TRIGGER_HIGH,
>> +				       tx_chn->tx_chn_name, tx_chn);
>> +		if (ret) {
>> +			dev_err(dev, "failure requesting tx%u irq %u, %d\n",
>> +				tx_chn->id, tx_chn->irq, ret);
>> +			goto err;
> 
> Shouldn't you rewind all of the successful irq requests on error path?
> 
>> +		}
>>  	}
>>  
>> -	return 0;
>> +err:
>> +	return ret;
>>  }
>>  
>>  static void am65_cpsw_nuss_cleanup_ndev(struct am65_cpsw_common *common)
>> @@ -2597,7 +2599,11 @@ static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
>>  	struct am65_cpsw_port *port;
>>  	int ret = 0, i;
>>  
>> -	ret = am65_cpsw_nuss_ndev_add_tx_napi(common);
>> +	/* init tx channels */
>> +	ret = am65_cpsw_nuss_init_tx_chns(common);
>> +	if (ret)
>> +		return ret;
>> +	ret = am65_cpsw_nuss_init_rx_chns(common);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -2645,10 +2651,8 @@ int am65_cpsw_nuss_update_tx_chns(struct am65_cpsw_common *common, int num_tx)
>>  
>>  	common->tx_ch_num = num_tx;
>>  	ret = am65_cpsw_nuss_init_tx_chns(common);
>> -	if (ret)
>> -		return ret;
>>  
>> -	return am65_cpsw_nuss_ndev_add_tx_napi(common);
>> +	return ret;
>>  }
>>  
>>  struct am65_cpsw_soc_pdata {
>> @@ -2756,6 +2760,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
>>  	if (common->port_num < 1 || common->port_num > AM65_CPSW_MAX_PORTS)
>>  		return -ENOENT;
>>  
>> +	common->rx_flow_id_base = -1;
>>  	init_completion(&common->tdown_complete);
>>  	common->tx_ch_num = 1;
>>  	common->pf_p0_rx_ptype_rrobin = false;
>> @@ -2918,6 +2923,9 @@ 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);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -2929,6 +2937,17 @@ static int am65_cpsw_nuss_resume(struct device *dev)
>>  	int i, ret;
>>  	struct am65_cpsw_host *host_p = am65_common_get_host(common);
>>  
>> +	ret = am65_cpsw_nuss_init_tx_chns(common);
>> +	if (ret)
>> +		return ret;
>> +	ret = am65_cpsw_nuss_init_rx_chns(common);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* If RX IRQ was disabled before suspend, keep it disabled */
>> +	if (common->rx_irq_disabled)
>> +		disable_irq(common->rx_chns.irq);
>> +
>>  	am65_cpts_resume(common->cpts);
>>  
>>  	for (i = 0; i < common->port_num; i++) {
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 1/4] net: ethernet: ti: am65-cpsw: Fix set channel operation

[Roger Quadros]

On 22/11/2022 02:58, Andrew Lunn wrote:
> On Mon, Nov 21, 2022 at 04:22:57PM +0200, Roger Quadros wrote:
>> The set channel operation "ethtool -L tx <n>" broke with
>> the recent suspend/resume changes.
>>
>> Revert back to original driver behaviour of not freeing
>> the TX/RX IRQs at am65_cpsw_nuss_common_stop(). We will
>> now free them only on .suspend() as we need to release
>> the DMA channels (as DMA looses context) and re-acquiring
>> them on .resume() may not necessarily give us the same
>> IRQs.
>>
>> Introduce am65_cpsw_nuss_remove_rx_chns() which is similar
>> to am65_cpsw_nuss_remove_tx_chns() and invoke them both in
>> .suspend().
>>
>> At .resume() call am65_cpsw_nuss_init_rx/tx_chns() to
>> acquire the DMA channels.
>>
>> To as IRQs need to be requested after knowing the IRQ
>> numbers, move am65_cpsw_nuss_ndev_add_tx_napi() call to
>> am65_cpsw_nuss_init_tx_chns().
> 
> It is probably easier to review if you first do a revert and then add
> the new code to make suspend/resume work.

Thanks! This will make it much easier to review.

> 
>     Andrew

cheers,
-roger