From: Rohan G Thomas <rohan.g.thomas@altera.com>
Currently, in the AF_XDP transmit paths, the CIC bit of
TX Desc3 is set for all packets. Setting this bit for
packets transmitting through queues that don't support
checksum offloading causes the TX DMA to get stuck after
transmitting some packets. This patch ensures the CIC bit
of TX Desc3 is set only if the TX queue supports checksum
offloading.
Fixes: 132c32ee5bc0 ("net: stmmac: Add TX via XDP zero-copy socket")
Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9a77390b7f9da4199ad6ac15a2149e2c703900ce..88b7e0aed14428c1884f4c3610c4112e9be8fd59 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2584,6 +2584,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
struct netdev_queue *nq = netdev_get_tx_queue(priv->dev, queue);
struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[queue];
+ bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;
struct xsk_buff_pool *pool = tx_q->xsk_pool;
unsigned int entry = tx_q->cur_tx;
struct dma_desc *tx_desc = NULL;
@@ -2671,7 +2672,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
}
stmmac_prepare_tx_desc(priv, tx_desc, 1, xdp_desc.len,
- true, priv->mode, true, true,
+ csum, priv->mode, true, true,
xdp_desc.len);
stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
@@ -4983,6 +4984,7 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
{
struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[queue];
struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
+ bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;
unsigned int entry = tx_q->cur_tx;
struct dma_desc *tx_desc;
dma_addr_t dma_addr;
@@ -5034,7 +5036,7 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
stmmac_set_desc_addr(priv, tx_desc, dma_addr);
stmmac_prepare_tx_desc(priv, tx_desc, 1, xdpf->len,
- true, priv->mode, true, true,
+ csum, priv->mode, true, true,
xdpf->len);
tx_q->tx_count_frames++;
--
2.32.0On Sat, 16 Aug 2025 00:55:25 +0800 Rohan G Thomas via B4 Relay wrote: > + bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported; Hopefully the slight pointer chasing here doesn't impact performance? XDP itself doesn't support checksum so perhaps we could always pass false?
Hi Jakub, Thanks for reviewing the patch. On 8/20/2025 6:52 AM, Jakub Kicinski wrote: > On Sat, 16 Aug 2025 00:55:25 +0800 Rohan G Thomas via B4 Relay wrote: >> + bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported; > > Hopefully the slight pointer chasing here doesn't impact performance? > XDP itself doesn't support checksum so perhaps we could always pass > false? I'm not certain whether some XDP applications might be benefiting from checksum offloading currently, and so passing false unconditionally could cause regressions. Best Regards, Rohan
On Wed, 20 Aug 2025 12:44:18 +0530 G Thomas, Rohan wrote: > On 8/20/2025 6:52 AM, Jakub Kicinski wrote: > > On Sat, 16 Aug 2025 00:55:25 +0800 Rohan G Thomas via B4 Relay wrote: > >> + bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported; > > > > Hopefully the slight pointer chasing here doesn't impact performance? > > XDP itself doesn't support checksum so perhaps we could always pass > > false? > > I'm not certain whether some XDP applications might be benefiting from > checksum offloading currently Checksum offload is not supported in real XDP, AFAIK, and in AF_XDP the driver must implement a checksum callback which stmmac does not do. IOW it's not possible to use Tx checksum offload in stmmac today from XDP.
On Wed, 20 Aug 2025 08:54:46 -0700 Jakub Kicinski wrote: > On Wed, 20 Aug 2025 12:44:18 +0530 G Thomas, Rohan wrote: > > On 8/20/2025 6:52 AM, Jakub Kicinski wrote: > > > Hopefully the slight pointer chasing here doesn't impact performance? > > > XDP itself doesn't support checksum so perhaps we could always pass > > > false? > > > > I'm not certain whether some XDP applications might be benefiting from > > checksum offloading currently > > Checksum offload is not supported in real XDP, AFAIK, and in AF_XDP > the driver must implement a checksum callback which stmmac does not do. > IOW it's not possible to use Tx checksum offload in stmmac today from > XDP. To be clear -- this is just for context. I don't understand the details of what the CIC bit controls, so the final decision is up to you.
Hi Jakub, On 8/20/2025 9:26 PM, Jakub Kicinski wrote: > On Wed, 20 Aug 2025 08:54:46 -0700 Jakub Kicinski wrote: >> On Wed, 20 Aug 2025 12:44:18 +0530 G Thomas, Rohan wrote: >>> On 8/20/2025 6:52 AM, Jakub Kicinski wrote: >>>> Hopefully the slight pointer chasing here doesn't impact performance? >>>> XDP itself doesn't support checksum so perhaps we could always pass >>>> false? >>> >>> I'm not certain whether some XDP applications might be benefiting from >>> checksum offloading currently >> >> Checksum offload is not supported in real XDP, AFAIK, and in AF_XDP >> the driver must implement a checksum callback which stmmac does not do. >> IOW it's not possible to use Tx checksum offload in stmmac today from >> XDP. > > To be clear -- this is just for context. I don't understand the details > of what the CIC bit controls, so the final decision is up to you. Currently, in the stmmac driver, even though tmo_request_checksum is not implemented, checksum offloading is still effectively enabled for AF_XDP frames, as CIC bit for tx desc are set, which implies checksum calculation and insertion by hardware for IP packets. So, I'm thinking it is better to keep it as false only for queues that do not support COE. Best Regards, Rohan
On Thu, 21 Aug 2025 19:20:02 +0530 G Thomas, Rohan wrote: > > To be clear -- this is just for context. I don't understand the details > > of what the CIC bit controls, so the final decision is up to you. > > Currently, in the stmmac driver, even though tmo_request_checksum is not > implemented, checksum offloading is still effectively enabled for AF_XDP > frames, as CIC bit for tx desc are set, which implies checksum > calculation and insertion by hardware for IP packets. So, I'm thinking > it is better to keep it as false only for queues that do not support > COE. Oh, so the device parses the packet and inserts the checksum whether user asked for it or not? Damn, I guess it may indeed be too late to fix, but that certainly _not_ how AF_XDP is supposed to work. The frame should not be modified without user asking for it..
Hi Jakub, On 8/21/2025 7:47 PM, Jakub Kicinski wrote: >> Currently, in the stmmac driver, even though tmo_request_checksum is not >> implemented, checksum offloading is still effectively enabled for AF_XDP >> frames, as CIC bit for tx desc are set, which implies checksum >> calculation and insertion by hardware for IP packets. So, I'm thinking >> it is better to keep it as false only for queues that do not support >> COE. > Oh, so the device parses the packet and inserts the checksum whether > user asked for it or not? Damn, I guess it may indeed be too late > to fix, but that certainly_not_ how AF_XDP is supposed to work. > The frame should not be modified without user asking for it.. Yes, I also agreed. But since not sure, currently any XDP applications are benefiting from hw checksum, I think it's more reasonable to keep csum flag as false only for queues that do not support COE, while maintaining current behavior for queues that do support it. Please let me know if you think otherwise. Best Regards, Rohan
On Fri, 22 Aug 2025 18:19:01 +0530 G Thomas, Rohan wrote: > On 8/21/2025 7:47 PM, Jakub Kicinski wrote: > >> Currently, in the stmmac driver, even though tmo_request_checksum is not > >> implemented, checksum offloading is still effectively enabled for AF_XDP > >> frames, as CIC bit for tx desc are set, which implies checksum > >> calculation and insertion by hardware for IP packets. So, I'm thinking > >> it is better to keep it as false only for queues that do not support > >> COE. > > Oh, so the device parses the packet and inserts the checksum whether > > user asked for it or not? Damn, I guess it may indeed be too late > > to fix, but that certainly_not_ how AF_XDP is supposed to work. > > The frame should not be modified without user asking for it.. > > Yes, I also agreed. But since not sure, currently any XDP applications > are benefiting from hw checksum, I think it's more reasonable to keep > csum flag as false only for queues that do not support COE, while > maintaining current behavior for queues that do support it. Please let > me know if you think otherwise. Agreed.
© 2016 - 2025 Red Hat, Inc.