linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: mvpp2: avoid bouncing buffers
@ 2018-08-20  2:47 Brian Brooks
  2018-08-20  2:55 ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Brian Brooks @ 2018-08-20  2:47 UTC (permalink / raw)
  To: davem
  Cc: antoine.tenart, maxime.chevallier, ymarkman, stefanc, netdev,
	linux-kernel, bjorn.topel, brian.brooks, Brian Brooks

Some memory regions used by this device need to share the same
upper 8 bits of the 40-bit bus address. Currently, a coherent
DMA mask of 32 bits is used so that dma_alloc_coherent() regions
have the same upper 8 bits. Packet buffers are not allocated via
DMA APIs, and the device does not require these memory regions
to have the same upper 8 bits. However, packet buffers are being
bounced during streaming mappings because streaming and coherent
DMA are using the same DMA mask, i.e. dev->dma_mask points to
dev->coherent_dma_mask.

Avoid bouncing packet buffers by ensuring streaming DMA uses a
mask of 40 bits and coherent DMA uses a mask of 32 bits. iperf3
shows throughput increases from 4.04 Gbps to 9.14 Gbps.

Signed-off-by: Brian Brooks <brian.brooks@linaro.org>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2.h      | 3 +++
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index def00dc3eb4e..3eb0c3ede8d2 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -676,6 +676,9 @@ struct mvpp2 {
 	struct clk *mg_core_clk;
 	struct clk *axi_clk;
 
+	/* DMA mask for streaming mappings */
+	u64 dma_mask;
+
 	/* List of pointers to port structures */
 	int port_count;
 	struct mvpp2_port *port_list[MVPP2_MAX_PORTS];
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 0319ed9ef8b8..3a190c489589 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -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;
+
 		err = dma_set_mask(&pdev->dev, MVPP2_DESC_DMA_MASK);
 		if (err)
 			goto err_axi_clk;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] net: mvpp2: avoid bouncing buffers
  2018-08-20  2:47 [PATCH] net: mvpp2: avoid bouncing buffers Brian Brooks
@ 2018-08-20  2:55 ` David Miller
  2018-08-20  6:23   ` Christoph Hellwig
  0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2018-08-20  2:55 UTC (permalink / raw)
  To: brian.brooks
  Cc: antoine.tenart, maxime.chevallier, ymarkman, stefanc, netdev,
	linux-kernel, bjorn.topel, brian.brooks

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.

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] net: mvpp2: avoid bouncing buffers
  2018-08-20  2:55 ` David Miller
@ 2018-08-20  6:23   ` Christoph Hellwig
  2018-08-20  7:02     ` Yan Markman
  2018-08-27 13:55     ` Brian Brooks
  0 siblings, 2 replies; 9+ messages in thread
From: Christoph Hellwig @ 2018-08-20  6:23 UTC (permalink / raw)
  To: David Miller
  Cc: brian.brooks, antoine.tenart, maxime.chevallier, ymarkman,
	stefanc, netdev, linux-kernel, bjorn.topel, brian.brooks

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] net: mvpp2: avoid bouncing buffers
  2018-08-20  6:23   ` Christoph Hellwig
@ 2018-08-20  7:02     ` Yan Markman
  2018-08-27 13:55     ` Brian Brooks
  1 sibling, 0 replies; 9+ messages in thread
From: Yan Markman @ 2018-08-20  7:02 UTC (permalink / raw)
  To: Christoph Hellwig, David Miller
  Cc: brian.brooks, antoine.tenart, maxime.chevallier, Stefan Chulski,
	netdev, linux-kernel, bjorn.topel, brian.brooks

[-- 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] net: mvpp2: avoid bouncing buffers
  2018-08-20  6:23   ` Christoph Hellwig
  2018-08-20  7:02     ` Yan Markman
@ 2018-08-27 13:55     ` Brian Brooks
  2018-08-27 15:48       ` Christoph Hellwig
  1 sibling, 1 reply; 9+ messages in thread
From: Brian Brooks @ 2018-08-27 13:55 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: David Miller, antoine.tenart, maxime.chevallier, ymarkman,
	stefanc, netdev, linux-kernel, bjorn.topel, brian.brooks

On 08/19 23:23:26, Christoph Hellwig wrote:
> 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?

This is Armada 8040 SoC with PPv2 net device on chip. MACCHIATObin board.

Both DT and ACPI have the following snippet:

	/*
	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
	 * setup the correct supported mask.
	 */
	if (!dev->coherent_dma_mask)
		dev->coherent_dma_mask = DMA_BIT_MASK(32);

	/*
	 * Set it to coherent_dma_mask by default if the architecture
	 * code has not set it.
	 */
	if (!dev->dma_mask)
		dev->dma_mask = &dev->coherent_dma_mask;

Some architectures code setup DMA masks, but it does not seem appropriate
to add mvpp2 specific code in arch/arm64. The mvpp2 driver appeared to be
the least invasive place to resolve the limitation of this net device.

Another DMA API could be added to allocate storage for the streaming
mask to ensure masks can take on separate values when the existing DMA
APIs are used by the driver to set those values. But, this would be the
only driver using such API. Would that be how to handle this?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] net: mvpp2: avoid bouncing buffers
  2018-08-27 13:55     ` Brian Brooks
@ 2018-08-27 15:48       ` Christoph Hellwig
  2018-09-02  2:10         ` Brian Brooks
  2019-03-01 14:26         ` Antoine Tenart
  0 siblings, 2 replies; 9+ messages in thread
From: Christoph Hellwig @ 2018-08-27 15:48 UTC (permalink / raw)
  To: Brian Brooks
  Cc: Christoph Hellwig, David Miller, antoine.tenart,
	maxime.chevallier, ymarkman, stefanc, netdev, linux-kernel,
	bjorn.topel, brian.brooks

WE should basically never have dev->dma_mask = &dev->coherent_dma_mask,
so until that is the case you are doctoring around the symptoms and
not the problem.

Does the patch below help your case?

----
From 6294e0e330851ee06e66ab85b348f1d92d375d7a Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Mon, 27 Aug 2018 17:23:24 +0200
Subject: driver core: initialize a default DMA mask for platform device

We still treat devices without a DMA mask as defaulting to 32-bits for
both mask, but a few releases ago we've started warning about such
cases, as they require special cases to work around this sloppyness.
Add a dma_mask field to struct platform_object so that we can initialize
the dma_mask pointer in struct device and initialize both masks to
32-bits by default.  Architectures can still override this in
arch_setup_pdev_archdata if needed.

Note that the code looks a little odd with the various conditionals
because we have to support platform_device structures that are
statically allocated.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/base/platform.c         | 15 +++++++++++++--
 include/linux/platform_device.h |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dff82a3c2caa..baf4b06cf2d9 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -225,6 +225,17 @@ struct platform_object {
 	char name[];
 };
 
+static void setup_pdev_archdata(struct platform_device *pdev)
+{
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	if (!pdev->dma_mask)
+		pdev->dma_mask = DMA_BIT_MASK(32);
+	if (!pdev->dev.dma_mask)
+		pdev->dev.dma_mask = &pdev->dma_mask;
+	arch_setup_pdev_archdata(pdev);
+};
+
 /**
  * platform_device_put - destroy a platform device
  * @pdev: platform device to free
@@ -271,7 +282,7 @@ struct platform_device *platform_device_alloc(const char *name, int id)
 		pa->pdev.id = id;
 		device_initialize(&pa->pdev.dev);
 		pa->pdev.dev.release = platform_device_release;
-		arch_setup_pdev_archdata(&pa->pdev);
+		setup_pdev_archdata(&pa->pdev);
 	}
 
 	return pa ? &pa->pdev : NULL;
@@ -472,7 +483,7 @@ EXPORT_SYMBOL_GPL(platform_device_del);
 int platform_device_register(struct platform_device *pdev)
 {
 	device_initialize(&pdev->dev);
-	arch_setup_pdev_archdata(pdev);
+	setup_pdev_archdata(pdev);
 	return platform_device_add(pdev);
 }
 EXPORT_SYMBOL_GPL(platform_device_register);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 1a9f38f27f65..d84ec1de6022 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -25,6 +25,7 @@ struct platform_device {
 	int		id;
 	bool		id_auto;
 	struct device	dev;
+	dma_addr_t	dma_mask;
 	u32		num_resources;
 	struct resource	*resource;
 
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] net: mvpp2: avoid bouncing buffers
  2018-08-27 15:48       ` Christoph Hellwig
@ 2018-09-02  2:10         ` Brian Brooks
  2019-03-01 14:26         ` Antoine Tenart
  1 sibling, 0 replies; 9+ messages in thread
From: Brian Brooks @ 2018-09-02  2:10 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: David Miller, antoine.tenart, maxime.chevallier, ymarkman,
	stefanc, netdev, linux-kernel, bjorn.topel, brian.brooks

On 08/27 08:48:43, Christoph Hellwig wrote:
> WE should basically never have dev->dma_mask = &dev->coherent_dma_mask,
> so until that is the case you are doctoring around the symptoms and
> not the problem.
> 
> Does the patch below help your case?

Yes. Just tested it. Works great.

I see how this patch addresses the issue in the platform code instead of
the driver code. Thanks.

> ----
> From 6294e0e330851ee06e66ab85b348f1d92d375d7a Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Mon, 27 Aug 2018 17:23:24 +0200
> Subject: driver core: initialize a default DMA mask for platform device
> 
> We still treat devices without a DMA mask as defaulting to 32-bits for
> both mask, but a few releases ago we've started warning about such
> cases, as they require special cases to work around this sloppyness.
> Add a dma_mask field to struct platform_object so that we can initialize
> the dma_mask pointer in struct device and initialize both masks to
> 32-bits by default.  Architectures can still override this in
> arch_setup_pdev_archdata if needed.
> 
> Note that the code looks a little odd with the various conditionals
> because we have to support platform_device structures that are
> statically allocated.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/base/platform.c         | 15 +++++++++++++--
>  include/linux/platform_device.h |  1 +
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index dff82a3c2caa..baf4b06cf2d9 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -225,6 +225,17 @@ struct platform_object {
>  	char name[];
>  };
>  
> +static void setup_pdev_archdata(struct platform_device *pdev)
> +{
> +	if (!pdev->dev.coherent_dma_mask)
> +		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +	if (!pdev->dma_mask)
> +		pdev->dma_mask = DMA_BIT_MASK(32);
> +	if (!pdev->dev.dma_mask)
> +		pdev->dev.dma_mask = &pdev->dma_mask;
> +	arch_setup_pdev_archdata(pdev);
> +};
> +
>  /**
>   * platform_device_put - destroy a platform device
>   * @pdev: platform device to free
> @@ -271,7 +282,7 @@ struct platform_device *platform_device_alloc(const char *name, int id)
>  		pa->pdev.id = id;
>  		device_initialize(&pa->pdev.dev);
>  		pa->pdev.dev.release = platform_device_release;
> -		arch_setup_pdev_archdata(&pa->pdev);
> +		setup_pdev_archdata(&pa->pdev);
>  	}
>  
>  	return pa ? &pa->pdev : NULL;
> @@ -472,7 +483,7 @@ EXPORT_SYMBOL_GPL(platform_device_del);
>  int platform_device_register(struct platform_device *pdev)
>  {
>  	device_initialize(&pdev->dev);
> -	arch_setup_pdev_archdata(pdev);
> +	setup_pdev_archdata(pdev);
>  	return platform_device_add(pdev);
>  }
>  EXPORT_SYMBOL_GPL(platform_device_register);
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 1a9f38f27f65..d84ec1de6022 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -25,6 +25,7 @@ struct platform_device {
>  	int		id;
>  	bool		id_auto;
>  	struct device	dev;
> +	dma_addr_t	dma_mask;

Hmm.. should struct device use dma_addr_t instead of u64 for masks too?

>  	u32		num_resources;
>  	struct resource	*resource;
>  
> -- 
> 2.18.0
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] net: mvpp2: avoid bouncing buffers
  2018-08-27 15:48       ` Christoph Hellwig
  2018-09-02  2:10         ` Brian Brooks
@ 2019-03-01 14:26         ` Antoine Tenart
  2019-03-11 15:46           ` Christoph Hellwig
  1 sibling, 1 reply; 9+ messages in thread
From: Antoine Tenart @ 2019-03-01 14:26 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Brian Brooks, David Miller, antoine.tenart, maxime.chevallier,
	ymarkman, stefanc, netdev, linux-kernel, bjorn.topel,
	brian.brooks

Hi Christoph,

I saw you sent this patch as part of another series back in August, but
it seems it was never applied and I can't find it even in -next. We made
some tests and this patch is helping a lot the PPv2 engine driver in
improving its performances.

Do you plan on re-sending it, or reworking it? Are there current issues
with it that prevent it from being merged upstream? Can we help in any
way?

Thanks!
Antoine

On Mon, Aug 27, 2018 at 08:48:43AM -0700, Christoph Hellwig wrote:
> WE should basically never have dev->dma_mask = &dev->coherent_dma_mask,
> so until that is the case you are doctoring around the symptoms and
> not the problem.
> 
> Does the patch below help your case?
> 
> ----
> From 6294e0e330851ee06e66ab85b348f1d92d375d7a Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Mon, 27 Aug 2018 17:23:24 +0200
> Subject: driver core: initialize a default DMA mask for platform device
> 
> We still treat devices without a DMA mask as defaulting to 32-bits for
> both mask, but a few releases ago we've started warning about such
> cases, as they require special cases to work around this sloppyness.
> Add a dma_mask field to struct platform_object so that we can initialize
> the dma_mask pointer in struct device and initialize both masks to
> 32-bits by default.  Architectures can still override this in
> arch_setup_pdev_archdata if needed.
> 
> Note that the code looks a little odd with the various conditionals
> because we have to support platform_device structures that are
> statically allocated.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/base/platform.c         | 15 +++++++++++++--
>  include/linux/platform_device.h |  1 +
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index dff82a3c2caa..baf4b06cf2d9 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -225,6 +225,17 @@ struct platform_object {
>  	char name[];
>  };
>  
> +static void setup_pdev_archdata(struct platform_device *pdev)
> +{
> +	if (!pdev->dev.coherent_dma_mask)
> +		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +	if (!pdev->dma_mask)
> +		pdev->dma_mask = DMA_BIT_MASK(32);
> +	if (!pdev->dev.dma_mask)
> +		pdev->dev.dma_mask = &pdev->dma_mask;
> +	arch_setup_pdev_archdata(pdev);
> +};
> +
>  /**
>   * platform_device_put - destroy a platform device
>   * @pdev: platform device to free
> @@ -271,7 +282,7 @@ struct platform_device *platform_device_alloc(const char *name, int id)
>  		pa->pdev.id = id;
>  		device_initialize(&pa->pdev.dev);
>  		pa->pdev.dev.release = platform_device_release;
> -		arch_setup_pdev_archdata(&pa->pdev);
> +		setup_pdev_archdata(&pa->pdev);
>  	}
>  
>  	return pa ? &pa->pdev : NULL;
> @@ -472,7 +483,7 @@ EXPORT_SYMBOL_GPL(platform_device_del);
>  int platform_device_register(struct platform_device *pdev)
>  {
>  	device_initialize(&pdev->dev);
> -	arch_setup_pdev_archdata(pdev);
> +	setup_pdev_archdata(pdev);
>  	return platform_device_add(pdev);
>  }
>  EXPORT_SYMBOL_GPL(platform_device_register);
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 1a9f38f27f65..d84ec1de6022 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -25,6 +25,7 @@ struct platform_device {
>  	int		id;
>  	bool		id_auto;
>  	struct device	dev;
> +	dma_addr_t	dma_mask;
>  	u32		num_resources;
>  	struct resource	*resource;
>  
> -- 
> 2.18.0
> 

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] net: mvpp2: avoid bouncing buffers
  2019-03-01 14:26         ` Antoine Tenart
@ 2019-03-11 15:46           ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2019-03-11 15:46 UTC (permalink / raw)
  To: Antoine Tenart
  Cc: Christoph Hellwig, Brian Brooks, David Miller, maxime.chevallier,
	ymarkman, stefanc, netdev, linux-kernel, bjorn.topel,
	brian.brooks

On Fri, Mar 01, 2019 at 03:26:02PM +0100, Antoine Tenart wrote:
> Hi Christoph,
> 
> I saw you sent this patch as part of another series back in August, but
> it seems it was never applied and I can't find it even in -next. We made
> some tests and this patch is helping a lot the PPv2 engine driver in
> improving its performances.
> 
> Do you plan on re-sending it, or reworking it? Are there current issues
> with it that prevent it from being merged upstream? Can we help in any
> way?

Hi Antoine,

it's been a while, but my vague memory is that this caused failures
in some ARM setups, and I was tought why for AMBA setting up the
default mask is a bad idea.  I don't really remember more, though.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-03-11 15:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-20  2:47 [PATCH] net: mvpp2: avoid bouncing buffers Brian Brooks
2018-08-20  2:55 ` David Miller
2018-08-20  6:23   ` Christoph Hellwig
2018-08-20  7:02     ` Yan Markman
2018-08-27 13:55     ` Brian Brooks
2018-08-27 15:48       ` Christoph Hellwig
2018-09-02  2:10         ` Brian Brooks
2019-03-01 14:26         ` Antoine Tenart
2019-03-11 15:46           ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).