Messages in this thread | | | Date | Thu, 12 Oct 2023 22:08:58 +0530 | Subject | Re: [PATCH] net: ti: icssg-prueth: Fix tx_total_bytes count | From | "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
| |