: ixp4xx: Fix 3.7 regression for IXP4xx ethernet driver
cover : ixp4xx: Fix 3.7 regression for IXP4xx ethernet driver
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.
// Simon
PATCH RESEND 1/2 ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation
Non-PCI devices can use the entire 32-bit range, PCI dittos are limited to the first 64MiB.
Also actually setup coherent_dma_mask.
--- arch/arm/mach-ixp4xx/common-pci.c | 8 -------- arch/arm/mach-ixp4xx/common.c | 11 +++++++++++ 2 files changed, 11 insertions(+), 8 deletions(-)
arch/arm/mach-ixp4xx/common-pci.c +0 -8
@@ -481,14 +481,6 @@ 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);
arch/arm/mach-ixp4xx/common.c +11 -0
@@ -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,16 @@ 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 ; +} +#ifdef CONFIG_IXP4XX_INDIRECT_PCI /* * In the case of using indirect PCI, we simply return the actual PCI
Signature · 1 line
1.7.9.6
PATCH RESEND 1/2 ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation
Quoted 9 lines · Simon K?gstr?m
arch/arm/mach-ixp4xx/common-pci.c +0
-1@@ -481,14 +481,6 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)return 1 ; } -int dma_set_coherent_mask (struct device *dev,u64 mask)
arch/arm/mach-ixp4xx/common-pci.c int ixp4xx_setup(int nr, struct pci_sys_data *sys) · line 486
Quoted 10 lines
arch/arm/mach-ixp4xx/common-pci.c +0
-7-{ -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);
When you move this function, you should really move the 'EXPORT_SYMBOL' along with it.
Arnd
PATCH RESEND 1/2 ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation
Quoted 15 lines · On Mon, 17 Mar 2014 12:29:38 +0100 Arnd Bergmann <[email protected]> wrote:
arch/arm/mach-ixp4xx/common-pci.c int ixp4xx_setup(int nr, struct pci_sys_data *sys) · line 486
Quoted 12 lines
arch/arm/mach-ixp4xx/common-pci.c +0
-8-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);When you move this function, you should really move the '
EXPORT_SYMBOL' along with it.
Yes, you are right. Will fix and resend the patches!
// Simon
PATCH RESEND 1/2 ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation
In reply to Simon KågströmHello,
This patch is incorrect, the functionality is reversed:
Review int ixp4xx_setup(int nr, struct pci_sys_data *sys) · line 489
Quoted 11 lines · Simon K?gstr?m
+++
b/arch/arm/mach-ixp4xx/common-pci.c(quoted diff) +0
-6@@ -481,14 +481,6 @@ 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;
Originally, coherent masks equal to 64 MiB or wider are accepted.
arch/arm/mach-ixp4xx/common.c
Quoted 7 lines
arch/arm/mach-ixp4xx/common.c +4
-0+int dma_set_coherent_mask (struct device *dev,u64 mask) +{ +if (dev_is_pci (dev) && mask >=SZ_64M ) +return -EIO ;
Now, you reject them. However, with coherent allocations, big masks (usually 32-bit) are ok. They have to be internally limited to 64 MiB, though.
It's a narrow mask (e.g. 16 MiB) which can't be used, because we only have two memory pools (4 GiB and 64 MiB), and we simply have no means to request e.g. memory from 16 MiB range. Doesn't matter in practice, otherwise we would have additional memory pool.
This differs a bit from the streaming mask. For streaming, we still accept basically everything (because we obviously want to support 32-bit capable devices) and we limit the masks to 64 MiB internally, but we then have to use dmabounce or whatever (perhaps swiotlb could be an option?).
Also, we can limit some heavy duty allocations to 64 MiB (e.g. skbuffs on routers, with a custom patch). We don't do DAC, IXP4xx address space is 32-bit anyway. BTW possibility to use some system-wide DMA mask while creating skbuffs, disk buffers etc. could be an improvement (drivers would still need to check the addresses with dmabounce).
Believe it or not, the correct patch is the one I'm attaching. One can add extra *set_masks* (e.g. the new call setting both streaming and coherent masks) in the drivers, but essentially it must do what this one does. Also, converting the mask (in the dev struct) from a pointer to a simple value would IMHO make sense, too.
Also, comparing bit masks with ">=" etc. doesn't look very valid to me. What if some device wants a mask which isn't a power of 2 - 1?
I'm unable to look at this ATM but I will update the patch to the new kernel, perhaps soon.
IXP4xx: Fix DMA masks.
Now, devices will have 32-bit default DMA masks (0xFFFFFFFF) as per DMA API, and the masks will actually work.
arch/arm/mach-ixp4xx/common-pci.c +0 -38
@@ -316,32 +316,6 @@ static int abort_handler(unsigned long addr, unsigned int fsr, struct pt_regs *r } -static int ixp4xx_needs_bounce (struct device *dev,dma_addr_t dma_addr,size_t size) -{ -return (dma_addr + size) >=SZ_64M ; -} - -/* -* Setup DMA mask to 64MB on PCI devices. Ignore all other devices. -*/ -static int ixp4xx_pci_platform_notify (struct device *dev) -{ -if (dev->bus == &pci_bus_type) { - *dev->dma_mask =SZ_64M -1 ; - dev->coherent_dma_mask =SZ_64M -1 ; -dmabounce_register_dev (dev,2048 ,4096 , ixp4xx_needs_bounce); - } -return 0 ; -} - -static int ixp4xx_pci_platform_notify_remove (struct device *dev) -{ -if (dev->bus == &pci_bus_type) { -dmabounce_unregister_dev (dev); - } -return 0 ; -} -void __init ixp4xx_pci_preinit (void ) {unsigned long cpuid =read_cpuid_id ();
@@ -475,20 +449,8 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)pci_add_resource_offset (&sys->resources, &res[0 ], sys->io_offset);pci_add_resource_offset (&sys->resources, &res[1 ], sys->mem_offset); - platform_notify = ixp4xx_pci_platform_notify; - platform_notify_remove = ixp4xx_pci_platform_notify_remove; -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);
arch/arm/mach-ixp4xx/common.c +58 -5
91 lines not on this page · raw message
Signature · 4 lines
Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
PATCH RESEND 1/2 ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation
Quoted 5 lines · On Tue, 18 Mar 2014 08:41:12 +0100 Krzysztof Ha?asa <[email protected]> wrote:
Believe it or not, the correct patch is the one I'm attaching. One can add extra *set_masks* (e.g. the new call setting both streaming and coherent masks) in the drivers, but essentially it must do what this one does. Also, converting the mask (in the dev struct) from a pointer to a simple value would IMHO make sense, too.
OK, you know the hardware better so if you say so!
Quoted 2 lines
I'm unable to look at this ATM but I will update the patch to the new kernel, perhaps soon.
Some comments below though:
arch/arm/mach-ixp4xx/common.c void ixp4xx_restart(enum reboot_mode mode, const char *cmd) · line 580
Quoted 8 lines
#include <
asm/mach/map.h>
#include <asm/mach/irq.h>
#include <asm/mach/time.h>arch/arm/mach-ixp4xx/common.c +1
-0@@ -578,6 +577,56 @@ void ixp4xx_restart(enum reboot_mode mode, const char *cmd) } } +#ifdef CONFIG_PCI
To me it seems the conditional compilation is superfluous - dev_is_pci is used to check for PCI-devices anyway, and the compiler should be smart enough to remove dead code for the non-PCI case.
arch/arm/mach-ixp4xx/common.c void ixp4xx_restart(enum reboot_mode mode, const char *cmd) · line 588
Quoted 8 lines
arch/arm/mach-ixp4xx/common.c +8
-0+static int ixp4xx_needs_bounce (struct device *dev,dma_addr_t dma_addr,size_t size) +{ +return (dma_addr + size) >=SZ_64M ; +} + +static int ixp4xx_platform_notify_remove (struct device *dev) +{ +if (dev->bus == &pci_bus_type)
if (dev_is_pci(dev))
arch/arm/mach-ixp4xx/common.c void ixp4xx_restart(enum reboot_mode mode, const char *cmd) · line 592
Quoted 4 lines
arch/arm/mach-ixp4xx/common.c +4
-0+dmabounce_unregister_dev (dev); + +return 0 ; +}
// Simon
PATCH RESEND 1/2 ARM: ixp4xx: Make dma_set_coherent_mask common, correct implementation
Quoted 5 lines · Simon K?gstr?m
Quoted 1 line
+#ifdef
CONFIG_PCITo me it seems the conditional compilation is superfluous -
dev_is_pciis used to check for PCI-devices anyway, and the compiler should be smart enough to remove dead code for the non-PCI case.
Right.
Quoted 10 lines
arch/arm/mach-ixp4xx/common.c void ixp4xx_restart(enum reboot_mode mode, const char *cmd) · line 588
Quoted 8 lines
arch/arm/mach-ixp4xx/common.c +8
-0+static int ixp4xx_needs_bounce (struct device *dev,dma_addr_t dma_addr,size_t size) +{ +return (dma_addr + size) >=SZ_64M ; +} + +static int ixp4xx_platform_notify_remove (struct device *dev) +{ +if (dev->bus == &pci_bus_type)if (
dev_is_pci(dev))
Sure.
I will finally fix this, but I want it to at least successfully build and boot on a couple of boards from different makers before I send anything to be applied.
Just not now :-(
Signature · 4 lines
Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
PATCH RESEND 2/2 ixp4xx_eth: Setup coherent_dma_mask
In reply to Simon KågströmThis 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.
--- drivers/net/ethernet/xscale/ixp4xx_eth.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/net/ethernet/xscale/ixp4xx_eth.c +4 -0
@@ -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;
Signature · 1 line
1.7.9.6