Message-ID: <c5fe662c-8726-43f9-8c68-f3e45208fc56@arm.com>
Date: Tue, 30 Sep 2025 11:28:09 +0100
From: Robin Murphy <robin.murphy@....com>
To: Hans Verkuil <hverkuil+cisco@...nel.org>, Jason Gunthorpe <jgg@...dia.com>
Cc: iommu@...ts.linux.dev, Linux Kernel <linux-kernel@...r.kernel.org>,
 Linux Media Mailing List <linux-media@...r.kernel.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>
Subject: Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL
 blocking_domain

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@...asonboard.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@...asonboard.com>
> Acked-by: Sakari Ailus <sakari.ailus@....fi>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@...sung.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@...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@....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

Powered by blists - more mailing lists