Re: [PATCH] iommu/exynos: add missing set_platform_dma_ops callback

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 13, 2023 at 12:03:55AM +0200, Marek Szyprowski wrote:
> On 07.04.2023 01:37, Jason Gunthorpe wrote:
> > 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:
> >>>> 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?
> 
> Maybe it would be easier to simply switch ARM32 to generic IOMMU-DMA layer?

It is the long term goal, yes

In the short term it would be nice if we could at least keep the hacks
out of the drivers

Jason