RE: [PATCH] net: mvpp2: avoid bouncing buffers

[Yan Markman <ymarkman@marvell.com>]

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

Hi everybody
The MVPP2 code already has DMA's patch taken from OLD Backport and placed by Antoine Tenart.
Please refer the attached 
Best regards
Yan Markman


-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org] 
Sent: Monday, August 20, 2018 9:23 AM
To: David Miller <davem@davemloft.net>
Cc: brian.brooks@linaro.org; antoine.tenart@bootlin.com; maxime.chevallier@bootlin.com; Yan Markman <ymarkman@marvell.com>; Stefan Chulski <stefanc@marvell.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; bjorn.topel@intel.com; brian.brooks@arm.com
Subject: Re: [PATCH] net: mvpp2: avoid bouncing buffers

On Sun, Aug 19, 2018 at 07:55:05PM -0700, David Miller wrote:
> From: Brian Brooks <brian.brooks@linaro.org>
> Date: Sun, 19 Aug 2018 21:47:30 -0500
> 
> > @@ -5126,6 +5126,12 @@ static int mvpp2_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	if (priv->hw_version == MVPP22) {
> > +		/* Platform code may have set dev->dma_mask to point
> > +		 * to dev->coherent_dma_mask, but we want to ensure
> > +		 * they take different values due to comment below.
> > +		 */
> > +		pdev->dev.dma_mask = &priv->dma_mask;
> 
> The platform code might be doing this exactly because it cannot 
> support different coherent and streaming DMA masks.
> 
> Well, in any case, the platform code is doing it for a reason and 
> overriding this in a "driver" of all places seems totally 
> inappropriate and at best a layering violation.
> 
> I would rather you fix this in a way that involves well defined APIs 
> that set the DMA masks or whatever to the values that you need, rather 
> than going behind the platform code's back and changing the DMA mask 
> pointer like this.

Agreed.  What platform do you see this issue on?

[-- Attachment #2: 0009-net-mvpp2-fix-the-dma_mask-and-coherent_dma_mask-set.patch --]
[-- Type: application/octet-stream, Size: 3572 bytes --]

From 810faf4533d343fe1df84853b7e22fc2733a8ea9 Mon Sep 17 00:00:00 2001
From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Wed, 8 Aug 2018 14:30:58 +0300
Subject: [PATCH 09/40] net: mvpp2: fix the dma_mask and coherent_dma_mask
 settings

The dev->dma_mask usually points to dev->coherent_dma_mask. This is an
issue as setting both of them will override the other. This is
problematic here as the PPv2 driver uses a 32-bit-mask for coherent
accesses (txq, rxq, bm) and a 40-bit mask for all other accesses due to
an hardware limitation.

This can lead to a memory remap for all dma_map_single() calls when
dealing with memory above 4GB.

Change-Id: Ib8a04a06a1f30bfa43b62250f21cfe09464d91ed
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
Signed-off-by: Alex Zemtzov <azemtzov@marvell.com>
Signed-off-by: Yan Markman <ymarkman@marvell.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h      |  2 ++
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 33 +++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index f2d90e3..52a8e1d 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -708,6 +708,8 @@ struct mvpp2 {
 	/* Workqueue to gather hardware statistics */
 	char queue_name[30];
 	struct workqueue_struct *stats_queue;
+
+	bool custom_dma_mask;
 };
 
 struct mvpp2_pcpu_stats {
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index f107c08..a80727e 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -5130,10 +5130,29 @@ static int mvpp2_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	priv->custom_dma_mask = false;
 	if (priv->hw_version != MVPP21) {
+		/* If dma_mask points to coherent_dma_mask, setting both will
+		 * override the value of the other. This is problematic as the
+		 * PPv2 driver uses a 32-bit-mask for coherent accesses (txq,
+		 * rxq, bm) and a 40-bit mask for all other accesses.
+		 */
+		if (pdev->dev.dma_mask == &pdev->dev.coherent_dma_mask) {
+			pdev->dev.dma_mask =
+				kzalloc(sizeof(*pdev->dev.dma_mask),
+					GFP_KERNEL);
+			if (!pdev->dev.dma_mask) {
+				err = -ENOMEM;
+				goto err_mg_clk;
+			}
+
+			priv->custom_dma_mask = true;
+		}
+
 		err = dma_set_mask(&pdev->dev, MVPP2_DESC_DMA_MASK);
 		if (err)
-			goto err_axi_clk;
+			goto err_dma_mask;
+
 		/* Sadly, the BM pools all share the same register to
 		 * store the high 32 bits of their address. So they
 		 * must all have the same high 32 bits, which forces
@@ -5141,7 +5160,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 		 */
 		err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 		if (err)
-			goto err_axi_clk;
+			goto err_dma_mask;
 	}
 
 	/* Initialize network controller */
@@ -5189,6 +5208,11 @@ static int mvpp2_probe(struct platform_device *pdev)
 			mvpp2_port_remove(priv->port_list[i]);
 		i++;
 	}
+err_dma_mask:
+	if (priv->custom_dma_mask) {
+		kfree(pdev->dev.dma_mask);
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	}
 err_axi_clk:
 	clk_disable_unprepare(priv->axi_clk);
 
@@ -5238,6 +5262,11 @@ static int mvpp2_remove(struct platform_device *pdev)
 				  aggr_txq->descs_dma);
 	}
 
+	if (priv->custom_dma_mask) {
+		kfree(pdev->dev.dma_mask);
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	}
+
 	if (is_acpi_node(port_fwnode))
 		return 0;
 
-- 
2.0.4