* [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
@ 2019-02-07 19:36 Geert Uytterhoeven
2019-02-08 7:52 ` Christoph Hellwig
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2019-02-07 19:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Robin Murphy, Christoph Hellwig,
Marek Szyprowski, Joerg Roedel, Rafael J . Wysocki
Cc: iommu, linux-arm-kernel, linux-renesas-soc, linux-kernel,
Geert Uytterhoeven
When unbinding the (IOMMU-enabled) R-Car SATA device on Salvator-XS
(R-Car H3 ES2.0), in preparation of rebinding against vfio-platform for
device pass-through for virtualization:
echo ee300000.sata > /sys/bus/platform/drivers/sata_rcar/unbind
the kernel crashes with:
Unable to handle kernel paging request at virtual address ffffffbf029ffffc
Mem abort info:
ESR = 0x96000006
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000006
CM = 0, WnR = 0
swapper pgtable: 4k pages, 39-bit VAs, pgdp = 000000007e8c586c
[ffffffbf029ffffc] pgd=000000073bfc6003, pud=000000073bfc6003, pmd=0000000000000000
Internal error: Oops: 96000006 [#1] SMP
Modules linked in:
CPU: 0 PID: 1098 Comm: bash Not tainted 5.0.0-rc5-salvator-x-00452-g37596f884f4318ef #287
Hardware name: Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ (DT)
pstate: 60400005 (nZCv daif +PAN -UAO)
pc : __free_pages+0x8/0x58
lr : __dma_direct_free_pages+0x50/0x5c
sp : ffffff801268baa0
x29: ffffff801268baa0 x28: 0000000000000000
x27: ffffffc6f9c60bf0 x26: ffffffc6f9c60bf0
x25: ffffffc6f9c60810 x24: 0000000000000000
x23: 00000000fffff000 x22: ffffff8012145000
x21: 0000000000000800 x20: ffffffbf029fffc8
x19: 0000000000000000 x18: ffffffc6f86c42c8
x17: 0000000000000000 x16: 0000000000000070
x15: 0000000000000003 x14: 0000000000000000
x13: ffffff801103d7f8 x12: 0000000000000028
x11: ffffff8011117604 x10: 0000000000009ad8
x9 : ffffff80110126d0 x8 : ffffffc6f7563000
x7 : 6b6b6b6b6b6b6b6b x6 : 0000000000000018
x5 : ffffff8011cf3cc8 x4 : 0000000000004000
x3 : 0000000000080000 x2 : 0000000000000001
x1 : 0000000000000000 x0 : ffffffbf029fffc8
Process bash (pid: 1098, stack limit = 0x00000000c38e3e32)
Call trace:
__free_pages+0x8/0x58
__dma_direct_free_pages+0x50/0x5c
arch_dma_free+0x1c/0x98
dma_direct_free+0x14/0x24
dma_free_attrs+0x9c/0xdc
dmam_release+0x18/0x20
release_nodes+0x25c/0x28c
devres_release_all+0x48/0x4c
device_release_driver_internal+0x184/0x1f0
device_release_driver+0x14/0x1c
unbind_store+0x70/0xb8
drv_attr_store+0x24/0x34
sysfs_kf_write+0x4c/0x64
kernfs_fop_write+0x154/0x1c4
__vfs_write+0x34/0x164
vfs_write+0xb4/0x16c
ksys_write+0x5c/0xbc
__arm64_sys_write+0x14/0x1c
el0_svc_common+0x98/0x114
el0_svc_handler+0x1c/0x24
el0_svc+0x8/0xc
Code: d51b4234 17fffffa a9bf7bfd 910003fd (b9403404)
---[ end trace 8c564cdd3a1a840f ]---
While I've bisected this to commit e8e683ae9a736407 ("iommu/of: Fix
probe-deferral"), and reverting that commit on post-v5.0-rc4 kernels
does fix the problem, this turned out to be a red herring.
On arm64, arch_teardown_dma_ops() resets dev->dma_ops to NULL.
Hence if a driver has used a managed DMA allocation API, the allocated
DMA memory will be freed using the direct DMA ops, while it may have
been allocated using a custom DMA ops (iommu_dma_ops in this case).
Fix this by reversing the order of the calls to devres_release_all() and
arch_teardown_dma_ops().
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Question:
Is this safe on arm32, which calls arm_teardown_iommu_dma_ops() instead
of resetting dev->dma_ops?
---
Adding some debug code, and comparing before/after commit
e8e683ae9a736407:
sata_rcar ee300000.sata: of_iommu_configure:227: err = -517
-sata_rcar ee300000.sata: of_iommu_configure:230: calling iommu_probe_device()
-sata_rcar ee300000.sata: iommu_probe_device:122: Calling ipmmu_add_device
-sata_rcar ee300000.sata: ipmmu_add_device:893
-sata_rcar ee300000.sata: of_iommu_configure:232: iommu_probe_device() returned -19
-sata_rcar ee300000.sata: dma_alloc_attrs:257: size 2048, ops = (null)
-sata_rcar ee300000.sata: __dma_direct_alloc_pages:104: size 4096
-sata_rcar ee300000.sata: __dma_direct_alloc_pages:115: trying dma_alloc_from_contiguous()
-sata_rcar ee300000.sata: dma_alloc_from_contiguous:202: cma_alloc(1) returned page ffffffbf00d20e00
-sata_rcar ee300000.sata: dma_alloc_attrs:271: allocated using dma_direct_alloc()
-sata_rcar ee300000.sata: dmam_alloc_attrs:98: size 2048 vaddr ffffff8011e95000 dma_handle 0x0x000000007c040000 attrs 0x0
-scsi host0: sata_rcar
-ata1: SATA max UDMA/133 irq 172
-ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
DMA memory used to be allocated from CMA, dma_map_ops = NULL.
The SATA driver was probed and initialized.
+sata_rcar ee300000.sata: of_iommu_configure:234: skipping iommu_probe_device()
SATA driver initialization is now deferred.
Later, it is retried:
+sata_rcar ee300000.sata: of_iommu_configure:227: err = 0
+sata_rcar ee300000.sata: of_iommu_configure:230: calling iommu_probe_device()
+sata_rcar ee300000.sata: iommu_probe_device:122: Calling ipmmu_add_device
+sata_rcar ee300000.sata: ipmmu_add_device:893
+sata_rcar ee300000.sata: Using IPMMU context 1
+iommu: Adding device ee300000.sata to group 0
+sata_rcar ee300000.sata: ipmmu_add_device:908: Success
+sata_rcar ee300000.sata: of_iommu_configure:232: iommu_probe_device() returned 0
+sata_rcar ee300000.sata: dma_alloc_attrs:257: size 2048, ops = iommu_dma_ops
+sata_rcar ee300000.sata: dma_alloc_attrs:274: allocated using __iommu_alloc_attrs()
+sata_rcar ee300000.sata: dmam_alloc_attrs:98: size 2048 vaddr ffffff8012135000 dma_handle 0x0x00000000fffff000 attrs 0x0
+scsi host0: sata_rcar
+ata1: SATA max UDMA/133 irq 172
+ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
DMA memory is now allocated using __iommu_alloc_attrs(), as dma_map_ops
= iommu_dma_ops.
Unbind:
-sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8011e95000 dma_handle 0x0x000000007c040000 attrs 0x0
+sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8012135000 dma_handle 0x0x00000000fffff000 attrs 0x0
sata_rcar ee300000.sata: dma_free_attrs:289: size 2048, ops = (null)
dma_map_ops = NULL, both before/after!
sata_rcar ee300000.sata: dma_free_attrs:308: calling dma_direct_free()
-sata_rcar ee300000.sata: __dma_direct_free_pages:189: size 2048 page ffffffbf00d20e00
-sata_rcar ee300000.sata: dma_release_from_contiguous:223: cma_release(1) freed
-sata_rcar ee300000.sata: __dma_direct_free_pages:194: skipping __free_pages()
DMA memory was correctly freed using CMA.
+sata_rcar ee300000.sata: __dma_direct_free_pages:189: size 2048 page ffffffbf029fffc8
+sata_rcar ee300000.sata: dma_release_from_contiguous:225: cma_release(1) failed, cma area ffffff8011cf5530
+sata_rcar ee300000.sata: __dma_direct_free_pages:191: calling __free_pages()
+Unable to handle kernel paging request at virtual address ffffffbf029ffffc
Memory is incorrectly freed using the direct ops, as dma_map_ops = NULL.
Oops...
After reversing the order of the calls to arch_teardown_dma_ops() and
devres_release_all(), dma_map_ops is still valid, and the DMA memory is
now released using __iommu_free_attrs():
+sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8012145000 dma_handle 0x0x00000000fffff000 attrs 0x0
+sata_rcar ee300000.sata: dma_free_attrs:289: size 2048, ops = iommu_dma_ops
+sata_rcar ee300000.sata: dma_free_attrs:311: calling __iommu_free_attrs()
---
drivers/base/dd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 8ac10af17c0043a3..d62487d024559620 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -968,9 +968,9 @@ static void __device_release_driver(struct device *dev, struct device *parent)
drv->remove(dev);
device_links_driver_cleanup(dev);
- arch_teardown_dma_ops(dev);
devres_release_all(dev);
+ arch_teardown_dma_ops(dev);
dev->driver = NULL;
dev_set_drvdata(dev, NULL);
if (dev->pm_domain && dev->pm_domain->dismiss)
--
2.17.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
2019-02-07 19:36 [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release Geert Uytterhoeven
@ 2019-02-08 7:52 ` Christoph Hellwig
2019-02-08 9:41 ` Rafael J. Wysocki
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2019-02-08 7:52 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Robin Murphy, Christoph Hellwig,
Marek Szyprowski, Joerg Roedel, Rafael J . Wysocki, iommu,
linux-arm-kernel, linux-renesas-soc, linux-kernel
Yes, we should not reset the DMA ops before releasing all resources:
Acked-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
2019-02-07 19:36 [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release Geert Uytterhoeven
2019-02-08 7:52 ` Christoph Hellwig
@ 2019-02-08 9:41 ` Rafael J. Wysocki
2019-02-08 11:34 ` Robin Murphy
2019-02-08 16:40 ` Joerg Roedel
3 siblings, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2019-02-08 9:41 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Robin Murphy, Christoph Hellwig,
Marek Szyprowski, Joerg Roedel, Rafael J . Wysocki,
open list:AMD IOMMU (AMD-VI),
Linux ARM, Linux-Renesas, Linux Kernel Mailing List
On Thu, Feb 7, 2019 at 8:42 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> When unbinding the (IOMMU-enabled) R-Car SATA device on Salvator-XS
> (R-Car H3 ES2.0), in preparation of rebinding against vfio-platform for
> device pass-through for virtualization:
>
> echo ee300000.sata > /sys/bus/platform/drivers/sata_rcar/unbind
>
> the kernel crashes with:
>
> Unable to handle kernel paging request at virtual address ffffffbf029ffffc
> Mem abort info:
> ESR = 0x96000006
> Exception class = DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x00000006
> CM = 0, WnR = 0
> swapper pgtable: 4k pages, 39-bit VAs, pgdp = 000000007e8c586c
> [ffffffbf029ffffc] pgd=000000073bfc6003, pud=000000073bfc6003, pmd=0000000000000000
> Internal error: Oops: 96000006 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 1098 Comm: bash Not tainted 5.0.0-rc5-salvator-x-00452-g37596f884f4318ef #287
> Hardware name: Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ (DT)
> pstate: 60400005 (nZCv daif +PAN -UAO)
> pc : __free_pages+0x8/0x58
> lr : __dma_direct_free_pages+0x50/0x5c
> sp : ffffff801268baa0
> x29: ffffff801268baa0 x28: 0000000000000000
> x27: ffffffc6f9c60bf0 x26: ffffffc6f9c60bf0
> x25: ffffffc6f9c60810 x24: 0000000000000000
> x23: 00000000fffff000 x22: ffffff8012145000
> x21: 0000000000000800 x20: ffffffbf029fffc8
> x19: 0000000000000000 x18: ffffffc6f86c42c8
> x17: 0000000000000000 x16: 0000000000000070
> x15: 0000000000000003 x14: 0000000000000000
> x13: ffffff801103d7f8 x12: 0000000000000028
> x11: ffffff8011117604 x10: 0000000000009ad8
> x9 : ffffff80110126d0 x8 : ffffffc6f7563000
> x7 : 6b6b6b6b6b6b6b6b x6 : 0000000000000018
> x5 : ffffff8011cf3cc8 x4 : 0000000000004000
> x3 : 0000000000080000 x2 : 0000000000000001
> x1 : 0000000000000000 x0 : ffffffbf029fffc8
> Process bash (pid: 1098, stack limit = 0x00000000c38e3e32)
> Call trace:
> __free_pages+0x8/0x58
> __dma_direct_free_pages+0x50/0x5c
> arch_dma_free+0x1c/0x98
> dma_direct_free+0x14/0x24
> dma_free_attrs+0x9c/0xdc
> dmam_release+0x18/0x20
> release_nodes+0x25c/0x28c
> devres_release_all+0x48/0x4c
> device_release_driver_internal+0x184/0x1f0
> device_release_driver+0x14/0x1c
> unbind_store+0x70/0xb8
> drv_attr_store+0x24/0x34
> sysfs_kf_write+0x4c/0x64
> kernfs_fop_write+0x154/0x1c4
> __vfs_write+0x34/0x164
> vfs_write+0xb4/0x16c
> ksys_write+0x5c/0xbc
> __arm64_sys_write+0x14/0x1c
> el0_svc_common+0x98/0x114
> el0_svc_handler+0x1c/0x24
> el0_svc+0x8/0xc
> Code: d51b4234 17fffffa a9bf7bfd 910003fd (b9403404)
> ---[ end trace 8c564cdd3a1a840f ]---
>
> While I've bisected this to commit e8e683ae9a736407 ("iommu/of: Fix
> probe-deferral"), and reverting that commit on post-v5.0-rc4 kernels
> does fix the problem, this turned out to be a red herring.
>
> On arm64, arch_teardown_dma_ops() resets dev->dma_ops to NULL.
> Hence if a driver has used a managed DMA allocation API, the allocated
> DMA memory will be freed using the direct DMA ops, while it may have
> been allocated using a custom DMA ops (iommu_dma_ops in this case).
>
> Fix this by reversing the order of the calls to devres_release_all() and
> arch_teardown_dma_ops().
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> Question:
> Is this safe on arm32, which calls arm_teardown_iommu_dma_ops() instead
> of resetting dev->dma_ops?
>
> ---
> Adding some debug code, and comparing before/after commit
> e8e683ae9a736407:
>
> sata_rcar ee300000.sata: of_iommu_configure:227: err = -517
> -sata_rcar ee300000.sata: of_iommu_configure:230: calling iommu_probe_device()
> -sata_rcar ee300000.sata: iommu_probe_device:122: Calling ipmmu_add_device
> -sata_rcar ee300000.sata: ipmmu_add_device:893
> -sata_rcar ee300000.sata: of_iommu_configure:232: iommu_probe_device() returned -19
> -sata_rcar ee300000.sata: dma_alloc_attrs:257: size 2048, ops = (null)
> -sata_rcar ee300000.sata: __dma_direct_alloc_pages:104: size 4096
> -sata_rcar ee300000.sata: __dma_direct_alloc_pages:115: trying dma_alloc_from_contiguous()
> -sata_rcar ee300000.sata: dma_alloc_from_contiguous:202: cma_alloc(1) returned page ffffffbf00d20e00
> -sata_rcar ee300000.sata: dma_alloc_attrs:271: allocated using dma_direct_alloc()
> -sata_rcar ee300000.sata: dmam_alloc_attrs:98: size 2048 vaddr ffffff8011e95000 dma_handle 0x0x000000007c040000 attrs 0x0
> -scsi host0: sata_rcar
> -ata1: SATA max UDMA/133 irq 172
> -ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>
> DMA memory used to be allocated from CMA, dma_map_ops = NULL.
> The SATA driver was probed and initialized.
>
> +sata_rcar ee300000.sata: of_iommu_configure:234: skipping iommu_probe_device()
>
> SATA driver initialization is now deferred.
> Later, it is retried:
>
> +sata_rcar ee300000.sata: of_iommu_configure:227: err = 0
> +sata_rcar ee300000.sata: of_iommu_configure:230: calling iommu_probe_device()
> +sata_rcar ee300000.sata: iommu_probe_device:122: Calling ipmmu_add_device
> +sata_rcar ee300000.sata: ipmmu_add_device:893
> +sata_rcar ee300000.sata: Using IPMMU context 1
> +iommu: Adding device ee300000.sata to group 0
> +sata_rcar ee300000.sata: ipmmu_add_device:908: Success
> +sata_rcar ee300000.sata: of_iommu_configure:232: iommu_probe_device() returned 0
> +sata_rcar ee300000.sata: dma_alloc_attrs:257: size 2048, ops = iommu_dma_ops
> +sata_rcar ee300000.sata: dma_alloc_attrs:274: allocated using __iommu_alloc_attrs()
> +sata_rcar ee300000.sata: dmam_alloc_attrs:98: size 2048 vaddr ffffff8012135000 dma_handle 0x0x00000000fffff000 attrs 0x0
> +scsi host0: sata_rcar
> +ata1: SATA max UDMA/133 irq 172
> +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>
> DMA memory is now allocated using __iommu_alloc_attrs(), as dma_map_ops
> = iommu_dma_ops.
>
> Unbind:
>
> -sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8011e95000 dma_handle 0x0x000000007c040000 attrs 0x0
> +sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8012135000 dma_handle 0x0x00000000fffff000 attrs 0x0
> sata_rcar ee300000.sata: dma_free_attrs:289: size 2048, ops = (null)
>
> dma_map_ops = NULL, both before/after!
>
> sata_rcar ee300000.sata: dma_free_attrs:308: calling dma_direct_free()
> -sata_rcar ee300000.sata: __dma_direct_free_pages:189: size 2048 page ffffffbf00d20e00
> -sata_rcar ee300000.sata: dma_release_from_contiguous:223: cma_release(1) freed
> -sata_rcar ee300000.sata: __dma_direct_free_pages:194: skipping __free_pages()
>
> DMA memory was correctly freed using CMA.
>
> +sata_rcar ee300000.sata: __dma_direct_free_pages:189: size 2048 page ffffffbf029fffc8
> +sata_rcar ee300000.sata: dma_release_from_contiguous:225: cma_release(1) failed, cma area ffffff8011cf5530
> +sata_rcar ee300000.sata: __dma_direct_free_pages:191: calling __free_pages()
> +Unable to handle kernel paging request at virtual address ffffffbf029ffffc
>
> Memory is incorrectly freed using the direct ops, as dma_map_ops = NULL.
> Oops...
>
> After reversing the order of the calls to arch_teardown_dma_ops() and
> devres_release_all(), dma_map_ops is still valid, and the DMA memory is
> now released using __iommu_free_attrs():
>
> +sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8012145000 dma_handle 0x0x00000000fffff000 attrs 0x0
> +sata_rcar ee300000.sata: dma_free_attrs:289: size 2048, ops = iommu_dma_ops
> +sata_rcar ee300000.sata: dma_free_attrs:311: calling __iommu_free_attrs()
> ---
> drivers/base/dd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 8ac10af17c0043a3..d62487d024559620 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -968,9 +968,9 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> drv->remove(dev);
>
> device_links_driver_cleanup(dev);
> - arch_teardown_dma_ops(dev);
>
> devres_release_all(dev);
> + arch_teardown_dma_ops(dev);
> dev->driver = NULL;
> dev_set_drvdata(dev, NULL);
> if (dev->pm_domain && dev->pm_domain->dismiss)
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
2019-02-07 19:36 [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release Geert Uytterhoeven
2019-02-08 7:52 ` Christoph Hellwig
2019-02-08 9:41 ` Rafael J. Wysocki
@ 2019-02-08 11:34 ` Robin Murphy
2019-02-08 16:40 ` Joerg Roedel
3 siblings, 0 replies; 8+ messages in thread
From: Robin Murphy @ 2019-02-08 11:34 UTC (permalink / raw)
To: Geert Uytterhoeven, Greg Kroah-Hartman, Christoph Hellwig,
Marek Szyprowski, Joerg Roedel, Rafael J . Wysocki
Cc: iommu, linux-arm-kernel, linux-renesas-soc, linux-kernel
On 07/02/2019 19:36, Geert Uytterhoeven wrote:
> When unbinding the (IOMMU-enabled) R-Car SATA device on Salvator-XS
> (R-Car H3 ES2.0), in preparation of rebinding against vfio-platform for
> device pass-through for virtualization:
>
> echo ee300000.sata > /sys/bus/platform/drivers/sata_rcar/unbind
>
> the kernel crashes with:
>
> Unable to handle kernel paging request at virtual address ffffffbf029ffffc
> Mem abort info:
> ESR = 0x96000006
> Exception class = DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x00000006
> CM = 0, WnR = 0
> swapper pgtable: 4k pages, 39-bit VAs, pgdp = 000000007e8c586c
> [ffffffbf029ffffc] pgd=000000073bfc6003, pud=000000073bfc6003, pmd=0000000000000000
> Internal error: Oops: 96000006 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 1098 Comm: bash Not tainted 5.0.0-rc5-salvator-x-00452-g37596f884f4318ef #287
> Hardware name: Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ (DT)
> pstate: 60400005 (nZCv daif +PAN -UAO)
> pc : __free_pages+0x8/0x58
> lr : __dma_direct_free_pages+0x50/0x5c
> sp : ffffff801268baa0
> x29: ffffff801268baa0 x28: 0000000000000000
> x27: ffffffc6f9c60bf0 x26: ffffffc6f9c60bf0
> x25: ffffffc6f9c60810 x24: 0000000000000000
> x23: 00000000fffff000 x22: ffffff8012145000
> x21: 0000000000000800 x20: ffffffbf029fffc8
> x19: 0000000000000000 x18: ffffffc6f86c42c8
> x17: 0000000000000000 x16: 0000000000000070
> x15: 0000000000000003 x14: 0000000000000000
> x13: ffffff801103d7f8 x12: 0000000000000028
> x11: ffffff8011117604 x10: 0000000000009ad8
> x9 : ffffff80110126d0 x8 : ffffffc6f7563000
> x7 : 6b6b6b6b6b6b6b6b x6 : 0000000000000018
> x5 : ffffff8011cf3cc8 x4 : 0000000000004000
> x3 : 0000000000080000 x2 : 0000000000000001
> x1 : 0000000000000000 x0 : ffffffbf029fffc8
> Process bash (pid: 1098, stack limit = 0x00000000c38e3e32)
> Call trace:
> __free_pages+0x8/0x58
> __dma_direct_free_pages+0x50/0x5c
> arch_dma_free+0x1c/0x98
> dma_direct_free+0x14/0x24
> dma_free_attrs+0x9c/0xdc
> dmam_release+0x18/0x20
> release_nodes+0x25c/0x28c
> devres_release_all+0x48/0x4c
> device_release_driver_internal+0x184/0x1f0
> device_release_driver+0x14/0x1c
> unbind_store+0x70/0xb8
> drv_attr_store+0x24/0x34
> sysfs_kf_write+0x4c/0x64
> kernfs_fop_write+0x154/0x1c4
> __vfs_write+0x34/0x164
> vfs_write+0xb4/0x16c
> ksys_write+0x5c/0xbc
> __arm64_sys_write+0x14/0x1c
> el0_svc_common+0x98/0x114
> el0_svc_handler+0x1c/0x24
> el0_svc+0x8/0xc
> Code: d51b4234 17fffffa a9bf7bfd 910003fd (b9403404)
> ---[ end trace 8c564cdd3a1a840f ]---
>
> While I've bisected this to commit e8e683ae9a736407 ("iommu/of: Fix
> probe-deferral"), and reverting that commit on post-v5.0-rc4 kernels
> does fix the problem, this turned out to be a red herring.
>
> On arm64, arch_teardown_dma_ops() resets dev->dma_ops to NULL.
> Hence if a driver has used a managed DMA allocation API, the allocated
> DMA memory will be freed using the direct DMA ops, while it may have
> been allocated using a custom DMA ops (iommu_dma_ops in this case).
>
> Fix this by reversing the order of the calls to devres_release_all() and
> arch_teardown_dma_ops().
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Question:
> Is this safe on arm32, which calls arm_teardown_iommu_dma_ops() instead
> of resetting dev->dma_ops?
Should be - the principle is the same, and even if it did break that
would only be indicative of a separate bug being hidden by this one.
This fix looks entirely valid and correct to me:
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> ---
> Adding some debug code, and comparing before/after commit
> e8e683ae9a736407:
>
> sata_rcar ee300000.sata: of_iommu_configure:227: err = -517
> -sata_rcar ee300000.sata: of_iommu_configure:230: calling iommu_probe_device()
> -sata_rcar ee300000.sata: iommu_probe_device:122: Calling ipmmu_add_device
> -sata_rcar ee300000.sata: ipmmu_add_device:893
> -sata_rcar ee300000.sata: of_iommu_configure:232: iommu_probe_device() returned -19
> -sata_rcar ee300000.sata: dma_alloc_attrs:257: size 2048, ops = (null)
> -sata_rcar ee300000.sata: __dma_direct_alloc_pages:104: size 4096
> -sata_rcar ee300000.sata: __dma_direct_alloc_pages:115: trying dma_alloc_from_contiguous()
> -sata_rcar ee300000.sata: dma_alloc_from_contiguous:202: cma_alloc(1) returned page ffffffbf00d20e00
> -sata_rcar ee300000.sata: dma_alloc_attrs:271: allocated using dma_direct_alloc()
> -sata_rcar ee300000.sata: dmam_alloc_attrs:98: size 2048 vaddr ffffff8011e95000 dma_handle 0x0x000000007c040000 attrs 0x0
> -scsi host0: sata_rcar
> -ata1: SATA max UDMA/133 irq 172
> -ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>
> DMA memory used to be allocated from CMA, dma_map_ops = NULL.
> The SATA driver was probed and initialized.
>
> +sata_rcar ee300000.sata: of_iommu_configure:234: skipping iommu_probe_device()
>
> SATA driver initialization is now deferred.
> Later, it is retried:
>
> +sata_rcar ee300000.sata: of_iommu_configure:227: err = 0
> +sata_rcar ee300000.sata: of_iommu_configure:230: calling iommu_probe_device()
> +sata_rcar ee300000.sata: iommu_probe_device:122: Calling ipmmu_add_device
> +sata_rcar ee300000.sata: ipmmu_add_device:893
> +sata_rcar ee300000.sata: Using IPMMU context 1
> +iommu: Adding device ee300000.sata to group 0
> +sata_rcar ee300000.sata: ipmmu_add_device:908: Success
> +sata_rcar ee300000.sata: of_iommu_configure:232: iommu_probe_device() returned 0
> +sata_rcar ee300000.sata: dma_alloc_attrs:257: size 2048, ops = iommu_dma_ops
> +sata_rcar ee300000.sata: dma_alloc_attrs:274: allocated using __iommu_alloc_attrs()
> +sata_rcar ee300000.sata: dmam_alloc_attrs:98: size 2048 vaddr ffffff8012135000 dma_handle 0x0x00000000fffff000 attrs 0x0
> +scsi host0: sata_rcar
> +ata1: SATA max UDMA/133 irq 172
> +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>
> DMA memory is now allocated using __iommu_alloc_attrs(), as dma_map_ops
> = iommu_dma_ops.
>
> Unbind:
>
> -sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8011e95000 dma_handle 0x0x000000007c040000 attrs 0x0
> +sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8012135000 dma_handle 0x0x00000000fffff000 attrs 0x0
> sata_rcar ee300000.sata: dma_free_attrs:289: size 2048, ops = (null)
>
> dma_map_ops = NULL, both before/after!
>
> sata_rcar ee300000.sata: dma_free_attrs:308: calling dma_direct_free()
> -sata_rcar ee300000.sata: __dma_direct_free_pages:189: size 2048 page ffffffbf00d20e00
> -sata_rcar ee300000.sata: dma_release_from_contiguous:223: cma_release(1) freed
> -sata_rcar ee300000.sata: __dma_direct_free_pages:194: skipping __free_pages()
>
> DMA memory was correctly freed using CMA.
>
> +sata_rcar ee300000.sata: __dma_direct_free_pages:189: size 2048 page ffffffbf029fffc8
> +sata_rcar ee300000.sata: dma_release_from_contiguous:225: cma_release(1) failed, cma area ffffff8011cf5530
> +sata_rcar ee300000.sata: __dma_direct_free_pages:191: calling __free_pages()
> +Unable to handle kernel paging request at virtual address ffffffbf029ffffc
>
> Memory is incorrectly freed using the direct ops, as dma_map_ops = NULL.
> Oops...
>
> After reversing the order of the calls to arch_teardown_dma_ops() and
> devres_release_all(), dma_map_ops is still valid, and the DMA memory is
> now released using __iommu_free_attrs():
>
> +sata_rcar ee300000.sata: dmam_release:32: size 2048 vaddr ffffff8012145000 dma_handle 0x0x00000000fffff000 attrs 0x0
> +sata_rcar ee300000.sata: dma_free_attrs:289: size 2048, ops = iommu_dma_ops
> +sata_rcar ee300000.sata: dma_free_attrs:311: calling __iommu_free_attrs()
> ---
> drivers/base/dd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 8ac10af17c0043a3..d62487d024559620 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -968,9 +968,9 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> drv->remove(dev);
>
> device_links_driver_cleanup(dev);
> - arch_teardown_dma_ops(dev);
>
> devres_release_all(dev);
> + arch_teardown_dma_ops(dev);
> dev->driver = NULL;
> dev_set_drvdata(dev, NULL);
> if (dev->pm_domain && dev->pm_domain->dismiss)
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
2019-02-07 19:36 [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release Geert Uytterhoeven
` (2 preceding siblings ...)
2019-02-08 11:34 ` Robin Murphy
@ 2019-02-08 16:40 ` Joerg Roedel
2019-02-08 17:55 ` Robin Murphy
3 siblings, 1 reply; 8+ messages in thread
From: Joerg Roedel @ 2019-02-08 16:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Robin Murphy, Christoph Hellwig,
Marek Szyprowski, Rafael J . Wysocki, iommu, linux-arm-kernel,
linux-renesas-soc, linux-kernel
Hi Geert,
On Thu, Feb 07, 2019 at 08:36:53PM +0100, Geert Uytterhoeven wrote:
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 8ac10af17c0043a3..d62487d024559620 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -968,9 +968,9 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> drv->remove(dev);
>
> device_links_driver_cleanup(dev);
> - arch_teardown_dma_ops(dev);
>
> devres_release_all(dev);
> + arch_teardown_dma_ops(dev);
> dev->driver = NULL;
> dev_set_drvdata(dev, NULL);
> if (dev->pm_domain && dev->pm_domain->dismiss)
Thanks for the fix! Should it also be tagged for stable and get a Fixes
tag? I know it only triggers with a fix in v5.0-rc, but still...
Regards,
Joerg
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
2019-02-08 16:40 ` Joerg Roedel
@ 2019-02-08 17:55 ` Robin Murphy
2019-02-08 18:55 ` Geert Uytterhoeven
0 siblings, 1 reply; 8+ messages in thread
From: Robin Murphy @ 2019-02-08 17:55 UTC (permalink / raw)
To: Joerg Roedel, Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Christoph Hellwig, Marek Szyprowski,
Rafael J . Wysocki, iommu, linux-arm-kernel, linux-renesas-soc,
linux-kernel
On 08/02/2019 16:40, Joerg Roedel wrote:
> Hi Geert,
>
> On Thu, Feb 07, 2019 at 08:36:53PM +0100, Geert Uytterhoeven wrote:
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 8ac10af17c0043a3..d62487d024559620 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -968,9 +968,9 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>> drv->remove(dev);
>>
>> device_links_driver_cleanup(dev);
>> - arch_teardown_dma_ops(dev);
>>
>> devres_release_all(dev);
>> + arch_teardown_dma_ops(dev);
>> dev->driver = NULL;
>> dev_set_drvdata(dev, NULL);
>> if (dev->pm_domain && dev->pm_domain->dismiss)
>
> Thanks for the fix! Should it also be tagged for stable and get a Fixes
> tag? I know it only triggers with a fix in v5.0-rc, but still...
I think so:
Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time
for platform/amba/pci bus devices")
There aren't many drivers using dmam_alloc_*(), let alone which would
also find themselves behind an IOMMU on an Arm system, but it turns out
I actually have another one which can reproduce the BUG() with 5.0-rc.
I've tried a 4.12 kernel with a bit of instrumentation[1] and sure
enough the devres-managed buffer is freed with the wrong ops[2] even
then. How it manages not to blow up more catastrophically I have no
idea... I guess at best it just leaks the buffers and IOMMU mappings,
and at worst quietly frees random other pages instead.
Robin.
--------------
[1]
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4f3eecedca2d..f4dbaa5598e3 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -491,6 +491,7 @@ static inline void *dma_alloc_attrs(struct device
*dev, size_t size,
return NULL;
cpu_addr = ops->alloc(dev, size, dma_handle, flag, attrs);
+ dev_info(dev, "alloc %lx %lx\n", (unsigned long)cpu_addr, (unsigned
long)ops);
debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr);
return cpu_addr;
}
@@ -512,6 +513,7 @@ static inline void dma_free_attrs(struct device
*dev, size_t size,
debug_dma_free_coherent(dev, size, cpu_addr, dma_handle);
ops->free(dev, size, cpu_addr, dma_handle, attrs);
+ dev_info(dev, "free %lx %lx\n", (unsigned long)cpu_addr, (unsigned
long)ops);
}
static inline void *dma_alloc_coherent(struct device *dev, size_t size,
-------------
[2]
/ # echo '0000:03:00.0' > /sys/bus/pci/drivers/sata_sil24/bind
[ 107.417252] sata_sil24 0000:03:00.0: alloc ffff00000a6f9000
ffff0000089b8090
[ 107.424397] sata_sil24 0000:03:00.0: alloc ffff00000a719000
ffff0000089b8090
[ 107.432216] scsi host0: sata_sil24
[ 107.436134] scsi host1: sata_sil24
[ 107.439853] ata7: SATA max UDMA/100 host m128@0x50084000 port
0x50080000 irq 51
[ 107.447228] ata8: SATA max UDMA/100 host m128@0x50084000 port
0x50082000 irq 51
/ # echo '0000:03:00.0' > /sys/bus/pci/drivers/sata_sil24/unbind
...
[ 112.048654] sata_sil24 0000:03:00.0: free ffff00000a719000
ffff0000089b8120
[ 112.055579] sata_sil24 0000:03:00.0: free ffff00000a6f9000
ffff0000089b8120
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
2019-02-08 17:55 ` Robin Murphy
@ 2019-02-08 18:55 ` Geert Uytterhoeven
2019-02-11 10:22 ` Robin Murphy
0 siblings, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2019-02-08 18:55 UTC (permalink / raw)
To: Robin Murphy
Cc: Joerg Roedel, Geert Uytterhoeven, Greg Kroah-Hartman,
Christoph Hellwig, Marek Szyprowski, Rafael J . Wysocki,
Linux IOMMU, Linux ARM, Linux-Renesas, Linux Kernel Mailing List
Hi Robin,
On Fri, Feb 8, 2019 at 6:55 PM Robin Murphy <robin.murphy@arm.com> wrote:
> On 08/02/2019 16:40, Joerg Roedel wrote:
> > On Thu, Feb 07, 2019 at 08:36:53PM +0100, Geert Uytterhoeven wrote:
> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >> index 8ac10af17c0043a3..d62487d024559620 100644
> >> --- a/drivers/base/dd.c
> >> +++ b/drivers/base/dd.c
> >> @@ -968,9 +968,9 @@ static void __device_release_driver(struct device *dev, struct device *parent)
> >> drv->remove(dev);
> >>
> >> device_links_driver_cleanup(dev);
> >> - arch_teardown_dma_ops(dev);
> >>
> >> devres_release_all(dev);
> >> + arch_teardown_dma_ops(dev);
> >> dev->driver = NULL;
> >> dev_set_drvdata(dev, NULL);
> >> if (dev->pm_domain && dev->pm_domain->dismiss)
> >
> > Thanks for the fix! Should it also be tagged for stable and get a Fixes
FTR, Greg has added it to driver-core-testing, with a CC to stable.
> > tag? I know it only triggers with a fix in v5.0-rc, but still...
>
> I think so:
>
> Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time
> for platform/amba/pci bus devices")
Thanks! It won't backport cleanly due to commit dc3c05504d38849f
("dma-mapping: remove dma_deconfigure") in v4.20, though.
> There aren't many drivers using dmam_alloc_*(), let alone which would
> also find themselves behind an IOMMU on an Arm system, but it turns out
> I actually have another one which can reproduce the BUG() with 5.0-rc.
SATA core uses dmam_alloc_*().
> I've tried a 4.12 kernel with a bit of instrumentation[1] and sure
> enough the devres-managed buffer is freed with the wrong ops[2] even
> then. How it manages not to blow up more catastrophically I have no
> idea... I guess at best it just leaks the buffers and IOMMU mappings,
> and at worst quietly frees random other pages instead.
May depend on the actual ops, and whether CMA is used or not.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
2019-02-08 18:55 ` Geert Uytterhoeven
@ 2019-02-11 10:22 ` Robin Murphy
0 siblings, 0 replies; 8+ messages in thread
From: Robin Murphy @ 2019-02-11 10:22 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Joerg Roedel, Geert Uytterhoeven, Greg Kroah-Hartman,
Christoph Hellwig, Marek Szyprowski, Rafael J . Wysocki,
Linux IOMMU, Linux ARM, Linux-Renesas, Linux Kernel Mailing List
On 08/02/2019 18:55, Geert Uytterhoeven wrote:
> Hi Robin,
>
> On Fri, Feb 8, 2019 at 6:55 PM Robin Murphy <robin.murphy@arm.com> wrote:
>> On 08/02/2019 16:40, Joerg Roedel wrote:
>>> On Thu, Feb 07, 2019 at 08:36:53PM +0100, Geert Uytterhoeven wrote:
>>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>>>> index 8ac10af17c0043a3..d62487d024559620 100644
>>>> --- a/drivers/base/dd.c
>>>> +++ b/drivers/base/dd.c
>>>> @@ -968,9 +968,9 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>>>> drv->remove(dev);
>>>>
>>>> device_links_driver_cleanup(dev);
>>>> - arch_teardown_dma_ops(dev);
>>>>
>>>> devres_release_all(dev);
>>>> + arch_teardown_dma_ops(dev);
>>>> dev->driver = NULL;
>>>> dev_set_drvdata(dev, NULL);
>>>> if (dev->pm_domain && dev->pm_domain->dismiss)
>>>
>>> Thanks for the fix! Should it also be tagged for stable and get a Fixes
>
> FTR, Greg has added it to driver-core-testing, with a CC to stable.
So I see, great!
>>> tag? I know it only triggers with a fix in v5.0-rc, but still...
>>
>> I think so:
>>
>> Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time
>> for platform/amba/pci bus devices")
>
> Thanks! It won't backport cleanly due to commit dc3c05504d38849f
> ("dma-mapping: remove dma_deconfigure") in v4.20, though.
Ah yes - backports beyond that should simply be a case of moving the
dma_deconfigure() wrapper in the same manner.
Thanks,
Robin.
>> There aren't many drivers using dmam_alloc_*(), let alone which would
>> also find themselves behind an IOMMU on an Arm system, but it turns out
>> I actually have another one which can reproduce the BUG() with 5.0-rc.
>
> SATA core uses dmam_alloc_*().
>
>> I've tried a 4.12 kernel with a bit of instrumentation[1] and sure
>> enough the devres-managed buffer is freed with the wrong ops[2] even
>> then. How it manages not to blow up more catastrophically I have no
>> idea... I guess at best it just leaks the buffers and IOMMU mappings,
>> and at worst quietly frees random other pages instead.
>
> May depend on the actual ops, and whether CMA is used or not.
>
> Gr{oetje,eeting}s,
>
> Geert
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, back to index
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 19:36 [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release Geert Uytterhoeven
2019-02-08 7:52 ` Christoph Hellwig
2019-02-08 9:41 ` Rafael J. Wysocki
2019-02-08 11:34 ` Robin Murphy
2019-02-08 16:40 ` Joerg Roedel
2019-02-08 17:55 ` Robin Murphy
2019-02-08 18:55 ` Geert Uytterhoeven
2019-02-11 10:22 ` Robin Murphy
Linux-Renesas-SoC Archive on lore.kernel.org
Archives are clonable:
git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
public-inbox-index linux-renesas-soc
Newsgroup available over NNTP:
nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc
AGPL code for this site: git clone https://public-inbox.org/ public-inbox