drivers/iommu/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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.2On 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
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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.