[PATCH v2 net 0/9] net: enetc: fix some known issues

[PATCH v2 net 0/9] net: enetc: fix some known issues

[Wei Fang]

There are some issues with the enetc driver, some of which are specific
to the LS1028A platform, and some of which were introduced recently when
i.MX95 ENETC support was added, so this patch set aims to clean up those
issues.

---
v1 link: https://lore.kernel.org/imx/20250217093906.506214-1-wei.fang@nxp.com/
v2 changes:
1. Remove the unneeded semicolon from patch 1
2. Modify the commit message of patch 1
3. Add new patch 9 to fix another off-by-one issue
---

Wei Fang (9):
  net: enetc: fix the off-by-one issue in enetc_map_tx_buffs()
  net: enetc: correct the tx_swbd statistics
  net: enetc: correct the xdp_tx statistics
  net: enetc: VFs do not support HWTSTAMP_TX_ONESTEP_SYNC
  net: enetc: update UDP checksum when updating originTimestamp field
  net: enetc: add missing enetc4_link_deinit()
  net: enetc: remove the mm_lock from the ENETC v4 driver
  net: enetc: correct the EMDIO base offset for ENETC v4
  net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

 drivers/net/ethernet/freescale/enetc/enetc.c  | 59 ++++++++++++++-----
 .../net/ethernet/freescale/enetc/enetc4_hw.h  |  3 +
 .../net/ethernet/freescale/enetc/enetc4_pf.c  |  2 +-
 .../ethernet/freescale/enetc/enetc_ethtool.c  |  7 ++-
 .../freescale/enetc/enetc_pf_common.c         | 10 +++-
 5 files changed, 63 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in enetc_map_tx_buffs()

[Wei Fang]

When a DMA mapping error occurs while processing skb frags, it will free
one more tx_swbd than expected, so fix this off-by-one issue.

Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 6a6fc819dfde..01c09fd26f9f 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -372,13 +372,13 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 dma_err:
 	dev_err(tx_ring->dev, "DMA map error");
 
-	do {
+	while (count--) {
 		tx_swbd = &tx_ring->tx_swbd[i];
 		enetc_free_tx_frame(tx_ring, tx_swbd);
 		if (i == 0)
 			i = tx_ring->bd_count;
 		i--;
-	} while (count--);
+	}
 
 	return 0;
 }
-- 
2.34.1

[PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Wei Fang]

When creating a TSO header, if the skb is VLAN tagged, the extended BD
will be used and the 'count' should be increased by 2 instead of 1.
Otherwise, when an error occurs, less tx_swbd will be freed than the
actual number.

Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 01c09fd26f9f..0658c06a23c1 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -759,6 +759,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
+	bool ext_bd = skb_vlan_tag_present(skb);
 	int hdr_len, total_len, data_len;
 	struct enetc_tx_swbd *tx_swbd;
 	union enetc_tx_bd *txbd;
@@ -792,7 +793,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 		csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos);
 		enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, data_len);
 		bd_data_num = 0;
-		count++;
+		count += ext_bd ? 2 : 1;
 
 		while (data_len > 0) {
 			int size;
-- 
2.34.1

[PATCH v2 net 3/9] net: enetc: correct the xdp_tx statistics

[Wei Fang]

The 'xdp_tx' is used to count the number of XDP_TX frames sent, not the
number of Tx BDs.

Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 0658c06a23c1..83f9e8a9ab2b 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1902,7 +1902,7 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
 				enetc_xdp_drop(rx_ring, orig_i, i);
 				tx_ring->stats.xdp_tx_drops++;
 			} else {
-				tx_ring->stats.xdp_tx += xdp_tx_bd_cnt;
+				tx_ring->stats.xdp_tx++;
 				rx_ring->xdp.xdp_tx_in_flight += xdp_tx_bd_cnt;
 				xdp_tx_frm_cnt++;
 				/* The XDP_TX enqueue was successful, so we
-- 
2.34.1

[PATCH v2 net 4/9] net: enetc: VFs do not support HWTSTAMP_TX_ONESTEP_SYNC

[Wei Fang]

Actually ENETC VFs do not support HWTSTAMP_TX_ONESTEP_SYNC because only
ENETC PF can access PMa_SINGLE_STEP registers. And there will be a crash
if VFs are used to test one-step timestamp, the crash log as follows.

[  129.110909] Unable to handle kernel paging request at virtual address 00000000000080c0
[  129.287769] Call trace:
[  129.290219]  enetc_port_mac_wr+0x30/0xec (P)
[  129.294504]  enetc_start_xmit+0xda4/0xe74
[  129.298525]  enetc_xmit+0x70/0xec
[  129.301848]  dev_hard_start_xmit+0x98/0x118

Fixes: 41514737ecaa ("enetc: add get_ts_info interface for ethtool")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c         | 3 +++
 drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 7 +++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 83f9e8a9ab2b..77f8ef5358b6 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -3229,6 +3229,9 @@ static int enetc_hwtstamp_set(struct net_device *ndev, struct ifreq *ifr)
 		new_offloads |= ENETC_F_TX_TSTAMP;
 		break;
 	case HWTSTAMP_TX_ONESTEP_SYNC:
+		if (!enetc_si_is_pf(priv->si))
+			return -EOPNOTSUPP;
+
 		new_offloads &= ~ENETC_F_TX_TSTAMP_MASK;
 		new_offloads |= ENETC_F_TX_ONESTEP_SYNC_TSTAMP;
 		break;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index bf34b5bb1e35..ece3ae28ba82 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -832,6 +832,7 @@ static int enetc_set_coalesce(struct net_device *ndev,
 static int enetc_get_ts_info(struct net_device *ndev,
 			     struct kernel_ethtool_ts_info *info)
 {
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int *phc_idx;
 
 	phc_idx = symbol_get(enetc_phc_index);
@@ -852,8 +853,10 @@ static int enetc_get_ts_info(struct net_device *ndev,
 				SOF_TIMESTAMPING_TX_SOFTWARE;
 
 	info->tx_types = (1 << HWTSTAMP_TX_OFF) |
-			 (1 << HWTSTAMP_TX_ON) |
-			 (1 << HWTSTAMP_TX_ONESTEP_SYNC);
+			 (1 << HWTSTAMP_TX_ON);
+
+	if (enetc_si_is_pf(priv->si))
+		info->tx_types |= (1 << HWTSTAMP_TX_ONESTEP_SYNC);
 
 	info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
 			   (1 << HWTSTAMP_FILTER_ALL);
-- 
2.34.1

[PATCH v2 net 5/9] net: enetc: update UDP checksum when updating originTimestamp field

[Wei Fang]

There is an issue with one-step timestamp based on UDP/IP. The peer will
discard the sync packet because of the wrong UDP checksum. For ENETC v1,
the software needs to update the UDP checksum when updating the
originTimestamp field, so that the hardware can correctly update the UDP
checksum when updating the correction field. Otherwise, the UDP checksum
in the sync packet will be wrong.

Fixes: 7294380c5211 ("enetc: support PTP Sync packet one-step timestamping")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 41 ++++++++++++++++----
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 77f8ef5358b6..9a24d1176479 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -279,9 +279,11 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 		}
 
 		if (do_onestep_tstamp) {
-			u32 lo, hi, val;
-			u64 sec, nsec;
+			__be32 new_sec_l, new_nsec;
+			u32 lo, hi, nsec, val;
+			__be16 new_sec_h;
 			u8 *data;
+			u64 sec;
 
 			lo = enetc_rd_hot(hw, ENETC_SICTR0);
 			hi = enetc_rd_hot(hw, ENETC_SICTR1);
@@ -295,13 +297,38 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 			/* Update originTimestamp field of Sync packet
 			 * - 48 bits seconds field
 			 * - 32 bits nanseconds field
+			 *
+			 * In addition, the UDP checksum needs to be updated
+			 * by software after updating originTimestamp field,
+			 * otherwise the hardware will calculate the wrong
+			 * checksum when updating the correction field and
+			 * update it to the packet.
 			 */
 			data = skb_mac_header(skb);
-			*(__be16 *)(data + offset2) =
-				htons((sec >> 32) & 0xffff);
-			*(__be32 *)(data + offset2 + 2) =
-				htonl(sec & 0xffffffff);
-			*(__be32 *)(data + offset2 + 6) = htonl(nsec);
+			new_sec_h = htons((sec >> 32) & 0xffff);
+			new_sec_l = htonl(sec & 0xffffffff);
+			new_nsec = htonl(nsec);
+			if (udp) {
+				struct udphdr *uh = udp_hdr(skb);
+				__be32 old_sec_l, old_nsec;
+				__be16 old_sec_h;
+
+				old_sec_h = *(__be16 *)(data + offset2);
+				inet_proto_csum_replace2(&uh->check, skb, old_sec_h,
+							 new_sec_h, false);
+
+				old_sec_l = *(__be32 *)(data + offset2 + 2);
+				inet_proto_csum_replace4(&uh->check, skb, old_sec_l,
+							 new_sec_l, false);
+
+				old_nsec = *(__be32 *)(data + offset2 + 6);
+				inet_proto_csum_replace4(&uh->check, skb, old_nsec,
+							 new_nsec, false);
+			}
+
+			*(__be16 *)(data + offset2) = new_sec_h;
+			*(__be32 *)(data + offset2 + 2) = new_sec_l;
+			*(__be32 *)(data + offset2 + 6) = new_nsec;
 
 			/* Configure single-step register */
 			val = ENETC_PM0_SINGLE_STEP_EN;
-- 
2.34.1

[PATCH v2 net 6/9] net: enetc: add missing enetc4_link_deinit()

[Wei Fang]

The enetc4_link_init() is called when the PF driver probes to create
phylink and MDIO bus, but we forgot to call enetc4_link_deinit() to
free the phylink and MDIO bus when the driver was unbound. so add
missing enetc4_link_deinit() to enetc4_pf_netdev_destroy().

Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc4_pf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
index fc41078c4f5d..48861c8b499a 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
@@ -684,6 +684,7 @@ static void enetc4_pf_netdev_destroy(struct enetc_si *si)
 	struct net_device *ndev = si->ndev;
 
 	unregister_netdev(ndev);
+	enetc4_link_deinit(priv);
 	enetc_free_msix(priv);
 	free_netdev(ndev);
 }
-- 
2.34.1

[PATCH v2 net 7/9] net: enetc: remove the mm_lock from the ENETC v4 driver

[Wei Fang]

Currently, the ENETC v4 driver has not added the MAC merge layer support
in the upstream, so the mm_lock is not initialized and used, so remove
the mm_lock from the driver.

Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc4_pf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
index 48861c8b499a..73ac8c6afb3a 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
@@ -672,7 +672,6 @@ static int enetc4_pf_netdev_create(struct enetc_si *si)
 err_alloc_msix:
 err_config_si:
 err_clk_get:
-	mutex_destroy(&priv->mm_lock);
 	free_netdev(ndev);
 
 	return err;
-- 
2.34.1

[PATCH v2 net 8/9] net: enetc: correct the EMDIO base offset for ENETC v4

[Wei Fang]

In addition to centrally managing external PHYs through EMIDO device,
each ENETC has a set of EMDIO registers to access and manage its own
external PHY. When adding i.MX95 ENETC support, the EMDIO base offset
was forgot to be updated, which will result in ENETC being unable to
manage its external PHY through its own EMDIO registers.

Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc4_hw.h       |  3 +++
 drivers/net/ethernet/freescale/enetc/enetc_pf_common.c | 10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
index 695cb07c74bc..02d627e2cca6 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_hw.h
@@ -175,4 +175,7 @@
 #define   SSP_1G			2
 #define  PM_IF_MODE_ENA			BIT(15)
 
+/* Port external MDIO Base address, use to access off-chip PHY */
+#define ENETC4_EMDIO_BASE		0x5c00
+
 #endif
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
index 3fd9b0727875..13e2db561c22 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
@@ -154,6 +154,14 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
 }
 EXPORT_SYMBOL_GPL(enetc_pf_netdev_setup);
 
+static int enetc_get_mdio_base(struct enetc_si *si)
+{
+	if (is_enetc_rev1(si))
+		return ENETC_EMDIO_BASE;
+
+	return ENETC4_EMDIO_BASE;
+}
+
 static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
 {
 	struct device *dev = &pf->si->pdev->dev;
@@ -173,7 +181,7 @@ static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
 	bus->parent = dev;
 	mdio_priv = bus->priv;
 	mdio_priv->hw = &pf->si->hw;
-	mdio_priv->mdio_base = ENETC_EMDIO_BASE;
+	mdio_priv->mdio_base = enetc_get_mdio_base(pf->si);
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
 
 	err = of_mdiobus_register(bus, np);
-- 
2.34.1

[PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Wei Fang]

There is an off-by-one issue for the err_chained_bd path, it will free
one more tx_swbd than expected. But there is no such issue for the
err_map_data path. To fix this off-by-one issue and make the two error
handling consistent, the loop condition of error handling is modified
and the 'count++' operation is moved before enetc_map_tx_tso_data().

Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 9a24d1176479..fe3967268a19 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -832,6 +832,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 			txbd = ENETC_TXBD(*tx_ring, i);
 			tx_swbd = &tx_ring->tx_swbd[i];
 			prefetchw(txbd);
+			count++;
 
 			/* Compute the checksum over this segment of data and
 			 * add it to the csum already computed (over the L4
@@ -848,7 +849,6 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 				goto err_map_data;
 
 			data_len -= size;
-			count++;
 			bd_data_num++;
 			tso_build_data(skb, &tso, size);
 
@@ -874,13 +874,13 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 	dev_err(tx_ring->dev, "DMA map error");
 
 err_chained_bd:
-	do {
+	while (count--) {
 		tx_swbd = &tx_ring->tx_swbd[i];
 		enetc_free_tx_frame(tx_ring, tx_swbd);
 		if (i == 0)
 			i = tx_ring->bd_count;
 		i--;
-	} while (count--);
+	}
 
 	return 0;
 }
-- 
2.34.1

RE: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in enetc_map_tx_buffs()

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Wednesday, February 19, 2025 7:43 AM
[...]
> Subject: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in
> enetc_map_tx_buffs()
> 
> When a DMA mapping error occurs while processing skb frags, it will free
> one more tx_swbd than expected, so fix this off-by-one issue.
> 
> Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet
> drivers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>

Re: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Michal Swiatkowski]

On Wed, Feb 19, 2025 at 01:42:47PM +0800, Wei Fang wrote:
> There is an off-by-one issue for the err_chained_bd path, it will free
> one more tx_swbd than expected. But there is no such issue for the
> err_map_data path. To fix this off-by-one issue and make the two error
> handling consistent, the loop condition of error handling is modified
> and the 'count++' operation is moved before enetc_map_tx_tso_data().
> 
> Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 9a24d1176479..fe3967268a19 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -832,6 +832,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  			txbd = ENETC_TXBD(*tx_ring, i);
>  			tx_swbd = &tx_ring->tx_swbd[i];
>  			prefetchw(txbd);
> +			count++;
>  
>  			/* Compute the checksum over this segment of data and
>  			 * add it to the csum already computed (over the L4
> @@ -848,7 +849,6 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  				goto err_map_data;
>  
>  			data_len -= size;
> -			count++;
>  			bd_data_num++;
>  			tso_build_data(skb, &tso, size);
>  
> @@ -874,13 +874,13 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  	dev_err(tx_ring->dev, "DMA map error");
>  
>  err_chained_bd:
> -	do {
> +	while (count--) {
>  		tx_swbd = &tx_ring->tx_swbd[i];
>  		enetc_free_tx_frame(tx_ring, tx_swbd);
>  		if (i == 0)
>  			i = tx_ring->bd_count;
>  		i--;
> -	} while (count--);
> +	}
>  
>  	return 0;
>  }

Thanks for fixing it.
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> -- 
> 2.34.1
> 

RE: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Wednesday, February 19, 2025 7:43 AM
[...]
> Subject: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in
> enetc_map_tx_tso_buffs()
> 
> There is an off-by-one issue for the err_chained_bd path, it will free
> one more tx_swbd than expected. But there is no such issue for the
> err_map_data path. To fix this off-by-one issue and make the two error
> handling consistent, the loop condition of error handling is modified
> and the 'count++' operation is moved before enetc_map_tx_tso_data().
> 
> Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 9a24d1176479..fe3967268a19 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -832,6 +832,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb
>  			txbd = ENETC_TXBD(*tx_ring, i);
>  			tx_swbd = &tx_ring->tx_swbd[i];
>  			prefetchw(txbd);
> +			count++;
> 
>  			/* Compute the checksum over this segment of data and
>  			 * add it to the csum already computed (over the L4
> @@ -848,7 +849,6 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb
>  				goto err_map_data;
> 
>  			data_len -= size;
> -			count++;

Hi Wei,

My issue is that:
enetc_map_tx_tso_hdr() not only updates the current tx_swbd (so 1 count++
needed), but in case of extension flag it advances 'tx_swbd' and 'i' with another
position so and extra 'count++' would be needed in that case.

Thanks,
-Claudiu

Re: [PATCH v2 net 8/9] net: enetc: correct the EMDIO base offset for ENETC v4

[Andrew Lunn]

On Wed, Feb 19, 2025 at 01:42:46PM +0800, Wei Fang wrote:
> In addition to centrally managing external PHYs through EMIDO device,
> each ENETC has a set of EMDIO registers to access and manage its own
> external PHY. When adding i.MX95 ENETC support, the EMDIO base offset
> was forgot to be updated, which will result in ENETC being unable to
> manage its external PHY through its own EMDIO registers.

So this never worked?

If it never worked, does it actually bother anybody?

Stable rules say:

  It must either fix a real bug that bothers people or just add a device ID.

	Andrew

RE: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Wei Fang]

> > -----Original Message-----
> > From: Wei Fang <wei.fang@nxp.com>
> > Sent: Wednesday, February 19, 2025 7:43 AM
> [...]
> > Subject: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in
> > enetc_map_tx_tso_buffs()
> >
> > There is an off-by-one issue for the err_chained_bd path, it will free
> > one more tx_swbd than expected. But there is no such issue for the
> > err_map_data path. To fix this off-by-one issue and make the two error
> > handling consistent, the loop condition of error handling is modified
> > and the 'count++' operation is moved before enetc_map_tx_tso_data().
> >
> > Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> > b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 9a24d1176479..fe3967268a19 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -832,6 +832,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> > *tx_ring, struct sk_buff *skb
> >  			txbd = ENETC_TXBD(*tx_ring, i);
> >  			tx_swbd = &tx_ring->tx_swbd[i];
> >  			prefetchw(txbd);
> > +			count++;
> >
> >  			/* Compute the checksum over this segment of data and
> >  			 * add it to the csum already computed (over the L4
> > @@ -848,7 +849,6 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> > *tx_ring, struct sk_buff *skb
> >  				goto err_map_data;
> >
> >  			data_len -= size;
> > -			count++;
> 
> Hi Wei,
> 
> My issue is that:
> enetc_map_tx_tso_hdr() not only updates the current tx_swbd (so 1 count++
> needed), but in case of extension flag it advances 'tx_swbd' and 'i' with
> another
> position so and extra 'count++' would be needed in that case.
> 

I think the patch 2 (net: enetc: correct the tx_swbd statistics) is to resolve
your issue.

RE: [PATCH v2 net 8/9] net: enetc: correct the EMDIO base offset for ENETC v4

[Wei Fang]

> On Wed, Feb 19, 2025 at 01:42:46PM +0800, Wei Fang wrote:
> > In addition to centrally managing external PHYs through EMIDO device,
> > each ENETC has a set of EMDIO registers to access and manage its own
> > external PHY. When adding i.MX95 ENETC support, the EMDIO base offset
> > was forgot to be updated, which will result in ENETC being unable to
> > manage its external PHY through its own EMDIO registers.
> 
> So this never worked?
> 
Yes, for i.MX95 we use EMDIO device to manage all external PHYs

> If it never worked, does it actually bother anybody?
> 
No, just fix it at the code level, the offset of ENETC v4 is not correct.
So I will remove it from this patch set and add it to net-next tree.
Thanks.

Re: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Ioana Ciornei]

On Wed, Feb 19, 2025 at 01:42:40PM +0800, Wei Fang wrote:
> When creating a TSO header, if the skb is VLAN tagged, the extended BD
> will be used and the 'count' should be increased by 2 instead of 1.
> Otherwise, when an error occurs, less tx_swbd will be freed than the
> actual number.
> 
> Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>

Re: [PATCH v2 net 3/9] net: enetc: correct the xdp_tx statistics

[Ioana Ciornei]

On Wed, Feb 19, 2025 at 01:42:41PM +0800, Wei Fang wrote:
> The 'xdp_tx' is used to count the number of XDP_TX frames sent, not the
> number of Tx BDs.
> 
> Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>

Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>

RE: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Thursday, February 20, 2025 7:07 AM
[...]
> Subject: RE: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in
> enetc_map_tx_tso_buffs()
> 
> > > -----Original Message-----
> > > From: Wei Fang <wei.fang@nxp.com>
> > > Sent: Wednesday, February 19, 2025 7:43 AM
> > [...]
> > > Subject: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in
> > > enetc_map_tx_tso_buffs()
> > >
> > > There is an off-by-one issue for the err_chained_bd path, it will
> > > free one more tx_swbd than expected. But there is no such issue for
> > > the err_map_data path. To fix this off-by-one issue and make the two
> > > error handling consistent, the loop condition of error handling is
> > > modified and the 'count++' operation is moved before
> enetc_map_tx_tso_data().
> > >
> > > Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Wei Fang <wei.fang@nxp.com>

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>

> > > ---
> > >  drivers/net/ethernet/freescale/enetc/enetc.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> > > b/drivers/net/ethernet/freescale/enetc/enetc.c
> > > index 9a24d1176479..fe3967268a19 100644
> > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > > @@ -832,6 +832,7 @@ static int enetc_map_tx_tso_buffs(struct
> > > enetc_bdr *tx_ring, struct sk_buff *skb
> > >  			txbd = ENETC_TXBD(*tx_ring, i);
> > >  			tx_swbd = &tx_ring->tx_swbd[i];
> > >  			prefetchw(txbd);
> > > +			count++;
> > >
> > >  			/* Compute the checksum over this segment of data
> and
> > >  			 * add it to the csum already computed (over the L4
> @@ -848,7
> > > +849,6 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> > > *tx_ring, struct sk_buff *skb
> > >  				goto err_map_data;
> > >
> > >  			data_len -= size;
> > > -			count++;
> >
> > Hi Wei,
> >
> > My issue is that:
> > enetc_map_tx_tso_hdr() not only updates the current tx_swbd (so 1
> > count++ needed), but in case of extension flag it advances 'tx_swbd'
> > and 'i' with another position so and extra 'count++' would be needed
> > in that case.
> >
> 
> I think the patch 2 (net: enetc: correct the tx_swbd statistics) is to resolve your
> issue.

Ok, I missed that. Thanks.

Re: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in enetc_map_tx_buffs()

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:39PM +0800, Wei Fang wrote:
> When a DMA mapping error occurs while processing skb frags, it will free
> one more tx_swbd than expected, so fix this off-by-one issue.
> 
> Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

After running with some test data, I agree that the bug exists and that
the fix is correct.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

It's just that there's yet one more (correct) dma_err snippet in
enetc_lso_hw_offload() which achieves the same thing, but expressed
differently, added by you in December 2024.

For fixing a bug from 2019, I agree that you've made the right choice in
not creating a dependency on that later code. But I like slightly less
the fact that it leaves 2 slightly different, both non-obvious, code
paths for unmapping DMA buffers. You could have at least copied the
dma_err handling from enetc_lso_hw_offload(), to make it obvious that
one is correct and the other is not, and not complicate things with yet
a 3rd implementation.

You don't need to change this unless there's some other reason to resend
the patch set, but at least, once "net" merges back into "net-next",
could you please make a mental note to consolidate the 2 code snippets
into a single function?

Also, the first dma_mapping_error() from enetc_map_tx_buffs() does not
need to "goto dma_err". It would be dead code. Maybe you could simplify
that as well. In general, the convention of naming error path labels is
to name them after what they do, rather than after the function that
failed when you jump to them. It's easier to manually verify correctness
this way.

Also, the dev_err(tx_ring->dev, "DMA map error"); message should be rate
limited, since it's called from a fast path and can kill the console if
the error is persistent.

A lot of things to improve still, thanks for doing this :)

Re: [PATCH v2 net 5/9] net: enetc: update UDP checksum when updating originTimestamp field

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:43PM +0800, Wei Fang wrote:
> There is an issue with one-step timestamp based on UDP/IP. The peer will
> discard the sync packet because of the wrong UDP checksum. For ENETC v1,
> the software needs to update the UDP checksum when updating the
> originTimestamp field, so that the hardware can correctly update the UDP
> checksum when updating the correction field. Otherwise, the UDP checksum
> in the sync packet will be wrong.
> 
> Fixes: 7294380c5211 ("enetc: support PTP Sync packet one-step timestamping")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Really good catch!

Re: [PATCH v2 net 8/9] net: enetc: correct the EMDIO base offset for ENETC v4

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:46PM +0800, Wei Fang wrote:
> In addition to centrally managing external PHYs through EMIDO device,
                                                          ~~~~~
                                                          EMDIO

> each ENETC has a set of EMDIO registers to access and manage its own
> external PHY. When adding i.MX95 ENETC support, the EMDIO base offset
> was forgot to be updated, which will result in ENETC being unable to
> manage its external PHY through its own EMDIO registers.
> 
> Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Though I have to agree with Andrew. This looks more like "new feature"
material for net-next, than fixing a bug in an already supported feature.

Re: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:40PM +0800, Wei Fang wrote:
> When creating a TSO header, if the skb is VLAN tagged, the extended BD
> will be used and the 'count' should be increased by 2 instead of 1.
> Otherwise, when an error occurs, less tx_swbd will be freed than the
> actual number.
> 
> Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---

I'm not sure "correct the statistics" is the best way to describe this
change. Maybe "keep track of correct TXBD count in enetc_map_tx_tso_buffs()"?
The bug is that not all TX buffers are freed on error, not that some
statistics are wrong.

>  drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 01c09fd26f9f..0658c06a23c1 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -759,6 +759,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>  static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
> +	bool ext_bd = skb_vlan_tag_present(skb);
>  	int hdr_len, total_len, data_len;
>  	struct enetc_tx_swbd *tx_swbd;
>  	union enetc_tx_bd *txbd;
> @@ -792,7 +793,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  		csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos);
>  		enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, data_len);
>  		bd_data_num = 0;
> -		count++;
> +		count += ext_bd ? 2 : 1;
>  
>  		while (data_len > 0) {
>  			int size;
> -- 
> 2.34.1
>

stylistic nitpick: I think this implementation choice obscures the fact,
to an unfamiliar reader, that the requirement for an extended TXBD comes
from enetc_map_tx_tso_hdr(). This is because you repeat the condition
for skb_vlan_tag_present(), but it's not obvious it's correlated to the
other one. Something like the change below is more expressive in this
regard, in my opinion:

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index fe3967268a19..6178157611db 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -410,14 +410,15 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 	return 0;
 }
 
-static void enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb,
-				 struct enetc_tx_swbd *tx_swbd,
-				 union enetc_tx_bd *txbd, int *i, int hdr_len,
-				 int data_len)
+static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb,
+				struct enetc_tx_swbd *tx_swbd,
+				union enetc_tx_bd *txbd, int *i, int hdr_len,
+				int data_len)
 {
 	union enetc_tx_bd txbd_tmp;
 	u8 flags = 0, e_flags = 0;
 	dma_addr_t addr;
+	int count = 1;
 
 	enetc_clear_tx_bd(&txbd_tmp);
 	addr = tx_ring->tso_headers_dma + *i * TSO_HEADER_SIZE;
@@ -460,7 +461,10 @@ static void enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 		/* Write the BD */
 		txbd_tmp.ext.e_flags = e_flags;
 		*txbd = txbd_tmp;
+		count++;
 	}
+
+	return count;
 }
 
 static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff *skb,
@@ -786,7 +790,6 @@ static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
-	bool ext_bd = skb_vlan_tag_present(skb);
 	int hdr_len, total_len, data_len;
 	struct enetc_tx_swbd *tx_swbd;
 	union enetc_tx_bd *txbd;
@@ -818,9 +821,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 
 		/* compute the csum over the L4 header */
 		csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos);
-		enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, data_len);
+		count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i,
+					      hdr_len, data_len);
 		bd_data_num = 0;
-		count += ext_bd ? 2 : 1;
 
 		while (data_len > 0) {
 			int size;

Re: [PATCH v2 net 7/9] net: enetc: remove the mm_lock from the ENETC v4 driver

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:45PM +0800, Wei Fang wrote:
> Currently, the ENETC v4 driver has not added the MAC merge layer support
> in the upstream, so the mm_lock is not initialized and used, so remove
> the mm_lock from the driver.
> 
> Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:47PM +0800, Wei Fang wrote:
> There is an off-by-one issue for the err_chained_bd path, it will free
> one more tx_swbd than expected. But there is no such issue for the
> err_map_data path.

It's clear that one of err_chained_bd or err_map_data is wrong, because
they operate with a different "count" but same "i". But how did you
determine which one is wrong? Is it based on static analysis? Because I
think the other one is wrong, more below.

> To fix this off-by-one issue and make the two error
> handling consistent, the loop condition of error handling is modified
> and the 'count++' operation is moved before enetc_map_tx_tso_data().
> 
> Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 9a24d1176479..fe3967268a19 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -832,6 +832,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  			txbd = ENETC_TXBD(*tx_ring, i);
>  			tx_swbd = &tx_ring->tx_swbd[i];
>  			prefetchw(txbd);
> +			count++;
>  
>  			/* Compute the checksum over this segment of data and
>  			 * add it to the csum already computed (over the L4
> @@ -848,7 +849,6 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  				goto err_map_data;
>  
>  			data_len -= size;
> -			count++;
>  			bd_data_num++;
>  			tso_build_data(skb, &tso, size);
>  
> @@ -874,13 +874,13 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  	dev_err(tx_ring->dev, "DMA map error");
>  
>  err_chained_bd:
> -	do {
> +	while (count--) {
>  		tx_swbd = &tx_ring->tx_swbd[i];
>  		enetc_free_tx_frame(tx_ring, tx_swbd);
>  		if (i == 0)
>  			i = tx_ring->bd_count;
>  		i--;
> -	} while (count--);
> +	}
>  
>  	return 0;
>  }

ah, there you go, here's the 3rd instance of TX DMA buffer unmapping :-/

Forget what I said in reply to patch 1/9 about having common code later.
After going through the whole set and now seeing this, I now think it's
better that you create the helper now, and consolidate the 2 instances
you touch anyway. Later you can make enetc_lso_hw_offload() reuse this
helper in net-next.

It should be something like this in the end (sorry, just 1 squashed diff):

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 6178157611db..a70e92dcbe2c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -106,6 +106,24 @@ static void enetc_free_tx_frame(struct enetc_bdr *tx_ring,
 	}
 }
 
+/**
+ * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer TX frame
+ * @tx_ring: Pointer to the TX ring on which the buffer descriptors are located
+ * @count: Number of TX buffer descriptors which need to be unmapped
+ * @i: Index of the last successfully mapped TX buffer descriptor
+ */
+static void enetc_unwind_tx_frame(struct enetc_bdr *tx_ring, int count, int i)
+{
+	while (count--) {
+		struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
+
+		enetc_free_tx_frame(tx_ring, tx_swbd);
+		if (i == 0)
+			i = tx_ring->bd_count;
+		i--;
+	}
+}
+
 /* Let H/W know BD ring has been updated */
 static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
 {
@@ -399,13 +417,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 dma_err:
 	dev_err(tx_ring->dev, "DMA map error");
 
-	while (count--) {
-		tx_swbd = &tx_ring->tx_swbd[i];
-		enetc_free_tx_frame(tx_ring, tx_swbd);
-		if (i == 0)
-			i = tx_ring->bd_count;
-		i--;
-	}
+	enetc_unwind_tx_frame(tx_ring, count, i);
 
 	return 0;
 }
@@ -752,7 +764,6 @@ static int enetc_lso_map_data(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 
 static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 {
-	struct enetc_tx_swbd *tx_swbd;
 	struct enetc_lso_t lso = {0};
 	int err, i, count = 0;
 
@@ -776,13 +787,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 	return count;
 
 dma_err:
-	do {
-		tx_swbd = &tx_ring->tx_swbd[i];
-		enetc_free_tx_frame(tx_ring, tx_swbd);
-		if (i == 0)
-			i = tx_ring->bd_count;
-		i--;
-	} while (--count);
+	enetc_unwind_tx_frame(tx_ring, count, i);
 
 	return 0;
 }
@@ -877,13 +882,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 	dev_err(tx_ring->dev, "DMA map error");
 
 err_chained_bd:
-	while (count--) {
-		tx_swbd = &tx_ring->tx_swbd[i];
-		enetc_free_tx_frame(tx_ring, tx_swbd);
-		if (i == 0)
-			i = tx_ring->bd_count;
-		i--;
-	}
+	enetc_unwind_tx_frame(tx_ring, count, i);
 
 	return 0;
 }

With the definitions laid out explicitly in a kernel-doc, doesn't the
rest of the patch look a bit wrong? Why would you increment "count"
before enetc_map_tx_tso_data() succeeds? Why isn't the problem that "i"
needs to be rolled back on enetc_map_tx_tso_data() failure instead?

Re: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Vladimir Oltean]

On Thu, Feb 20, 2025 at 06:32:26PM +0200, Vladimir Oltean wrote:
> +/**
> + * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer TX frame

Ok, maybe "multi-buffer TX frame" is not the best way of describing it,
because with software TSO it's actually multiple TX frames. Perhaps
"multiple TX BDs" is a better way of putting it, but we can argue about
semantics later.

Re: [PATCH v2 net 6/9] net: enetc: add missing enetc4_link_deinit()

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:44PM +0800, Wei Fang wrote:
> The enetc4_link_init() is called when the PF driver probes to create
> phylink and MDIO bus, but we forgot to call enetc4_link_deinit() to
> free the phylink and MDIO bus when the driver was unbound. so add
> missing enetc4_link_deinit() to enetc4_pf_netdev_destroy().
> 
> Fixes: 99100d0d9922 ("net: enetc: add preliminary support for i.MX95 ENETC PF")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v2 net 4/9] net: enetc: VFs do not support HWTSTAMP_TX_ONESTEP_SYNC

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:42PM +0800, Wei Fang wrote:
> Actually ENETC VFs do not support HWTSTAMP_TX_ONESTEP_SYNC because only
> ENETC PF can access PMa_SINGLE_STEP registers. And there will be a crash
> if VFs are used to test one-step timestamp, the crash log as follows.
> 
> [  129.110909] Unable to handle kernel paging request at virtual address 00000000000080c0
> [  129.287769] Call trace:
> [  129.290219]  enetc_port_mac_wr+0x30/0xec (P)
> [  129.294504]  enetc_start_xmit+0xda4/0xe74
> [  129.298525]  enetc_xmit+0x70/0xec
> [  129.301848]  dev_hard_start_xmit+0x98/0x118
> 
> Fixes: 41514737ecaa ("enetc: add get_ts_info interface for ethtool")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v2 net 3/9] net: enetc: correct the xdp_tx statistics

[Vladimir Oltean]

On Wed, Feb 19, 2025 at 01:42:41PM +0800, Wei Fang wrote:
> The 'xdp_tx' is used to count the number of XDP_TX frames sent, not the
> number of Tx BDs.
> 
> Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

RE: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in enetc_map_tx_buffs()

[Wei Fang]

> 
> After running with some test data, I agree that the bug exists and that
> the fix is correct.
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> It's just that there's yet one more (correct) dma_err snippet in
> enetc_lso_hw_offload() which achieves the same thing, but expressed
> differently, added by you in December 2024.
> 
> For fixing a bug from 2019, I agree that you've made the right choice in
> not creating a dependency on that later code. But I like slightly less
> the fact that it leaves 2 slightly different, both non-obvious, code
> paths for unmapping DMA buffers. You could have at least copied the
> dma_err handling from enetc_lso_hw_offload(), to make it obvious that
> one is correct and the other is not, and not complicate things with yet
> a 3rd implementation.
> 
> You don't need to change this unless there's some other reason to resend
> the patch set, but at least, once "net" merges back into "net-next",
> could you please make a mental note to consolidate the 2 code snippets
> into a single function?
> 
Yes, I plan to use a helper function to replace the same code blocks in net-next
tree.

> Also, the first dma_mapping_error() from enetc_map_tx_buffs() does not
> need to "goto dma_err". It would be dead code. Maybe you could simplify
> that as well. In general, the convention of naming error path labels is
> to name them after what they do, rather than after the function that
> failed when you jump to them. It's easier to manually verify correctness
> this way.
> 
> Also, the dev_err(tx_ring->dev, "DMA map error"); message should be rate
> limited, since it's called from a fast path and can kill the console if
> the error is persistent.
> 
> A lot of things to improve still, thanks for doing this :)

RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Wei Fang]

> I'm not sure "correct the statistics" is the best way to describe this
> change. Maybe "keep track of correct TXBD count in
> enetc_map_tx_tso_buffs()"?

Hi Vladimir,

Inspired by Michal, I think we don't need to keep the count variable, because
we already have index "i", we just need to record the value of the initial i at the
beginning. So I plan to do this optimization on the net-next tree in the future.
So I don't think it is necessary to modify enetc_map_tx_tso_hdr().

> The bug is that not all TX buffers are freed on error, not that some
> statistics are wrong.
> 
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 01c09fd26f9f..0658c06a23c1 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -759,6 +759,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr
> *tx_ring, struct sk_buff *skb)
> >  static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff
> *skb)
> >  {
> >  	struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
> > +	bool ext_bd = skb_vlan_tag_present(skb);
> >  	int hdr_len, total_len, data_len;
> >  	struct enetc_tx_swbd *tx_swbd;
> >  	union enetc_tx_bd *txbd;
> > @@ -792,7 +793,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb
> >  		csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos);
> >  		enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len,
> data_len);
> >  		bd_data_num = 0;
> > -		count++;
> > +		count += ext_bd ? 2 : 1;
> >
> >  		while (data_len > 0) {
> >  			int size;
> > --
> > 2.34.1
> >
> 
> stylistic nitpick: I think this implementation choice obscures the fact,
> to an unfamiliar reader, that the requirement for an extended TXBD comes
> from enetc_map_tx_tso_hdr(). This is because you repeat the condition
> for skb_vlan_tag_present(), but it's not obvious it's correlated to the
> other one. Something like the change below is more expressive in this
> regard, in my opinion:
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index fe3967268a19..6178157611db 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -410,14 +410,15 @@ static int enetc_map_tx_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb)
>  	return 0;
>  }
> 
> -static void enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff
> *skb,
> -				 struct enetc_tx_swbd *tx_swbd,
> -				 union enetc_tx_bd *txbd, int *i, int hdr_len,
> -				 int data_len)
> +static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff
> *skb,
> +				struct enetc_tx_swbd *tx_swbd,
> +				union enetc_tx_bd *txbd, int *i, int hdr_len,
> +				int data_len)
>  {
>  	union enetc_tx_bd txbd_tmp;
>  	u8 flags = 0, e_flags = 0;
>  	dma_addr_t addr;
> +	int count = 1;
> 
>  	enetc_clear_tx_bd(&txbd_tmp);
>  	addr = tx_ring->tso_headers_dma + *i * TSO_HEADER_SIZE;
> @@ -460,7 +461,10 @@ static void enetc_map_tx_tso_hdr(struct enetc_bdr
> *tx_ring, struct sk_buff *skb,
>  		/* Write the BD */
>  		txbd_tmp.ext.e_flags = e_flags;
>  		*txbd = txbd_tmp;
> +		count++;
>  	}
> +
> +	return count;
>  }
> 
>  static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff
> *skb,
> @@ -786,7 +790,6 @@ static int enetc_lso_hw_offload(struct enetc_bdr
> *tx_ring, struct sk_buff *skb)
>  static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff
> *skb)
>  {
>  	struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
> -	bool ext_bd = skb_vlan_tag_present(skb);
>  	int hdr_len, total_len, data_len;
>  	struct enetc_tx_swbd *tx_swbd;
>  	union enetc_tx_bd *txbd;
> @@ -818,9 +821,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb
> 
>  		/* compute the csum over the L4 header */
>  		csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos);
> -		enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len,
> data_len);
> +		count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i,
> +					      hdr_len, data_len);
>  		bd_data_num = 0;
> -		count += ext_bd ? 2 : 1;
> 
>  		while (data_len > 0) {
>  			int size;

RE: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Wei Fang]

> On Wed, Feb 19, 2025 at 01:42:47PM +0800, Wei Fang wrote:
> > There is an off-by-one issue for the err_chained_bd path, it will free
> > one more tx_swbd than expected. But there is no such issue for the
> > err_map_data path.
> 
> It's clear that one of err_chained_bd or err_map_data is wrong, because
> they operate with a different "count" but same "i". But how did you
> determine which one is wrong? Is it based on static analysis? Because I
> think the other one is wrong, more below.
> 
> > To fix this off-by-one issue and make the two error
> > handling consistent, the loop condition of error handling is modified
> > and the 'count++' operation is moved before enetc_map_tx_tso_data().
> >
> > Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 9a24d1176479..fe3967268a19 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -832,6 +832,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb
> >  			txbd = ENETC_TXBD(*tx_ring, i);
> >  			tx_swbd = &tx_ring->tx_swbd[i];
> >  			prefetchw(txbd);
> > +			count++;
> >
> >  			/* Compute the checksum over this segment of data and
> >  			 * add it to the csum already computed (over the L4
> > @@ -848,7 +849,6 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb
> >  				goto err_map_data;
> >
> >  			data_len -= size;
> > -			count++;
> >  			bd_data_num++;
> >  			tso_build_data(skb, &tso, size);
> >
> > @@ -874,13 +874,13 @@ static int enetc_map_tx_tso_buffs(struct
> enetc_bdr *tx_ring, struct sk_buff *skb
> >  	dev_err(tx_ring->dev, "DMA map error");
> >
> >  err_chained_bd:
> > -	do {
> > +	while (count--) {
> >  		tx_swbd = &tx_ring->tx_swbd[i];
> >  		enetc_free_tx_frame(tx_ring, tx_swbd);
> >  		if (i == 0)
> >  			i = tx_ring->bd_count;
> >  		i--;
> > -	} while (count--);
> > +	}
> >
> >  	return 0;
> >  }
> 
> ah, there you go, here's the 3rd instance of TX DMA buffer unmapping :-/
> 
> Forget what I said in reply to patch 1/9 about having common code later.
> After going through the whole set and now seeing this, I now think it's
> better that you create the helper now, and consolidate the 2 instances
> you touch anyway. Later you can make enetc_lso_hw_offload() reuse this
> helper in net-next.
> 
> It should be something like this in the end (sorry, just 1 squashed diff):

I agree with you that we finally need a helper function to replace all the
same code blocks, but I'd like to do that for net-next tree, because as I
replied in patch 2, we don't need the count variable. Currently I am more
focused on solving the problem itself rather than optimizing it. Of course
if you think this is necessary, I can add these changes in the next version. :)

> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 6178157611db..a70e92dcbe2c 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -106,6 +106,24 @@ static void enetc_free_tx_frame(struct enetc_bdr
> *tx_ring,
>  	}
>  }
> 
> +/**
> + * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer TX
> frame
> + * @tx_ring: Pointer to the TX ring on which the buffer descriptors are located
> + * @count: Number of TX buffer descriptors which need to be unmapped
> + * @i: Index of the last successfully mapped TX buffer descriptor
> + */
> +static void enetc_unwind_tx_frame(struct enetc_bdr *tx_ring, int count, int i)
> +{
> +	while (count--) {
> +		struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
> +
> +		enetc_free_tx_frame(tx_ring, tx_swbd);
> +		if (i == 0)
> +			i = tx_ring->bd_count;
> +		i--;
> +	}
> +}
> +
>  /* Let H/W know BD ring has been updated */
>  static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
>  {
> @@ -399,13 +417,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb)
>  dma_err:
>  	dev_err(tx_ring->dev, "DMA map error");
> 
> -	while (count--) {
> -		tx_swbd = &tx_ring->tx_swbd[i];
> -		enetc_free_tx_frame(tx_ring, tx_swbd);
> -		if (i == 0)
> -			i = tx_ring->bd_count;
> -		i--;
> -	}
> +	enetc_unwind_tx_frame(tx_ring, count, i);
> 
>  	return 0;
>  }
> @@ -752,7 +764,6 @@ static int enetc_lso_map_data(struct enetc_bdr
> *tx_ring, struct sk_buff *skb,
> 
>  static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>  {
> -	struct enetc_tx_swbd *tx_swbd;
>  	struct enetc_lso_t lso = {0};
>  	int err, i, count = 0;
> 
> @@ -776,13 +787,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr
> *tx_ring, struct sk_buff *skb)
>  	return count;
> 
>  dma_err:
> -	do {
> -		tx_swbd = &tx_ring->tx_swbd[i];
> -		enetc_free_tx_frame(tx_ring, tx_swbd);
> -		if (i == 0)
> -			i = tx_ring->bd_count;
> -		i--;
> -	} while (--count);
> +	enetc_unwind_tx_frame(tx_ring, count, i);
> 
>  	return 0;
>  }
> @@ -877,13 +882,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> *tx_ring, struct sk_buff *skb
>  	dev_err(tx_ring->dev, "DMA map error");
> 
>  err_chained_bd:
> -	while (count--) {
> -		tx_swbd = &tx_ring->tx_swbd[i];
> -		enetc_free_tx_frame(tx_ring, tx_swbd);
> -		if (i == 0)
> -			i = tx_ring->bd_count;
> -		i--;
> -	}
> +	enetc_unwind_tx_frame(tx_ring, count, i);
> 
>  	return 0;
>  }
> 
> With the definitions laid out explicitly in a kernel-doc, doesn't the
> rest of the patch look a bit wrong? Why would you increment "count"

Sorry, I don't understand what you mean " With the definitions laid ou
explicitly in a kernel-doc", which kernel-doc?

> before enetc_map_tx_tso_data() succeeds? Why isn't the problem that "i"
> needs to be rolled back on enetc_map_tx_tso_data() failure instead?

The root cause of this issue as you said is that "I" and "count" are not
synchronized. Either moving count++ before enetc_map_tx_tso_data()
or rolling back 'i' after enetc_map_tx_tso_data() fails should solve the
issue. There is no problem with the former, it just loops one more time,
and actually the loop does nothing.

RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, February 21, 2025 3:42 AM
> To: Vladimir Oltean <vladimir.oltean@nxp.com>
> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Ioana Ciornei <ioana.ciornei@nxp.com>; Y.B. Lu
> <yangbo.lu@nxp.com>; michal.swiatkowski@linux.intel.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev;
> stable@vger.kernel.org
> Subject: RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics
> 
> > I'm not sure "correct the statistics" is the best way to describe this
> > change. Maybe "keep track of correct TXBD count in
> > enetc_map_tx_tso_buffs()"?
> 
> Hi Vladimir,
> 
> Inspired by Michal, I think we don't need to keep the count variable, because
> we already have index "i", we just need to record the value of the initial i at the
> beginning. So I plan to do this optimization on the net-next tree in the future.
> So I don't think it is necessary to modify enetc_map_tx_tso_hdr().
> 

And what if 'i' wraps around at least one time and becomes greater than the 
initial 'i'? Instead of 'count' you would have to record the number of wraps.
Even if not possible now in specific cases, there should be no limitation on
whether 'i' can wrap around in the loop or not (i.e. maybe some users want to
try very small Tx rings etc.)

RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Wei Fang]

> > > I'm not sure "correct the statistics" is the best way to describe this
> > > change. Maybe "keep track of correct TXBD count in
> > > enetc_map_tx_tso_buffs()"?
> >
> > Hi Vladimir,
> >
> > Inspired by Michal, I think we don't need to keep the count variable, because
> > we already have index "i", we just need to record the value of the initial i at
> the
> > beginning. So I plan to do this optimization on the net-next tree in the future.
> > So I don't think it is necessary to modify enetc_map_tx_tso_hdr().
> >
> 
> And what if 'i' wraps around at least one time and becomes greater than the
> initial 'i'? Instead of 'count' you would have to record the number of wraps.

I think this situation will not happen, because when calling
enetc_map_tx_tso_buffs()/enetc_map_tx_buffs()/enetc_lso_hw_offload(),
we always check whether the current free BDs are enough. The number of
free BDs is always <= bdr->bd_count, in the case you mentioned, the frame
will occupy more BDs than bdr->bd_count.

> Even if not possible now in specific cases, there should be no limitation on
> whether 'i' can wrap around in the loop or not (i.e. maybe some users want to
> try very small Tx rings etc.)

Re: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Vladimir Oltean]

On Fri, Feb 21, 2025 at 03:42:05AM +0200, Wei Fang wrote:
> > I'm not sure "correct the statistics" is the best way to describe this
> > change. Maybe "keep track of correct TXBD count in
> > enetc_map_tx_tso_buffs()"?
> 
> Hi Vladimir,
> 
> Inspired by Michal, I think we don't need to keep the count variable, because
> we already have index "i", we just need to record the value of the initial i at the
> beginning. So I plan to do this optimization on the net-next tree in the future.
> So I don't think it is necessary to modify enetc_map_tx_tso_hdr().

You are saying this in reply to my observation that the title of the
change does not correctly represent the change. But I don't see how what
you're saying is connected to that. Currently there exists a "count"
variable based on which TX BDs are unmapped, and these patches are not
changing that fact.

> > stylistic nitpick: I think this implementation choice obscures the fact,
> > to an unfamiliar reader, that the requirement for an extended TXBD comes
> > from enetc_map_tx_tso_hdr(). This is because you repeat the condition
> > for skb_vlan_tag_present(), but it's not obvious it's correlated to the
> > other one. Something like the change below is more expressive in this
> > regard, in my opinion:

It seems you were objecting to this other change suggestion instead.
Sure, I mean, ignore it if you want, but you're saying "well I'm going
to change the scheme for net-next, so no point in making the code more
obviously correct in stable branches", but the stable branches aren't
going to pick up the net-next patch - they are essentially frozen except
for bug fixes. I would still recommend writing code that makes the most
sense for stable (to the extent that this is logically part of fixing a
bug), and then writing code that makes most sense for net-next, even if
it implies changing some of it back the way it was.

RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, February 21, 2025 10:34 AM
[...]
> Subject: RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics
> 
> > > > I'm not sure "correct the statistics" is the best way to describe this
> > > > change. Maybe "keep track of correct TXBD count in
> > > > enetc_map_tx_tso_buffs()"?
> > >
> > > Hi Vladimir,
> > >
> > > Inspired by Michal, I think we don't need to keep the count variable,
> because
> > > we already have index "i", we just need to record the value of the initial i at
> > the
> > > beginning. So I plan to do this optimization on the net-next tree in the
> future.
> > > So I don't think it is necessary to modify enetc_map_tx_tso_hdr().
> > >
> >
> > And what if 'i' wraps around at least one time and becomes greater than the
> > initial 'i'? Instead of 'count' you would have to record the number of wraps.
> 
> I think this situation will not happen, because when calling
> enetc_map_tx_tso_buffs()/enetc_map_tx_buffs()/enetc_lso_hw_offload(),
> we always check whether the current free BDs are enough. The number of
> free BDs is always <= bdr->bd_count, in the case you mentioned, the frame
> will occupy more BDs than bdr->bd_count.
> 

Ok, let's see the net-next patches and discuss then.

Re: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Vladimir Oltean]

On Fri, Feb 21, 2025 at 04:56:03AM +0200, Wei Fang wrote:
> I agree with you that we finally need a helper function to replace all the
> same code blocks, but I'd like to do that for net-next tree, because as I
> replied in patch 2, we don't need the count variable. Currently I am more
> focused on solving the problem itself rather than optimizing it. Of course
> if you think this is necessary, I can add these changes in the next version. :)

Does it cost anything extra to centralize the logic that these patches
are _already_ touching into a single function? My "unsquashed" diff
below centralizes all occurrences of that logic, but you don't need to
centralize for "net" the occurences that the bug fix patches aren't touching.

> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> > b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 6178157611db..a70e92dcbe2c 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -106,6 +106,24 @@ static void enetc_free_tx_frame(struct enetc_bdr
> > *tx_ring,
> >  	}
> >  }
> > 
> > +/**
> > + * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer TX
> > frame
> > + * @tx_ring: Pointer to the TX ring on which the buffer descriptors are located
> > + * @count: Number of TX buffer descriptors which need to be unmapped
> > + * @i: Index of the last successfully mapped TX buffer descriptor
> > + */
> > +static void enetc_unwind_tx_frame(struct enetc_bdr *tx_ring, int count, int i)
> > +{
> > +	while (count--) {
> > +		struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
> > +
> > +		enetc_free_tx_frame(tx_ring, tx_swbd);
> > +		if (i == 0)
> > +			i = tx_ring->bd_count;
> > +		i--;
> > +	}
> > +}
> > +
> >  /* Let H/W know BD ring has been updated */
> >  static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
> >  {
> > @@ -399,13 +417,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr
> > *tx_ring, struct sk_buff *skb)
> >  dma_err:
> >  	dev_err(tx_ring->dev, "DMA map error");
> > 
> > -	while (count--) {
> > -		tx_swbd = &tx_ring->tx_swbd[i];
> > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > -		if (i == 0)
> > -			i = tx_ring->bd_count;
> > -		i--;
> > -	}
> > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > 
> >  	return 0;
> >  }
> > @@ -752,7 +764,6 @@ static int enetc_lso_map_data(struct enetc_bdr
> > *tx_ring, struct sk_buff *skb,
> > 
> >  static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb)
> >  {
> > -	struct enetc_tx_swbd *tx_swbd;
> >  	struct enetc_lso_t lso = {0};
> >  	int err, i, count = 0;
> > 
> > @@ -776,13 +787,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr
> > *tx_ring, struct sk_buff *skb)
> >  	return count;
> > 
> >  dma_err:
> > -	do {
> > -		tx_swbd = &tx_ring->tx_swbd[i];
> > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > -		if (i == 0)
> > -			i = tx_ring->bd_count;
> > -		i--;
> > -	} while (--count);
> > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > 
> >  	return 0;
> >  }
> > @@ -877,13 +882,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr
> > *tx_ring, struct sk_buff *skb
> >  	dev_err(tx_ring->dev, "DMA map error");
> > 
> >  err_chained_bd:
> > -	while (count--) {
> > -		tx_swbd = &tx_ring->tx_swbd[i];
> > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > -		if (i == 0)
> > -			i = tx_ring->bd_count;
> > -		i--;
> > -	}
> > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > 
> >  	return 0;
> >  }
> > 
> > With the definitions laid out explicitly in a kernel-doc, doesn't the
> > rest of the patch look a bit wrong? Why would you increment "count"
> 
> Sorry, I don't understand what you mean " With the definitions laid ou
> explicitly in a kernel-doc", which kernel-doc?

This kernel-doc:

/**
 * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer TX frame
 * @count: Number of TX buffer descriptors which need to be unmapped
 * @i: Index of the last successfully mapped TX buffer descriptor

The definitions of "count" and "i" are what I'm talking about. It's
clear to me that the "i" that is passed is not the index of the last
successfully mapped TX BD.

> > before enetc_map_tx_tso_data() succeeds? Why isn't the problem that "i"
> > needs to be rolled back on enetc_map_tx_tso_data() failure instead?
> 
> The root cause of this issue as you said is that "I" and "count" are not
> synchronized. Either moving count++ before enetc_map_tx_tso_data()
> or rolling back 'i' after enetc_map_tx_tso_data() fails should solve the
> issue. There is no problem with the former, it just loops one more time,
> and actually the loop does nothing.

Sorry, I don't understand "there is no problem, it just loops one more
time, actually the loop does nothing"? What do you mean, could you
explain more? Why wouldn't it be a problem, if the loop runs one more
time than TX BDs were added to the ring, that we try to unmap the DMA
buffer of a TXBD that was previously passed to hardware as part of a
previous enetc_update_tx_ring_tail()?

RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics

[Wei Fang]

> On Fri, Feb 21, 2025 at 03:42:05AM +0200, Wei Fang wrote:
> > > I'm not sure "correct the statistics" is the best way to describe this
> > > change. Maybe "keep track of correct TXBD count in
> > > enetc_map_tx_tso_buffs()"?
> >
> > Hi Vladimir,
> >
> > Inspired by Michal, I think we don't need to keep the count variable, because
> > we already have index "i", we just need to record the value of the initial i at
> the
> > beginning. So I plan to do this optimization on the net-next tree in the future.
> > So I don't think it is necessary to modify enetc_map_tx_tso_hdr().
> 
> You are saying this in reply to my observation that the title of the
> change does not correctly represent the change. But I don't see how what
> you're saying is connected to that. Currently there exists a "count"
> variable based on which TX BDs are unmapped, and these patches are not
> changing that fact.
> 
> > > stylistic nitpick: I think this implementation choice obscures the fact,
> > > to an unfamiliar reader, that the requirement for an extended TXBD comes
> > > from enetc_map_tx_tso_hdr(). This is because you repeat the condition
> > > for skb_vlan_tag_present(), but it's not obvious it's correlated to the
> > > other one. Something like the change below is more expressive in this
> > > regard, in my opinion:
> 
> It seems you were objecting to this other change suggestion instead.
> Sure, I mean, ignore it if you want, but you're saying "well I'm going
> to change the scheme for net-next, so no point in making the code more
> obviously correct in stable branches", but the stable branches aren't
> going to pick up the net-next patch - they are essentially frozen except
> for bug fixes. I would still recommend writing code that makes the most
> sense for stable (to the extent that this is logically part of fixing a
> bug), and then writing code that makes most sense for net-next, even if
> it implies changing some of it back the way it was.

Okay, agree with you, I will improve the patch.

RE: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Wei Fang]

> > > +/**
> > > + * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer
> TX
> > > frame
> > > + * @tx_ring: Pointer to the TX ring on which the buffer descriptors are
> located
> > > + * @count: Number of TX buffer descriptors which need to be unmapped
> > > + * @i: Index of the last successfully mapped TX buffer descriptor
> > > + */
> > > +static void enetc_unwind_tx_frame(struct enetc_bdr *tx_ring, int count,
> int i)
> > > +{
> > > +	while (count--) {
> > > +		struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
> > > +
> > > +		enetc_free_tx_frame(tx_ring, tx_swbd);
> > > +		if (i == 0)
> > > +			i = tx_ring->bd_count;
> > > +		i--;
> > > +	}
> > > +}
> > > +
> > >  /* Let H/W know BD ring has been updated */
> > >  static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
> > >  {
> > > @@ -399,13 +417,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr
> > > *tx_ring, struct sk_buff *skb)
> > >  dma_err:
> > >  	dev_err(tx_ring->dev, "DMA map error");
> > >
> > > -	while (count--) {
> > > -		tx_swbd = &tx_ring->tx_swbd[i];
> > > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > > -		if (i == 0)
> > > -			i = tx_ring->bd_count;
> > > -		i--;
> > > -	}
> > > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > >
> > >  	return 0;
> > >  }
> > > @@ -752,7 +764,6 @@ static int enetc_lso_map_data(struct enetc_bdr
> > > *tx_ring, struct sk_buff *skb,
> > >
> > >  static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff
> *skb)
> > >  {
> > > -	struct enetc_tx_swbd *tx_swbd;
> > >  	struct enetc_lso_t lso = {0};
> > >  	int err, i, count = 0;
> > >
> > > @@ -776,13 +787,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr
> > > *tx_ring, struct sk_buff *skb)
> > >  	return count;
> > >
> > >  dma_err:
> > > -	do {
> > > -		tx_swbd = &tx_ring->tx_swbd[i];
> > > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > > -		if (i == 0)
> > > -			i = tx_ring->bd_count;
> > > -		i--;
> > > -	} while (--count);
> > > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > >
> > >  	return 0;
> > >  }
> > > @@ -877,13 +882,7 @@ static int enetc_map_tx_tso_buffs(struct
> enetc_bdr
> > > *tx_ring, struct sk_buff *skb
> > >  	dev_err(tx_ring->dev, "DMA map error");
> > >
> > >  err_chained_bd:
> > > -	while (count--) {
> > > -		tx_swbd = &tx_ring->tx_swbd[i];
> > > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > > -		if (i == 0)
> > > -			i = tx_ring->bd_count;
> > > -		i--;
> > > -	}
> > > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > >
> > >  	return 0;
> > >  }
> > >
> > > With the definitions laid out explicitly in a kernel-doc, doesn't the
> > > rest of the patch look a bit wrong? Why would you increment "count"
> >
> > Sorry, I don't understand what you mean " With the definitions laid ou
> > explicitly in a kernel-doc", which kernel-doc?
> 
> This kernel-doc:
> 
> /**
>  * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer TX
> frame
>  * @count: Number of TX buffer descriptors which need to be unmapped
>  * @i: Index of the last successfully mapped TX buffer descriptor
> 
> The definitions of "count" and "i" are what I'm talking about. It's
> clear to me that the "i" that is passed is not the index of the last
> successfully mapped TX BD.
> 
> > > before enetc_map_tx_tso_data() succeeds? Why isn't the problem that "i"
> > > needs to be rolled back on enetc_map_tx_tso_data() failure instead?
> >
> > The root cause of this issue as you said is that "I" and "count" are not
> > synchronized. Either moving count++ before enetc_map_tx_tso_data()
> > or rolling back 'i' after enetc_map_tx_tso_data() fails should solve the
> > issue. There is no problem with the former, it just loops one more time,
> > and actually the loop does nothing.
> 
> Sorry, I don't understand "there is no problem, it just loops one more
> time, actually the loop does nothing"? What do you mean, could you
> explain more? Why wouldn't it be a problem, if the loop runs one more
> time than TX BDs were added to the ring, that we try to unmap the DMA
> buffer of a TXBD that was previously passed to hardware as part of a
> previous enetc_update_tx_ring_tail()?

Based on your definitions of 'i' and 'count', you are right to say that it's
necessary to roll back 'i' because the last 'i' did not successfully map the
Tx BD.

Regarding to I said moving 'count++' to before enetc_map_tx_tso_data() is
fine because the increment of 'i' and 'count' remain in sync, so 'i' will not
roll back to a value less than the initial value when err_map_data occurs.
So it does not affect the DMA buffer of TXBD which was previously passed
to the hardware as part of the previous enetc_update_tx_ring_tail().