[PATCH 0/3] Rockchip IOMMU fixes

[PATCH 0/3] Rockchip IOMMU fixes

[Robin Murphy]

Hi all,

I was expecting my DT probing change to flush out issues, and it seems
some weaknesses in rockchip-iommu were the first to show. The first two
patches here get things working again for me on my RK3399 NanoPC-T4
(with a console on HDMI exercising the VOP MMU), the 3rd patch just
finishes the cleanup since I was there and could test it.

Thanks,
Robin.

Robin Murphy (3):
  iommu/rockchip: Allocate per-device data sensibly
  iommu/rockchip: Register in a sensible order
  iommu/rockchip: Retire global dma_dev workaround

 drivers/iommu/rockchip-iommu.c | 61 +++++++++++++++-------------------
 1 file changed, 26 insertions(+), 35 deletions(-)

-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 1/3] iommu/rockchip: Allocate per-device data sensibly

[Robin Murphy]

Now that DT-based probing is finally happening in the right order again,
it reveals an issue in Rockchip's of_xlate, which can now be called
during registration, but is using the global dma_dev which is only
assigned later. However, this makes little sense when we're already
looking up the correct IOMMU device, who should logically be the owner
of the devm allocation anyway.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Anders Roxell <anders.roxell@linaro.org>
Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/rockchip-iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 323cc665c357..48826d1ccfd8 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1148,12 +1148,12 @@ static int rk_iommu_of_xlate(struct device *dev,
 	struct platform_device *iommu_dev;
 	struct rk_iommudata *data;
 
-	data = devm_kzalloc(dma_dev, sizeof(*data), GFP_KERNEL);
+	iommu_dev = of_find_device_by_node(args->np);
+
+	data = devm_kzalloc(&iommu_dev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	iommu_dev = of_find_device_by_node(args->np);
-
 	data->iommu = platform_get_drvdata(iommu_dev);
 	data->iommu->domain = &rk_identity_domain;
 	dev_iommu_priv_set(dev, data);
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 2/3] iommu/rockchip: Register in a sensible order

[Robin Murphy]

Currently Rockchip calls iommu_device_register() before it's finished
setting up the hardware and driver state, and as such it now gets
unhappy in various ways when registration starts working the way it was
always intended to, and probing client devices straight away. Reorder
the operations to ensure that what we're registering is a prepared and
functional IOMMU instance.

Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/rockchip-iommu.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 48826d1ccfd8..9d2120a9232a 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1256,14 +1256,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, dev_name(dev));
-	if (err)
-		goto err_unprepare_clocks;
-
-	err = iommu_device_register(&iommu->iommu, &rk_iommu_ops, dev);
-	if (err)
-		goto err_remove_sysfs;
-
 	/*
 	 * Use the first registered IOMMU device for domain to use with DMA
 	 * API, since a domain might not physically correspond to a single
@@ -1290,12 +1282,19 @@ static int rk_iommu_probe(struct platform_device *pdev)
 
 	dma_set_mask_and_coherent(dev, rk_ops->dma_bit_mask);
 
+	err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, dev_name(dev));
+	if (err)
+		goto err_pm_disable;
+
+	err = iommu_device_register(&iommu->iommu, &rk_iommu_ops, dev);
+	if (err)
+		goto err_remove_sysfs;
+
 	return 0;
-err_pm_disable:
-	pm_runtime_disable(dev);
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);
-err_unprepare_clocks:
+err_pm_disable:
+	pm_runtime_disable(dev);
 	clk_bulk_unprepare(iommu->num_clocks, iommu->clocks);
 	return err;
 }
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 3/3] iommu/rockchip: Retire global dma_dev workaround

[Robin Murphy]

The global dma_dev trick was mostly because the old domain_alloc op
provided no context, so no way to know which IOMMU was to own the
pagetable, or if a suitable one even existed at all. In the new
multi-instance world with domain_alloc_paging this is no longer a
concern - now we know that the given device must be associated with a
valid IOMMU instance which provided the op to call in the first place,
and therefore that instance can and should be the pagetable owner. To
avoid worrying about the lifetime and stability of the rk_domain->iommus
list, and keep the lookups simple and efficient, we'll still stash a
dma_dev pointer, but now it's accurately per-domain.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/rockchip-iommu.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9d2120a9232a..af4cc91b2bbf 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -88,6 +88,7 @@ struct rk_iommu_domain {
 	dma_addr_t dt_dma;
 	spinlock_t iommus_lock; /* lock for iommus list */
 	spinlock_t dt_lock; /* lock for modifying page directory table */
+	struct device *dma_dev;
 
 	struct iommu_domain domain;
 };
@@ -123,7 +124,6 @@ struct rk_iommudata {
 	struct rk_iommu *iommu;
 };
 
-static struct device *dma_dev;
 static const struct rk_iommu_ops *rk_ops;
 static struct iommu_domain rk_identity_domain;
 
@@ -132,7 +132,7 @@ static inline void rk_table_flush(struct rk_iommu_domain *dom, dma_addr_t dma,
 {
 	size_t size = count * sizeof(u32); /* count of u32 entry */
 
-	dma_sync_single_for_device(dma_dev, dma, size, DMA_TO_DEVICE);
+	dma_sync_single_for_device(dom->dma_dev, dma, size, DMA_TO_DEVICE);
 }
 
 static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom)
@@ -734,9 +734,9 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
 	if (!page_table)
 		return ERR_PTR(-ENOMEM);
 
-	pt_dma = dma_map_single(dma_dev, page_table, SPAGE_SIZE, DMA_TO_DEVICE);
-	if (dma_mapping_error(dma_dev, pt_dma)) {
-		dev_err(dma_dev, "DMA mapping error while allocating page table\n");
+	pt_dma = dma_map_single(rk_domain->dma_dev, page_table, SPAGE_SIZE, DMA_TO_DEVICE);
+	if (dma_mapping_error(rk_domain->dma_dev, pt_dma)) {
+		dev_err(rk_domain->dma_dev, "DMA mapping error while allocating page table\n");
 		iommu_free_page(page_table);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -1051,9 +1051,7 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 static struct iommu_domain *rk_iommu_domain_alloc_paging(struct device *dev)
 {
 	struct rk_iommu_domain *rk_domain;
-
-	if (!dma_dev)
-		return NULL;
+	struct rk_iommu *iommu;
 
 	rk_domain = kzalloc(sizeof(*rk_domain), GFP_KERNEL);
 	if (!rk_domain)
@@ -1068,10 +1066,12 @@ static struct iommu_domain *rk_iommu_domain_alloc_paging(struct device *dev)
 	if (!rk_domain->dt)
 		goto err_free_domain;
 
-	rk_domain->dt_dma = dma_map_single(dma_dev, rk_domain->dt,
+	iommu = rk_iommu_from_dev(dev);
+	rk_domain->dma_dev = iommu->dev;
+	rk_domain->dt_dma = dma_map_single(rk_domain->dma_dev, rk_domain->dt,
 					   SPAGE_SIZE, DMA_TO_DEVICE);
-	if (dma_mapping_error(dma_dev, rk_domain->dt_dma)) {
-		dev_err(dma_dev, "DMA map error for DT\n");
+	if (dma_mapping_error(rk_domain->dma_dev, rk_domain->dt_dma)) {
+		dev_err(rk_domain->dma_dev, "DMA map error for DT\n");
 		goto err_free_dt;
 	}
 
@@ -1105,13 +1105,13 @@ static void rk_iommu_domain_free(struct iommu_domain *domain)
 		if (rk_dte_is_pt_valid(dte)) {
 			phys_addr_t pt_phys = rk_ops->pt_address(dte);
 			u32 *page_table = phys_to_virt(pt_phys);
-			dma_unmap_single(dma_dev, pt_phys,
+			dma_unmap_single(rk_domain->dma_dev, pt_phys,
 					 SPAGE_SIZE, DMA_TO_DEVICE);
 			iommu_free_page(page_table);
 		}
 	}
 
-	dma_unmap_single(dma_dev, rk_domain->dt_dma,
+	dma_unmap_single(rk_domain->dma_dev, rk_domain->dt_dma,
 			 SPAGE_SIZE, DMA_TO_DEVICE);
 	iommu_free_page(rk_domain->dt);
 
@@ -1256,14 +1256,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	/*
-	 * Use the first registered IOMMU device for domain to use with DMA
-	 * API, since a domain might not physically correspond to a single
-	 * IOMMU device..
-	 */
-	if (!dma_dev)
-		dma_dev = &pdev->dev;
-
 	pm_runtime_enable(dev);
 
 	for (i = 0; i < iommu->num_irq; i++) {
-- 
2.39.2.101.g768bb238c484.dirty

Re: [PATCH 0/3] Rockchip IOMMU fixes

[Nicolas Frattaroli]

On Thursday, 13 March 2025 18:31:03 Central European Standard Time Robin 
Murphy wrote:
> Hi all,
> 
> I was expecting my DT probing change to flush out issues, and it seems
> some weaknesses in rockchip-iommu were the first to show. The first two
> patches here get things working again for me on my RK3399 NanoPC-T4
> (with a console on HDMI exercising the VOP MMU), the 3rd patch just
> finishes the cleanup since I was there and could test it.
> 
> Thanks,
> Robin.
> 
> Robin Murphy (3):
>   iommu/rockchip: Allocate per-device data sensibly
>   iommu/rockchip: Register in a sensible order
>   iommu/rockchip: Retire global dma_dev workaround
> 
>  drivers/iommu/rockchip-iommu.c | 61 +++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)

Tested this series on an RK3576, which indeed fixes the boot panic I get with 
next-20250314. After looking at the patches with the full function context, I 
can't see any obvious issues.

For the whole series:

Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Kind regards,
Nicolas Frattaroli

Re: [PATCH 0/3] Rockchip IOMMU fixes

[Dang Huynh]

On Thursday, March 13, 2025 5:31:03 PM UTC Robin Murphy wrote:
> Hi all,
> 
> I was expecting my DT probing change to flush out issues, and it seems
> some weaknesses in rockchip-iommu were the first to show. The first two
> patches here get things working again for me on my RK3399 NanoPC-T4
> (with a console on HDMI exercising the VOP MMU), the 3rd patch just
> finishes the cleanup since I was there and could test it.
> 
> Thanks,
> Robin.
> 
> Robin Murphy (3):
>   iommu/rockchip: Allocate per-device data sensibly
>   iommu/rockchip: Register in a sensible order
>   iommu/rockchip: Retire global dma_dev workaround
> 
>  drivers/iommu/rockchip-iommu.c | 61 +++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)

Tested this series on a RK3566 device and this fixed kernel panic/oops on boot 
since next-20250314.

For the whole series:

Tested-by: Dang Huynh <danct12@riseup.net>

Re: [PATCH 0/3] Rockchip IOMMU fixes

[Quentin Schulz]

Hi Robin,

On 3/13/25 6:31 PM, Robin Murphy wrote:
> Hi all,
> 
> I was expecting my DT probing change to flush out issues, and it seems
> some weaknesses in rockchip-iommu were the first to show. The first two
> patches here get things working again for me on my RK3399 NanoPC-T4
> (with a console on HDMI exercising the VOP MMU), the 3rd patch just
> finishes the cleanup since I was there and could test it.
> 
> Thanks,
> Robin.
> 
> Robin Murphy (3):
>    iommu/rockchip: Allocate per-device data sensibly
>    iommu/rockchip: Register in a sensible order
>    iommu/rockchip: Retire global dma_dev workaround
> 

Fixes the issue seen on next-20250314 on my RK3588 Tiger Haikou:

"""
[    2.007506] Unable to handle kernel NULL pointer dereference at 
virtual address 0000000000000298
[    2.017346] Mem abort info:
[    2.020465]   ESR = 0x0000000096000004
[    2.024654]   EC = 0x25: DABT (current EL), IL = 32 bits
[    2.030591]   SET = 0, FnV = 0
[    2.033999]   EA = 0, S1PTW = 0
[    2.037506]   FSC = 0x04: level 0 translation fault
[    2.042958] Data abort info:
[    2.046170]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[    2.052302]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[    2.057949]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    2.063885] [0000000000000298] user address but active_mm is swapper
[    2.070989] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[    2.077994] Modules linked in:
[    2.081405] CPU: 7 UID: 0 PID: 1 Comm: swapper/0 Not tainted 
6.14.0-rc6-next-20250314-dirty #10
[    2.091132] Hardware name: Theobroma Systems RK3588-Q7 SoM on Haikou 
devkit (DT)
[    2.099397] pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[    2.107179] pc : devm_kmalloc+0x28/0x100
[    2.111568] lr : rk_iommu_of_xlate+0x30/0x90
[    2.116341] sp : ffff800081d0b840
[    2.120040] x29: ffff800081d0b840 x28: 0000000000000000 x27: 
ffff8000815d3e20
[    2.128010] x26: 0000000000000001 x25: ffff8000815d4ed8 x24: 
ffff0001f08e1f60
[    2.135989] x23: 0000000000000000 x22: ffff8000811b3f58 x21: 
0000000000000000
[    2.143966] x20: 0000000000000010 x19: 0000000000000000 x18: 
00000000ffffffff
[    2.151944] x17: ffff0001f14acc00 x16: ffff0001f14aca00 x15: 
ffff0001f148b27c
[    2.159922] x14: ffff800081d0b820 x13: ffff0001f148ba1c x12: 
0101010101010101
[    2.167899] x11: 7f7f7f7f7f7f7f7f x10: 00000000000a2cc8 x9 : 
0000000000000003
[    2.175877] x8 : ffff0001f08e3540 x7 : 0000000000000000 x6 : 
1d150c11f5f38080
[    2.183855] x5 : ffff0001f0278000 x4 : 0000000000000000 x3 : 
ffff800081a77218
[    2.191832] x2 : 0000000000000dc0 x1 : 0000000000000010 x0 : 
0000000000000090
[    2.199812] Call trace:
[    2.202538]  devm_kmalloc+0x28/0x100 (P)
[    2.206924]  rk_iommu_of_xlate+0x30/0x90
[    2.211307]  of_iommu_xlate+0x94/0xc0
[    2.215400]  of_iommu_configure+0x1cc/0x250
[    2.220075]  of_dma_configure_id+0x144/0x24c
[    2.224848]  platform_dma_configure+0x8c/0xb4
[    2.229716]  __iommu_probe_device+0x2e4/0x4a0
[    2.234583]  probe_iommu_group+0x3c/0x68
[    2.238965]  bus_for_each_dev+0x7c/0xe0
[    2.243244]  iommu_device_register+0xd8/0x228
[    2.248111]  rk_iommu_probe+0x204/0x360
[    2.252397]  platform_probe+0x68/0xdc
[    2.256488]  really_probe+0xbc/0x2c0
[    2.260484]  __driver_probe_device+0x78/0x120
[    2.265355]  driver_probe_device+0x3c/0x154
[    2.270032]  __driver_attach+0x90/0x1a0
[    2.274315]  bus_for_each_dev+0x7c/0xe0
[    2.278602]  driver_attach+0x24/0x30
[    2.282599]  bus_add_driver+0xe4/0x208
[    2.286781]  driver_register+0x68/0x130
[    2.291065]  __platform_driver_register+0x24/0x30
[    2.296322]  rk_iommu_driver_init+0x1c/0x28
[    2.300997]  do_one_initcall+0x60/0x1d4
[    2.305283]  kernel_init_freeable+0x210/0x274
[    2.310153]  kernel_init+0x20/0x140
[    2.314051]  ret_from_fork+0x10/0x20
[    2.318048] Code: a90153f3 b1020020 f9001bf7 aa0103f4 (b9429ab7)
[    2.324857] ---[ end trace 0000000000000000 ]---
[    2.330029] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b
[    2.338587] SMP: stopping secondary CPUs
[    2.342971] Kernel Offset: disabled
[    2.346862] CPU features: 0x0c00,000000e0,01202650,8200720b
[    2.353089] Memory Limit: none
[    2.356499] ---[ end Kernel panic - not syncing: Attempted to kill 
init! exitcode=0x0000000b ]---
"""

Therefore:

Tested-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 0/3] Rockchip IOMMU fixes

[Joerg Roedel]

On Thu, Mar 13, 2025 at 05:31:03PM +0000, Robin Murphy wrote:
> Hi all,
> 
> I was expecting my DT probing change to flush out issues, and it seems
> some weaknesses in rockchip-iommu were the first to show. The first two
> patches here get things working again for me on my RK3399 NanoPC-T4
> (with a console on HDMI exercising the VOP MMU), the 3rd patch just
> finishes the cleanup since I was there and could test it.
> 
> Thanks,
> Robin.
> 
> Robin Murphy (3):
>   iommu/rockchip: Allocate per-device data sensibly
>   iommu/rockchip: Register in a sensible order
>   iommu/rockchip: Retire global dma_dev workaround
> 
>  drivers/iommu/rockchip-iommu.c | 61 +++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)

Applied, thanks.