[PATCH net] net: fec: remove unnecessary DMA mapping of TSO header

[PATCH net] net: fec: remove unnecessary DMA mapping of TSO header

[Dheeraj Reddy Jonnalagadda]

The TSO header buffer is pre-allocated DMA memory, so there's no need to
map it again with dma_map_single() in fec_enet_txq_put_hdr_tso(). Remove
this redundant mapping operation.

Fixes: 79f339125ea3 ("net: fec: Add software TSO support")
Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 68725506a095..039de4c5044e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -805,15 +805,6 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q *txq,
 
 		if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
 			swap_buffer(bufaddr, hdr_len);
-
-		dmabuf = dma_map_single(&fep->pdev->dev, bufaddr,
-					hdr_len, DMA_TO_DEVICE);
-		if (dma_mapping_error(&fep->pdev->dev, dmabuf)) {
-			dev_kfree_skb_any(skb);
-			if (net_ratelimit())
-				netdev_err(ndev, "Tx DMA memory map failed\n");
-			return NETDEV_TX_OK;
-		}
 	}
 
 	bdp->cbd_bufaddr = cpu_to_fec32(dmabuf);
-- 
2.34.1

Re: [PATCH net] net: fec: remove unnecessary DMA mapping of TSO header

[Simon Horman]

On Wed, Jan 22, 2025 at 04:13:07PM +0530, Dheeraj Reddy Jonnalagadda wrote:
> The TSO header buffer is pre-allocated DMA memory, so there's no need to
> map it again with dma_map_single() in fec_enet_txq_put_hdr_tso(). Remove
> this redundant mapping operation.
> 
> Fixes: 79f339125ea3 ("net: fec: Add software TSO support")
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 68725506a095..039de4c5044e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -805,15 +805,6 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q *txq,
>  
>  		if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
>  			swap_buffer(bufaddr, hdr_len);
> -
> -		dmabuf = dma_map_single(&fep->pdev->dev, bufaddr,
> -					hdr_len, DMA_TO_DEVICE);
> -		if (dma_mapping_error(&fep->pdev->dev, dmabuf)) {
> -			dev_kfree_skb_any(skb);
> -			if (net_ratelimit())
> -				netdev_err(ndev, "Tx DMA memory map failed\n");
> -			return NETDEV_TX_OK;
> -		}
>  	}
>  
>  	bdp->cbd_bufaddr = cpu_to_fec32(dmabuf);

Hi Dheeraj,

It seems to me that at this point of execution, without your patch, dmabuf
can be one of two values:

	1. txq->tso_hdrs_dma + index * TSO_HEADER_SIZE
	   This is the default case.

	2. txq->tx_bounce[index]
	   This is the special case, which is handed in the conditional
	   block that your patch updates.

In case 1, I agree that tso_hdrs_dma is already mapped.
But this case is not affected by your patch as the
conditional block that includes dma_map_single() is not executed.

In case 2, I do not see where txq->tx_bounce[index] is already mapped.

Am I missing something?

RE: [PATCH net] net: fec: remove unnecessary DMA mapping of TSO header

[Wei Fang]

> The TSO header buffer is pre-allocated DMA memory, so there's no need to
> map it again with dma_map_single() in fec_enet_txq_put_hdr_tso(). Remove
> this redundant mapping operation.
> 
> Fixes: 79f339125ea3 ("net: fec: Add software TSO support")
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 68725506a095..039de4c5044e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -805,15 +805,6 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q
> *txq,
> 
>  		if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
>  			swap_buffer(bufaddr, hdr_len);
> -
> -		dmabuf = dma_map_single(&fep->pdev->dev, bufaddr,
> -					hdr_len, DMA_TO_DEVICE);
> -		if (dma_mapping_error(&fep->pdev->dev, dmabuf)) {
> -			dev_kfree_skb_any(skb);
> -			if (net_ratelimit())
> -				netdev_err(ndev, "Tx DMA memory map failed\n");
> -			return NETDEV_TX_OK;
> -		}
>  	}
> 
>  	bdp->cbd_bufaddr = cpu_to_fec32(dmabuf);
> --
> 2.34.1

Hi Dheeraj,

I must admit that I misread it too. There is another case in the TSO
header where txq->tx_bounce may be used in some cases. I think
the most correct fix is to make txq->tso_hdrs aligned to 32/64 bytes
when allocating tso_hdrs, then we do not need to use txq->tx_bounce
in fec_enet_txq_put_hdr_tso(), because (bufaddr) & fep->tx_align)
will not be true. This way we can safely remove dma_map_single()
from fec_enet_txq_put_hdr_tso().

Re: [PATCH net] net: fec: remove unnecessary DMA mapping of TSO header

[Dheeraj Reddy Jonnalagadda]

On Thu, Jan 23, 2025 at 02:58:32AM +0000, Wei Fang wrote:
> 
> Hi Dheeraj,
> 
> I must admit that I misread it too. There is another case in the TSO
> header where txq->tx_bounce may be used in some cases. I think
> the most correct fix is to make txq->tso_hdrs aligned to 32/64 bytes
> when allocating tso_hdrs, then we do not need to use txq->tx_bounce
> in fec_enet_txq_put_hdr_tso(), because (bufaddr) & fep->tx_align)
> will not be true. This way we can safely remove dma_map_single()
> from fec_enet_txq_put_hdr_tso().

Hi Fang, Simon,

Thank you for the feedback. I have a clarification question regarding 
the alignment of txq->tso_hdrs.

In the current code, txq->tso_hdrs is allocated using fec_dma_alloc(), 
which internally calls dma_alloc_coherent(). As I understand it, 
dma_alloc_coherent() guarantees that the allocated buffer is properly aligned.

Given this, should we remove the alignment check 
((unsigned long)bufaddr) & fep->tx_align and the associated dma_map_single() 
logic entirely from fec_enet_txq_put_hdr_tso() as you have suggested?

-Dheeraj

RE: [PATCH net] net: fec: remove unnecessary DMA mapping of TSO header

[Wei Fang]

> > Hi Dheeraj,
> >
> > I must admit that I misread it too. There is another case in the TSO
> > header where txq->tx_bounce may be used in some cases. I think
> > the most correct fix is to make txq->tso_hdrs aligned to 32/64 bytes
> > when allocating tso_hdrs, then we do not need to use txq->tx_bounce
> > in fec_enet_txq_put_hdr_tso(), because (bufaddr) & fep->tx_align)
> > will not be true. This way we can safely remove dma_map_single()
> > from fec_enet_txq_put_hdr_tso().
> 
> Hi Fang, Simon,
> 
> Thank you for the feedback. I have a clarification question regarding
> the alignment of txq->tso_hdrs.
> 
> In the current code, txq->tso_hdrs is allocated using fec_dma_alloc(),
> which internally calls dma_alloc_coherent(). As I understand it,
> dma_alloc_coherent() guarantees that the allocated buffer is properly aligned.
> 
> Given this, should we remove the alignment check
> ((unsigned long)bufaddr) & fep->tx_align and the associated dma_map_single()
> logic entirely from fec_enet_txq_put_hdr_tso() as you have suggested?
> 

The maximum of fep->tx_align is 0xf, so we need to ensure the buffer
allocated by dma_alloc_coherent() is at least 16-byte aligned, but I'm
not sure whether dma_alloc_coherent() guarantees this alignment.
A safe approach is to apply for a memory block longer than expected,
for example, if we want the alignment is 32-byte, we can apply for a
memory block that is 32 bytes longer than the expected length. Then
use ALGIN and PTR_ALIGN to get the aligned addr.

txq->tso_hdrs_base = fec_dma_alloc(&fep->pdev->dev,
					     	  txq->bd.ring_size * TSO_HEADER_SIZE + 32,
					     	  &txq->tso_hdrs_dma_base, GFP_KERNEL);
txq->tso_hdrs = PTR_ALIGN(txq->tso_hdrs_base, 32);
txq->tso_hdrs_dma = ALIGN(txq->tso_hdrs_dma_base, 32);

This is just my personal experience, if you have a better way that would
be great.