On Mon, Feb 20, 2023 at 01:58:40PM +0000, Robin Murphy wrote: > On 2023-02-17 12:35, Jason Gunthorpe wrote: > > On Fri, Feb 17, 2023 at 12:08:42PM +0100, Marek Szyprowski wrote: > > > Hi, > > > > > > I'm sorry for a delay in replying, but I was busy with other stuff. > > > > > > On 23.01.2023 22:00, Jason Gunthorpe wrote: > > > > On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote: > > > > > Add set_platform_dma_ops() required for proper driver operation on ARM > > > > > 32bit arch after recent changes in the IOMMU framework (detach ops > > > > > removal). > > > > Thanks for looking into this! > > > > > > > > Can you explain more about how this actually solves the problem in the > > > > commit message? I don't get it. > > > > > > Exynos DRM driver calls arm_iommu_detach_device(), then > > > arm_iommu_attach_device() with a difrent 'mapping', see > > > drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops > > > leads to a warning in iommu_group_do_set_platform_dma(). The other case > > > of calling arm_iommu_detach_device() is after unsuccessful probe of the > > > platform device. > > > > Why can't this just use the normal iommu path in all cases? > > > > It looks like it is trying to copy the DMA API domain from a parent > > device to a sub device. > > > > Even when using arm_iommu an iommu_domain is still present, so the > > copy code should work? > > The ARM DMA domain is a regular unmanaged domain owned by the ARM DMA code - > in order to use any *other* domain, a user has to detach from that first > (wrapped up in arm_iommu_detach_device() which also swizzles the DMA ops at > the same time). Without set_platform_dma, that detach is now impossible > (because no IOMMU API default domain exists either). I was looking at this again, and for completeness, the reason it doesn't have a default_domain is a bit subtle. The driver does support IOMMU_DOMAIN_DMA, and it will go through all the iommu_group_alloc_default_domain() stuff with that.. But... __iommu_domain_alloc() calls iommu_get_dma_cookie() which will be wired to fail on ARM32, so the core code nixes the IOMMU_DOMAIN_DMA after the driver successfully created it. I wonder if that is actually what we want? As it seems like it would be OK for ARM32 to have, effectively, a permanently empty unmanaged domain as the default_domain? If instead of failing due to iommu_get_dma_cookie() we set the type to UNMANAGED and make that the default domain it could fix all of this, even for all the other ARM32 drivers that haven't reported this yet? Alternatively since we taught these drivers to support IDENTITY, we should be able to remove the set_platform_dma_ops and instead implement for ARM32 def_domain_type fixed to return IDENTITY? Jason
