net: stmmac: fix interrupt coalescing
Subsystems (4)
v2 cover net: stmmac: fix interrupt coalescing
Hi,
While cleaning up the descriptor handling, I noticed that the accounting of transmit "packets" for interrupt coalescing was buggy in that it takes the difference of the two indexes into the circular list of transmit discriptors and merely subtracts one from the other without regard for the indexes wrapping.
This can result in a negative number or very large positive number which would have the effect of either reducing tx_q->tx_count_frames or making that very large.
Either way, the result is numerically incorrect, and could trigger interrupts or not trigger interrupts when required.
This series converts stmmac to use the circ_buf helpers, and then fixes this problem.
v2: move build fix to correct patch
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 28 ++++++++--------------- 1 file changed, 10 insertions(+), 18 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
PATCH v2 1/2 net: stmmac: use circ_buf helpers for descriptors
The stmmac descriptor queues are circular buffers, operated as far as the hardware is concerned as either a ring, or a chain that loops back on itself. From the software perspective, it forms a circular buffer.
We have a few places which calculate the number of in-use and free entries in these circular buffers, for which we have macros for. Use CIRC_CNT() and CIRC_SPACE() as appropriate to calculate these values.
Validating, for stmmac_tx_avail(), which uses CIRC_SPACE():
dirty_tx = 1, cur_tx = 0 -> 0
dirty_tx = 0, cur_tx = 0 -> dma_tx_size - 1
dirty_tx = 0, cur_tx = 1 -> dma_tx_size - 2
dirty_tx passed as end, reduced by one. cur_tx passed as start. Output on sane computers is identical.
For stmmac_rx_dirty(), which uses CIRC_CNT():
dirty_rx = 1, cur_rx = 0 -> dma_rx_size - 1
dirty_rx = 0, cur_rx = 0 -> 0
dirty_rx = 0, cur_rx = 1 -> 1
dirty_rx passed as start, cur_rx passed as end. Output is identical.
Same validation performed on the is_last_segment calculation, which also gets converted to CIRC_CNT().
--- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -16
@@ -14,6 +14,7 @@https://bugzilla.stlinux.com/ *******************************************************************************/ +#include <linux/circ_buf.h> #include <linux/clk.h> #include <linux/kernel.h> #include <linux/interrupt.h>
@@ -355,14 +356,9 @@ static void print_pkt(unsigned char *buf, int len)static inline u32 stmmac_tx_avail (struct stmmac_priv *priv,u32 queue) {struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue]; -u32 avail; -if (tx_q->dirty_tx > tx_q->cur_tx) - avail = tx_q->dirty_tx - tx_q->cur_tx -1 ; -else - avail = priv->dma_conf.dma_tx_size - tx_q->cur_tx + tx_q->dirty_tx -1 ; - -return avail; +return CIRC_SPACE (tx_q->cur_tx, tx_q->dirty_tx, + priv->dma_conf.dma_tx_size); }/**
@@ -373,14 +369,9 @@ static inline u32 stmmac_tx_avail(struct stmmac_priv *priv, u32 queue)static inline u32 stmmac_rx_dirty (struct stmmac_priv *priv,u32 queue) {struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue]; -u32 dirty; - -if (rx_q->dirty_rx <= rx_q->cur_rx) - dirty = rx_q->cur_rx - rx_q->dirty_rx; -else - dirty = priv->dma_conf.dma_rx_size - rx_q->dirty_rx + rx_q->cur_rx; -return dirty; +return CIRC_CNT (rx_q->cur_rx, rx_q->dirty_rx, + priv->dma_conf.dma_rx_size); }static bool stmmac_eee_tx_busy (struct stmmac_priv *priv)
@@ -4571,8 +4562,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)/* If we only have one entry used, then the first entry is the last * segment. */ - is_last_segment = ((tx_q->cur_tx - first_entry) & - (priv->dma_conf.dma_tx_size -1 )) ==1 ; + is_last_segment =CIRC_CNT (tx_q->cur_tx, first_entry, + priv->dma_conf.dma_tx_size) ==1 ;/* Complete the first descriptor before granting the DMA */ stmmac_prepare_tso_tx_desc(priv, first, 1, proto_hdr_len, 0, 1,
Signature · 1 line
2.47.3
PATCH v2 2/2 net: stmmac: fix transmit interrupt coalescing
In reply to Russell King (Oracle)The accounting for transmit frames does not count the descriptors correctly. It uses:
tx_packets = (tx_q->cur_tx + 1) - first_tx;
however, these are indexes into a circular buffer, so cur_tx can be less than first_tx, and when that happens, tx_packets becomes a very large unsigned integer. When this is added to tx_q->tx_count_frames, it has the effect of reducing the count of frames, possibly causing it to also wrap to a very large unsigned integer.
Fix this by using CIRC_CNT() to calculate the number of descriptors used.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +3 -2
@@ -4503,7 +4503,8 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev) tx_q->tx_skbuff_dma[tx_q->cur_tx].buf_type =STMMAC_TXBUF_T_SKB ;/* Manage tx mitigation */ - tx_packets = (tx_q->cur_tx +1 ) - first_tx; + tx_packets =CIRC_CNT (tx_q->cur_tx +1 , first_tx, + priv->dma_conf.dma_tx_size); tx_q->tx_count_frames += tx_packets;if ((skb_shinfo (skb)->tx_flags &SKBTX_HW_TSTAMP ) && priv->hwts_tx_en)
@@ -4781,7 +4782,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)* This approach takes care about the fragments: desc is the first * element in case of no SG. */ - tx_packets = (entry +1 ) - first_tx; + tx_packets =CIRC_CNT (entry +1 , first_tx, priv->dma_conf.dma_tx_size); tx_q->tx_count_frames += tx_packets;if ((skb_shinfo (skb)->tx_flags &SKBTX_HW_TSTAMP ) && priv->hwts_tx_en)
Signature · 1 line
2.47.3
Quoted 15 lines · Russell King (Oracle)
Hi,
While cleaning up the descriptor handling, I noticed that the accounting of transmit "packets" for interrupt coalescing was buggy in that it takes the difference of the two indexes into the circular list of transmit discriptors and merely subtracts one from the other without regard for the indexes wrapping.
This can result in a negative number or very large positive number which would have the effect of either reducing
tx_q->tx_count_framesor making that very large.Either way, the result is numerically incorrect, and could trigger interrupts or not trigger interrupts when required.
Quoted 8 lines
This series converts stmmac to use the
circ_bufhelpers, and then fixes this problem.v2: move build fix to correct patch
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 28 ++++++++--------------- 1 file changed, 10 insertions(+), 18 deletions(-)
Thanks Russell.
For the series,
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Quoted 9 lines · On Tue, 24 Feb 2026 09:01:13 +0000 you wrote:
Hi,
While cleaning up the descriptor handling, I noticed that the accounting of transmit "packets" for interrupt coalescing was buggy in that it takes the difference of the two indexes into the circular list of transmit discriptors and merely subtracts one from the other without regard for the indexes wrapping.
[...]
Here is the summary with links:
- [net-next,v2,1/2] net: stmmac: use circ_buf helpers for descriptors
https://git.kernel.org/netdev/net-next/c/819101c3c158 - [net-next,v2,2/2] net: stmmac: fix transmit interrupt coalescing https://git.kernel.org/netdev/net-next/c/dd53a0e85969
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html