[PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Hans Verkuil]

Loading the omap3isp driver fails in __iommu_attach_group:
group->blocking_domain is NULL, and so the check
group->domain != group->blocking_domain is always true and it
returns -EBUSY.

Only return -EBUSY if group->blocking_domain is non-NULL.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Fixes: 0286300e6045 ("iommu: iommu_group_claim_dma_owner() must always assign a domain")
---
Since I am unfamiliar with the iommu core code, I am uncertain whether I am
just papering over a bug elsewhere, or whether this is really the correct solution.

The omap3isp code in question is here:

drivers/media/platform/ti/omap3isp/isp.c, function isp_attach_iommu().

This omap3isp code predates the addition of blocking_domain and it used to work
before that feature was added.

I've tested this patch with my Beagle XM board.

If this patch is addressing the issue in the wrong place, then advise
on what the correct solution is would be very much appreciated!

I have a bunch of media omap3isp cleanup patches pending, but there is no point in
posting those until this issue is resolved.

Regards,

	Hans
---
 drivers/iommu/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 060ebe330ee1..0ab1671ee850 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2220,7 +2220,7 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 	struct device *dev;

 	if (group->domain && group->domain != group->default_domain &&
-	    group->domain != group->blocking_domain)
+	    group->blocking_domain && group->domain != group->blocking_domain)
 		return -EBUSY;

 	dev = iommu_group_first_dev(group);
-- 
2.47.2

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Jason Gunthorpe]

On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:

> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
> just papering over a bug elsewhere, or whether this is really the correct solution.

It is papering over something, group->domain is not supposed to be
NULL at this point.. That probably means the iommu driver has not been
fully setup yet.

This is ARM32? It should have gone down this path:

static int iommu_get_default_domain_type(struct iommu_group *group,
					 int target_type)
{
	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
				IS_ENABLED(CONFIG_IOMMU_DMA)));
		driver_type = IOMMU_DOMAIN_IDENTITY;

And I thought there shouldn't be a way to get here:

> drivers/media/platform/ti/omap3isp/isp.c, function isp_attach_iommu().

With a NULL group->domain?

Maybe it didn't call iommu_setup_default_domain() in the right
sequence for some reason?

Can you inspect around this?

Jason

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Hans Verkuil]

On 29/09/2025 14:07, Jason Gunthorpe wrote:
> On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
> 
>> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
>> just papering over a bug elsewhere, or whether this is really the correct solution.
> 
> It is papering over something, group->domain is not supposed to be
> NULL at this point.. That probably means the iommu driver has not been

It's group->blocking_domain that's NULL, not group->domain.

Regards,

	Hans

> fully setup yet.
> 
> This is ARM32? It should have gone down this path:
> 
> static int iommu_get_default_domain_type(struct iommu_group *group,
> 					 int target_type)
> {
> 	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
> 		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
> 				IS_ENABLED(CONFIG_IOMMU_DMA)));
> 		driver_type = IOMMU_DOMAIN_IDENTITY;
> 
> And I thought there shouldn't be a way to get here:
> 
>> drivers/media/platform/ti/omap3isp/isp.c, function isp_attach_iommu().
> 
> With a NULL group->domain?
> 
> Maybe it didn't call iommu_setup_default_domain() in the right
> sequence for some reason?
> 
> Can you inspect around this?
> 
> Jason

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Jason Gunthorpe]

On Mon, Sep 29, 2025 at 02:18:50PM +0200, Hans Verkuil wrote:
> On 29/09/2025 14:07, Jason Gunthorpe wrote:
> > On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
> > 
> >> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
> >> just papering over a bug elsewhere, or whether this is really the correct solution.
> > 
> > It is papering over something, group->domain is not supposed to be
> > NULL at this point.. That probably means the iommu driver has not been
> 
> It's group->blocking_domain that's NULL, not group->domain.

Er, I thought you were hitting a false positive on this:

  group->domain != group->blocking_domain

ie NULL != NULL

But I suppose the whole expression is checking for group->domain
already.

All your patch does is entirely disable the safetly logic :\

What is isp_attach_iommu() trying to accomplish? It does
arm_iommu_detach_device() and then arm_iommu_attach_device() ?

Why?

Is this trying to force a non-identity translation for ISP?

Jason

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Hans Verkuil]

On 29/09/2025 15:02, Jason Gunthorpe wrote:
> On Mon, Sep 29, 2025 at 02:18:50PM +0200, Hans Verkuil wrote:
>> On 29/09/2025 14:07, Jason Gunthorpe wrote:
>>> On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
>>>
>>>> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
>>>> just papering over a bug elsewhere, or whether this is really the correct solution.
>>>
>>> It is papering over something, group->domain is not supposed to be
>>> NULL at this point.. That probably means the iommu driver has not been
>>
>> It's group->blocking_domain that's NULL, not group->domain.
> 
> Er, I thought you were hitting a false positive on this:
> 
>   group->domain != group->blocking_domain
> 
> ie NULL != NULL
> 
> But I suppose the whole expression is checking for group->domain
> already.
> 
> All your patch does is entirely disable the safetly logic :\
> 
> What is isp_attach_iommu() trying to accomplish? It does
> arm_iommu_detach_device() and then arm_iommu_attach_device() ?
> 
> Why?
> 
> Is this trying to force a non-identity translation for ISP?

I have absolutely no idea. The commit where this was added is this:

---------------------------------------------------------
commit 2a0a5472af5caa0d0df334abb9975dc496f045da
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Thu Jan 2 20:06:08 2014 -0300

[media] omap3isp: Use the ARM DMA IOMMU-aware operations

Attach an ARM DMA I/O virtual address space to the ISP device. This
switches to the IOMMU-aware ARM DMA backend, we can thus remove the
explicit calls to the OMAP IOMMU map and unmap functions.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---------------------------------------------------------


That's over 11 years ago. I've CC-ed Sakari in case he remembers something about
this.

Later this was simplified a bit:

---------------------------------------------------------
commit fd8e2d4b393252505783656471465c7f85f3c0a9
Author: Suman Anna <s-anna@ti.com>
Date:   Wed Apr 12 00:21:32 2017 -0500

omap3isp: Remove iommu_group related code

The OMAP IOMMU driver has added the support for IOMMU groups internally,
and the ISP device is automatically linked to the appropriate IOMMU group.
So, remove the explicit function calls that creates/deletes an iommu_group
and adds the ISP device to this group.
---------------------------------------------------------


And finally the code detaching the device (which you referred to in your
reply) was added here (much more recently):

---------------------------------------------------------
commit 6bc076eec6f85f778f33a8242b438e1bd9fcdd59
Author: Robin Murphy <robin.murphy@arm.com>
Date:   Mon Oct 28 17:58:36 2024 +0000

media: omap3isp: Handle ARM dma_iommu_mapping

It's no longer practical for the OMAP IOMMU driver to trick
arm_setup_iommu_dma_ops() into ignoring its presence, so let's use the
same tactic as other IOMMU API users on 32-bit ARM and explicitly kick
the arch code's dma_iommu_mapping out of the way to avoid problems.
---------------------------------------------------------


All I know is that something is wrong after blocking_domain was added, which
now causes the "failed to create ARM IOMMU mapping" error from isp.c when the
omap3isp driver is probed.

My (very likely flawed) reasoning for this patch was that if there is no
blocking_domain (i.e. it's a NULL pointer), then the group->domain should
just be accepted. But that reasoning was just based on the field names, and with
no actual understanding of what is going on here :-)

Regards,

	Hans

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Jason Gunthorpe]

On Mon, Sep 29, 2025 at 03:30:22PM +0200, Hans Verkuil wrote:
> On 29/09/2025 15:02, Jason Gunthorpe wrote:
> > On Mon, Sep 29, 2025 at 02:18:50PM +0200, Hans Verkuil wrote:
> >> On 29/09/2025 14:07, Jason Gunthorpe wrote:
> >>> On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
> >>>
> >>>> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
> >>>> just papering over a bug elsewhere, or whether this is really the correct solution.
> >>>
> >>> It is papering over something, group->domain is not supposed to be
> >>> NULL at this point.. That probably means the iommu driver has not been
> >>
> >> It's group->blocking_domain that's NULL, not group->domain.
> > 
> > Er, I thought you were hitting a false positive on this:
> > 
> >   group->domain != group->blocking_domain
> > 
> > ie NULL != NULL
> > 
> > But I suppose the whole expression is checking for group->domain
> > already.
> > 
> > All your patch does is entirely disable the safetly logic :\
> > 
> > What is isp_attach_iommu() trying to accomplish? It does
> > arm_iommu_detach_device() and then arm_iommu_attach_device() ?
> > 
> > Why?
> > 
> > Is this trying to force a non-identity translation for ISP?
> 
> I have absolutely no idea. The commit where this was added is this:

Maybe try deleting everything in the CONFIG_ARM_DMA_USE_IOMMU branches
and just succeed in isp_attach_iommu() ?

It is almost the same code flow as in arm_setup_iommu_dma_ops(),
except it ignores the DT.

arm_setup_iommu_dma_ops() should be called by ISP via arch_setup_dma_ops():

	if (device_iommu_mapped(dev))
		arm_setup_iommu_dma_ops(dev);

?

I'm thinking this is all dead code now. The original version was
creating iommu_groups, so most likely 11 years ago
device_iommu_mapped() == false during driver probe and it had to open
code the whole sequence.

But today the groups are clearly there.. So the iommu should be
setup..

The main direct difference was IPS hard wiring the 1G:

	mapping = arm_iommu_create_mapping(isp->dev, SZ_1G, SZ_2G);

In the generic code this should come from the DT:

	if (dev->dma_range_map) {
		dma_base = dma_range_map_min(dev->dma_range_map);
		size = dma_range_map_max(dev->dma_range_map) - dma_base;
	}
	mapping = arm_iommu_create_mapping(dev, dma_base, size);

So potentially a DT change is also needed?

Can you check around these sequences to see what is happening?

Jason

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Robin Murphy]

On 2025-09-29 2:30 pm, Hans Verkuil wrote:
> On 29/09/2025 15:02, Jason Gunthorpe wrote:
>> On Mon, Sep 29, 2025 at 02:18:50PM +0200, Hans Verkuil wrote:
>>> On 29/09/2025 14:07, Jason Gunthorpe wrote:
>>>> On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
>>>>
>>>>> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
>>>>> just papering over a bug elsewhere, or whether this is really the correct solution.
>>>>
>>>> It is papering over something, group->domain is not supposed to be
>>>> NULL at this point.. That probably means the iommu driver has not been
>>>
>>> It's group->blocking_domain that's NULL, not group->domain.
>>
>> Er, I thought you were hitting a false positive on this:
>>
>>    group->domain != group->blocking_domain
>>
>> ie NULL != NULL
>>
>> But I suppose the whole expression is checking for group->domain
>> already.
>>
>> All your patch does is entirely disable the safetly logic :\

Yeah, the real question is if group->domain is set to something non-NULL 
that isn't the default identity domain or the blocking domain, and 
shouldn't be the automatic ARM DMA domain because we've detached and got 
rid of that, then what the heck is it?

My best guess is that something went wrong with the prior detach back to 
(what should be) the identity domain, so the previous ARM domain is 
still in place and the failure to attach to a different user domain is 
technically legitimate - i.e. something's broken the intended logic of 
6bc076eec6f8 (which was apparently working at the time).

Thanks,
Robin.

>>
>> What is isp_attach_iommu() trying to accomplish? It does
>> arm_iommu_detach_device() and then arm_iommu_attach_device() ?
>>
>> Why?
>>
>> Is this trying to force a non-identity translation for ISP?
> 
> I have absolutely no idea. The commit where this was added is this:
> 
> ---------------------------------------------------------
> commit 2a0a5472af5caa0d0df334abb9975dc496f045da
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Thu Jan 2 20:06:08 2014 -0300
> 
> [media] omap3isp: Use the ARM DMA IOMMU-aware operations
> 
> Attach an ARM DMA I/O virtual address space to the ISP device. This
> switches to the IOMMU-aware ARM DMA backend, we can thus remove the
> explicit calls to the OMAP IOMMU map and unmap functions.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> ---------------------------------------------------------
> 
> 
> That's over 11 years ago. I've CC-ed Sakari in case he remembers something about
> this.
> 
> Later this was simplified a bit:
> 
> ---------------------------------------------------------
> commit fd8e2d4b393252505783656471465c7f85f3c0a9
> Author: Suman Anna <s-anna@ti.com>
> Date:   Wed Apr 12 00:21:32 2017 -0500
> 
> omap3isp: Remove iommu_group related code
> 
> The OMAP IOMMU driver has added the support for IOMMU groups internally,
> and the ISP device is automatically linked to the appropriate IOMMU group.
> So, remove the explicit function calls that creates/deletes an iommu_group
> and adds the ISP device to this group.
> ---------------------------------------------------------
> 
> 
> And finally the code detaching the device (which you referred to in your
> reply) was added here (much more recently):
> 
> ---------------------------------------------------------
> commit 6bc076eec6f85f778f33a8242b438e1bd9fcdd59
> Author: Robin Murphy <robin.murphy@arm.com>
> Date:   Mon Oct 28 17:58:36 2024 +0000
> 
> media: omap3isp: Handle ARM dma_iommu_mapping
> 
> It's no longer practical for the OMAP IOMMU driver to trick
> arm_setup_iommu_dma_ops() into ignoring its presence, so let's use the
> same tactic as other IOMMU API users on 32-bit ARM and explicitly kick
> the arch code's dma_iommu_mapping out of the way to avoid problems.
> ---------------------------------------------------------
> 
> 
> All I know is that something is wrong after blocking_domain was added, which
> now causes the "failed to create ARM IOMMU mapping" error from isp.c when the
> omap3isp driver is probed.
> 
> My (very likely flawed) reasoning for this patch was that if there is no
> blocking_domain (i.e. it's a NULL pointer), then the group->domain should
> just be accepted. But that reasoning was just based on the field names, and with
> no actual understanding of what is going on here :-)
> 
> Regards,
> 
> 	Hans

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Sakari Ailus]

Hi Hans, Jason,

On Mon, Sep 29, 2025 at 03:30:22PM +0200, Hans Verkuil wrote:
> On 29/09/2025 15:02, Jason Gunthorpe wrote:
> > On Mon, Sep 29, 2025 at 02:18:50PM +0200, Hans Verkuil wrote:
> >> On 29/09/2025 14:07, Jason Gunthorpe wrote:
> >>> On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
> >>>
> >>>> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
> >>>> just papering over a bug elsewhere, or whether this is really the correct solution.
> >>>
> >>> It is papering over something, group->domain is not supposed to be
> >>> NULL at this point.. That probably means the iommu driver has not been
> >>
> >> It's group->blocking_domain that's NULL, not group->domain.
> > 
> > Er, I thought you were hitting a false positive on this:
> > 
> >   group->domain != group->blocking_domain
> > 
> > ie NULL != NULL
> > 
> > But I suppose the whole expression is checking for group->domain
> > already.
> > 
> > All your patch does is entirely disable the safetly logic :\
> > 
> > What is isp_attach_iommu() trying to accomplish? It does
> > arm_iommu_detach_device() and then arm_iommu_attach_device() ?
> > 
> > Why?
> > 
> > Is this trying to force a non-identity translation for ISP?

The omap3isp driver expects to use its own virtual address space for the
ISP: the video buffers are mapped there as virtually contiguous (physically
they can be whatever).

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Jason Gunthorpe]

On Wed, Oct 01, 2025 at 09:03:37AM +0300, Sakari Ailus wrote:
> Hi Hans, Jason,
> 
> On Mon, Sep 29, 2025 at 03:30:22PM +0200, Hans Verkuil wrote:
> > On 29/09/2025 15:02, Jason Gunthorpe wrote:
> > > On Mon, Sep 29, 2025 at 02:18:50PM +0200, Hans Verkuil wrote:
> > >> On 29/09/2025 14:07, Jason Gunthorpe wrote:
> > >>> On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
> > >>>
> > >>>> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
> > >>>> just papering over a bug elsewhere, or whether this is really the correct solution.
> > >>>
> > >>> It is papering over something, group->domain is not supposed to be
> > >>> NULL at this point.. That probably means the iommu driver has not been
> > >>
> > >> It's group->blocking_domain that's NULL, not group->domain.
> > > 
> > > Er, I thought you were hitting a false positive on this:
> > > 
> > >   group->domain != group->blocking_domain
> > > 
> > > ie NULL != NULL
> > > 
> > > But I suppose the whole expression is checking for group->domain
> > > already.
> > > 
> > > All your patch does is entirely disable the safetly logic :\
> > > 
> > > What is isp_attach_iommu() trying to accomplish? It does
> > > arm_iommu_detach_device() and then arm_iommu_attach_device() ?
> > > 
> > > Why?
> > > 
> > > Is this trying to force a non-identity translation for ISP?
> 
> The omap3isp driver expects to use its own virtual address space for the
> ISP: the video buffers are mapped there as virtually contiguous (physically
> they can be whatever).

Sure, but where does it do the mapping? I didn't see an iommu_map or
any other iommu_* call in this driver.

I think it is using dma_map_* to do it - probably dma_map_sg though I
could not find it?

This is why I gave my remarks, if it is relying on the DMA API for
mapping then it should just rely on the DMA API to establish a paging
domain and not try to open code something like that.

Jason

Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking_domain

[Hans Verkuil]

Hi all,

On 29/09/2025 10:23, Hans Verkuil wrote:
> Loading the omap3isp driver fails in __iommu_attach_group:
> group->blocking_domain is NULL, and so the check
> group->domain != group->blocking_domain is always true and it
> returns -EBUSY.
> 
> Only return -EBUSY if group->blocking_domain is non-NULL.
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> Fixes: 0286300e6045 ("iommu: iommu_group_claim_dma_owner() must always assign a domain")
> ---

So just ignore this patch :-)

Today I dropped this patch from my branch, and retested on my Beagle xM and it all
worked fine.

I suspect that it might be related to the fact that I started testing with the Beagle
board (so not the xM variant), and I hit the issue there. But the Beagle board doesn't
have the connector for the camera, so later I switched to the xM variant. But I probably
never tried running it on the xM without that iommu patch until today.

In two weeks time I have access to my Beagle board again and I'll experiment a bit
to see if my theory is correct.

Apologies for all the noise.

Regards,

	Hans

> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
> just papering over a bug elsewhere, or whether this is really the correct solution.
> 
> The omap3isp code in question is here:
> 
> drivers/media/platform/ti/omap3isp/isp.c, function isp_attach_iommu().
> 
> This omap3isp code predates the addition of blocking_domain and it used to work
> before that feature was added.
> 
> I've tested this patch with my Beagle XM board.
> 
> If this patch is addressing the issue in the wrong place, then advise
> on what the correct solution is would be very much appreciated!
> 
> I have a bunch of media omap3isp cleanup patches pending, but there is no point in
> posting those until this issue is resolved.
> 
> Regards,
> 
> 	Hans
> ---
>  drivers/iommu/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 060ebe330ee1..0ab1671ee850 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2220,7 +2220,7 @@ static int __iommu_attach_group(struct iommu_domain *domain,
>  	struct device *dev;
> 
>  	if (group->domain && group->domain != group->default_domain &&
> -	    group->domain != group->blocking_domain)
> +	    group->blocking_domain && group->domain != group->blocking_domain)
>  		return -EBUSY;
> 
>  	dev = iommu_group_first_dev(group);