On 08/10/2024 5:13 pm, H. Nikolaus Schaller wrote:
Hi Robin,
Am 08.10.2024 um 16:00 schrieb Robin Murphy <robin.murphy@xxxxxxx>:
On 08/10/2024 2:32 pm, H. Nikolaus Schaller wrote:
Hi Robin,
Am 08.10.2024 um 15:17 schrieb Robin Murphy <robin.murphy@xxxxxxx>:
The OMAP driver still has its own traditional firmware parsing and
instance handling in omap_iommu_probe_device(), rather than using the
generic fwnode-based paths. However, it also passes a hwdev to
iommu_device_register(), thus registering a fwnode for each ops
instance, wherein __iommu_probe_device() then fails to find matching ops
for a client device with no fwspec and thus a NULL iommu_fwnode.
Since omap-iommu is not known to coexist with any other IOMMU hardware
and shares the same ops between all instances, we can reasonably remove
the hwdev/fwnode registration to put it back into "legacy" mode where
the ops are effectively global and ->probe_device remains responsible
for filtering individual clients.
Reported-by: Beleswar Padhi <b-padhi@xxxxxx>
Reported-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
Fixes: 17de3f5fdd35 ("iommu: Retire bus ops")
Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
---
drivers/iommu/omap-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index c9528065a59a..425ae8e551dc 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1230,7 +1230,7 @@ static int omap_iommu_probe(struct platform_device *pdev)
if (err)
return err;
- err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev);
+ err = iommu_device_register(&obj->iommu, &omap_iommu_ops, NULL);
Thanks for this proposal, but this prevents my OMAP3 system completely
from booting (at least with v6.8-rc1):
## Booting kernel from Legacy Image at 82000000 ...
Image Name: Linux-6.8.0-rc1-letux+
Image Type: ARM Linux Kernel Image (uncompressed)
Data Size: 6491520 Bytes = 6.2 MiB
Load Address: 80008000
Entry Point: 80008000
Verifying Checksum ... OK
## Flattened Device Tree blob at 81c00000
Booting using the fdt blob at 0x81c00000
Loading Kernel Image ... OK
Using Device Tree in place at 81c00000, end 81c1fe8e
Starting kernel ...
--- no further reaction --
Urgh... is it possible to get earlycon/ealyprintk output on this platform?
Ah, my mistake. earlycon did reveal a NULL pointer dereference coming from that
I have added some printk() to iommu_device_register() and friends. And one
assumed that hwdev is not a NULL pointer and we can print hwdev->kobj.name...
As a stab in the dark by inspection, there might potentially be some interaction with "iommu: Move bus setup to IOMMU device registration" as well, for which the additional diff below might help...
Thanks,
Robin.
---->8----
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 425ae8e551dc..9112178e22d8 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1691,7 +1691,8 @@ static struct iommu_device *omap_iommu_probe_device(struct device *dev)
if (!oiommu) {
of_node_put(np);
kfree(arch_data);
- return ERR_PTR(-EINVAL);
+ /* Not fatal, will re-probe once the right instance registers itself */
+ return ERR_PTR(-ENODEV);
}
tmp->iommu_dev = oiommu;
Now I can boot with both patches applied (and Jason's patch and my printk removed),
but there is still (exactly the same as with Jason's patch):
root@letux:~# uname -a
Linux letux 6.8.0-rc1-letux+ #19517 SMP PREEMPT Tue Oct 8 17:23:11 CEST 2024 armv7l GNU/Linux
root@letux:~# dmesg|fgrep iommu
[ 0.700195] iommu: Default domain type: Translated
[ 0.705078] iommu: DMA domain TLB invalidation policy: strict mode
[ 0.728393] platform 480bc000.isp: Adding to iommu group 0
[ 0.734161] omap-iommu 480bd400.mmu: 480bd400.mmu registered
[ 23.565216] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT
[ 23.605102] omap-iommu 480bd400.mmu: 480bd400.mmu: version 1.1
root@letux:~# dmesg|fgrep isp
[ 0.000000] OMAP3630/DM3730 ES1.2 (l2cache iva sgx neon isp 192mhz_clk)
[ 0.728393] platform 480bc000.isp: Adding to iommu group 0
[ 11.472045] omapdss_dss 48050000.dss: bound 48050400.dispc (ops dsi_vc_flush_receive_data [omapdrm]) <-- false positive of fgrep
[ 23.557586] omap3isp 480bc000.isp: deferred probe timeout, ignoring dependency
[ 23.565216] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT <-- duplicate with fgrep iommu
[ 23.625183] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator
[ 23.657135] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator
[ 23.665832] omap3isp 480bc000.isp: Revision 15.0 found
[ 23.700073] omap3isp 480bc000.isp: failed to attach device to VA mapping
[ 23.724182] omap3isp 480bc000.isp: unable to attach to IOMMU
[ 23.731262] omap3isp: probe of 480bc000.isp failed with error -16
root@letux:~#
The -ETIMEDOUT seems to come from of_iommu_configure().
Oof, yeah, now we've wound up with the opposite problem that because
it's the generic "iommus" binding, it gets as far as of_iommu_xlate()
but now the NULL fwnode no longer matches the phandle target so that
thinks it's waiting for an instance which hasn't registered yet :(
OK, different track, does the diff below fare any better? (I've not
written it up fully yet as the DRA7 special cases will need some more
work still)
Thanks,
Robin.
----->8-----
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index c9528065a59a..44e09d60e861 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1723,12 +1723,19 @@ static void omap_iommu_release_device(struct
device *dev)
}
+int omap_iommu_of_xlate(struct device *dev, const struct
of_phandle_args *args)
+{
+ /* TODO: collect args->np to save re-parsing in probe above */
+ return 0;
+}
+
static const struct iommu_ops omap_iommu_ops = {
.identity_domain = &omap_iommu_identity_domain,
.domain_alloc_paging = omap_iommu_domain_alloc_paging,
.probe_device = omap_iommu_probe_device,
.release_device = omap_iommu_release_device,
.device_group = generic_single_device_group,
+ .of_xlate = omap_iommu_of_xlate,
.pgsize_bitmap = OMAP_IOMMU_PGSIZES,
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = omap_iommu_attach_dev,
