RE: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool

[Jose Abreu <Jose.Abreu@synopsys.com>]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

From: Jon Hunter <jonathanh@nvidia.com>
Date: Jul/29/2019, 22:33:04 (UTC+00:00)

> 
> On 29/07/2019 15:08, Jose Abreu wrote:
> 
> ...
> 
> >>> Hi Catalin and Will,
> >>>
> >>> Sorry to add you in such a long thread but we are seeing a DMA issue
> >>> with stmmac driver in an ARM64 platform with IOMMU enabled.
> >>>
> >>> The issue seems to be solved when buffers allocation for DMA based
> >>> transfers are *not* mapped with the DMA_ATTR_SKIP_CPU_SYNC flag *OR*
> >>> when IOMMU is disabled.
> >>>
> >>> Notice that after transfer is done we do use
> >>> dma_sync_single_for_{cpu,device} and then we reuse *the same* page for
> >>> another transfer.
> >>>
> >>> Can you please comment on whether DMA_ATTR_SKIP_CPU_SYNC can not be used
> >>> in ARM64 platforms with IOMMU ?
> >>
> >> In terms of what they do, there should be no difference on arm64 between:
> >>
> >> dma_map_page(..., dir);
> >> ...
> >> dma_unmap_page(..., dir);
> >>
> >> and:
> >>
> >> dma_map_page_attrs(..., dir, DMA_ATTR_SKIP_CPU_SYNC);
> >> dma_sync_single_for_device(..., dir);
> >> ...
> >> dma_sync_single_for_cpu(..., dir);
> >> dma_unmap_page_attrs(..., dir, DMA_ATTR_SKIP_CPU_SYNC);
> >>
> >> provided that the first sync covers the whole buffer and any subsequent 
> >> ones cover at least the parts of the buffer which may have changed. Plus 
> >> for coherent hardware it's entirely moot either way.
> > 
> > Thanks for confirming. That's indeed what stmmac is doing when buffer is 
> > received by syncing the packet size to CPU.
> > 
> >>
> >> Given Jon's previous findings, I would lean towards the idea that 
> >> performing the extra (redundant) cache maintenance plus barrier in 
> >> dma_unmap is mostly just perturbing timing in the same way as the debug 
> >> print which also made things seem OK.
> > 
> > Mikko said that Tegra186 is not coherent so we have to explicit flush 
> > pipeline but I don't understand why sync_single() is not doing it ...
> > 
> > Jon, can you please remove *all* debug prints, hacks, etc ... and test 
> > this one in attach with plain -net tree ?
> 
> So far I have just been testing on the mainline kernel branch. The issue
> still persists after applying this on mainline. I can test on the -net
> tree, but I am not sure that will make a difference.
> 
> Cheers
> Jon
> 
> -- 
> nvpublic

I looked at netsec implementation and I noticed that we are syncing the 
old buffer for device instead of the new one. netsec syncs the buffer 
for device immediately after the allocation which may be what we have to 
do. Maybe the attached patch can make things work for you ?

---
Thanks,
Jose Miguel Abreu

[-- Attachment #2: 0001-net-stmmac-Sync-RX-Buffer-upon-allocation.patch --]
[-- Type: application/octet-stream, Size: 2465 bytes --]

From 3601e3ae4357d48b3294f42781d0f19095d1b00e Mon Sep 17 00:00:00 2001
Message-Id: <3601e3ae4357d48b3294f42781d0f19095d1b00e.1564479382.git.joabreu@synopsys.com>
From: Jose Abreu <joabreu@synopsys.com>
Date: Tue, 30 Jul 2019 11:36:13 +0200
Subject: [PATCH net] net: stmmac: Sync RX Buffer upon allocation

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 98b1a5c6d537..9a4a56ad35cd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3271,9 +3271,11 @@ static inline int stmmac_rx_threshold_count(struct stmmac_rx_queue *rx_q)
 static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 {
 	struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
-	int dirty = stmmac_rx_dirty(priv, queue);
+	int len, dirty = stmmac_rx_dirty(priv, queue);
 	unsigned int entry = rx_q->dirty_rx;
 
+	len = DIV_ROUND_UP(priv->dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
+
 	while (dirty-- > 0) {
 		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[entry];
 		struct dma_desc *p;
@@ -3291,6 +3293,13 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 		}
 
 		buf->addr = page_pool_get_dma_addr(buf->page);
+
+		/* Sync whole allocation to device. This will invalidate old
+		 * data.
+		 */
+		dma_sync_single_for_device(priv->device, buf->addr, len,
+					   DMA_FROM_DEVICE);
+
 		stmmac_set_desc_addr(priv, p, buf->addr);
 		stmmac_refill_desc3(priv, rx_q, p);
 
@@ -3425,8 +3434,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			skb_copy_to_linear_data(skb, page_address(buf->page),
 						frame_len);
 			skb_put(skb, frame_len);
-			dma_sync_single_for_device(priv->device, buf->addr,
-						   frame_len, DMA_FROM_DEVICE);
 
 			if (netif_msg_pktdata(priv)) {
 				netdev_dbg(priv->dev, "frame received (%dbytes)",
-- 
2.7.4


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel