| Message ID | 20231011063700.1824093-1-danishanwar@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ti: icssg-prueth: Fix tx_total_bytes count | expand |
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; > } > } >
> @@ -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
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
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; >> } >> } >> >
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
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
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; } }
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(+)