[PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[MD Danish Anwar]

ICSSG HW stats on TX side considers 8 preamble bytes as data bytes. Due
to this the tx_total_bytes of one interface doesn't match the
rx_total_bytes of other interface when two ICSSG interfaces are
connected with each other. There is no public errata available yet.

As a workaround to fix this, decrease tx_total_bytes by 8 bytes for every
tx frame.

Fixes: c1e10d5dc7a1 ("net: ti: icssg-prueth: Add ICSSG Stats")
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_stats.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
index bb0b33927e3b..dc12edcbac02 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
@@ -18,6 +18,7 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
 	struct prueth *prueth = emac->prueth;
 	int slice = prueth_emac_slice(emac);
 	u32 base = stats_base[slice];
+	u32 tx_pkt_cnt = 0;
 	u32 val;
 	int i;
 
@@ -29,7 +30,12 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
 			     base + icssg_all_stats[i].offset,
 			     val);
 
+		if (!strncmp(icssg_ethtool_stats[i].name, "tx_good_frames", ETH_GSTRING_LEN))
+			tx_pkt_cnt = val;
+
 		emac->stats[i] += val;
+		if (!strncmp(icssg_ethtool_stats[i].name, "tx_total_bytes", ETH_GSTRING_LEN))
+			emac->stats[i] -= tx_pkt_cnt * 8;
 	}
 }
 
-- 
2.34.1

Re: [PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[Ravi Gunasekaran]

On 10/11/23 12:07 PM, MD Danish Anwar wrote:
> ICSSG HW stats on TX side considers 8 preamble bytes as data bytes. Due
> to this the tx_total_bytes of one interface doesn't match the
> rx_total_bytes of other interface when two ICSSG interfaces are

The errata is on the ICSSG Tx side regardless of which interface it is
connected to. Please rephrase this part of the message to something like,
"rx_total_bytes of the link partner".

> connected with each other. There is no public errata available yet.
> 
> As a workaround to fix this, decrease tx_total_bytes by 8 bytes for every
> tx frame.
> 
> Fixes: c1e10d5dc7a1 ("net: ti: icssg-prueth: Add ICSSG Stats")
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_stats.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> index bb0b33927e3b..dc12edcbac02 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> @@ -18,6 +18,7 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
>  	struct prueth *prueth = emac->prueth;
>  	int slice = prueth_emac_slice(emac);
>  	u32 base = stats_base[slice];
> +	u32 tx_pkt_cnt = 0;
>  	u32 val;
>  	int i;
>  
> @@ -29,7 +30,12 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
>  			     base + icssg_all_stats[i].offset,
>  			     val);
>  
> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_good_frames", ETH_GSTRING_LEN))
> +			tx_pkt_cnt = val;
> +
>  		emac->stats[i] += val;
> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_total_bytes", ETH_GSTRING_LEN))
> +			emac->stats[i] -= tx_pkt_cnt * 8;
>  	}
>  }
>  

-- 
Regards,
Ravi

Re: [PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[Andrew Lunn]

> @@ -29,7 +30,12 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
>  			     base + icssg_all_stats[i].offset,
>  			     val);
>  
> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_good_frames", ETH_GSTRING_LEN))
> +			tx_pkt_cnt = val;

Doing a strncmp seems very expensive. Could you make use of
icssg_stats.offset?

	Andrew

Re: [PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[MD Danish Anwar]

Hi Andrew,

On 11/10/23 18:11, Andrew Lunn wrote:
>> @@ -29,7 +30,12 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
>>  			     base + icssg_all_stats[i].offset,
>>  			     val);
>>  
>> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_good_frames", ETH_GSTRING_LEN))
>> +			tx_pkt_cnt = val;
> 
> Doing a strncmp seems very expensive. Could you make use of
> icssg_stats.offset?
> 

Sure. I can define the offset of these two stats and then use them in if
condition as below.

#define ICSSG_TX_PACKET_OFFSET 0xA0
#define ICSSG_TX_BYTE_OFFSET   0xEC

if (icssg_ethtool_stats[i].offset == ICSSG_TX_PACKET_OFFSET)
	tx_pkt_cnt = val;

if (icssg_ethtool_stats[i].offset == ICSSG_TX_BYTE_OFFSET)
	emac->stats[i] -= tx_pkt_cnt * 8;


Pls let me know if this looks OK.

> 	Andrew

-- 
Thanks and Regards,
Danish

Re: [PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[MD Danish Anwar]

On 11/10/23 15:16, Ravi Gunasekaran wrote:
> 
> 
> On 10/11/23 12:07 PM, MD Danish Anwar wrote:
>> ICSSG HW stats on TX side considers 8 preamble bytes as data bytes. Due
>> to this the tx_total_bytes of one interface doesn't match the
>> rx_total_bytes of other interface when two ICSSG interfaces are
> 
> The errata is on the ICSSG Tx side regardless of which interface it is
> connected to. Please rephrase this part of the message to something like,
> "rx_total_bytes of the link partner".
> 

Sure Ravi, I'll update the commit message.

>> connected with each other. There is no public errata available yet.
>>
>> As a workaround to fix this, decrease tx_total_bytes by 8 bytes for every
>> tx frame.
>>
>> Fixes: c1e10d5dc7a1 ("net: ti: icssg-prueth: Add ICSSG Stats")
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_stats.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
>> index bb0b33927e3b..dc12edcbac02 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
>> @@ -18,6 +18,7 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
>>  	struct prueth *prueth = emac->prueth;
>>  	int slice = prueth_emac_slice(emac);
>>  	u32 base = stats_base[slice];
>> +	u32 tx_pkt_cnt = 0;
>>  	u32 val;
>>  	int i;
>>  
>> @@ -29,7 +30,12 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
>>  			     base + icssg_all_stats[i].offset,
>>  			     val);
>>  
>> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_good_frames", ETH_GSTRING_LEN))
>> +			tx_pkt_cnt = val;
>> +
>>  		emac->stats[i] += val;
>> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_total_bytes", ETH_GSTRING_LEN))
>> +			emac->stats[i] -= tx_pkt_cnt * 8;
>>  	}
>>  }
>>  
> 

-- 
Thanks and Regards,
Danish

Re: [PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[Andrew Lunn]

On Thu, Oct 12, 2023 at 10:51:12AM +0530, MD Danish Anwar wrote:
> Hi Andrew,
> 
> On 11/10/23 18:11, Andrew Lunn wrote:
> >> @@ -29,7 +30,12 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
> >>  			     base + icssg_all_stats[i].offset,
> >>  			     val);
> >>  
> >> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_good_frames", ETH_GSTRING_LEN))
> >> +			tx_pkt_cnt = val;
> > 
> > Doing a strncmp seems very expensive. Could you make use of
> > icssg_stats.offset?
> > 
> 
> Sure. I can define the offset of these two stats and then use them in if
> condition as below.
> 
> #define ICSSG_TX_PACKET_OFFSET 0xA0
> #define ICSSG_TX_BYTE_OFFSET   0xEC
> 
> if (icssg_ethtool_stats[i].offset == ICSSG_TX_PACKET_OFFSET)
> 	tx_pkt_cnt = val;
> 
> if (icssg_ethtool_stats[i].offset == ICSSG_TX_BYTE_OFFSET)
> 	emac->stats[i] -= tx_pkt_cnt * 8;

That is much better. Also consider adding something like:

BUILD_BUG_ON(ICSSG_TX_PACKET_OFFSET < ICSSG_TX_BYTE_OFFSET)

I've no idea if this is correct. Just something to prove at build time
that ICSSG_TX_PACKET_OFFSET is read before ICSSG_TX_BYTE_OFFSET.

     Andrew

Re: [PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count

[Anwar, Md Danish]

On 10/12/2023 8:58 PM, Andrew Lunn wrote:
> On Thu, Oct 12, 2023 at 10:51:12AM +0530, MD Danish Anwar wrote:
>> Hi Andrew,
>>
>> On 11/10/23 18:11, Andrew Lunn wrote:
>>>> @@ -29,7 +30,12 @@ void emac_update_hardware_stats(struct prueth_emac *emac)
>>>>  			     base + icssg_all_stats[i].offset,
>>>>  			     val);
>>>>  
>>>> +		if (!strncmp(icssg_ethtool_stats[i].name, "tx_good_frames", ETH_GSTRING_LEN))
>>>> +			tx_pkt_cnt = val;
>>>
>>> Doing a strncmp seems very expensive. Could you make use of
>>> icssg_stats.offset?
>>>
>>
>> Sure. I can define the offset of these two stats and then use them in if
>> condition as below.
>>
>> #define ICSSG_TX_PACKET_OFFSET 0xA0
>> #define ICSSG_TX_BYTE_OFFSET   0xEC
>>
>> if (icssg_ethtool_stats[i].offset == ICSSG_TX_PACKET_OFFSET)
>> 	tx_pkt_cnt = val;
>>
>> if (icssg_ethtool_stats[i].offset == ICSSG_TX_BYTE_OFFSET)
>> 	emac->stats[i] -= tx_pkt_cnt * 8;
> 
> That is much better. Also consider adding something like:
> 
> BUILD_BUG_ON(ICSSG_TX_PACKET_OFFSET < ICSSG_TX_BYTE_OFFSET)
> 
> I've no idea if this is correct. Just something to prove at build time
> that ICSSG_TX_PACKET_OFFSET is read before ICSSG_TX_BYTE_OFFSET.
> 

These registers are defined sequentially in the structure
miig_stats_regs. The offset for rx_packets is 0x0, rx_broadcast_frames
is 0x4 and so on. Basically the offset for i'th stat is i * sizeof(u32).

In the structure, tx_packet is defined first (index 40, offset 0xA0) and
then tx_bytes is defined (index 59, offset 0xEC).

In emac_update_hardware_stats() all these registers are read
sequentially. Meaning first tx_packet register is read and then tx_byte.

emac_update_hardware_stats() is called every 25s (by workqueue). Every
time first tx_packet is read and then tx_byte. So every time we are
decrementing tx_bytes by 8 bytes * num of packets, the num of packets
always exists and it is read before doing this calculation.

So I don't think any check is required to make sure
ICSSG_TX_PACKET_OFFSET is read before ICSSG_TX_BYTE_OFFSET.

The hardware design is such a way that these registers are read in a
sequence and the same sequence is followed in driver (struct
miig_stats_regs)

>      Andrew

-- 
Thanks and Regards,
Md Danish Anwar