| Message ID | 1422022909-31044-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote: > +/** > + * arm_iommu_detach_device > + * @dev: valid struct device pointer > + * > + * Detaches the provided device from a previously attached map. > + * This voids the dma operations (dma_map_ops pointer) > + */ > +void arm_iommu_detach_device(struct device *dev) > +{ > + __arm_iommu_detach_device(dev); > + set_dma_ops(dev, NULL); > +} > EXPORT_SYMBOL_GPL(arm_iommu_detach_device); > > Would this introduce a regression in the case where the device is cache coherent and needs arm_coherent_dma_ops set? Arnd
Hi Arnd, On Friday 23 January 2015 16:27:24 Arnd Bergmann wrote: > On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote: > > +/** > > + * arm_iommu_detach_device > > + * @dev: valid struct device pointer > > + * > > + * Detaches the provided device from a previously attached map. > > + * This voids the dma operations (dma_map_ops pointer) > > + */ > > +void arm_iommu_detach_device(struct device *dev) > > +{ > > + __arm_iommu_detach_device(dev); > > + set_dma_ops(dev, NULL); > > +} > > > > EXPORT_SYMBOL_GPL(arm_iommu_detach_device); > > Would this introduce a regression in the case where the device is > cache coherent and needs arm_coherent_dma_ops set? I think we need to handle that case (as well as the coherent IOMMU case in arm_iommu_attach_device), but this patch only tries to restore the previous behaviour to fix the bug detailed in the commit message. Would you prefer fixing both issues in one go ?
On Friday 23 January 2015 17:55:25 Laurent Pinchart wrote: > > On Friday 23 January 2015 16:27:24 Arnd Bergmann wrote: > > On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote: > > > +/** > > > + * arm_iommu_detach_device > > > + * @dev: valid struct device pointer > > > + * > > > + * Detaches the provided device from a previously attached map. > > > + * This voids the dma operations (dma_map_ops pointer) > > > + */ > > > +void arm_iommu_detach_device(struct device *dev) > > > +{ > > > + __arm_iommu_detach_device(dev); > > > + set_dma_ops(dev, NULL); > > > +} > > > > > > EXPORT_SYMBOL_GPL(arm_iommu_detach_device); > > > > Would this introduce a regression in the case where the device is > > cache coherent and needs arm_coherent_dma_ops set? > > I think we need to handle that case (as well as the coherent IOMMU case in > arm_iommu_attach_device), but this patch only tries to restore the previous > behaviour to fix the bug detailed in the commit message. Would you prefer > fixing both issues in one go ? > No, I was specifically trying to find out whether the new behavior would be worse than what we had in 3.18. If it's a preexisting bug that nobody has run into, there is no hurry. It's quite likely that to date, all IOMMU users on ARM32 are not cache coherent. Arnd
On Fri, Jan 23, 2015 at 05:00:45PM +0000, Arnd Bergmann wrote: > On Friday 23 January 2015 17:55:25 Laurent Pinchart wrote: > > > > On Friday 23 January 2015 16:27:24 Arnd Bergmann wrote: > > > On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote: > > > > +/** > > > > + * arm_iommu_detach_device > > > > + * @dev: valid struct device pointer > > > > + * > > > > + * Detaches the provided device from a previously attached map. > > > > + * This voids the dma operations (dma_map_ops pointer) > > > > + */ > > > > +void arm_iommu_detach_device(struct device *dev) > > > > +{ > > > > + __arm_iommu_detach_device(dev); > > > > + set_dma_ops(dev, NULL); > > > > +} > > > > > > > > EXPORT_SYMBOL_GPL(arm_iommu_detach_device); > > > > > > Would this introduce a regression in the case where the device is > > > cache coherent and needs arm_coherent_dma_ops set? > > > > I think we need to handle that case (as well as the coherent IOMMU case in > > arm_iommu_attach_device), but this patch only tries to restore the previous > > behaviour to fix the bug detailed in the commit message. Would you prefer > > fixing both issues in one go ? > > > > No, I was specifically trying to find out whether the new behavior would > be worse than what we had in 3.18. If it's a preexisting bug that nobody > has run into, there is no hurry. Agreed, I think Laurent's proposal here is the smallest thing that fixes the issue without breaking the new of_xlate-based configuration. Acked-by: Will Deacon <will.deacon@arm.com> > It's quite likely that to date, all IOMMU users on ARM32 are not > cache coherent. Highbank has an ARM SMMU, but I don't see the iommu_coherent_ops getting used, so your assertion is probably right. Will
Am Freitag, 23. Januar 2015, 16:21:49 schrieb Laurent Pinchart: > Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops > into arch_setup_dma_ops") moved the setting of the DMA operations from > arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA > operations to be used are selected based on whether the device is > connected to an IOMMU. However, the IOMMU detection scheme requires the > IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver > has been ported yet, this effectively breaks all IOMMU ARM users that > depend on the IOMMU being handled transparently by the DMA mapping API. > > Fix this by restoring the setting of DMA IOMMU ops in > arm_iommu_attach_device() and splitting the rest of the function into a > new internal __arm_iommu_attach_device() function, called by > arch_setup_dma_ops(). > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> on a rk3288 board with the currently failing drm driver this patch brought it back to display something :-) , so Tested-by: Heiko Stuebner <heiko@sntech.de> Thanks Heiko
On Fri, Jan 23, 2015 at 04:21:49PM +0200, Laurent Pinchart wrote: > Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops > into arch_setup_dma_ops") moved the setting of the DMA operations from > arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA > operations to be used are selected based on whether the device is > connected to an IOMMU. However, the IOMMU detection scheme requires the > IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver > has been ported yet, this effectively breaks all IOMMU ARM users that > depend on the IOMMU being handled transparently by the DMA mapping API. > > Fix this by restoring the setting of DMA IOMMU ops in > arm_iommu_attach_device() and splitting the rest of the function into a > new internal __arm_iommu_attach_device() function, called by > arch_setup_dma_ops(). > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > arch/arm/mm/dma-mapping.c | 53 +++++++++++++++++++++++++++++++++-------------- > 1 file changed, 38 insertions(+), 15 deletions(-) I guess this patch will be merged by one of the ARM trees? Joerg
Hi Arnd, Olof, Am Freitag, 23. Januar 2015, 16:21:49 schrieb Laurent Pinchart: > Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops > into arch_setup_dma_ops") moved the setting of the DMA operations from > arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA > operations to be used are selected based on whether the device is > connected to an IOMMU. However, the IOMMU detection scheme requires the > IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver > has been ported yet, this effectively breaks all IOMMU ARM users that > depend on the IOMMU being handled transparently by the DMA mapping API. > > Fix this by restoring the setting of DMA IOMMU ops in > arm_iommu_attach_device() and splitting the rest of the function into a > new internal __arm_iommu_attach_device() function, called by > arch_setup_dma_ops(). > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> in the original submission arm@kernel.org was not included, but it looks like the patch should go through arm-soc. We have two tags Acked-by: Will Deacon <will.deacon@arm.com> Tested-by: Heiko Stuebner <heiko@sntech.de> Can you find the original patch somehow or should it be resend to include arm@kernel.org ? Thanks Heiko
On Wed, Jan 28, 2015 at 01:25:35AM +0100, Heiko Stübner wrote: > Hi Arnd, Olof, > > Am Freitag, 23. Januar 2015, 16:21:49 schrieb Laurent Pinchart: > > Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops > > into arch_setup_dma_ops") moved the setting of the DMA operations from > > arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA > > operations to be used are selected based on whether the device is > > connected to an IOMMU. However, the IOMMU detection scheme requires the > > IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver > > has been ported yet, this effectively breaks all IOMMU ARM users that > > depend on the IOMMU being handled transparently by the DMA mapping API. > > > > Fix this by restoring the setting of DMA IOMMU ops in > > arm_iommu_attach_device() and splitting the rest of the function into a > > new internal __arm_iommu_attach_device() function, called by > > arch_setup_dma_ops(). > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > in the original submission arm@kernel.org was not included, but it looks like > the patch should go through arm-soc. > > We have two tags > Acked-by: Will Deacon <will.deacon@arm.com> > Tested-by: Heiko Stuebner <heiko@sntech.de> > > Can you find the original patch somehow or should it be resend to include > arm@kernel.org ? The patch was posted on a list that I am subscribed to, so it popped up in the same thread this time. I've applied it to fixes for 3.19. Thanks, -Olof
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 7864797609b3..a673c7f7e208 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1940,13 +1940,32 @@ void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping) } EXPORT_SYMBOL_GPL(arm_iommu_release_mapping); +static int __arm_iommu_attach_device(struct device *dev, + struct dma_iommu_mapping *mapping) +{ + int err; + + err = iommu_attach_device(mapping->domain, dev); + if (err) + return err; + + kref_get(&mapping->kref); + dev->archdata.mapping = mapping; + + pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev)); + return 0; +} + /** * arm_iommu_attach_device * @dev: valid struct device pointer * @mapping: io address space mapping structure (returned from * arm_iommu_create_mapping) * - * Attaches specified io address space mapping to the provided device, + * Attaches specified io address space mapping to the provided device. + * This replaces the dma operations (dma_map_ops pointer) with the + * IOMMU aware version. + * * More than one client might be attached to the same io address space * mapping. */ @@ -1955,25 +1974,16 @@ int arm_iommu_attach_device(struct device *dev, { int err; - err = iommu_attach_device(mapping->domain, dev); + err = __arm_iommu_attach_device(dev, mapping); if (err) return err; - kref_get(&mapping->kref); - dev->archdata.mapping = mapping; - - pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev)); + set_dma_ops(dev, &iommu_ops); return 0; } EXPORT_SYMBOL_GPL(arm_iommu_attach_device); -/** - * arm_iommu_detach_device - * @dev: valid struct device pointer - * - * Detaches the provided device from a previously attached map. - */ -void arm_iommu_detach_device(struct device *dev) +static void __arm_iommu_detach_device(struct device *dev) { struct dma_iommu_mapping *mapping; @@ -1989,6 +1999,19 @@ void arm_iommu_detach_device(struct device *dev) pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev)); } + +/** + * arm_iommu_detach_device + * @dev: valid struct device pointer + * + * Detaches the provided device from a previously attached map. + * This voids the dma operations (dma_map_ops pointer) + */ +void arm_iommu_detach_device(struct device *dev) +{ + __arm_iommu_detach_device(dev); + set_dma_ops(dev, NULL); +} EXPORT_SYMBOL_GPL(arm_iommu_detach_device); static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent) @@ -2011,7 +2034,7 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, return false; } - if (arm_iommu_attach_device(dev, mapping)) { + if (__arm_iommu_attach_device(dev, mapping)) { pr_warn("Failed to attached device %s to IOMMU_mapping\n", dev_name(dev)); arm_iommu_release_mapping(mapping); @@ -2025,7 +2048,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev) { struct dma_iommu_mapping *mapping = dev->archdata.mapping; - arm_iommu_detach_device(dev); + __arm_iommu_detach_device(dev); arm_iommu_release_mapping(mapping); }
Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops") moved the setting of the DMA operations from arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA operations to be used are selected based on whether the device is connected to an IOMMU. However, the IOMMU detection scheme requires the IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver has been ported yet, this effectively breaks all IOMMU ARM users that depend on the IOMMU being handled transparently by the DMA mapping API. Fix this by restoring the setting of DMA IOMMU ops in arm_iommu_attach_device() and splitting the rest of the function into a new internal __arm_iommu_attach_device() function, called by arch_setup_dma_ops(). Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- arch/arm/mm/dma-mapping.c | 53 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 15 deletions(-) This fixes a v3.19 regression that breaks IOMMU on the Exynos, OMAP3, Renesas and Rockchip platforms. The patch has been tested on a Renesas M2 platform with the ipmmu-vmsa driver.