| Message ID | 20190114132240.12125-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()" | expand |
On Mon, Jan 14, 2019 at 02:22:40PM +0100, Marek Szyprowski wrote: > This reverts commit 1874619a7df4b14b23b14877e705ae15325773e3. > > That patch broke IOMMU support for devices, which fails to probe for the > first time and use deferred probe approach. When non-NULL dma_ops is set > in arm_iommu_detach_device(), the given device later ignored by > arch_setup_dma_ops() and stays with non-IOMMU dma_ops. > > Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > Fixes: 1874619a7df4 "ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()" > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > arch/arm/mm/dma-mapping.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) Can you point out exactly what drivers break because of this change? We need to find a solution that works for everyone. Reverting is only marginally useful because somebody will just end up wanting to revert the revert because a different driver is now broken. Thierry
On 14/01/2019 16:09, Thierry Reding wrote: > On Mon, Jan 14, 2019 at 02:22:40PM +0100, Marek Szyprowski wrote: >> This reverts commit 1874619a7df4b14b23b14877e705ae15325773e3. >> >> That patch broke IOMMU support for devices, which fails to probe for the >> first time and use deferred probe approach. When non-NULL dma_ops is set >> in arm_iommu_detach_device(), the given device later ignored by >> arch_setup_dma_ops() and stays with non-IOMMU dma_ops. >> >> Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Fixes: 1874619a7df4 "ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()" >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> arch/arm/mm/dma-mapping.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) > > Can you point out exactly what drivers break because of this change? We > need to find a solution that works for everyone. Reverting is only > marginally useful because somebody will just end up wanting to revert > the revert because a different driver is now broken. At first glance, it sounds like what we really want is for arch_teardown_iommu_ops() to completely clear any ops that arch_setup_dma_ops() installed - does the below suffice? Robin. ----->8----- diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index f1e2922e447c..1e3e08a1c456 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -2390,4 +2390,6 @@ void arch_teardown_dma_ops(struct device *dev) return; arm_teardown_iommu_dma_ops(dev); + /* Let arch_setup_dma_ops() start again from scratch upon re-probe */ + set_dma_ops(dev, NULL); }
Hello everyone, first of all thanks to Marek for looking into this. @Robin: This works for me. The drivers now probe and bind correctly again. With best wishes, Tobias Robin Murphy wrote: > On 14/01/2019 16:09, Thierry Reding wrote: >> On Mon, Jan 14, 2019 at 02:22:40PM +0100, Marek Szyprowski wrote: >>> This reverts commit 1874619a7df4b14b23b14877e705ae15325773e3. >>> >>> That patch broke IOMMU support for devices, which fails to probe for the >>> first time and use deferred probe approach. When non-NULL dma_ops is set >>> in arm_iommu_detach_device(), the given device later ignored by >>> arch_setup_dma_ops() and stays with non-IOMMU dma_ops. >>> >>> Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>> Fixes: 1874619a7df4 "ARM: dma-mapping: Set proper DMA ops in >>> arm_iommu_detach_device()" >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> arch/arm/mm/dma-mapping.c | 12 ++++++------ >>> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> Can you point out exactly what drivers break because of this change? We >> need to find a solution that works for everyone. Reverting is only >> marginally useful because somebody will just end up wanting to revert >> the revert because a different driver is now broken. > > At first glance, it sounds like what we really want is for > arch_teardown_iommu_ops() to completely clear any ops that arch_setup_dma_ops() > installed - does the below suffice? > > Robin. > > ----->8----- > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index f1e2922e447c..1e3e08a1c456 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -2390,4 +2390,6 @@ void arch_teardown_dma_ops(struct device *dev) > return; > > arm_teardown_iommu_dma_ops(dev); > + /* Let arch_setup_dma_ops() start again from scratch upon re-probe */ > + set_dma_ops(dev, NULL); > }
On Mon, Jan 14, 2019 at 04:38:20PM +0000, Robin Murphy wrote: > On 14/01/2019 16:09, Thierry Reding wrote: > > On Mon, Jan 14, 2019 at 02:22:40PM +0100, Marek Szyprowski wrote: > > > This reverts commit 1874619a7df4b14b23b14877e705ae15325773e3. > > > > > > That patch broke IOMMU support for devices, which fails to probe for the > > > first time and use deferred probe approach. When non-NULL dma_ops is set > > > in arm_iommu_detach_device(), the given device later ignored by > > > arch_setup_dma_ops() and stays with non-IOMMU dma_ops. > > > > > > Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > > > Fixes: 1874619a7df4 "ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()" > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > --- > > > arch/arm/mm/dma-mapping.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > Can you point out exactly what drivers break because of this change? We > > need to find a solution that works for everyone. Reverting is only > > marginally useful because somebody will just end up wanting to revert > > the revert because a different driver is now broken. > > At first glance, it sounds like what we really want is for > arch_teardown_iommu_ops() to completely clear any ops that > arch_setup_dma_ops() installed - does the below suffice? > > Robin. > > ----->8----- > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index f1e2922e447c..1e3e08a1c456 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -2390,4 +2390,6 @@ void arch_teardown_dma_ops(struct device *dev) > return; > > arm_teardown_iommu_dma_ops(dev); > + /* Let arch_setup_dma_ops() start again from scratch upon re-probe */ > + set_dma_ops(dev, NULL); > } Seems to work fine on Tegra: Tested-by: Thierry Reding <treding@nvidia.com>
Hi Robin, On 2019-01-14 17:38, Robin Murphy wrote: > On 14/01/2019 16:09, Thierry Reding wrote: >> On Mon, Jan 14, 2019 at 02:22:40PM +0100, Marek Szyprowski wrote: >>> This reverts commit 1874619a7df4b14b23b14877e705ae15325773e3. >>> >>> That patch broke IOMMU support for devices, which fails to probe for >>> the >>> first time and use deferred probe approach. When non-NULL dma_ops is >>> set >>> in arm_iommu_detach_device(), the given device later ignored by >>> arch_setup_dma_ops() and stays with non-IOMMU dma_ops. >>> >>> Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>> Fixes: 1874619a7df4 "ARM: dma-mapping: Set proper DMA ops in >>> arm_iommu_detach_device()" >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> arch/arm/mm/dma-mapping.c | 12 ++++++------ >>> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> Can you point out exactly what drivers break because of this change? We >> need to find a solution that works for everyone. Reverting is only >> marginally useful because somebody will just end up wanting to revert >> the revert because a different driver is now broken. > > At first glance, it sounds like what we really want is for > arch_teardown_iommu_ops() to completely clear any ops that > arch_setup_dma_ops() installed - does the below suffice? I've initially also thought about similar fix, but then I found the commit 1874619a7df4b14b23b14877e705ae15325773e3, which was the source of this problem. Robin: do you plan to send this fix as a complete patch or do you want me to resend it with the above commit message and your's suggested-by? > > Robin. > > ----->8----- > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index f1e2922e447c..1e3e08a1c456 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -2390,4 +2390,6 @@ void arch_teardown_dma_ops(struct device *dev) > return; > > arm_teardown_iommu_dma_ops(dev); > + /* Let arch_setup_dma_ops() start again from scratch upon > re-probe */ > + set_dma_ops(dev, NULL); > } > > Best regards
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 3c8534904209..5827a89b7de1 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1145,11 +1145,6 @@ int arm_dma_supported(struct device *dev, u64 mask) return __dma_supported(dev, mask, false); } -static const struct dma_map_ops *arm_get_dma_map_ops(bool coherent) -{ - return coherent ? &arm_coherent_dma_ops : &arm_dma_ops; -} - #ifdef CONFIG_ARM_DMA_USE_IOMMU static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long attrs) @@ -2294,7 +2289,7 @@ void arm_iommu_detach_device(struct device *dev) iommu_detach_device(mapping->domain, dev); kref_put(&mapping->kref, release_iommu_mapping); to_dma_iommu_mapping(dev) = NULL; - set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent)); + set_dma_ops(dev, NULL); pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev)); } @@ -2355,6 +2350,11 @@ static void arm_teardown_iommu_dma_ops(struct device *dev) { } #endif /* CONFIG_ARM_DMA_USE_IOMMU */ +static const struct dma_map_ops *arm_get_dma_map_ops(bool coherent) +{ + return coherent ? &arm_coherent_dma_ops : &arm_dma_ops; +} + void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, const struct iommu_ops *iommu, bool coherent) {
This reverts commit 1874619a7df4b14b23b14877e705ae15325773e3. That patch broke IOMMU support for devices, which fails to probe for the first time and use deferred probe approach. When non-NULL dma_ops is set in arm_iommu_detach_device(), the given device later ignored by arch_setup_dma_ops() and stays with non-IOMMU dma_ops. Reported-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> Fixes: 1874619a7df4 "ARM: dma-mapping: Set proper DMA ops in arm_iommu_detach_device()" Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- arch/arm/mm/dma-mapping.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)