[net PATCH] octeon_ep: fix tx dma unmap len values in SG

[net PATCH] octeon_ep: fix tx dma unmap len values in SG

[Shinas Rasheed]

Lengths of SG pointers are in big-endian

Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
 drivers/net/ethernet/marvell/octeon_ep/octep_tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
index 5a520d37bea0..7e99486c274b 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
@@ -69,12 +69,12 @@ int octep_iq_process_completions(struct octep_iq *iq, u16 budget)
 		compl_sg++;
 
 		dma_unmap_single(iq->dev, tx_buffer->sglist[0].dma_ptr[0],
-				 tx_buffer->sglist[0].len[0], DMA_TO_DEVICE);
+				 tx_buffer->sglist[0].len[3], DMA_TO_DEVICE);
 
 		i = 1; /* entry 0 is main skb, unmapped above */
 		while (frags--) {
 			dma_unmap_page(iq->dev, tx_buffer->sglist[i >> 2].dma_ptr[i & 3],
-				       tx_buffer->sglist[i >> 2].len[i & 3], DMA_TO_DEVICE);
+				       tx_buffer->sglist[i >> 2].len[3 - (i & 3)], DMA_TO_DEVICE);
 			i++;
 		}
 
-- 
2.25.1

Re: [net PATCH] octeon_ep: fix tx dma unmap len values in SG

[Simon Horman]

On Mon, Sep 11, 2023 at 02:23:06AM -0700, Shinas Rasheed wrote:
> Lengths of SG pointers are in big-endian
> 
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
>  drivers/net/ethernet/marvell/octeon_ep/octep_tx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
> index 5a520d37bea0..7e99486c274b 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
> @@ -69,12 +69,12 @@ int octep_iq_process_completions(struct octep_iq *iq, u16 budget)
>  		compl_sg++;
>  
>  		dma_unmap_single(iq->dev, tx_buffer->sglist[0].dma_ptr[0],
> -				 tx_buffer->sglist[0].len[0], DMA_TO_DEVICE);
> +				 tx_buffer->sglist[0].len[3], DMA_TO_DEVICE);
>  
>  		i = 1; /* entry 0 is main skb, unmapped above */
>  		while (frags--) {
>  			dma_unmap_page(iq->dev, tx_buffer->sglist[i >> 2].dma_ptr[i & 3],
> -				       tx_buffer->sglist[i >> 2].len[i & 3], DMA_TO_DEVICE);
> +				       tx_buffer->sglist[i >> 2].len[3 - (i & 3)], DMA_TO_DEVICE);
>  			i++;
>  		}

Hi Shinas,

is this change also needed in octep_iq_process_completions() ?
The code there looks rather similar.

Re: [net PATCH] octeon_ep: fix tx dma unmap len values in SG

[Shinas Rasheed]

Hi Simon,

This change is required in octep_iq_process_completions, as given in the patch,
since the scatter gather pointer lengths arrive as big-endian in hardware.

Re: [net PATCH] octeon_ep: fix tx dma unmap len values in SG

[Paolo Abeni]

On Tue, 2023-09-12 at 00:04 -0700, Shinas Rasheed wrote:
> This change is required in octep_iq_process_completions, as given in the patch,
> since the scatter gather pointer lengths arrive as big-endian in hardware.

I guess Simon intended asking about octep_iq_free_pending(), and AFAICT
your reply confirm that the change is required there, too.

Additionally the changelog really need to be expanded. I don't
understand how this change relates to endianess: if the ring format is
big endian I expect some be16_to_cpu(len) instead of complement-to-4 of
indexes.

Please clarify and expand the changelog, thanks!

Paolo

Re: [EXT] Re: [net PATCH] octeon_ep: fix tx dma unmap len values in SG

[Shinas Rasheed]

Hi Paolo, Hi Simon,


From: Paolo Abeni <pabeni@redhat.com>
Sent: Tuesday, September 12, 2023 2:15 PM
To: Shinas Rasheed <srasheed@marvell.com>; horms@kernel.org <horms@kernel.org>
Cc: Abhijit Ayarekar <aayarekar@marvell.com>; davem@davemloft.net <davem@davemloft.net>; edumazet@google.com <edumazet@google.com>; egallen@redhat.com <egallen@redhat.com>; Haseeb Gani <hgani@marvell.com>; kuba@kernel.org <kuba@kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; mschmidt@redhat.com <mschmidt@redhat.com>; netdev@vger.kernel.org <netdev@vger.kernel.org>; Satananda Burla <sburla@marvell.com>; Sathesh B Edara <sedara@marvell.com>; Veerasenareddy Burru <vburru@marvell.com>; Vimlesh Kumar <vimleshk@marvell.com>
Subject: [EXT] Re: [net PATCH] octeon_ep: fix tx dma unmap len values in SG 
 
External Email

----------------------------------------------------------------------


>I guess Simon intended asking about octep_iq_free_pending(), and AFAICT
your reply confirm that the change is required there, too.

You are correct in that the change is also required in octep_iq_free_pending as well. Thanks for pointing that out!
I will submit another version of this patchset including that.

>Additionally the changelog really need to be expanded. I don't
understand how this change relates to endianess: if the ring format is
big endian I expect some be16_to_cpu(len) instead of complement-to-4 of
indexes.


The bytes are in itself not big endian, but rather the each of the 16 bytes are kept in memory in
a kind of big-endian order. Apologizing for the confusion.

63              48 47             32  31           16 15       0
|      Len0       |     Len1         |   Len2         |   Len3  |

I shall provide an ascii figure like above in the code to explain and also update the changelog accordingly. Thanks for your time!

[net PATCH v2] octeon_ep: fix tx dma unmap len values in SG

[Shinas Rasheed]

Lengths of SG pointers are kept in the following order in
the SG entries in hardware.
 63      48|47     32|31     16|15       0
 -----------------------------------------
 |  Len 0  |  Len 1  |  Len 2  |  Len 3  |
 -----------------------------------------
 |                Ptr 0                  |
 -----------------------------------------
 |                Ptr 1                  |
 -----------------------------------------
 |                Ptr 2                  |
 -----------------------------------------
 |                Ptr 3                  |
 -----------------------------------------
Dma pointers have to be unmapped based on their
respective lengths given in this format.

Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
 .../net/ethernet/marvell/octeon_ep/octep_main.c  |  8 ++++----
 .../net/ethernet/marvell/octeon_ep/octep_tx.c    |  8 ++++----
 .../net/ethernet/marvell/octeon_ep/octep_tx.h    | 16 +++++++++++++++-
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 4424de2ffd70..dbc518ff8276 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -734,13 +734,13 @@ static netdev_tx_t octep_start_xmit(struct sk_buff *skb,
 dma_map_sg_err:
 	if (si > 0) {
 		dma_unmap_single(iq->dev, sglist[0].dma_ptr[0],
-				 sglist[0].len[0], DMA_TO_DEVICE);
-		sglist[0].len[0] = 0;
+				 sglist[0].len[3], DMA_TO_DEVICE);
+		sglist[0].len[3] = 0;
 	}
 	while (si > 1) {
 		dma_unmap_page(iq->dev, sglist[si >> 2].dma_ptr[si & 3],
-			       sglist[si >> 2].len[si & 3], DMA_TO_DEVICE);
-		sglist[si >> 2].len[si & 3] = 0;
+			       sglist[si >> 2].len[3 - (si & 3)], DMA_TO_DEVICE);
+		sglist[si >> 2].len[3 - (si & 3)] = 0;
 		si--;
 	}
 	tx_buffer->gather = 0;
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
index 5a520d37bea0..d0adb82d65c3 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.c
@@ -69,12 +69,12 @@ int octep_iq_process_completions(struct octep_iq *iq, u16 budget)
 		compl_sg++;
 
 		dma_unmap_single(iq->dev, tx_buffer->sglist[0].dma_ptr[0],
-				 tx_buffer->sglist[0].len[0], DMA_TO_DEVICE);
+				 tx_buffer->sglist[0].len[3], DMA_TO_DEVICE);
 
 		i = 1; /* entry 0 is main skb, unmapped above */
 		while (frags--) {
 			dma_unmap_page(iq->dev, tx_buffer->sglist[i >> 2].dma_ptr[i & 3],
-				       tx_buffer->sglist[i >> 2].len[i & 3], DMA_TO_DEVICE);
+				       tx_buffer->sglist[i >> 2].len[3 - (i & 3)], DMA_TO_DEVICE);
 			i++;
 		}
 
@@ -131,13 +131,13 @@ static void octep_iq_free_pending(struct octep_iq *iq)
 
 		dma_unmap_single(iq->dev,
 				 tx_buffer->sglist[0].dma_ptr[0],
-				 tx_buffer->sglist[0].len[0],
+				 tx_buffer->sglist[0].len[3],
 				 DMA_TO_DEVICE);
 
 		i = 1; /* entry 0 is main skb, unmapped above */
 		while (frags--) {
 			dma_unmap_page(iq->dev, tx_buffer->sglist[i >> 2].dma_ptr[i & 3],
-				       tx_buffer->sglist[i >> 2].len[i & 3], DMA_TO_DEVICE);
+				       tx_buffer->sglist[i >> 2].len[3 - (i & 3)], DMA_TO_DEVICE);
 			i++;
 		}
 
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
index 2ef57980eb47..21e75ff9f5e7 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_tx.h
@@ -17,7 +17,21 @@
 #define TX_BUFTYPE_NET_SG        2
 #define NUM_TX_BUFTYPES          3
 
-/* Hardware format for Scatter/Gather list */
+/* Hardware format for Scatter/Gather list
+ *
+ * 63      48|47     32|31     16|15       0
+ * -----------------------------------------
+ * |  Len 0  |  Len 1  |  Len 2  |  Len 3  |
+ * -----------------------------------------
+ * |                Ptr 0                  |
+ * -----------------------------------------
+ * |                Ptr 1                  |
+ * -----------------------------------------
+ * |                Ptr 2                  |
+ * -----------------------------------------
+ * |                Ptr 3                  |
+ * -----------------------------------------
+ */
 struct octep_tx_sglist_desc {
 	u16 len[4];
 	dma_addr_t dma_ptr[4];
-- 
2.25.1

Re: [net PATCH v2] octeon_ep: fix tx dma unmap len values in SG

[Simon Horman]

On Wed, Sep 13, 2023 at 01:41:56AM -0700, Shinas Rasheed wrote:
> Lengths of SG pointers are kept in the following order in
> the SG entries in hardware.
>  63      48|47     32|31     16|15       0
>  -----------------------------------------
>  |  Len 0  |  Len 1  |  Len 2  |  Len 3  |
>  -----------------------------------------
>  |                Ptr 0                  |
>  -----------------------------------------
>  |                Ptr 1                  |
>  -----------------------------------------
>  |                Ptr 2                  |
>  -----------------------------------------
>  |                Ptr 3                  |
>  -----------------------------------------
> Dma pointers have to be unmapped based on their
> respective lengths given in this format.
> 
> Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support")
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [net PATCH v2] octeon_ep: fix tx dma unmap len values in SG

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 13 Sep 2023 01:41:56 -0700 you wrote:
> Lengths of SG pointers are kept in the following order in
> the SG entries in hardware.
>  63      48|47     32|31     16|15       0
>  -----------------------------------------
>  |  Len 0  |  Len 1  |  Len 2  |  Len 3  |
>  -----------------------------------------
>  |                Ptr 0                  |
> 
> [...]

Here is the summary with links:
  - [net,v2] octeon_ep: fix tx dma unmap len values in SG
    https://git.kernel.org/netdev/net/c/350db8a59eb3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [EXT] Re: [net PATCH] octeon_ep: fix tx dma unmap len values in SG

[Simon Horman]

On Tue, Sep 12, 2023 at 06:37:46AM +0000, Shinas Rasheed wrote:
> Hi Simon,
> 
> This change is required in octep_iq_process_completions, as given in the patch, since the scatter gather pointer lengths arrive as big-endian in hardware.

Hi,

yes, I see that. And sorry for asking such a silly question.
But what I meant to ask is, if the change is also needed in
octep_iq_free_pending()?