[PATCH net v8 0/4] Fix race conditions in ndo_get_stats64

[PATCH net v8 0/4] Fix race conditions in ndo_get_stats64

[Shinas Rasheed]

Fix race conditions in ndo_get_stats64 by storing tx/rx stats
locally and not availing per queue resources which could be torn
down during interface stop. Also remove stats fetch from
firmware which is currently unnecessary

Changes:
V8:
  - Reordered patches

V7: https://lore.kernel.org/all/20250114125124.2570660-1-srasheed@marvell.com/
  - Updated octep_get_stats64() to be reentrant

V6: https://lore.kernel.org/all/20250110122730.2551863-1-srasheed@marvell.com/
  - Corrected patch 2/4 which was not applying properly

V5: https://lore.kernel.org/all/20250109103221.2544467-1-srasheed@marvell.com/
  - Store tx/rx stats locally and avail use stats in ndo_get_stats64()
    instead of availing per queue resources there.

V4: https://lore.kernel.org/all/20250102112246.2494230-1-srasheed@marvell.com/
  - Check if netdev is running, as decision for accessing resources
    rather than availing lock implementations, in ndo_get_stats64()

V3: https://lore.kernel.org/all/20241218115111.2407958-1-srasheed@marvell.com/
  - Added warn log that happened due to rcu_read_lock in commit message

V2: https://lore.kernel.org/all/20241216075842.2394606-1-srasheed@marvell.com/
  - Changed sync mechanism to fix race conditions from using an atomic
    set_bit ops to a much simpler synchronize_net()

V1: https://lore.kernel.org/all/20241203072130.2316913-1-srasheed@marvell.com/

Shinas Rasheed (4):
  octeon_ep: remove firmware stats fetch in ndo_get_stats64
  octeon_ep: update tx/rx stats locally for persistence
  octeon_ep_vf: remove firmware stats fetch in ndo_get_stats64
  octeon_ep_vf: update tx/rx stats locally for persistence

 .../marvell/octeon_ep/octep_ethtool.c         | 41 ++++++++-----------
 .../ethernet/marvell/octeon_ep/octep_main.c   | 29 ++++---------
 .../ethernet/marvell/octeon_ep/octep_main.h   | 11 +++++
 .../net/ethernet/marvell/octeon_ep/octep_rx.c | 12 +++---
 .../net/ethernet/marvell/octeon_ep/octep_rx.h |  4 +-
 .../net/ethernet/marvell/octeon_ep/octep_tx.c |  7 ++--
 .../net/ethernet/marvell/octeon_ep/octep_tx.h |  4 +-
 .../marvell/octeon_ep_vf/octep_vf_ethtool.c   | 29 +++++--------
 .../marvell/octeon_ep_vf/octep_vf_main.c      | 25 ++++-------
 .../marvell/octeon_ep_vf/octep_vf_main.h      | 11 +++++
 .../marvell/octeon_ep_vf/octep_vf_rx.c        | 10 +++--
 .../marvell/octeon_ep_vf/octep_vf_rx.h        |  2 +-
 .../marvell/octeon_ep_vf/octep_vf_tx.c        |  7 ++--
 .../marvell/octeon_ep_vf/octep_vf_tx.h        |  2 +-
 14 files changed, 92 insertions(+), 102 deletions(-)

-- 
2.25.1

[PATCH net v8 1/4] octeon_ep: remove firmware stats fetch in ndo_get_stats64

[Shinas Rasheed]

The per queue stats are available already and are retrieved
from register reads during ndo_get_stats64. The firmware stats
fetch call that happens in ndo_get_stats64() is currently not
required

The warn log is given below:

[  123.316837] ------------[ cut here ]------------
[  123.316840] Voluntary context switch within RCU read-side critical section!
[  123.316917] pc : rcu_note_context_switch+0x2e4/0x300
[  123.316919] lr : rcu_note_context_switch+0x2e4/0x300
[  123.316947] Call trace:
[  123.316949]  rcu_note_context_switch+0x2e4/0x300
[  123.316952]  __schedule+0x84/0x584
[  123.316955]  schedule+0x38/0x90
[  123.316956]  schedule_timeout+0xa0/0x1d4
[  123.316959]  octep_send_mbox_req+0x190/0x230 [octeon_ep]
[  123.316966]  octep_ctrl_net_get_if_stats+0x78/0x100 [octeon_ep]
[  123.316970]  octep_get_stats64+0xd4/0xf0 [octeon_ep]
[  123.316975]  dev_get_stats+0x4c/0x114
[  123.316977]  dev_seq_printf_stats+0x3c/0x11c
[  123.316980]  dev_seq_show+0x1c/0x40
[  123.316982]  seq_read_iter+0x3cc/0x4e0
[  123.316985]  seq_read+0xc8/0x110
[  123.316987]  proc_reg_read+0x9c/0xec
[  123.316990]  vfs_read+0xc8/0x2ec
[  123.316993]  ksys_read+0x70/0x100
[  123.316995]  __arm64_sys_read+0x20/0x30
[  123.316997]  invoke_syscall.constprop.0+0x7c/0xd0
[  123.317000]  do_el0_svc+0xb4/0xd0
[  123.317002]  el0_svc+0xe8/0x1f4
[  123.317005]  el0t_64_sync_handler+0x134/0x150
[  123.317006]  el0t_64_sync+0x17c/0x180
[  123.317008] ---[ end trace 63399811432ab69b ]---

Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V8:
  - Reordered patch

V7: https://lore.kernel.org/all/20250114125124.2570660-3-srasheed@marvell.com/
  - No changes

V6: https://lore.kernel.org/all/20250110122730.2551863-3-srasheed@marvell.com/
  - Corrected patch to apply properly

V5: https://lore.kernel.org/all/20250109103221.2544467-3-srasheed@marvell.com/
  - No changes

V4: https://lore.kernel.org/all/20250102112246.2494230-3-srasheed@marvell.com/
  - No changes

V3: https://lore.kernel.org/all/20241218115111.2407958-3-srasheed@marvell.com/
  - Added warn log that happened due to rcu_read_lock in commit message

V2: https://lore.kernel.org/all/20241216075842.2394606-3-srasheed@marvell.com/
  - No changes

V1: https://lore.kernel.org/all/20241203072130.2316913-3-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 549436efc204..730aa5632cce 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -995,12 +995,6 @@ static void octep_get_stats64(struct net_device *netdev,
 	struct octep_device *oct = netdev_priv(netdev);
 	int q;
 
-	if (netif_running(netdev))
-		octep_ctrl_net_get_if_stats(oct,
-					    OCTEP_CTRL_NET_INVALID_VFID,
-					    &oct->iface_rx_stats,
-					    &oct->iface_tx_stats);
-
 	tx_packets = 0;
 	tx_bytes = 0;
 	rx_packets = 0;
@@ -1018,10 +1012,6 @@ static void octep_get_stats64(struct net_device *netdev,
 	stats->tx_bytes = tx_bytes;
 	stats->rx_packets = rx_packets;
 	stats->rx_bytes = rx_bytes;
-	stats->multicast = oct->iface_rx_stats.mcast_pkts;
-	stats->rx_errors = oct->iface_rx_stats.err_pkts;
-	stats->collisions = oct->iface_tx_stats.xscol;
-	stats->tx_fifo_errors = oct->iface_tx_stats.undflw;
 }
 
 /**
-- 
2.25.1

[PATCH net v8 2/4] octeon_ep: update tx/rx stats locally for persistence

[Shinas Rasheed]

Update tx/rx stats locally, so that ndo_get_stats64()
can use that and not rely on per queue resources to obtain statistics.
The latter used to cause race conditions when the device stopped.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V8:
  - Reordered patch

V7: https://lore.kernel.org/all/20250114125124.2570660-2-srasheed@marvell.com/
  - Updated octep_get_stats64() to be reentrant

V6: https://lore.kernel.org/all/20250110122730.2551863-2-srasheed@marvell.com/
  - No changes

V5: https://lore.kernel.org/all/20250109103221.2544467-2-srasheed@marvell.com/
  - Patch introduced

 .../marvell/octeon_ep/octep_ethtool.c         | 41 ++++++++-----------
 .../ethernet/marvell/octeon_ep/octep_main.c   | 19 ++++-----
 .../ethernet/marvell/octeon_ep/octep_main.h   | 11 +++++
 .../net/ethernet/marvell/octeon_ep/octep_rx.c | 12 +++---
 .../net/ethernet/marvell/octeon_ep/octep_rx.h |  4 +-
 .../net/ethernet/marvell/octeon_ep/octep_tx.c |  7 ++--
 .../net/ethernet/marvell/octeon_ep/octep_tx.h |  4 +-
 7 files changed, 51 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ethtool.c b/drivers/net/ethernet/marvell/octeon_ep/octep_ethtool.c
index 4f4d58189118..79d66426c1da 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_ethtool.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ethtool.c
@@ -150,17 +150,14 @@ octep_get_ethtool_stats(struct net_device *netdev,
 				    iface_rx_stats,
 				    iface_tx_stats);
 
-	for (q = 0; q < oct->num_oqs; q++) {
-		struct octep_iq *iq = oct->iq[q];
-		struct octep_oq *oq = oct->oq[q];
-
-		tx_packets += iq->stats.instr_completed;
-		tx_bytes += iq->stats.bytes_sent;
-		tx_busy_errors += iq->stats.tx_busy;
-
-		rx_packets += oq->stats.packets;
-		rx_bytes += oq->stats.bytes;
-		rx_alloc_errors += oq->stats.alloc_failures;
+	for (q = 0; q < oct->num_ioq_stats; q++) {
+		tx_packets += oct->stats_iq[q].instr_completed;
+		tx_bytes += oct->stats_iq[q].bytes_sent;
+		tx_busy_errors += oct->stats_iq[q].tx_busy;
+
+		rx_packets += oct->stats_oq[q].packets;
+		rx_bytes += oct->stats_oq[q].bytes;
+		rx_alloc_errors += oct->stats_oq[q].alloc_failures;
 	}
 	i = 0;
 	data[i++] = rx_packets;
@@ -198,22 +195,18 @@ octep_get_ethtool_stats(struct net_device *netdev,
 	data[i++] = iface_rx_stats->err_pkts;
 
 	/* Per Tx Queue stats */
-	for (q = 0; q < oct->num_iqs; q++) {
-		struct octep_iq *iq = oct->iq[q];
-
-		data[i++] = iq->stats.instr_posted;
-		data[i++] = iq->stats.instr_completed;
-		data[i++] = iq->stats.bytes_sent;
-		data[i++] = iq->stats.tx_busy;
+	for (q = 0; q < oct->num_ioq_stats; q++) {
+		data[i++] = oct->stats_iq[q].instr_posted;
+		data[i++] = oct->stats_iq[q].instr_completed;
+		data[i++] = oct->stats_iq[q].bytes_sent;
+		data[i++] = oct->stats_iq[q].tx_busy;
 	}
 
 	/* Per Rx Queue stats */
-	for (q = 0; q < oct->num_oqs; q++) {
-		struct octep_oq *oq = oct->oq[q];
-
-		data[i++] = oq->stats.packets;
-		data[i++] = oq->stats.bytes;
-		data[i++] = oq->stats.alloc_failures;
+	for (q = 0; q < oct->num_ioq_stats; q++) {
+		data[i++] = oct->stats_oq[q].packets;
+		data[i++] = oct->stats_oq[q].bytes;
+		data[i++] = oct->stats_oq[q].alloc_failures;
 	}
 }
 
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 730aa5632cce..133694a1658d 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -822,7 +822,7 @@ static inline int octep_iq_full_check(struct octep_iq *iq)
 	if (unlikely(IQ_INSTR_SPACE(iq) >
 		     OCTEP_WAKE_QUEUE_THRESHOLD)) {
 		netif_start_subqueue(iq->netdev, iq->q_no);
-		iq->stats.restart_cnt++;
+		iq->stats->restart_cnt++;
 		return 0;
 	}
 
@@ -960,7 +960,7 @@ static netdev_tx_t octep_start_xmit(struct sk_buff *skb,
 	wmb();
 	/* Ring Doorbell to notify the NIC of new packets */
 	writel(iq->fill_cnt, iq->doorbell_reg);
-	iq->stats.instr_posted += iq->fill_cnt;
+	iq->stats->instr_posted += iq->fill_cnt;
 	iq->fill_cnt = 0;
 	return NETDEV_TX_OK;
 
@@ -991,22 +991,19 @@ static netdev_tx_t octep_start_xmit(struct sk_buff *skb,
 static void octep_get_stats64(struct net_device *netdev,
 			      struct rtnl_link_stats64 *stats)
 {
-	u64 tx_packets, tx_bytes, rx_packets, rx_bytes;
 	struct octep_device *oct = netdev_priv(netdev);
+	u64 tx_packets, tx_bytes, rx_packets, rx_bytes;
 	int q;
 
 	tx_packets = 0;
 	tx_bytes = 0;
 	rx_packets = 0;
 	rx_bytes = 0;
-	for (q = 0; q < oct->num_oqs; q++) {
-		struct octep_iq *iq = oct->iq[q];
-		struct octep_oq *oq = oct->oq[q];
-
-		tx_packets += iq->stats.instr_completed;
-		tx_bytes += iq->stats.bytes_sent;
-		rx_packets += oq->stats.packets;
-		rx_bytes += oq->stats.bytes;
+	for (q = 0; q < oct->num_ioq_stats; q++) {
+		tx_packets += oct->stats_iq[q].instr_completed;
+		tx_bytes += oct->stats_iq[q].bytes_sent;
+		rx_packets += oct->stats_oq[q].packets;
+		rx_bytes += oct->stats_oq[q].bytes;
 	}
 	stats->tx_packets = tx_packets;
 	stats->tx_bytes = tx_bytes;
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
index fee59e0e0138..231ff863310b 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
@@ -257,11 +257,22 @@ struct octep_device {
 	/* Pointers to Octeon Tx queues */
 	struct octep_iq *iq[OCTEP_MAX_IQ];
 
+	/* Per iq stats */
+	struct octep_iq_stats stats_iq[OCTEP_MAX_IQ];
+
 	/* Rx queues (OQ: Output Queue) */
 	u16 num_oqs;
 	/* Pointers to Octeon Rx queues */
 	struct octep_oq *oq[OCTEP_MAX_OQ];
 
+	/* Number oq stats preserved
+	 * This number would remain constant when device goes down
+	 * This will be updated when device comes back up
+	 */
+	u16 num_ioq_stats;
+	/* Per oq stats */
+	struct octep_oq_stats stats_oq[OCTEP_MAX_OQ];
+
 	/* Hardware port number of the PCIe interface */
 	u16 pcie_port;
 
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
index 8af75cb37c3e..a7337f391a21 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
@@ -87,7 +87,7 @@ static int octep_oq_refill(struct octep_device *oct, struct octep_oq *oq)
 		page = dev_alloc_page();
 		if (unlikely(!page)) {
 			dev_err(oq->dev, "refill: rx buffer alloc failed\n");
-			oq->stats.alloc_failures++;
+			oq->stats->alloc_failures++;
 			break;
 		}
 
@@ -98,7 +98,7 @@ static int octep_oq_refill(struct octep_device *oct, struct octep_oq *oq)
 				"OQ-%d buffer refill: DMA mapping error!\n",
 				oq->q_no);
 			put_page(page);
-			oq->stats.alloc_failures++;
+			oq->stats->alloc_failures++;
 			break;
 		}
 		oq->buff_info[refill_idx].page = page;
@@ -134,6 +134,7 @@ static int octep_setup_oq(struct octep_device *oct, int q_no)
 	oq->netdev = oct->netdev;
 	oq->dev = &oct->pdev->dev;
 	oq->q_no = q_no;
+	oq->stats = &oct->stats_oq[q_no];
 	oq->max_count = CFG_GET_OQ_NUM_DESC(oct->conf);
 	oq->ring_size_mask = oq->max_count - 1;
 	oq->buffer_size = CFG_GET_OQ_BUF_SIZE(oct->conf);
@@ -262,6 +263,7 @@ int octep_setup_oqs(struct octep_device *oct)
 		dev_dbg(&oct->pdev->dev, "Successfully setup OQ(RxQ)-%d.\n", i);
 	}
 
+	oct->num_ioq_stats = oct->num_oqs;
 	return 0;
 
 oq_setup_err:
@@ -443,7 +445,7 @@ static int __octep_oq_process_rx(struct octep_device *oct,
 		if (!skb) {
 			octep_oq_drop_rx(oq, buff_info,
 					 &read_idx, &desc_used);
-			oq->stats.alloc_failures++;
+			oq->stats->alloc_failures++;
 			continue;
 		}
 		skb_reserve(skb, data_offset);
@@ -494,8 +496,8 @@ static int __octep_oq_process_rx(struct octep_device *oct,
 
 	oq->host_read_idx = read_idx;
 	oq->refill_count += desc_used;
-	oq->stats.packets += pkt;
-	oq->stats.bytes += rx_bytes;
+	oq->stats->packets += pkt;
+	oq->stats->bytes += rx_bytes;
 
 	return pkt;
 }
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h
index 3b08e2d560dc..b4696c93d0e6 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.h
@@ -186,8 +186,8 @@ struct octep_oq {
 	 */
 	u8 __iomem *pkts_sent_reg;
 
-	/* Statistics for this OQ. */
-	struct octep_oq_stats stats;
+	/* Pointer to statistics for this OQ. */
+	struct octep_oq_stats *stats;
 
 	/* Packets pending to be processed */
 	u32 pkts_pending;
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
index 06851b78aa28..08ee90013fef 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
@@ -81,9 +81,9 @@ int octep_iq_process_completions(struct octep_iq *iq, u16 budget)
 	}
 
 	iq->pkts_processed += compl_pkts;
-	iq->stats.instr_completed += compl_pkts;
-	iq->stats.bytes_sent += compl_bytes;
-	iq->stats.sgentry_sent += compl_sg;
+	iq->stats->instr_completed += compl_pkts;
+	iq->stats->bytes_sent += compl_bytes;
+	iq->stats->sgentry_sent += compl_sg;
 	iq->flush_index = fi;
 
 	netdev_tx_completed_queue(iq->netdev_q, compl_pkts, compl_bytes);
@@ -187,6 +187,7 @@ static int octep_setup_iq(struct octep_device *oct, int q_no)
 	iq->netdev = oct->netdev;
 	iq->dev = &oct->pdev->dev;
 	iq->q_no = q_no;
+	iq->stats = &oct->stats_iq[q_no];
 	iq->max_count = CFG_GET_IQ_NUM_DESC(oct->conf);
 	iq->ring_size_mask = iq->max_count - 1;
 	iq->fill_threshold = CFG_GET_IQ_DB_MIN(oct->conf);
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
index 875a2c34091f..58fb39dda977 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
@@ -170,8 +170,8 @@ struct octep_iq {
 	 */
 	u16 flush_index;
 
-	/* Statistics for this input queue. */
-	struct octep_iq_stats stats;
+	/* Pointer to statistics for this input queue. */
+	struct octep_iq_stats *stats;
 
 	/* Pointer to the Virtual Base addr of the input ring. */
 	struct octep_tx_desc_hw *desc_ring;
-- 
2.25.1

[PATCH net v8 3/4] octeon_ep_vf: remove firmware stats fetch in ndo_get_stats64

[Shinas Rasheed]

The per queue stats are available already and are retrieved
from register reads during ndo_get_stats64. The firmware stats
fetch call that happens in ndo_get_stats64() is currently not
required

The warn log is given below:

[  123.316837] ------------[ cut here ]------------
[  123.316840] Voluntary context switch within RCU read-side critical section!
[  123.316917] pc : rcu_note_context_switch+0x2e4/0x300
[  123.316919] lr : rcu_note_context_switch+0x2e4/0x300
[  123.316947] Call trace:
[  123.316949]  rcu_note_context_switch+0x2e4/0x300
[  123.316952]  __schedule+0x84/0x584
[  123.316955]  schedule+0x38/0x90
[  123.316956]  schedule_timeout+0xa0/0x1d4
[  123.316959]  octep_send_mbox_req+0x190/0x230 [octeon_ep]
[  123.316966]  octep_ctrl_net_get_if_stats+0x78/0x100 [octeon_ep]
[  123.316970]  octep_get_stats64+0xd4/0xf0 [octeon_ep]
[  123.316975]  dev_get_stats+0x4c/0x114
[  123.316977]  dev_seq_printf_stats+0x3c/0x11c
[  123.316980]  dev_seq_show+0x1c/0x40
[  123.316982]  seq_read_iter+0x3cc/0x4e0
[  123.316985]  seq_read+0xc8/0x110
[  123.316987]  proc_reg_read+0x9c/0xec
[  123.316990]  vfs_read+0xc8/0x2ec
[  123.316993]  ksys_read+0x70/0x100
[  123.316995]  __arm64_sys_read+0x20/0x30
[  123.316997]  invoke_syscall.constprop.0+0x7c/0xd0
[  123.317000]  do_el0_svc+0xb4/0xd0
[  123.317002]  el0_svc+0xe8/0x1f4
[  123.317005]  el0t_64_sync_handler+0x134/0x150
[  123.317006]  el0t_64_sync+0x17c/0x180
[  123.317008] ---[ end trace 63399811432ab69b ]---

Fixes: c3fad23cdc06 ("octeon_ep_vf: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V8:
  - Reordered patch

V7: https://lore.kernel.org/all/20250114125124.2570660-5-srasheed@marvell.com/
  - No changes

V6: https://lore.kernel.org/all/20250110122730.2551863-5-srasheed@marvell.com/
  - No changes

V5: https://lore.kernel.org/all/20250109103221.2544467-5-srasheed@marvell.com/
  - No changes

V4: https://lore.kernel.org/all/20250102112246.2494230-5-srasheed@marvell.com/
  - No changes

V3: https://lore.kernel.org/all/20241218115111.2407958-5-srasheed@marvell.com/
  - Added warn log that happened due to rcu_read_lock in commit message

V2: https://lore.kernel.org/all/20241216075842.2394606-5-srasheed@marvell.com/
  - No changes

V1: https://lore.kernel.org/all/20241203072130.2316913-5-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
index 7e6771c9cdbb..4c699514fd57 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
@@ -799,14 +799,6 @@ static void octep_vf_get_stats64(struct net_device *netdev,
 	stats->tx_bytes = tx_bytes;
 	stats->rx_packets = rx_packets;
 	stats->rx_bytes = rx_bytes;
-	if (!octep_vf_get_if_stats(oct)) {
-		stats->multicast = oct->iface_rx_stats.mcast_pkts;
-		stats->rx_errors = oct->iface_rx_stats.err_pkts;
-		stats->rx_dropped = oct->iface_rx_stats.dropped_pkts_fifo_full +
-				    oct->iface_rx_stats.err_pkts;
-		stats->rx_missed_errors = oct->iface_rx_stats.dropped_pkts_fifo_full;
-		stats->tx_dropped = oct->iface_tx_stats.dropped;
-	}
 }
 
 /**
-- 
2.25.1

[PATCH net v8 4/4] octeon_ep_vf: update tx/rx stats locally for persistence

[Shinas Rasheed]

Update tx/rx stats locally, so that ndo_get_stats64()
can use that and not rely on per queue resources to obtain statistics.
The latter used to cause race conditions when the device stopped.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V8:
  - Reordered patch

V7: https://lore.kernel.org/all/20250114125124.2570660-4-srasheed@marvell.com/
  - Updated octep_get_stats64() to be reentrant

V6: https://lore.kernel.org/all/20250110122730.2551863-4-srasheed@marvell.com/
  - No changes

V5: https://lore.kernel.org/all/20250109103221.2544467-4-srasheed@marvell.com/
  - Patch introduced

 .../marvell/octeon_ep_vf/octep_vf_ethtool.c   | 29 +++++++------------
 .../marvell/octeon_ep_vf/octep_vf_main.c      | 17 +++++------
 .../marvell/octeon_ep_vf/octep_vf_main.h      | 11 +++++++
 .../marvell/octeon_ep_vf/octep_vf_rx.c        | 10 ++++---
 .../marvell/octeon_ep_vf/octep_vf_rx.h        |  2 +-
 .../marvell/octeon_ep_vf/octep_vf_tx.c        |  7 +++--
 .../marvell/octeon_ep_vf/octep_vf_tx.h        |  2 +-
 7 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c
index 7b21439a315f..9966a2ee3de8 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c
@@ -114,12 +114,9 @@ static void octep_vf_get_ethtool_stats(struct net_device *netdev,
 	iface_tx_stats = &oct->iface_tx_stats;
 	iface_rx_stats = &oct->iface_rx_stats;
 
-	for (q = 0; q < oct->num_oqs; q++) {
-		struct octep_vf_iq *iq = oct->iq[q];
-		struct octep_vf_oq *oq = oct->oq[q];
-
-		tx_busy_errors += iq->stats.tx_busy;
-		rx_alloc_errors += oq->stats.alloc_failures;
+	for (q = 0; q < oct->num_ioq_stats; q++) {
+		tx_busy_errors += oct->stats_iq[q].tx_busy;
+		rx_alloc_errors += oct->stats_oq[q].alloc_failures;
 	}
 	i = 0;
 	data[i++] = rx_alloc_errors;
@@ -134,22 +131,18 @@ static void octep_vf_get_ethtool_stats(struct net_device *netdev,
 	data[i++] = iface_rx_stats->dropped_octets_fifo_full;
 
 	/* Per Tx Queue stats */
-	for (q = 0; q < oct->num_iqs; q++) {
-		struct octep_vf_iq *iq = oct->iq[q];
-
-		data[i++] = iq->stats.instr_posted;
-		data[i++] = iq->stats.instr_completed;
-		data[i++] = iq->stats.bytes_sent;
-		data[i++] = iq->stats.tx_busy;
+	for (q = 0; q < oct->num_ioq_stats; q++) {
+		data[i++] = oct->stats_iq[q].instr_posted;
+		data[i++] = oct->stats_iq[q].instr_completed;
+		data[i++] = oct->stats_iq[q].bytes_sent;
+		data[i++] = oct->stats_iq[q].tx_busy;
 	}
 
 	/* Per Rx Queue stats */
 	for (q = 0; q < oct->num_oqs; q++) {
-		struct octep_vf_oq *oq = oct->oq[q];
-
-		data[i++] = oq->stats.packets;
-		data[i++] = oq->stats.bytes;
-		data[i++] = oq->stats.alloc_failures;
+		data[i++] = oct->stats_oq[q].packets;
+		data[i++] = oct->stats_oq[q].bytes;
+		data[i++] = oct->stats_oq[q].alloc_failures;
 	}
 }
 
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
index 4c699514fd57..e7cb73de3116 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
@@ -574,7 +574,7 @@ static int octep_vf_iq_full_check(struct octep_vf_iq *iq)
 		  * caused queues to get re-enabled after
 		  * being stopped
 		  */
-		iq->stats.restart_cnt++;
+		iq->stats->restart_cnt++;
 		fallthrough;
 	case 1: /* Queue left enabled, since IQ is not yet full*/
 		return 0;
@@ -731,7 +731,7 @@ static netdev_tx_t octep_vf_start_xmit(struct sk_buff *skb,
 	/* Flush the hw descriptors before writing to doorbell */
 	smp_wmb();
 	writel(iq->fill_cnt, iq->doorbell_reg);
-	iq->stats.instr_posted += iq->fill_cnt;
+	iq->stats->instr_posted += iq->fill_cnt;
 	iq->fill_cnt = 0;
 	return NETDEV_TX_OK;
 }
@@ -786,14 +786,11 @@ static void octep_vf_get_stats64(struct net_device *netdev,
 	tx_bytes = 0;
 	rx_packets = 0;
 	rx_bytes = 0;
-	for (q = 0; q < oct->num_oqs; q++) {
-		struct octep_vf_iq *iq = oct->iq[q];
-		struct octep_vf_oq *oq = oct->oq[q];
-
-		tx_packets += iq->stats.instr_completed;
-		tx_bytes += iq->stats.bytes_sent;
-		rx_packets += oq->stats.packets;
-		rx_bytes += oq->stats.bytes;
+	for (q = 0; q < oct->num_ioq_stats; q++) {
+		tx_packets += oct->stats_iq[q].instr_completed;
+		tx_bytes += oct->stats_iq[q].bytes_sent;
+		rx_packets += oct->stats_oq[q].packets;
+		rx_bytes += oct->stats_oq[q].bytes;
 	}
 	stats->tx_packets = tx_packets;
 	stats->tx_bytes = tx_bytes;
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h
index 5769f62545cd..91cfaa105d06 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h
@@ -246,11 +246,22 @@ struct octep_vf_device {
 	/* Pointers to Octeon Tx queues */
 	struct octep_vf_iq *iq[OCTEP_VF_MAX_IQ];
 
+	/* Per iq stats */
+	struct octep_vf_iq_stats stats_iq[OCTEP_VF_MAX_IQ];
+
 	/* Rx queues (OQ: Output Queue) */
 	u16 num_oqs;
 	/* Pointers to Octeon Rx queues */
 	struct octep_vf_oq *oq[OCTEP_VF_MAX_OQ];
 
+	/* Number oq stats preserved
+	 * This number would remain constant when device goes down
+	 * This will be updated when device comes back up
+	 */
+	u16 num_ioq_stats;
+	/* Per oq stats */
+	struct octep_vf_oq_stats stats_oq[OCTEP_VF_MAX_OQ];
+
 	/* Hardware port number of the PCIe interface */
 	u16 pcie_port;
 
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c
index 82821bc28634..72399f388973 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c
@@ -87,7 +87,7 @@ static int octep_vf_oq_refill(struct octep_vf_device *oct, struct octep_vf_oq *o
 		page = dev_alloc_page();
 		if (unlikely(!page)) {
 			dev_err(oq->dev, "refill: rx buffer alloc failed\n");
-			oq->stats.alloc_failures++;
+			oq->stats->alloc_failures++;
 			break;
 		}
 
@@ -98,7 +98,7 @@ static int octep_vf_oq_refill(struct octep_vf_device *oct, struct octep_vf_oq *o
 				"OQ-%d buffer refill: DMA mapping error!\n",
 				oq->q_no);
 			put_page(page);
-			oq->stats.alloc_failures++;
+			oq->stats->alloc_failures++;
 			break;
 		}
 		oq->buff_info[refill_idx].page = page;
@@ -134,6 +134,7 @@ static int octep_vf_setup_oq(struct octep_vf_device *oct, int q_no)
 	oq->netdev = oct->netdev;
 	oq->dev = &oct->pdev->dev;
 	oq->q_no = q_no;
+	oq->stats = &oct->stats_oq[q_no];
 	oq->max_count = CFG_GET_OQ_NUM_DESC(oct->conf);
 	oq->ring_size_mask = oq->max_count - 1;
 	oq->buffer_size = CFG_GET_OQ_BUF_SIZE(oct->conf);
@@ -263,6 +264,7 @@ int octep_vf_setup_oqs(struct octep_vf_device *oct)
 		dev_dbg(&oct->pdev->dev, "Successfully setup OQ(RxQ)-%d.\n", i);
 	}
 
+	oct->num_ioq_stats = oct->num_oqs;
 	return 0;
 
 oq_setup_err:
@@ -458,8 +460,8 @@ static int __octep_vf_oq_process_rx(struct octep_vf_device *oct,
 
 	oq->host_read_idx = read_idx;
 	oq->refill_count += desc_used;
-	oq->stats.packets += pkt;
-	oq->stats.bytes += rx_bytes;
+	oq->stats->packets += pkt;
+	oq->stats->bytes += rx_bytes;
 
 	return pkt;
 }
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.h b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.h
index fe46838b5200..9e296b7d7e34 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.h
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.h
@@ -187,7 +187,7 @@ struct octep_vf_oq {
 	u8 __iomem *pkts_sent_reg;
 
 	/* Statistics for this OQ. */
-	struct octep_vf_oq_stats stats;
+	struct octep_vf_oq_stats *stats;
 
 	/* Packets pending to be processed */
 	u32 pkts_pending;
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.c
index 47a5c054fdb6..8180e5ce3d7e 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.c
@@ -82,9 +82,9 @@ int octep_vf_iq_process_completions(struct octep_vf_iq *iq, u16 budget)
 	}
 
 	iq->pkts_processed += compl_pkts;
-	iq->stats.instr_completed += compl_pkts;
-	iq->stats.bytes_sent += compl_bytes;
-	iq->stats.sgentry_sent += compl_sg;
+	iq->stats->instr_completed += compl_pkts;
+	iq->stats->bytes_sent += compl_bytes;
+	iq->stats->sgentry_sent += compl_sg;
 	iq->flush_index = fi;
 
 	netif_subqueue_completed_wake(iq->netdev, iq->q_no, compl_pkts,
@@ -186,6 +186,7 @@ static int octep_vf_setup_iq(struct octep_vf_device *oct, int q_no)
 	iq->netdev = oct->netdev;
 	iq->dev = &oct->pdev->dev;
 	iq->q_no = q_no;
+	iq->stats = &oct->stats_iq[q_no];
 	iq->max_count = CFG_GET_IQ_NUM_DESC(oct->conf);
 	iq->ring_size_mask = iq->max_count - 1;
 	iq->fill_threshold = CFG_GET_IQ_DB_MIN(oct->conf);
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.h b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.h
index f338b975103c..1cede90e3a5f 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.h
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_tx.h
@@ -129,7 +129,7 @@ struct octep_vf_iq {
 	u16 flush_index;
 
 	/* Statistics for this input queue. */
-	struct octep_vf_iq_stats stats;
+	struct octep_vf_iq_stats *stats;
 
 	/* Pointer to the Virtual Base addr of the input ring. */
 	struct octep_vf_tx_desc_hw *desc_ring;
-- 
2.25.1

Re: [PATCH net v8 2/4] octeon_ep: update tx/rx stats locally for persistence

[Larysa Zaremba]

On Thu, Jan 16, 2025 at 12:38:23AM -0800, Shinas Rasheed wrote:
> Update tx/rx stats locally, so that ndo_get_stats64()
> can use that and not rely on per queue resources to obtain statistics.
> The latter used to cause race conditions when the device stopped.
> 
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
> V8:
>   - Reordered patch
> 
> V7: https://lore.kernel.org/all/20250114125124.2570660-2-srasheed@marvell.com/
>   - Updated octep_get_stats64() to be reentrant
> 
> V6: https://lore.kernel.org/all/20250110122730.2551863-2-srasheed@marvell.com/
>   - No changes
> 
> V5: https://lore.kernel.org/all/20250109103221.2544467-2-srasheed@marvell.com/
>   - Patch introduced
> 
>  .../marvell/octeon_ep/octep_ethtool.c         | 41 ++++++++-----------
>  .../ethernet/marvell/octeon_ep/octep_main.c   | 19 ++++-----
>  .../ethernet/marvell/octeon_ep/octep_main.h   | 11 +++++
>  .../net/ethernet/marvell/octeon_ep/octep_rx.c | 12 +++---
>  .../net/ethernet/marvell/octeon_ep/octep_rx.h |  4 +-
>  .../net/ethernet/marvell/octeon_ep/octep_tx.c |  7 ++--
>  .../net/ethernet/marvell/octeon_ep/octep_tx.h |  4 +-
>  7 files changed, 51 insertions(+), 47 deletions(-)
>

[...]
 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 730aa5632cce..133694a1658d 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c

[...]

> @@ -991,22 +991,19 @@ static netdev_tx_t octep_start_xmit(struct sk_buff *skb,
>  static void octep_get_stats64(struct net_device *netdev,
>  			      struct rtnl_link_stats64 *stats)
>  {
> -	u64 tx_packets, tx_bytes, rx_packets, rx_bytes;
>  	struct octep_device *oct = netdev_priv(netdev);
> +	u64 tx_packets, tx_bytes, rx_packets, rx_bytes;
>  	int q;
>  
>  	tx_packets = 0;
>  	tx_bytes = 0;
>  	rx_packets = 0;
>  	rx_bytes = 0;
> -	for (q = 0; q < oct->num_oqs; q++) {
> -		struct octep_iq *iq = oct->iq[q];
> -		struct octep_oq *oq = oct->oq[q];
> -
> -		tx_packets += iq->stats.instr_completed;
> -		tx_bytes += iq->stats.bytes_sent;
> -		rx_packets += oq->stats.packets;
> -		rx_bytes += oq->stats.bytes;
> +	for (q = 0; q < oct->num_ioq_stats; q++) {
> +		tx_packets += oct->stats_iq[q].instr_completed;
> +		tx_bytes += oct->stats_iq[q].bytes_sent;
> +		rx_packets += oct->stats_oq[q].packets;
> +		rx_bytes += oct->stats_oq[q].bytes;

Correct me if I am wrong, but the interface-wide statistics should not change 
when changing queue number. In such case maybe it would be a good idea to 
always iterate over all OCTEP_MAX_QUEUES queues when calculating the stats.

>  	}
>  	stats->tx_packets = tx_packets;
>  	stats->tx_bytes = tx_bytes;

Re: [PATCH net v8 3/4] octeon_ep_vf: remove firmware stats fetch in ndo_get_stats64

[Larysa Zaremba]

On Thu, Jan 16, 2025 at 12:38:24AM -0800, Shinas Rasheed wrote:
> The per queue stats are available already and are retrieved
> from register reads during ndo_get_stats64.

Please update this after changing the patch order.

> The firmware stats
> fetch call that happens in ndo_get_stats64() is currently not
> required
> 
> The warn log is given below:
> 

This is not a call trace for VF functions, you should probably mention that in 
the commit message.

Other than that, I think the series is on the right track, though patches 2/4 
and 4/4 may need more attention.

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>

> [  123.316837] ------------[ cut here ]------------
> [  123.316840] Voluntary context switch within RCU read-side critical section!
> [  123.316917] pc : rcu_note_context_switch+0x2e4/0x300
> [  123.316919] lr : rcu_note_context_switch+0x2e4/0x300
> [  123.316947] Call trace:
> [  123.316949]  rcu_note_context_switch+0x2e4/0x300
> [  123.316952]  __schedule+0x84/0x584
> [  123.316955]  schedule+0x38/0x90
> [  123.316956]  schedule_timeout+0xa0/0x1d4
> [  123.316959]  octep_send_mbox_req+0x190/0x230 [octeon_ep]
> [  123.316966]  octep_ctrl_net_get_if_stats+0x78/0x100 [octeon_ep]
> [  123.316970]  octep_get_stats64+0xd4/0xf0 [octeon_ep]
> [  123.316975]  dev_get_stats+0x4c/0x114
> [  123.316977]  dev_seq_printf_stats+0x3c/0x11c
> [  123.316980]  dev_seq_show+0x1c/0x40
> [  123.316982]  seq_read_iter+0x3cc/0x4e0
> [  123.316985]  seq_read+0xc8/0x110
> [  123.316987]  proc_reg_read+0x9c/0xec
> [  123.316990]  vfs_read+0xc8/0x2ec
> [  123.316993]  ksys_read+0x70/0x100
> [  123.316995]  __arm64_sys_read+0x20/0x30
> [  123.316997]  invoke_syscall.constprop.0+0x7c/0xd0
> [  123.317000]  do_el0_svc+0xb4/0xd0
> [  123.317002]  el0_svc+0xe8/0x1f4
> [  123.317005]  el0t_64_sync_handler+0x134/0x150
> [  123.317006]  el0t_64_sync+0x17c/0x180
> [  123.317008] ---[ end trace 63399811432ab69b ]---
> 
> Fixes: c3fad23cdc06 ("octeon_ep_vf: add support for ndo ops")
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
> V8:
>   - Reordered patch
> 
> V7: https://lore.kernel.org/all/20250114125124.2570660-5-srasheed@marvell.com/
>   - No changes
> 
> V6: https://lore.kernel.org/all/20250110122730.2551863-5-srasheed@marvell.com/
>   - No changes
> 
> V5: https://lore.kernel.org/all/20250109103221.2544467-5-srasheed@marvell.com/
>   - No changes
> 
> V4: https://lore.kernel.org/all/20250102112246.2494230-5-srasheed@marvell.com/
>   - No changes
> 
> V3: https://lore.kernel.org/all/20241218115111.2407958-5-srasheed@marvell.com/
>   - Added warn log that happened due to rcu_read_lock in commit message
> 
> V2: https://lore.kernel.org/all/20241216075842.2394606-5-srasheed@marvell.com/
>   - No changes
> 
> V1: https://lore.kernel.org/all/20241203072130.2316913-5-srasheed@marvell.com/
> 
>  drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
> index 7e6771c9cdbb..4c699514fd57 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
> @@ -799,14 +799,6 @@ static void octep_vf_get_stats64(struct net_device *netdev,
>  	stats->tx_bytes = tx_bytes;
>  	stats->rx_packets = rx_packets;
>  	stats->rx_bytes = rx_bytes;
> -	if (!octep_vf_get_if_stats(oct)) {
> -		stats->multicast = oct->iface_rx_stats.mcast_pkts;
> -		stats->rx_errors = oct->iface_rx_stats.err_pkts;
> -		stats->rx_dropped = oct->iface_rx_stats.dropped_pkts_fifo_full +
> -				    oct->iface_rx_stats.err_pkts;
> -		stats->rx_missed_errors = oct->iface_rx_stats.dropped_pkts_fifo_full;
> -		stats->tx_dropped = oct->iface_tx_stats.dropped;
> -	}
>  }
>  
>  /**
> -- 
> 2.25.1
> 
> 

Re: [PATCH net v8 1/4] octeon_ep: remove firmware stats fetch in ndo_get_stats64

[Larysa Zaremba]

On Thu, Jan 16, 2025 at 12:38:22AM -0800, Shinas Rasheed wrote:
> The per queue stats are available already and are retrieved
> from register reads during ndo_get_stats64.

Same as for patch 3/4: please update the commit message after reordering.

Otherwise looks good.

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>

> The firmware stats
> fetch call that happens in ndo_get_stats64() is currently not
> required
> 
> The warn log is given below:
> 
> [  123.316837] ------------[ cut here ]------------
> [  123.316840] Voluntary context switch within RCU read-side critical section!
> [  123.316917] pc : rcu_note_context_switch+0x2e4/0x300
> [  123.316919] lr : rcu_note_context_switch+0x2e4/0x300
> [  123.316947] Call trace:
> [  123.316949]  rcu_note_context_switch+0x2e4/0x300
> [  123.316952]  __schedule+0x84/0x584
> [  123.316955]  schedule+0x38/0x90
> [  123.316956]  schedule_timeout+0xa0/0x1d4
> [  123.316959]  octep_send_mbox_req+0x190/0x230 [octeon_ep]
> [  123.316966]  octep_ctrl_net_get_if_stats+0x78/0x100 [octeon_ep]
> [  123.316970]  octep_get_stats64+0xd4/0xf0 [octeon_ep]
> [  123.316975]  dev_get_stats+0x4c/0x114
> [  123.316977]  dev_seq_printf_stats+0x3c/0x11c
> [  123.316980]  dev_seq_show+0x1c/0x40
> [  123.316982]  seq_read_iter+0x3cc/0x4e0
> [  123.316985]  seq_read+0xc8/0x110
> [  123.316987]  proc_reg_read+0x9c/0xec
> [  123.316990]  vfs_read+0xc8/0x2ec
> [  123.316993]  ksys_read+0x70/0x100
> [  123.316995]  __arm64_sys_read+0x20/0x30
> [  123.316997]  invoke_syscall.constprop.0+0x7c/0xd0
> [  123.317000]  do_el0_svc+0xb4/0xd0
> [  123.317002]  el0_svc+0xe8/0x1f4
> [  123.317005]  el0t_64_sync_handler+0x134/0x150
> [  123.317006]  el0t_64_sync+0x17c/0x180
> [  123.317008] ---[ end trace 63399811432ab69b ]---
> 
> Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
> V8:
>   - Reordered patch
> 
> V7: https://lore.kernel.org/all/20250114125124.2570660-3-srasheed@marvell.com/
>   - No changes
> 
> V6: https://lore.kernel.org/all/20250110122730.2551863-3-srasheed@marvell.com/
>   - Corrected patch to apply properly
> 
> V5: https://lore.kernel.org/all/20250109103221.2544467-3-srasheed@marvell.com/
>   - No changes
> 
> V4: https://lore.kernel.org/all/20250102112246.2494230-3-srasheed@marvell.com/
>   - No changes
> 
> V3: https://lore.kernel.org/all/20241218115111.2407958-3-srasheed@marvell.com/
>   - Added warn log that happened due to rcu_read_lock in commit message
> 
> V2: https://lore.kernel.org/all/20241216075842.2394606-3-srasheed@marvell.com/
>   - No changes
> 
> V1: https://lore.kernel.org/all/20241203072130.2316913-3-srasheed@marvell.com/
> 
>  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 549436efc204..730aa5632cce 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -995,12 +995,6 @@ static void octep_get_stats64(struct net_device *netdev,
>  	struct octep_device *oct = netdev_priv(netdev);
>  	int q;
>  
> -	if (netif_running(netdev))
> -		octep_ctrl_net_get_if_stats(oct,
> -					    OCTEP_CTRL_NET_INVALID_VFID,
> -					    &oct->iface_rx_stats,
> -					    &oct->iface_tx_stats);
> -
>  	tx_packets = 0;
>  	tx_bytes = 0;
>  	rx_packets = 0;
> @@ -1018,10 +1012,6 @@ static void octep_get_stats64(struct net_device *netdev,
>  	stats->tx_bytes = tx_bytes;
>  	stats->rx_packets = rx_packets;
>  	stats->rx_bytes = rx_bytes;
> -	stats->multicast = oct->iface_rx_stats.mcast_pkts;
> -	stats->rx_errors = oct->iface_rx_stats.err_pkts;
> -	stats->collisions = oct->iface_tx_stats.xscol;
> -	stats->tx_fifo_errors = oct->iface_tx_stats.undflw;
>  }
>  
>  /**
> -- 
> 2.25.1
> 
> 

Re: [PATCH net v8 2/4] octeon_ep: update tx/rx stats locally for persistence

[Jakub Kicinski]

On Thu, 16 Jan 2025 16:26:18 +0100 Larysa Zaremba wrote:
> > +	for (q = 0; q < oct->num_ioq_stats; q++) {
> > +		tx_packets += oct->stats_iq[q].instr_completed;
> > +		tx_bytes += oct->stats_iq[q].bytes_sent;
> > +		rx_packets += oct->stats_oq[q].packets;
> > +		rx_bytes += oct->stats_oq[q].bytes;  
> 
> Correct me if I am wrong, but the interface-wide statistics should not change 
> when changing queue number. In such case maybe it would be a good idea to 
> always iterate over all OCTEP_MAX_QUEUES queues when calculating the stats.

Good catch!
-- 
pw-bot: cr

RE: [EXTERNAL] Re: [PATCH net v8 2/4] octeon_ep: update tx/rx stats locally for persistence

[Shinas Rasheed]

Hi Jakub, Larysa

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, January 17, 2025 5:57 AM
> To: Larysa Zaremba <larysa.zaremba@intel.com>
> Cc: Shinas Rasheed <srasheed@marvell.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Haseeb Gani <hgani@marvell.com>; Sathesh B Edara
> <sedara@marvell.com>; Vimlesh Kumar <vimleshk@marvell.com>;
> thaller@redhat.com; wizhao@redhat.com; kheib@redhat.com;
> konguyen@redhat.com; horms@kernel.org; einstein.xue@synaxg.com;
> Veerasenareddy Burru <vburru@marvell.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>
> Subject: [EXTERNAL] Re: [PATCH net v8 2/4] octeon_ep: update tx/rx stats
> locally for persistence
> 
> On Thu, 16 Jan 2025 16:26:18 +0100 Larysa Zaremba wrote:
> > > +	for (q = 0; q < oct->num_ioq_stats; q++) {
> > > +		tx_packets += oct->stats_iq[q].instr_completed;
> > > +		tx_bytes += oct->stats_iq[q].bytes_sent;
> > > +		rx_packets += oct->stats_oq[q].packets;
> > > +		rx_bytes += oct->stats_oq[q].bytes;
> >
> > Correct me if I am wrong, but the interface-wide statistics should not change
> > when changing queue number. In such case maybe it would be a good idea
> to
> > always iterate over all OCTEP_MAX_QUEUES queues when calculating the
> stats.
> 
> Good catch!

The queues which are iterated over here refer to the hardware queues, and it is in fact iterating over all the queues.
Can you please clarify more?

Thanks

> --
> pw-bot: cr

RE: [EXTERNAL] Re: [PATCH net v8 2/4] octeon_ep: update tx/rx stats locally for persistence

[Shinas Rasheed]

> -----Original Message-----
> From: Shinas Rasheed
> Sent: Friday, January 17, 2025 9:42 AM
> To: Jakub Kicinski <kuba@kernel.org>; Larysa Zaremba
> <larysa.zaremba@intel.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Haseeb Gani
> <hgani@marvell.com>; Sathesh B Edara <sedara@marvell.com>; Vimlesh
> Kumar <vimleshk@marvell.com>; thaller@redhat.com; wizhao@redhat.com;
> kheib@redhat.com; konguyen@redhat.com; horms@kernel.org;
> einstein.xue@synaxg.com; Veerasenareddy Burru <vburru@marvell.com>;
> Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo
> Abeni <pabeni@redhat.com>
> Subject: RE: [EXTERNAL] Re: [PATCH net v8 2/4] octeon_ep: update tx/rx stats
> locally for persistence
> 
> Hi Jakub, Larysa
> 
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Friday, January 17, 2025 5:57 AM
> > To: Larysa Zaremba <larysa.zaremba@intel.com>
> > Cc: Shinas Rasheed <srasheed@marvell.com>; netdev@vger.kernel.org;
> linux-
> > kernel@vger.kernel.org; Haseeb Gani <hgani@marvell.com>; Sathesh B
> Edara
> > <sedara@marvell.com>; Vimlesh Kumar <vimleshk@marvell.com>;
> > thaller@redhat.com; wizhao@redhat.com; kheib@redhat.com;
> > konguyen@redhat.com; horms@kernel.org; einstein.xue@synaxg.com;
> > Veerasenareddy Burru <vburru@marvell.com>; Andrew Lunn
> > <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> > Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>
> > Subject: [EXTERNAL] Re: [PATCH net v8 2/4] octeon_ep: update tx/rx stats
> > locally for persistence
> >
> > On Thu, 16 Jan 2025 16:26:18 +0100 Larysa Zaremba wrote:
> > > > +	for (q = 0; q < oct->num_ioq_stats; q++) {
> > > > +		tx_packets += oct->stats_iq[q].instr_completed;
> > > > +		tx_bytes += oct->stats_iq[q].bytes_sent;
> > > > +		rx_packets += oct->stats_oq[q].packets;
> > > > +		rx_bytes += oct->stats_oq[q].bytes;
> > >
> > > Correct me if I am wrong, but the interface-wide statistics should not
> change
> > > when changing queue number. In such case maybe it would be a good
> idea
> > to
> > > always iterate over all OCTEP_MAX_QUEUES queues when calculating the
> > stats.
> >
> > Good catch!
> 
> The queues which are iterated over here refer to the hardware queues, and it
> is in fact iterating over all the queues.
> Can you please clarify more?
> 
> Thanks

Okay, I think I understand
> 
> > --
> > pw-bot: cr