[PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

[PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

[MD Danish Anwar]

The ICSSG firmware maintains set of stats called PA_STATS.
Currently the driver only dumps 4 stats. Add support for dumping more
stats.

The offset for different stats are defined as MACROs in icssg_switch_map.h
file. All the offsets are for Slice0. Slice1 offsets are slice0 + 4.
The offset calculation is taken care while reading the stats in
emac_update_hardware_stats().

The statistics are documented in
Documentation/networking/device_drivers/icssg_prueth.rst

Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
v1 - v2:
*) Created icssg_prueth.rst and added Documentation of firmware statistics
as suggested by Jakub Kicinski <kuba@kernel.org>
*) Removed unimplemented preemption statistics.
*) Collected RB tag from Simon Horman <horms@kernel.org>

v1 - https://lore.kernel.org/all/20250227093712.2130561-1-danishanwar@ti.com/

 .../device_drivers/ethernet/index.rst         |  1 +
 .../ethernet/ti/icssg_prueth.rst              | 56 ++++++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  2 +-
 drivers/net/ethernet/ti/icssg/icssg_stats.c   |  6 +-
 drivers/net/ethernet/ti/icssg/icssg_stats.h   | 58 ++++++++++++-------
 .../net/ethernet/ti/icssg/icssg_switch_map.h  | 33 +++++++++++
 6 files changed, 129 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/networking/device_drivers/ethernet/ti/icssg_prueth.rst

diff --git a/Documentation/networking/device_drivers/ethernet/index.rst b/Documentation/networking/device_drivers/ethernet/index.rst
index 6fc1961492b7..cd5f31dd07ce 100644
--- a/Documentation/networking/device_drivers/ethernet/index.rst
+++ b/Documentation/networking/device_drivers/ethernet/index.rst
@@ -55,6 +55,7 @@ Contents:
    ti/cpsw_switchdev
    ti/am65_nuss_cpsw_switchdev
    ti/tlan
+   ti/icssg_prueth
    toshiba/spider_net
    wangxun/txgbe
    wangxun/ngbe
diff --git a/Documentation/networking/device_drivers/ethernet/ti/icssg_prueth.rst b/Documentation/networking/device_drivers/ethernet/ti/icssg_prueth.rst
new file mode 100644
index 000000000000..da21ddf431bb
--- /dev/null
+++ b/Documentation/networking/device_drivers/ethernet/ti/icssg_prueth.rst
@@ -0,0 +1,56 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==============================================
+Texas Instruments ICSSG PRUETH ethernet driver
+==============================================
+
+:Version: 1.0
+
+ICSSG Firmware
+==============
+
+Every ICSSG core has two Programmable Real-Time Unit(PRUs), two auxiliary
+Real-Time Transfer Unit (RTUs), and two Transmit Real-Time Transfer Units
+(TX_PRUs). Each one of these runs its own firmware. The firmwares combnined are
+referred as ICSSG Firmware.
+
+Firmware Statistics
+===================
+
+The ICSSG firmware maintains certain statistics which are dumped by the driver
+via ``ethtool -S <interface>``
+
+These statistics are as follows,
+
+ - ``FW_RTU_PKT_DROP``: Diagnostic error counter which increments when RTU drops a locally injected packet due to port being disabled or rule violation.
+ - ``FW_Q0_OVERFLOW``: TX overflow counter for queue0
+ - ``FW_Q1_OVERFLOW``: TX overflow counter for queue1
+ - ``FW_Q2_OVERFLOW``: TX overflow counter for queue2
+ - ``FW_Q3_OVERFLOW``: TX overflow counter for queue3
+ - ``FW_Q4_OVERFLOW``: TX overflow counter for queue4
+ - ``FW_Q5_OVERFLOW``: TX overflow counter for queue5
+ - ``FW_Q6_OVERFLOW``: TX overflow counter for queue6
+ - ``FW_Q7_OVERFLOW``: TX overflow counter for queue7
+ - ``FW_DROPPED_PKT``: This counter is incremented when a packet is dropped at PRU because of rule violation.
+ - ``FW_RX_ERROR``: Incremented if there was a CRC error or Min/Max frame error at PRU
+ - ``FW_RX_DS_INVALID``: Incremented when RTU detects Data Status invalid condition
+ - ``FW_TX_DROPPED_PACKET``: Counter for packets dropped via TX Port
+ - ``FW_TX_TS_DROPPED_PACKET``: Counter for packets with TS flag dropped via TX Port
+ - ``FW_INF_PORT_DISABLED``: Incremented when RX frame is dropped due to port being disabled
+ - ``FW_INF_SAV``: Incremented when RX frame is dropped due to Source Address violation
+ - ``FW_INF_SA_DL``: Incremented when RX frame is dropped due to Source Address being in the denylist
+ - ``FW_INF_PORT_BLOCKED``: Incremented when RX frame is dropped due to port being blocked and frame being a special frame
+ - ``FW_INF_DROP_TAGGED`` : Incremented when RX frame is dropped for being tagged
+ - ``FW_INF_DROP_PRIOTAGGED``: Incremented when RX frame is dropped for being priority tagged
+ - ``FW_INF_DROP_NOTAG``: Incremented when RX frame is dropped for being untagged
+ - ``FW_INF_DROP_NOTMEMBER``: Incremented when RX frame is dropped for port not being member of VLAN
+ - ``FW_RX_EOF_SHORT_FRMERR``: Incremented if End Of Frame (EOF) task is scheduled without seeing RX_B1
+ - ``FW_RX_B0_DROP_EARLY_EOF``: Incremented when frame is dropped due to Early EOF
+ - ``FW_TX_JUMBO_FRM_CUTOFF``: Incremented when frame is cut off to prevent packet size > 2000 Bytes
+ - ``FW_RX_EXP_FRAG_Q_DROP``: Incremented when express frame is received in the same queue as the previous fragment
+ - ``FW_RX_FIFO_OVERRUN``: RX fifo overrun counter
+ - ``FW_CUT_THR_PKT``: Incremented when a packet is forwarded using Cut-Through forwarding method
+ - ``FW_HOST_RX_PKT_CNT``: Number of valid packets sent by Rx PRU to Host on PSI
+ - ``FW_HOST_TX_PKT_CNT``: Number of valid packets copied by RTU0 to Tx queues
+ - ``FW_HOST_EGRESS_Q_PRE_OVERFLOW``: Host Egress Q (Pre-emptible) Overflow Counter
+ - ``FW_HOST_EGRESS_Q_EXP_OVERFLOW``: Host Egress Q (Pre-emptible) Overflow Counter
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 329b46e9ee53..ff7fce26e851 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -50,7 +50,7 @@
 
 #define ICSSG_MAX_RFLOWS	8	/* per slice */
 
-#define ICSSG_NUM_PA_STATS	4
+#define ICSSG_NUM_PA_STATS	32
 #define ICSSG_NUM_MIIG_STATS	60
 /* Number of ICSSG related stats */
 #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
index 8800bd3a8d07..3f1400e0207c 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
@@ -11,7 +11,6 @@
 
 #define ICSSG_TX_PACKET_OFFSET	0xA0
 #define ICSSG_TX_BYTE_OFFSET	0xEC
-#define ICSSG_FW_STATS_BASE	0x0248
 
 static u32 stats_base[] = {	0x54c,	/* Slice 0 stats start */
 				0xb18,	/* Slice 1 stats start */
@@ -44,9 +43,8 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
 
 	if (prueth->pa_stats) {
 		for (i = 0; i < ARRAY_SIZE(icssg_all_pa_stats); i++) {
-			reg = ICSSG_FW_STATS_BASE +
-			      icssg_all_pa_stats[i].offset *
-			      PRUETH_NUM_MACS + slice * sizeof(u32);
+			reg = icssg_all_pa_stats[i].offset +
+			      slice * sizeof(u32);
 			regmap_read(prueth->pa_stats, reg, &val);
 			emac->pa_stats[i] += val;
 		}
diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
index e88b919f532c..5ec0b38e0c67 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
@@ -155,24 +155,10 @@ static const struct icssg_miig_stats icssg_all_miig_stats[] = {
 	ICSSG_MIIG_STATS(tx_bytes, true),
 };
 
-/**
- * struct pa_stats_regs - ICSSG Firmware maintained PA Stats register
- * @fw_rx_cnt: Number of valid packets sent by Rx PRU to Host on PSI
- * @fw_tx_cnt: Number of valid packets copied by RTU0 to Tx queues
- * @fw_tx_pre_overflow: Host Egress Q (Pre-emptible) Overflow Counter
- * @fw_tx_exp_overflow: Host Egress Q (Express) Overflow Counter
- */
-struct pa_stats_regs {
-	u32 fw_rx_cnt;
-	u32 fw_tx_cnt;
-	u32 fw_tx_pre_overflow;
-	u32 fw_tx_exp_overflow;
-};
-
-#define ICSSG_PA_STATS(field)			\
-{						\
-	#field,					\
-	offsetof(struct pa_stats_regs, field),	\
+#define ICSSG_PA_STATS(field)	\
+{				\
+	#field,			\
+	field,			\
 }
 
 struct icssg_pa_stats {
@@ -181,10 +167,38 @@ struct icssg_pa_stats {
 };
 
 static const struct icssg_pa_stats icssg_all_pa_stats[] = {
-	ICSSG_PA_STATS(fw_rx_cnt),
-	ICSSG_PA_STATS(fw_tx_cnt),
-	ICSSG_PA_STATS(fw_tx_pre_overflow),
-	ICSSG_PA_STATS(fw_tx_exp_overflow),
+	ICSSG_PA_STATS(FW_RTU_PKT_DROP),
+	ICSSG_PA_STATS(FW_Q0_OVERFLOW),
+	ICSSG_PA_STATS(FW_Q1_OVERFLOW),
+	ICSSG_PA_STATS(FW_Q2_OVERFLOW),
+	ICSSG_PA_STATS(FW_Q3_OVERFLOW),
+	ICSSG_PA_STATS(FW_Q4_OVERFLOW),
+	ICSSG_PA_STATS(FW_Q5_OVERFLOW),
+	ICSSG_PA_STATS(FW_Q6_OVERFLOW),
+	ICSSG_PA_STATS(FW_Q7_OVERFLOW),
+	ICSSG_PA_STATS(FW_DROPPED_PKT),
+	ICSSG_PA_STATS(FW_RX_ERROR),
+	ICSSG_PA_STATS(FW_RX_DS_INVALID),
+	ICSSG_PA_STATS(FW_TX_DROPPED_PACKET),
+	ICSSG_PA_STATS(FW_TX_TS_DROPPED_PACKET),
+	ICSSG_PA_STATS(FW_INF_PORT_DISABLED),
+	ICSSG_PA_STATS(FW_INF_SAV),
+	ICSSG_PA_STATS(FW_INF_SA_DL),
+	ICSSG_PA_STATS(FW_INF_PORT_BLOCKED),
+	ICSSG_PA_STATS(FW_INF_DROP_TAGGED),
+	ICSSG_PA_STATS(FW_INF_DROP_PRIOTAGGED),
+	ICSSG_PA_STATS(FW_INF_DROP_NOTAG),
+	ICSSG_PA_STATS(FW_INF_DROP_NOTMEMBER),
+	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR),
+	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF),
+	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF),
+	ICSSG_PA_STATS(FW_RX_EXP_FRAG_Q_DROP),
+	ICSSG_PA_STATS(FW_RX_FIFO_OVERRUN),
+	ICSSG_PA_STATS(FW_CUT_THR_PKT),
+	ICSSG_PA_STATS(FW_HOST_RX_PKT_CNT),
+	ICSSG_PA_STATS(FW_HOST_TX_PKT_CNT),
+	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_PRE_OVERFLOW),
+	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_EXP_OVERFLOW),
 };
 
 #endif /* __NET_TI_ICSSG_STATS_H */
diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
index 424a7e945ea8..490a9cc06fb0 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
@@ -231,4 +231,37 @@
 /* Start of 32 bits PA_STAT counters */
 #define PA_STAT_32b_START_OFFSET                           0x0080
 
+#define FW_RTU_PKT_DROP			0x0088
+#define FW_Q0_OVERFLOW			0x0090
+#define FW_Q1_OVERFLOW			0x0098
+#define FW_Q2_OVERFLOW			0x00A0
+#define FW_Q3_OVERFLOW			0x00A8
+#define FW_Q4_OVERFLOW			0x00B0
+#define FW_Q5_OVERFLOW			0x00B8
+#define FW_Q6_OVERFLOW			0x00C0
+#define FW_Q7_OVERFLOW			0x00C8
+#define FW_DROPPED_PKT			0x00F8
+#define FW_RX_ERROR			0x0100
+#define FW_RX_DS_INVALID		0x0108
+#define FW_TX_DROPPED_PACKET		0x0110
+#define FW_TX_TS_DROPPED_PACKET		0x0118
+#define FW_INF_PORT_DISABLED		0x0120
+#define FW_INF_SAV			0x0128
+#define FW_INF_SA_DL			0x0130
+#define FW_INF_PORT_BLOCKED		0x0138
+#define FW_INF_DROP_TAGGED		0x0140
+#define FW_INF_DROP_PRIOTAGGED		0x0148
+#define FW_INF_DROP_NOTAG		0x0150
+#define FW_INF_DROP_NOTMEMBER		0x0158
+#define FW_RX_EOF_SHORT_FRMERR		0x0188
+#define FW_RX_B0_DROP_EARLY_EOF		0x0190
+#define FW_TX_JUMBO_FRM_CUTOFF		0x0198
+#define FW_RX_EXP_FRAG_Q_DROP		0x01A0
+#define FW_RX_FIFO_OVERRUN		0x01A8
+#define FW_CUT_THR_PKT			0x01B0
+#define FW_HOST_RX_PKT_CNT		0x0248
+#define FW_HOST_TX_PKT_CNT		0x0250
+#define FW_HOST_EGRESS_Q_PRE_OVERFLOW	0x0258
+#define FW_HOST_EGRESS_Q_EXP_OVERFLOW	0x0260
+
 #endif /* __NET_TI_ICSSG_SWITCH_MAP_H  */

base-commit: f252f23ab657cd224cb8334ba69966396f3f629b
-- 
2.34.1

Re: [PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

[Jakub Kicinski]

On Wed, 5 Mar 2025 16:46:08 +0530 MD Danish Anwar wrote:
> + - ``FW_RTU_PKT_DROP``: Diagnostic error counter which increments when RTU drops a locally injected packet due to port being disabled or rule violation.
> + - ``FW_Q0_OVERFLOW``: TX overflow counter for queue0
> + - ``FW_Q1_OVERFLOW``: TX overflow counter for queue1
> + - ``FW_Q2_OVERFLOW``: TX overflow counter for queue2
> + - ``FW_Q3_OVERFLOW``: TX overflow counter for queue3
> + - ``FW_Q4_OVERFLOW``: TX overflow counter for queue4
> + - ``FW_Q5_OVERFLOW``: TX overflow counter for queue5
> + - ``FW_Q6_OVERFLOW``: TX overflow counter for queue6
> + - ``FW_Q7_OVERFLOW``: TX overflow counter for queue7
...

Thanks for the docs, it looks good. Now, do all of these get included
in the standard stats returned by icssg_ndo_get_stats64 ?
That's the primary source of information for the user regarding packet
loss.

>  	if (prueth->pa_stats) {
>  		for (i = 0; i < ARRAY_SIZE(icssg_all_pa_stats); i++) {
> -			reg = ICSSG_FW_STATS_BASE +
> -			      icssg_all_pa_stats[i].offset *
> -			      PRUETH_NUM_MACS + slice * sizeof(u32);
> +			reg = icssg_all_pa_stats[i].offset +
> +			      slice * sizeof(u32);
>  			regmap_read(prueth->pa_stats, reg, &val);
>  			emac->pa_stats[i] += val;

This gets called by icssg_ndo_get_stats64() which is under RCU 
protection and nothing else. I don't see any locking here, and
I hope the regmap doesn't sleep. cat /proc/net/dev to test.
You probably need to send some fixes to net.
-- 
pw-bot: cr

Re: [PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

[MD Danish Anwar]

Hi Jakub

On 07/03/25 6:25 am, Jakub Kicinski wrote:
> On Wed, 5 Mar 2025 16:46:08 +0530 MD Danish Anwar wrote:
>> + - ``FW_RTU_PKT_DROP``: Diagnostic error counter which increments when RTU drops a locally injected packet due to port being disabled or rule violation.
>> + - ``FW_Q0_OVERFLOW``: TX overflow counter for queue0
>> + - ``FW_Q1_OVERFLOW``: TX overflow counter for queue1
>> + - ``FW_Q2_OVERFLOW``: TX overflow counter for queue2
>> + - ``FW_Q3_OVERFLOW``: TX overflow counter for queue3
>> + - ``FW_Q4_OVERFLOW``: TX overflow counter for queue4
>> + - ``FW_Q5_OVERFLOW``: TX overflow counter for queue5
>> + - ``FW_Q6_OVERFLOW``: TX overflow counter for queue6
>> + - ``FW_Q7_OVERFLOW``: TX overflow counter for queue7
> ...
> 
> Thanks for the docs, it looks good. Now, do all of these get included
> in the standard stats returned by icssg_ndo_get_stats64 ?
> That's the primary source of information for the user regarding packet
> loss.

No, these are not reported via icssg_ndo_get_stats64.

.ndo_get_stats64 populates stats that are part of `struct
rtnl_link_stats64`. icssg_ndo_get_stats64 populates those stats wherever
applicable. These firmware stats are not same as the ones defined in
`icssg_ndo_get_stats64` hence they are not populated. They are not
standard stats, they will be dumped by `ethtool -S`. Wherever there is a
standard stats, I will make sure it gets dumped from the standard
interface instead of `ethtool -S`

Only below stats are included in the standard stats returned by
icssg_ndo_get_stats64
- rx_packets
- rx_bytes
- tx_packets
- tx_bytes
- rx_crc_errors
- rx_over_errors
- rx_multicast_frames

> 
>>  	if (prueth->pa_stats) {
>>  		for (i = 0; i < ARRAY_SIZE(icssg_all_pa_stats); i++) {
>> -			reg = ICSSG_FW_STATS_BASE +
>> -			      icssg_all_pa_stats[i].offset *
>> -			      PRUETH_NUM_MACS + slice * sizeof(u32);
>> +			reg = icssg_all_pa_stats[i].offset +
>> +			      slice * sizeof(u32);
>>  			regmap_read(prueth->pa_stats, reg, &val);
>>  			emac->pa_stats[i] += val;
> 
> This gets called by icssg_ndo_get_stats64() which is under RCU 

Yes, this does get called by icssg_ndo_get_stats64(). Apart from that
there is a workqueue (`icssg_stats_work_handler`) that calls this API
periodically and updates the emac->stats and emac->pa_stats arrays.

> protection and nothing else. I don't see any locking here, and

There is no locking here. I don't think this is related to the patch.
The API emac_update_hardware_stats() updates all the stats supported by
ICSSG not just standard stats.

> I hope the regmap doesn't sleep. cat /proc/net/dev to test.
> You probably need to send some fixes to net.

I checked cat /proc/net/dev and the stats shown there are not related to
the stats I am introducing in this series.

The fix could be to add a lock in this function, but we have close to 90
stats and this function is called not only from icssg_ndo_get_stats64()
but from emac_get_ethtool_stats(). The function also gets called
periodically (Every 25 Seconds for 1G Link). I think every time locking
90 regmap_reads() could result in performance degradation.

I only see couple of drivers acquiring spin lock before reading the
stats for .ndo_get_stats64. Most of the drivers are not using any lock.

I did some testing and did not see any discrepancy in the stats `cat
/proc/net/dev` without the lock.

Furthermore, the fix is independent of this patch. I can send out a
separate fix to net to add cpu locks to this function. But I don't think
there is any change needed in this patch.

Let me know what should be done here.

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

[Jakub Kicinski]

On Fri, 7 Mar 2025 16:00:40 +0530 MD Danish Anwar wrote:
> > Thanks for the docs, it looks good. Now, do all of these get included
> > in the standard stats returned by icssg_ndo_get_stats64 ?
> > That's the primary source of information for the user regarding packet
> > loss.  
> 
> No, these are not reported via icssg_ndo_get_stats64.
> 
> .ndo_get_stats64 populates stats that are part of `struct
> rtnl_link_stats64`. icssg_ndo_get_stats64 populates those stats wherever
> applicable. These firmware stats are not same as the ones defined in
> `icssg_ndo_get_stats64` hence they are not populated. They are not
> standard stats, they will be dumped by `ethtool -S`. Wherever there is a
> standard stats, I will make sure it gets dumped from the standard
> interface instead of `ethtool -S`
> 
> Only below stats are included in the standard stats returned by
> icssg_ndo_get_stats64
> - rx_packets
> - rx_bytes
> - tx_packets
> - tx_bytes
> - rx_crc_errors
> - rx_over_errors
> - rx_multicast_frames

Yes, but if the stats you're adding here relate to packets sent /
destined to the host which were lost you should include them
in the aggregate rx_errors / rx_dropped / tx_errors / tx_dropped.
I understand that there's unlikely to be a 1:1 match with specific
stats.

> > This gets called by icssg_ndo_get_stats64() which is under RCU   
> 
> Yes, this does get called by icssg_ndo_get_stats64(). Apart from that
> there is a workqueue (`icssg_stats_work_handler`) that calls this API
> periodically and updates the emac->stats and emac->pa_stats arrays.
>
> > protection and nothing else. I don't see any locking here, and  
> 
> There is no locking here. I don't think this is related to the patch.
> The API emac_update_hardware_stats() updates all the stats supported by
> ICSSG not just standard stats.

Yes, I'm saying you should send a separate fix, not really related or
blocking this patch (unless they conflict)

> > I hope the regmap doesn't sleep. cat /proc/net/dev to test.
> > You probably need to send some fixes to net.  
> 
> I checked cat /proc/net/dev and the stats shown there are not related to
> the stats I am introducing in this series.

You misunderstood. I pointed that you so you can check on a debug
kernel if there are any warnings (e.g. something tries to sleep
since /proc/net/dev read is under RCU lock).

> The fix could be to add a lock in this function, but we have close to 90
> stats and this function is called not only from icssg_ndo_get_stats64()
> but from emac_get_ethtool_stats(). The function also gets called
> periodically (Every 25 Seconds for 1G Link). I think every time locking
> 90 regmap_reads() could result in performance degradation.

Correctness comes first.

Re: [PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

[MD Danish Anwar]

On 07/03/25 10:09 pm, Jakub Kicinski wrote:
> On Fri, 7 Mar 2025 16:00:40 +0530 MD Danish Anwar wrote:
>>> Thanks for the docs, it looks good. Now, do all of these get included
>>> in the standard stats returned by icssg_ndo_get_stats64 ?
>>> That's the primary source of information for the user regarding packet
>>> loss.  
>>
>> No, these are not reported via icssg_ndo_get_stats64.
>>
>> .ndo_get_stats64 populates stats that are part of `struct
>> rtnl_link_stats64`. icssg_ndo_get_stats64 populates those stats wherever
>> applicable. These firmware stats are not same as the ones defined in
>> `icssg_ndo_get_stats64` hence they are not populated. They are not
>> standard stats, they will be dumped by `ethtool -S`. Wherever there is a
>> standard stats, I will make sure it gets dumped from the standard
>> interface instead of `ethtool -S`
>>
>> Only below stats are included in the standard stats returned by
>> icssg_ndo_get_stats64
>> - rx_packets
>> - rx_bytes
>> - tx_packets
>> - tx_bytes
>> - rx_crc_errors
>> - rx_over_errors
>> - rx_multicast_frames
> 
> Yes, but if the stats you're adding here relate to packets sent /
> destined to the host which were lost you should include them
> in the aggregate rx_errors / rx_dropped / tx_errors / tx_dropped.
> I understand that there's unlikely to be a 1:1 match with specific
> stats.
> 

Sure, I will try to add such stats.

>>> This gets called by icssg_ndo_get_stats64() which is under RCU   
>>
>> Yes, this does get called by icssg_ndo_get_stats64(). Apart from that
>> there is a workqueue (`icssg_stats_work_handler`) that calls this API
>> periodically and updates the emac->stats and emac->pa_stats arrays.
>>
>>> protection and nothing else. I don't see any locking here, and  
>>
>> There is no locking here. I don't think this is related to the patch.
>> The API emac_update_hardware_stats() updates all the stats supported by
>> ICSSG not just standard stats.
> 
> Yes, I'm saying you should send a separate fix, not really related or
> blocking this patch (unless they conflict)
> 

Sure. I will send v3 of this and a fix to net to add spin_lock before
reading stats. I will try to make sure that they don't conflict so that
they can be merged parallelly.

>>> I hope the regmap doesn't sleep. cat /proc/net/dev to test.
>>> You probably need to send some fixes to net.  
>>
>> I checked cat /proc/net/dev and the stats shown there are not related to
>> the stats I am introducing in this series.
> 
> You misunderstood. I pointed that you so you can check on a debug
> kernel if there are any warnings (e.g. something tries to sleep
> since /proc/net/dev read is under RCU lock).
> 
>> The fix could be to add a lock in this function, but we have close to 90
>> stats and this function is called not only from icssg_ndo_get_stats64()
>> but from emac_get_ethtool_stats(). The function also gets called
>> periodically (Every 25 Seconds for 1G Link). I think every time locking
>> 90 regmap_reads() could result in performance degradation.
> 
> Correctness comes first.

Understood.

-- 
Thanks and Regards,
Danish