[PATCH RESEND v2 0/2]: ixp4xx: Fix 3.7 regression for IXP4xx ethernet driver

[PATCH RESEND v2 0/2]: ixp4xx: Fix 3.7 regression for IXP4xx ethernet driver

[Simon Kågström]

Hi!

These two patches fixes the regression introduced in 3.7 by commit
1a4901177574083c35fafc24c4d151c2a7c7647c,

  ixp4xx_eth: avoid calling dma_pool_create() with NULL dev

The patch above is not incorrect itself, but causes the driver to fail
with

  [   33.055473] net eth1: coherent DMA mask is unset
  [   33.055523] net eth1: coherent allocation too big (requested 0x1000 mask 0x0)

The two patches fix this by generalizing and correcting the ixp4xx
dma_set_coherent_mask(), which earlier only dealt with the PCI case.
The second patch simply sets up the mask for the ixp4xx ethernet device.


The patches have been verified on a board with 256MiB memory, one
ipx4xx_eth device and a e100 PCI device, running Linus' tip.


This issue has been discussed before:

   https://lkml.org/lkml/2014/1/2/46
   http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/148811.html


This is a resend of the patch series. There was some discussion about it here:

  http://www.spinics.net/lists/arm-kernel/msg313082.html

and if I understand Russels comments (as per the documentation),
drivers should setup the coherent DMA mask even if it happens to be the
platform/CPU default.

ChangeLog:

v2: Move EXPORT_SYMBOL together with dma_set_coherent_mask

// Simon

[PATCH RESEND v2 1/2] ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation

[Simon Kågström]

Non-PCI devices can use the entire 32-bit range, PCI dittos are
limited to the first 64MiB.

Also actually setup coherent_dma_mask.

The patch has been verified on a board with 128MiB memory, one
ipx4xx_eth device and a e100 PCI device.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
ChangeLog:

* v2: Move EXPORT_SYMBOL together with dma_set_coherent_mask (Anrd Bergmann)

 arch/arm/mach-ixp4xx/common-pci.c |    9 ---------
 arch/arm/mach-ixp4xx/common.c     |   12 ++++++++++++
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-ixp4xx/common-pci.c b/arch/arm/mach-ixp4xx/common-pci.c
index 200970d..055d816 100644
--- a/arch/arm/mach-ixp4xx/common-pci.c
+++ b/arch/arm/mach-ixp4xx/common-pci.c
@@ -481,14 +481,5 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)
 	return 1;
 }
 
-int dma_set_coherent_mask(struct device *dev, u64 mask)
-{
-	if (mask >= SZ_64M - 1)
-		return 0;
-
-	return -EIO;
-}
-
 EXPORT_SYMBOL(ixp4xx_pci_read);
 EXPORT_SYMBOL(ixp4xx_pci_write);
-EXPORT_SYMBOL(dma_set_coherent_mask);
diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c
index 6d68aed..df82a2b 100644
--- a/arch/arm/mach-ixp4xx/common.c
+++ b/arch/arm/mach-ixp4xx/common.c
@@ -31,6 +31,7 @@
 #include <linux/gpio.h>
 #include <linux/cpu.h>
 #include <linux/sched_clock.h>
+#include <linux/pci.h>
 
 #include <mach/udc.h>
 #include <mach/hardware.h>
@@ -578,6 +579,17 @@ void ixp4xx_restart(enum reboot_mode mode, const char *cmd)
 	}
 }
 
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (dev_is_pci(dev) && mask >= SZ_64M)
+		return -EIO;
+
+	dev->coherent_dma_mask = mask;
+
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 #ifdef CONFIG_IXP4XX_INDIRECT_PCI
 /*
  * In the case of using indirect PCI, we simply return the actual PCI
-- 
1.7.9.6

[PATCH RESEND v2 1/2] ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation

[Arnd Bergmann]

On Monday 17 March 2014, Simon K?gstr?m wrote:
> Non-PCI devices can use the entire 32-bit range, PCI dittos are
> limited to the first 64MiB.
> 
> Also actually setup coherent_dma_mask.
> 
> The patch has been verified on a board with 128MiB memory, one
> ipx4xx_eth device and a e100 PCI device.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---

I've applied this patch to the next/fixes-non-critical branch of arm-soc.
Please let me know if you need to have it backported to older kernels
as well.

Patch 2 should go through the netdev tree.

	Arnd

[PATCH RESEND v2 1/2] ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation

[Simon Kågström]

On Tue, 18 Mar 2014 15:58:36 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 17 March 2014, Simon K?gstr?m wrote:
> > Non-PCI devices can use the entire 32-bit range, PCI dittos are
> > limited to the first 64MiB.
> > 
> > Also actually setup coherent_dma_mask.
> > 
> > The patch has been verified on a board with 128MiB memory, one
> > ipx4xx_eth device and a e100 PCI device.
> 
> I've applied this patch to the next/fixes-non-critical branch of arm-soc.
> Please let me know if you need to have it backported to older kernels
> as well.

Thanks. However, Krzysztof thinks that this implementation is
incorrect. It certainly fixes my ixp4xx issues, but might not be good
for everyone.

// Simon

[PATCH RESEND v2 1/2] ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation

[Krzysztof Hałasa]

Simon K?gstr?m <simon.kagstrom@netinsight.net> writes:

> Thanks. However, Krzysztof thinks that this implementation is
> incorrect. It certainly fixes my ixp4xx issues, but might not be good
> for everyone.

It because you don't have PCI devices setting coherent mask to 32-bits
(and that's what normal PCI devices do).

BTW PCI (and other) devices requesting 32-bit coherent allocs don't care
if they're given 26-bit memory. 26 bits fit perfectly in 32 bits. It's
the streaming mapping which is (may be) constrained by hardware.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH RESEND v2 1/2] ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation

[Arnd Bergmann]

On Thursday 20 March 2014, Krzysztof Ha?asa wrote:
> Simon K?gstr?m <simon.kagstrom@netinsight.net> writes:
> 
> > Thanks. However, Krzysztof thinks that this implementation is
> > incorrect. It certainly fixes my ixp4xx issues, but might not be good
> > for everyone.
> 
> It because you don't have PCI devices setting coherent mask to 32-bits
> (and that's what normal PCI devices do).
> 
> BTW PCI (and other) devices requesting 32-bit coherent allocs don't care
> if they're given 26-bit memory. 26 bits fit perfectly in 32 bits. It's
> the streaming mapping which is (may be) constrained by hardware.

Should I revert the patch then until we have a better fix?

	Arnd

[PATCH RESEND v2 1/2] ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation

[Simon Kågström]

On Thu, 20 Mar 2014 12:02:36 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Thursday 20 March 2014, Krzysztof Ha?asa wrote:
> > Simon K?gstr?m <simon.kagstrom@netinsight.net> writes:
> > 
> > > Thanks. However, Krzysztof thinks that this implementation is
> > > incorrect. It certainly fixes my ixp4xx issues, but might not be good
> > > for everyone.
> > 
> > It because you don't have PCI devices setting coherent mask to 32-bits
> > (and that's what normal PCI devices do).
> > 
> > BTW PCI (and other) devices requesting 32-bit coherent allocs don't care
> > if they're given 26-bit memory. 26 bits fit perfectly in 32 bits. It's
> > the streaming mapping which is (may be) constrained by hardware.
> 
> Should I revert the patch then until we have a better fix?

Yes, I think that would be good. Thanks to all for looking into it!

// Simon

[PATCH RESEND v2 2/2] ixp4xx_eth: Setup coherent_dma_mask

[Simon Kågström]

This driver has been broken since commit 1a4901177574083c35fafc24c4d151c2a7c7647c

  ixp4xx_eth: avoid calling dma_pool_create() with NULL dev

in 3.7 and would fail with

[   33.055473] net eth1: coherent DMA mask is unset
[   33.055523] net eth1: coherent allocation too big (requested 0x1000 mask 0x0)

Fix by setting up the DMA mask to the entire 32-bit range.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/ethernet/xscale/ixp4xx_eth.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c b/drivers/net/ethernet/xscale/ixp4xx_eth.c
index 25283f1..e540e51 100644
--- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -1426,6 +1426,10 @@ static int eth_init_one(struct platform_device *pdev)
 	port->netdev = dev;
 	port->id = pdev->id;
 
+	err = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
+	if (err < 0)
+		goto err_free;
+
 	switch (port->id) {
 	case IXP4XX_ETH_NPEA:
 		port->regs = (struct eth_regs __iomem *)IXP4XX_EthA_BASE_VIRT;
-- 
1.7.9.6

[PATCH RESEND v2 2/2] ixp4xx_eth: Setup coherent_dma_mask

[Krzysztof Hałasa]

Simon K?gstr?m <simon.kagstrom@netinsight.net> writes:

> +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> @@ -1426,6 +1426,10 @@ static int eth_init_one(struct platform_device *pdev)
>  	port->netdev = dev;
>  	port->id = pdev->id;
>  
> +	err = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
> +	if (err < 0)
> +		goto err_free;
> +

Yeah. One could also set both masks with the new single call. The driver
uses both streaming and coherent mapping.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH RESEND v2 2/2] ixp4xx_eth: Setup coherent_dma_mask

[Russell King - ARM Linux]

On Tue, Mar 18, 2014 at 08:43:09AM +0100, Krzysztof Ha?asa wrote:
> Simon K?gstr?m <simon.kagstrom@netinsight.net> writes:
> 
> > +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> > @@ -1426,6 +1426,10 @@ static int eth_init_one(struct platform_device *pdev)
> >  	port->netdev = dev;
> >  	port->id = pdev->id;
> >  
> > +	err = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
> > +	if (err < 0)
> > +		goto err_free;
> > +
> 
> Yeah. One could also set both masks with the new single call. The driver
> uses both streaming and coherent mapping.

The other issue with this patch set is (from DMA-API-HOWTO.txt):

  The coherent coherent mask will always be able to set the same or a
  smaller mask as the streaming mask.

and code in places assumes that's true.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.