BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[H. Nikolaus Schaller]

Hi,

I found that the camera on our OMAP3 based system (GTA04) stopped working with v6.8-rc1.
There was no bug in the camera driver but the OMAP3 ISP (image signal processor) emits

[   14.963684] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping
[   15.010192] omap3isp 480bc000.isp: unable to attach to IOMMU
[   15.023376] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7)
[   15.065399] omap3isp: probe of 480bc000.isp failed with error -12

Deeper analyses lead to this patch breaking operation. It is not fixed up to v6.12-rc1.

What seems to happen (in 6.8-rc1 code):

- omap_iommu_probe() passes &omap_iommu_ops to iommu_device_register()
- iommu_device_register() stores the ops in iommu->ops (only)
- __iommu_probe_device tries to read the ops from some fw_spec but not iommu->ops
- this calls iommu_ops_from_fwnode(NULL) which looks strange to me
- since it doesn't find any fw_spec or node matching a NULL fwspec it returns -ENODEV

So I wonder how the magic between setting the ops in omap_iommu_probe()
and finding them in __iommu_probe_device() is intended to work. Or how
the omap3isp driver should register some fw_node or fw_spec to make this
work.

What is a fix for this?

BR and thanks,
Nikolaus

Bug: 17de3f5fdd3567 ("iommu: Retire bus ops")


Note: Why did this bug not surface earlier? It appears that on OMAP3 the only driver
that uses iommu is omap3isp and connected cameras are not that widespread.
OMAP4 and 5 also use it for the DSP&IVA susbsystems which are also rarely used.
Hence it wasn't tested by anyone.

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[Jason Gunthorpe]

On Sun, Oct 06, 2024 at 09:40:00AM +0200, H. Nikolaus Schaller wrote:
> Hi,
> 
> I found that the camera on our OMAP3 based system (GTA04) stopped working with v6.8-rc1.
> There was no bug in the camera driver but the OMAP3 ISP (image signal processor) emits
> 
> [   14.963684] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping
> [   15.010192] omap3isp 480bc000.isp: unable to attach to IOMMU
> [   15.023376] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7)
> [   15.065399] omap3isp: probe of 480bc000.isp failed with error -12
> 
> Deeper analyses lead to this patch breaking operation. It is not fixed up to v6.12-rc1.
> 
> What seems to happen (in 6.8-rc1 code):
> 
> - omap_iommu_probe() passes &omap_iommu_ops to iommu_device_register()
> - iommu_device_register() stores the ops in iommu->ops (only)
> - __iommu_probe_device tries to read the ops from some fw_spec but not iommu->ops

Maybe like this?

@@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct platform_device *pdev)
                err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev);
                if (err)
                        goto out_sysfs;
+               /*
+                * omap has a DT reprensetation but can't use the common DT
+                * code. Setting fwnode to NULL causes probe to be called for
+                * every device.
+                */
+               obj->iommu.fwnode = NULL;
                obj->has_iommu_driver = true;
        }

Jason

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[H. Nikolaus Schaller]

Hi Jason,

> Am 07.10.2024 um 14:15 schrieb Jason Gunthorpe <jgg@nvidia.com>:
> 
> On Sun, Oct 06, 2024 at 09:40:00AM +0200, H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> I found that the camera on our OMAP3 based system (GTA04) stopped working with v6.8-rc1.
>> There was no bug in the camera driver but the OMAP3 ISP (image signal processor) emits
>> 
>> [   14.963684] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping
>> [   15.010192] omap3isp 480bc000.isp: unable to attach to IOMMU
>> [   15.023376] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7)
>> [   15.065399] omap3isp: probe of 480bc000.isp failed with error -12
>> 
>> Deeper analyses lead to this patch breaking operation. It is not fixed up to v6.12-rc1.
>> 
>> What seems to happen (in 6.8-rc1 code):
>> 
>> - omap_iommu_probe() passes &omap_iommu_ops to iommu_device_register()
>> - iommu_device_register() stores the ops in iommu->ops (only)
>> - __iommu_probe_device tries to read the ops from some fw_spec but not iommu->ops
> 
> Maybe like this?
> 
> @@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct platform_device *pdev)
>                err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev);
>                if (err)
>                        goto out_sysfs;
> +               /*
> +                * omap has a DT reprensetation but can't use the common DT
> +                * code. Setting fwnode to NULL causes probe to be called for
> +                * every device.
> +                */
> +               obj->iommu.fwnode = NULL;
>                obj->has_iommu_driver = true;
>        }

I'll give it a try asap and report back.

BR and thanks,
Nikolaus

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[Andreas Kemnade]

Am Mon, 7 Oct 2024 09:15:43 -0300
schrieb Jason Gunthorpe <jgg@nvidia.com>:

> On Sun, Oct 06, 2024 at 09:40:00AM +0200, H. Nikolaus Schaller wrote:
> > Hi,
> > 
> > I found that the camera on our OMAP3 based system (GTA04) stopped
> > working with v6.8-rc1. There was no bug in the camera driver but
> > the OMAP3 ISP (image signal processor) emits
> > 
> > [   14.963684] omap3isp 480bc000.isp: failed to create ARM IOMMU
> > mapping [   15.010192] omap3isp 480bc000.isp: unable to attach to
> > IOMMU [   15.023376] omap3isp 480bc000.isp: isp_xclk_set_rate:
> > cam_xclka set to 24685714 Hz (div 7) [   15.065399] omap3isp: probe
> > of 480bc000.isp failed with error -12
> > 
> > Deeper analyses lead to this patch breaking operation. It is not
> > fixed up to v6.12-rc1.
> > 
> > What seems to happen (in 6.8-rc1 code):
> > 
> > - omap_iommu_probe() passes &omap_iommu_ops to
> > iommu_device_register()
> > - iommu_device_register() stores the ops in iommu->ops (only)
> > - __iommu_probe_device tries to read the ops from some fw_spec but
> > not iommu->ops  
> 
> Maybe like this?
> 
> @@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct
> platform_device *pdev) err = iommu_device_register(&obj->iommu,
> &omap_iommu_ops, &pdev->dev); if (err)
>                         goto out_sysfs;
> +               /*
> +                * omap has a DT reprensetation but can't use the
> common DT
> +                * code. Setting fwnode to NULL causes probe to be
> called for
> +                * every device.
> +                */
> +               obj->iommu.fwnode = NULL;
>                 obj->has_iommu_driver = true;
>         }
> 
hmm, that looks nice for a regression fix.

Does it make sense to adopt dt so that the common code can be used to
ease future maintenance?

Regards,
Andreas

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[Jason Gunthorpe]

On Mon, Oct 07, 2024 at 04:01:17PM +0200, Andreas Kemnade wrote:
> > @@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct
> > platform_device *pdev) err = iommu_device_register(&obj->iommu,
> > &omap_iommu_ops, &pdev->dev); if (err)
> >                         goto out_sysfs;
> > +               /*
> > +                * omap has a DT reprensetation but can't use the
> > common DT
> > +                * code. Setting fwnode to NULL causes probe to be
> > called for
> > +                * every device.
> > +                */
> > +               obj->iommu.fwnode = NULL;
> >                 obj->has_iommu_driver = true;
> >         }
> > 
> hmm, that looks nice for a regression fix.
> 
> Does it make sense to adopt dt so that the common code can be used to
> ease future maintenance?

It would be nice, but I recall omap doesn't use the standard dt
layout?

Jason

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[Andreas Kemnade]

Am Mon, 7 Oct 2024 11:48:24 -0300
schrieb Jason Gunthorpe <jgg@nvidia.com>:

> On Mon, Oct 07, 2024 at 04:01:17PM +0200, Andreas Kemnade wrote:
> > > @@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct
> > > platform_device *pdev) err = iommu_device_register(&obj->iommu,
> > > &omap_iommu_ops, &pdev->dev); if (err)
> > >                         goto out_sysfs;
> > > +               /*
> > > +                * omap has a DT reprensetation but can't use the
> > > common DT
> > > +                * code. Setting fwnode to NULL causes probe to be
> > > called for
> > > +                * every device.
> > > +                */
> > > +               obj->iommu.fwnode = NULL;
> > >                 obj->has_iommu_driver = true;
> > >         }
> > >   
> > hmm, that looks nice for a regression fix.
> > 
> > Does it make sense to adopt dt so that the common code can be used
> > to ease future maintenance?  
> 
> It would be nice, but I recall omap doesn't use the standard dt
> layout?
> 
that is what is said in the comment. But what is missing? Is it one or
two properties at one place, or is it turing everything upside-down?

Regards,
Andreas

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[H. Nikolaus Schaller]

Hi Jason,

> Am 07.10.2024 um 14:15 schrieb Jason Gunthorpe <jgg@nvidia.com>:
> 
> On Sun, Oct 06, 2024 at 09:40:00AM +0200, H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> I found that the camera on our OMAP3 based system (GTA04) stopped working with v6.8-rc1.
>> There was no bug in the camera driver but the OMAP3 ISP (image signal processor) emits
>> 
>> [   14.963684] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping
>> [   15.010192] omap3isp 480bc000.isp: unable to attach to IOMMU
>> [   15.023376] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7)
>> [   15.065399] omap3isp: probe of 480bc000.isp failed with error -12
>> 
>> Deeper analyses lead to this patch breaking operation. It is not fixed up to v6.12-rc1.
>> 
>> What seems to happen (in 6.8-rc1 code):
>> 
>> - omap_iommu_probe() passes &omap_iommu_ops to iommu_device_register()
>> - iommu_device_register() stores the ops in iommu->ops (only)
>> - __iommu_probe_device tries to read the ops from some fw_spec but not iommu->ops
> 
> Maybe like this?
> 
> @@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct platform_device *pdev)
>               err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev);
>               if (err)
>                       goto out_sysfs;
> +               /*
> +                * omap has a DT reprensetation but can't use the common DT
> +                * code. Setting fwnode to NULL causes probe to be called for
> +                * every device.
> +                */
> +               obj->iommu.fwnode = NULL;
>               obj->has_iommu_driver = true;
>       }

Doesn't seem to solve the problems:

root@letux:~# uname -a
Linux letux 6.8.0-rc1-letux+ #19506 SMP PREEMPT Tue Oct  8 08:48:26 CEST 2024 armv7l GNU/Linux
root@letux:~# dmesg|fgrep iommu
[    0.402862] iommu: Default domain type: Translated
[    0.402893] iommu: DMA domain TLB invalidation policy: strict mode
[    0.405303] omap-iommu 480bd400.mmu: 480bd400.mmu registered
[    0.405944] platform 480bc000.isp: Adding to iommu group 0
[   24.829071] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT
[   24.880920] 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.405944] platform 480bc000.isp: Adding to iommu group 0
[   12.349334] omapdss_dss 48050000.dss: bound 48050400.dispc (ops dsi_vc_flush_receive_data [omapdrm])
[   24.821441] omap3isp 480bc000.isp: deferred probe timeout, ignoring dependency
[   24.829071] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT
[   24.887329] omap3isp 480bc000.isp: supply vdd-csiphy1 not found, using dummy regulator
[   24.928680] omap3isp 480bc000.isp: supply vdd-csiphy2 not found, using dummy regulator
[   24.951904] omap3isp 480bc000.isp: Revision 15.0 found
[   24.958160] omap3isp 480bc000.isp: failed to attach device to VA mapping
[   24.994232] omap3isp 480bc000.isp: unable to attach to IOMMU
[   25.013671] omap3isp: probe of 480bc000.isp failed with error -16
root@letux:~#

I'll dig deeper.

BR and thanks,
Nikolaus

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[Jason Gunthorpe]

On Tue, Oct 08, 2024 at 10:08:27AM +0200, H. Nikolaus Schaller wrote:
> Hi Jason,
> 
> > Am 07.10.2024 um 14:15 schrieb Jason Gunthorpe <jgg@nvidia.com>:
> > 
> > On Sun, Oct 06, 2024 at 09:40:00AM +0200, H. Nikolaus Schaller wrote:
> >> Hi,
> >> 
> >> I found that the camera on our OMAP3 based system (GTA04) stopped working with v6.8-rc1.
> >> There was no bug in the camera driver but the OMAP3 ISP (image signal processor) emits
> >> 
> >> [   14.963684] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping
> >> [   15.010192] omap3isp 480bc000.isp: unable to attach to IOMMU
> >> [   15.023376] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7)
> >> [   15.065399] omap3isp: probe of 480bc000.isp failed with error -12
> >> 
> >> Deeper analyses lead to this patch breaking operation. It is not fixed up to v6.12-rc1.
> >> 
> >> What seems to happen (in 6.8-rc1 code):
> >> 
> >> - omap_iommu_probe() passes &omap_iommu_ops to iommu_device_register()
> >> - iommu_device_register() stores the ops in iommu->ops (only)
> >> - __iommu_probe_device tries to read the ops from some fw_spec but not iommu->ops
> > 
> > Maybe like this?
> > 
> > @@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct platform_device *pdev)
> >               err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev);
> >               if (err)
> >                       goto out_sysfs;
> > +               /*
> > +                * omap has a DT reprensetation but can't use the common DT
> > +                * code. Setting fwnode to NULL causes probe to be called for
> > +                * every device.
> > +                */
> > +               obj->iommu.fwnode = NULL;
> >               obj->has_iommu_driver = true;
> >       }
> 
> Doesn't seem to solve the problems:
> 
> root@letux:~# uname -a
> Linux letux 6.8.0-rc1-letux+ #19506 SMP PREEMPT Tue Oct  8 08:48:26 CEST 2024 armv7l GNU/Linux
> root@letux:~# dmesg|fgrep iommu
> [    0.402862] iommu: Default domain type: Translated
> [    0.402893] iommu: DMA domain TLB invalidation policy: strict mode
> [    0.405303] omap-iommu 480bd400.mmu: 480bd400.mmu registered
> [    0.405944] platform 480bc000.isp: Adding to iommu group 0

This seems like the isp device was bound to the iommu if the group was
create and the device attached to it? Does that mean it got past this problem:

	ops = iommu_fwspec_ops(dev_iommu_fwspec_get(dev));
	if (!ops)
		return -ENODEV;
?

> [   24.829071] omap3isp 480bc000.isp: iommu configuration for device failed with -ETIMEDOUT

This is strange? It is not upstream? Seems worth investigating what
this is..

Jason

Re: BUG: "iommu: Retire bus ops" breaks omap-iommu and omap3isp

[Robin Murphy]

On 07/10/2024 1:15 pm, Jason Gunthorpe wrote:
> On Sun, Oct 06, 2024 at 09:40:00AM +0200, H. Nikolaus Schaller wrote:
>> Hi,
>>
>> I found that the camera on our OMAP3 based system (GTA04) stopped working with v6.8-rc1.
>> There was no bug in the camera driver but the OMAP3 ISP (image signal processor) emits
>>
>> [   14.963684] omap3isp 480bc000.isp: failed to create ARM IOMMU mapping
>> [   15.010192] omap3isp 480bc000.isp: unable to attach to IOMMU
>> [   15.023376] omap3isp 480bc000.isp: isp_xclk_set_rate: cam_xclka set to 24685714 Hz (div 7)
>> [   15.065399] omap3isp: probe of 480bc000.isp failed with error -12
>>
>> Deeper analyses lead to this patch breaking operation. It is not fixed up to v6.12-rc1.
>>
>> What seems to happen (in 6.8-rc1 code):
>>
>> - omap_iommu_probe() passes &omap_iommu_ops to iommu_device_register()
>> - iommu_device_register() stores the ops in iommu->ops (only)
>> - __iommu_probe_device tries to read the ops from some fw_spec but not iommu->ops
> 
> Maybe like this?
> 
> @@ -1233,6 +1233,12 @@ static int omap_iommu_probe(struct platform_device *pdev)
>                  err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev);

Nope, just s/&pdev->dev/NULL/ here. Apologies for overlooking this.

Thanks,
Robin.

>                  if (err)
>                          goto out_sysfs;
> +               /*
> +                * omap has a DT reprensetation but can't use the common DT
> +                * code. Setting fwnode to NULL causes probe to be called for
> +                * every device.
> +                */
> +               obj->iommu.fwnode = NULL;
>                  obj->has_iommu_driver = true;
>          }
> 
> Jason