search for: arm_iommu_detach_device

...ding at nvidia.com> > > Instead of setting the DMA ops pointer to NULL, set the correct, > non-IOMMU ops depending on the device's coherency setting. > > Signed-off-by: Thierry Reding <treding at nvidia.com> > --- > Changes in v4: > - new patch to fix existing arm_iommu_detach_device() to do what we need > > arch/arm/mm/dma-mapping.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) Christoph, Russell, could either of you provide an Acked-by for this? I think it makes the most sense for Ben to pick this up into the Nouveau tree along with patch 2...

From: Thierry Reding <treding at nvidia.com> Instead of setting the DMA ops pointer to NULL, set the correct, non-IOMMU ops depending on the device's coherency setting. Signed-off-by: Thierry Reding <treding at nvidia.com> --- Changes in v4: - new patch to fix existing arm_iommu_detach_device() to do what we need arch/arm/mm/dma-mapping.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index af27f1c22d93..87a0037574e4 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1151,6...

...+{ > +#ifdef CONFIG_ARM_DMA_USE_IOMMU > + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); > + > + if (!mapping) > + return; > + > + arm_iommu_release_mapping(mapping); Potentially freeing the mapping before you try to operate on it is never the best idea. Plus arm_iommu_detach_device() already releases a reference appropriately anyway, so it's a double-free. > + arm_iommu_detach_device(dev); > + > + set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent)); > +#endif > +} > +EXPORT_SYMBOL(arm_dma_iommu_detach_device); I really don't see w...

...ng = to_dma_iommu_mapping(dev); > > > + > > > + if (!mapping) > > > + return; > > > + > > > + arm_iommu_release_mapping(mapping); > > > > Potentially freeing the mapping before you try to operate on it is never the > > best idea. Plus arm_iommu_detach_device() already releases a reference > > appropriately anyway, so it's a double-free. > > But the reference released by arm_iommu_detach_device() is to balance > out the reference acquired by arm_iommu_attach_device(), isn't it? In > the above, the arm_iommu_release_mapping() i...

...; > + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); > > + > > + if (!mapping) > > + return; > > + > > + arm_iommu_release_mapping(mapping); > > Potentially freeing the mapping before you try to operate on it is never the > best idea. Plus arm_iommu_detach_device() already releases a reference > appropriately anyway, so it's a double-free. But the reference released by arm_iommu_detach_device() is to balance out the reference acquired by arm_iommu_attach_device(), isn't it? In the above, the arm_iommu_release_mapping() is supposed to drop the fi...

...at allows a driver to detach the device from the DMA/IOMMU mapping so that it can provide its own implementation for dealing with the SMMU. The second patch makes use of that new API in the Nouveau driver to fix the regression. Thierry Thierry Reding (2): ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device() drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping arch/arm/mm/dma-mapping.c | 12 ++++++------ drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 13 +++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) -- 2.17.0

...mapping(dev); >>>> + >>>> + if (!mapping) >>>> + return; >>>> + >>>> + arm_iommu_release_mapping(mapping); >>> >>> Potentially freeing the mapping before you try to operate on it is never the >>> best idea. Plus arm_iommu_detach_device() already releases a reference >>> appropriately anyway, so it's a double-free. >> >> But the reference released by arm_iommu_detach_device() is to balance >> out the reference acquired by arm_iommu_attach_device(), isn't it? In >> the above, the arm_iommu_r...

...0,20 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret; +#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) + if (dev->archdata.mapping) { + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + + arm_iommu_release_mapping(mapping); + arm_iommu_detach_device(dev); + + if (dev->archdata.dma_coherent) + set_dma_ops(dev, &arm_coherent_dma_ops); + else + set_dma_ops(dev, &arm_dma_ops); + } +#endif + if (!tdev->func->iommu_bit) return; -- 2.17.0

...@@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) > unsigned long pgsize_bitmap; > int ret; > > -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) > - if (dev->archdata.mapping) { > - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); > - > - arm_iommu_detach_device(dev); > - arm_iommu_release_mapping(mapping); > - } > -#endif > + /* > + * Skip explicit IOMMU initialization if the GPU is already attached > + * to an IOMMU domain. This can happen if the DMA API is backed by an > + * IOMMU. > + */ > + if (iommu_get_domain_for_dev(...

..._iommu_detach_device(struct device *dev) > +{ > +#ifdef CONFIG_ARM_DMA_USE_IOMMU > + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); > + const struct dma_map_ops *dma_ops; > + > + if (!mapping) > + return; > + > + arm_iommu_release_mapping(mapping); > + arm_iommu_detach_device(dev); > + > + dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent); > + set_dma_ops(dev, dma_ops); Why not simply: set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent));

From: Thierry Reding <treding at nvidia.com> An unfortunate interaction between the 32-bit ARM DMA/IOMMU mapping code and Tegra SMMU driver changes to support IOMMU groups introduced a boot- time regression on Tegra124. This was caught very late because none of the standard configurations that are tested on Tegra enable the ARM DMA/ IOMMU mapping code since it is not needed. The reason for

...0,20 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret; +#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) + if (dev->archdata.mapping) { + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + + arm_iommu_release_mapping(mapping); + arm_iommu_detach_device(dev); + + if (dev->archdata.dma_coherent) + set_dma_ops(dev, &arm_coherent_dma_ops); + else + set_dma_ops(dev, &arm_dma_ops); + } +#endif + if (!tdev->func->iommu_bit) return; -- 2.17.0

...ce_tegra *tdev) >>> unsigned long pgsize_bitmap; >>> int ret; >>> -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) >>> - if (dev->archdata.mapping) { >>> - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); >>> - >>> - arm_iommu_detach_device(dev); >>> - arm_iommu_release_mapping(mapping); >>> - } >>> -#endif >>> + /* >>> + * Skip explicit IOMMU initialization if the GPU is already attached >>> + * to an IOMMU domain. This can happen if the DMA API is backed by an >>> +...

...mapping). > > That's pretty much what I initially did and which was shot down by > Christoph. As I said earlier, at this point I don't really care what > color the shed will be. Can you and Christoph come to an agreement > on what it should be? What I was getting at is that arm_iommu_detach_device() already *is* the exact function Christoph was asking for, it just needs a minor fix instead of adding explicit set_dma_ops() fiddling at its callsites which only obfuscates the fact that it's supposed to be responsible for resetting the device's DMA ops already. Robin.

...0,20 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret; +#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) + if (dev->archdata.mapping) { + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + + arm_iommu_release_mapping(mapping); + arm_iommu_detach_device(dev); + + if (dev->archdata.dma_coherent) + set_dma_ops(dev, &arm_coherent_dma_ops); + else + set_dma_ops(dev, &arm_dma_ops); + } +#endif + if (!tdev->func->iommu_bit) return; -- 2.17.0

...u_dma_ops(struct device *dev) arm_iommu_release_mapping(mapping); } +void arch_iommu_detach_device(struct device *dev) +{ + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + const struct dma_map_ops *dma_ops; + + if (!mapping) + return; + + arm_iommu_release_mapping(mapping); + arm_iommu_detach_device(dev); + + dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent); + set_dma_ops(dev, dma_ops); +} + #else static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, @@ -2378,6 +2393,10 @@ static void arm_teardown_iommu_dma_ops(struct device *dev) { } #define arm_ge...

...+{ > > +#ifdef CONFIG_ARM_DMA_USE_IOMMU > > + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); > > + const struct dma_map_ops *dma_ops; > > + > > + if (!mapping) > > + return; > > + > > + arm_iommu_release_mapping(mapping); > > + arm_iommu_detach_device(dev); > > + > > + dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent); > > + set_dma_ops(dev, dma_ops); > > Why not simply: > > set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent)); I had that initially, but it looked cluttered to me, so I...

...arm_teardown_iommu_dma_ops(dev); } + +void arch_iommu_detach_device(struct device *dev) +{ +#ifdef CONFIG_ARM_DMA_USE_IOMMU + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + const struct dma_map_ops *dma_ops; + + if (!mapping) + return; + + arm_iommu_release_mapping(mapping); + arm_iommu_detach_device(dev); + + dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent); + set_dma_ops(dev, dma_ops); +#endif +} -- 2.17.0

...ce if you've explicitly attached the device to your own IOMMU domain then you're already bypassing everything it knows about and has control over. Arch code calling into iommu-dma to do something which makes arch code not use iommu-dma makes very little sense. AFAICS the only aspect of arm_iommu_detach_device() which actually matters in this case is the set_dma_ops() bit, so what we're really after is a generic way for drivers to say "Hey, I actually have my own MMU (or want to control the one you already know about) so please give me direct DMA ops", which the arch code handles as app...

...own_dma_ops(struct device *dev) arm_teardown_iommu_dma_ops(dev); } + +void arm_dma_iommu_detach_device(struct device *dev) +{ +#ifdef CONFIG_ARM_DMA_USE_IOMMU + struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + + if (!mapping) + return; + + arm_iommu_release_mapping(mapping); + arm_iommu_detach_device(dev); + + set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent)); +#endif +} +EXPORT_SYMBOL(arm_dma_iommu_detach_device); -- 2.17.0