linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [RFC] ARM64: simplify dma_get_ops
@ 2015-11-16 16:25 Arnd Bergmann
  2015-11-16 18:39 ` Catalin Marinas
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2015-11-16 16:25 UTC (permalink / raw)
  To: linux-arm-kernel

Including linux/acpi.h from asm/dma-mapping.h causes tons of compile-time
warnings, e.g.

 drivers/isdn/mISDN/dsp_ecdis.h:43:0: warning: "FALSE" redefined
 drivers/isdn/mISDN/dsp_ecdis.h:44:0: warning: "TRUE" redefined
 drivers/net/fddi/skfp/h/targetos.h:62:0: warning: "TRUE" redefined
 drivers/net/fddi/skfp/h/targetos.h:63:0: warning: "FALSE" redefined

However, it looks like the dependency should not even there as
I do not see why __generic_dma_ops() cares about whether we have
an ACPI based system or not.

The current behavior is to fall back to the global dma_ops when
a device has not set its own dma_ops, but only for DT based systems.
This seems dangerous, as a random device might have different
requirements regarding IOMMU or coherency, so we should really
never have that fallback and just forbid DMA when we have not
initialized DMA for a device.

This removes the global dma_ops variable and the special-casing
for ACPI, and just returns the dma ops that got set for the
device, or the dummy_dma_ops if none were present.

The original code has apparently been copied from arm32 where we
rely on it for ISA devices things like the floppy controller, but
we should have no such devices on ARM64.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index 54d0ead41afc..04e841a1c1f3 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -18,7 +18,6 @@
 
 #ifdef __KERNEL__
 
-#include <linux/acpi.h>
 #include <linux/types.h>
 #include <linux/vmalloc.h>
 
@@ -26,22 +25,17 @@
 #include <asm/xen/hypervisor.h>
 
 #define DMA_ERROR_CODE	(~(dma_addr_t)0)
-extern struct dma_map_ops *dma_ops;
 extern struct dma_map_ops dummy_dma_ops;
 
 static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
 {
-	if (unlikely(!dev))
-		return dma_ops;
-	else if (dev->archdata.dma_ops)
+	if (dev && dev->archdata.dma_ops)
 		return dev->archdata.dma_ops;
-	else if (acpi_disabled)
-		return dma_ops;
 
 	/*
-	 * When ACPI is enabled, if arch_set_dma_ops is not called,
-	 * we will disable device DMA capability by setting it
-	 * to dummy_dma_ops.
+	 * we expect no ISA devices, and all other DMA masters are
+	 * expected to have someone call arch_setup_dma_ops at
+	 * device creation time
 	 */
 	return &dummy_dma_ops;
 }
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 131a199114b4..9e351c1f89e2 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -18,6 +18,7 @@
  */
 
 #include <linux/gfp.h>
+#include <linux/acpi.h>
 #include <linux/export.h>
 #include <linux/slab.h>
 #include <linux/genalloc.h>
@@ -28,9 +29,6 @@
 
 #include <asm/cacheflush.h>
 
-struct dma_map_ops *dma_ops;
-EXPORT_SYMBOL(dma_ops);
-
 static pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
 				 bool coherent)
 {
@@ -515,13 +513,7 @@ EXPORT_SYMBOL(dummy_dma_ops);
 
 static int __init arm64_dma_init(void)
 {
-	int ret;
-
-	dma_ops = &swiotlb_dma_ops;
-
-	ret = atomic_pool_init();
-
-	return ret;
+	return atomic_pool_init();
 }
 arch_initcall(arm64_dma_init);
 
@@ -985,7 +977,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			struct iommu_ops *iommu, bool coherent)
 {
 	if (!acpi_disabled && !dev->archdata.dma_ops)
-		dev->archdata.dma_ops = dma_ops;
+		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC] ARM64: simplify dma_get_ops
  2015-11-16 16:25 [RFC] ARM64: simplify dma_get_ops Arnd Bergmann
@ 2015-11-16 18:39 ` Catalin Marinas
  2015-11-16 19:57   ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2015-11-16 18:39 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 16, 2015 at 05:25:48PM +0100, Arnd Bergmann wrote:
> Including linux/acpi.h from asm/dma-mapping.h causes tons of compile-time
> warnings, e.g.
> 
>  drivers/isdn/mISDN/dsp_ecdis.h:43:0: warning: "FALSE" redefined
>  drivers/isdn/mISDN/dsp_ecdis.h:44:0: warning: "TRUE" redefined
>  drivers/net/fddi/skfp/h/targetos.h:62:0: warning: "TRUE" redefined
>  drivers/net/fddi/skfp/h/targetos.h:63:0: warning: "FALSE" redefined
> 
> However, it looks like the dependency should not even there as
> I do not see why __generic_dma_ops() cares about whether we have
> an ACPI based system or not.
> 
> The current behavior is to fall back to the global dma_ops when
> a device has not set its own dma_ops, but only for DT based systems.
> This seems dangerous, as a random device might have different
> requirements regarding IOMMU or coherency, so we should really
> never have that fallback and just forbid DMA when we have not
> initialized DMA for a device.

I think this is from the days when we didn't have an
arch_setup_dma_ops() to be called from the DT code.

I did placed a WARN_ON() to see who's getting the dummy_dma_ops and it
triggered this call trace:

WARNING: at /work/Linux/linux-2.6-aarch64/arch/arm64/include/asm/dma-mapping.h:40
Modules linked in:

CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W       4.4.0-rc1+ #601
Hardware name: Juno (DT)
task: ffffffc9768a0000 ti: ffffffc9768a8000 task.ti: ffffffc9768a8000
PC is at ohci_platform_probe+0x2b8/0x518
LR is at ohci_platform_probe+0x2c/0x518
Call trace:
[<ffffffc0004920e0>] ohci_platform_probe+0x2b8/0x518
[<ffffffc0003e842c>] platform_drv_probe+0x54/0xb8
[<ffffffc0003e68d4>] driver_probe_device+0x1ec/0x2f0
[<ffffffc0003e6a74>] __driver_attach+0x9c/0xa0
[<ffffffc0003e4a10>] bus_for_each_dev+0x60/0xa0
[<ffffffc0003e61b8>] driver_attach+0x20/0x28
[<ffffffc0003e5dd8>] bus_add_driver+0x1d0/0x238
[<ffffffc0003e7218>] driver_register+0x60/0xf8
[<ffffffc0003e8328>] __platform_driver_register+0x40/0x48
[<ffffffc00086f720>] ohci_platform_init+0x50/0x60
[<ffffffc000082938>] do_one_initcall+0x90/0x1a0
[<ffffffc00084caac>] kernel_init_freeable+0x154/0x1f8
[<ffffffc0005e9c40>] kernel_init+0x10/0xe0
[<ffffffc000085c50>] ret_from_fork+0x10/0x40
ohci-platform: probe of 7ffb0000.ohci failed with error -5

I need to check whether we break anything by no longer returning
swiotlb_dma_ops.

> This removes the global dma_ops variable and the special-casing
> for ACPI, and just returns the dma ops that got set for the
> device, or the dummy_dma_ops if none were present.
> 
> The original code has apparently been copied from arm32 where we
> rely on it for ISA devices things like the floppy controller, but
> we should have no such devices on ARM64.

IIRC, this was required for DT before we had the arch_set_dma_ops()
(though I may be wrong, I haven't checked the logs). The dummy ops were
introduced with ACPI to avoid any default, I think this was related to
the _CCA property.

> @@ -985,7 +977,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  			struct iommu_ops *iommu, bool coherent)
>  {
>  	if (!acpi_disabled && !dev->archdata.dma_ops)
> -		dev->archdata.dma_ops = dma_ops;
> +		dev->archdata.dma_ops = &swiotlb_dma_ops;

Why do we still keep the !acpi_disabled check here? If I remove it, the
WARN_ON() above disappears.

-- 
Catalin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC] ARM64: simplify dma_get_ops
  2015-11-16 18:39 ` Catalin Marinas
@ 2015-11-16 19:57   ` Arnd Bergmann
  2015-11-17 12:22     ` Catalin Marinas
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2015-11-16 19:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 16 November 2015 18:39:41 Catalin Marinas wrote:
> On Mon, Nov 16, 2015 at 05:25:48PM +0100, Arnd Bergmann wrote:
> > Including linux/acpi.h from asm/dma-mapping.h causes tons of compile-time
> > warnings, e.g.
> > 
> >  drivers/isdn/mISDN/dsp_ecdis.h:43:0: warning: "FALSE" redefined
> >  drivers/isdn/mISDN/dsp_ecdis.h:44:0: warning: "TRUE" redefined
> >  drivers/net/fddi/skfp/h/targetos.h:62:0: warning: "TRUE" redefined
> >  drivers/net/fddi/skfp/h/targetos.h:63:0: warning: "FALSE" redefined
> > 
> > However, it looks like the dependency should not even there as
> > I do not see why __generic_dma_ops() cares about whether we have
> > an ACPI based system or not.
> > 
> > The current behavior is to fall back to the global dma_ops when
> > a device has not set its own dma_ops, but only for DT based systems.
> > This seems dangerous, as a random device might have different
> > requirements regarding IOMMU or coherency, so we should really
> > never have that fallback and just forbid DMA when we have not
> > initialized DMA for a device.
> 
> I think this is from the days when we didn't have an
> arch_setup_dma_ops() to be called from the DT code.

Makes sense.

> I did placed a WARN_ON() to see who's getting the dummy_dma_ops and it
> triggered this call trace:
> 
> WARNING: at /work/Linux/linux-2.6-aarch64/arch/arm64/include/asm/dma-mapping.h:40
> Modules linked in:
> 
> CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W       4.4.0-rc1+ #601
> Hardware name: Juno (DT)
> task: ffffffc9768a0000 ti: ffffffc9768a8000 task.ti: ffffffc9768a8000
> PC is at ohci_platform_probe+0x2b8/0x518
> LR is at ohci_platform_probe+0x2c/0x518
> Call trace:
> [<ffffffc0004920e0>] ohci_platform_probe+0x2b8/0x518
> [<ffffffc0003e842c>] platform_drv_probe+0x54/0xb8
> [<ffffffc0003e68d4>] driver_probe_device+0x1ec/0x2f0
> [<ffffffc0003e6a74>] __driver_attach+0x9c/0xa0
> [<ffffffc0003e4a10>] bus_for_each_dev+0x60/0xa0
> [<ffffffc0003e61b8>] driver_attach+0x20/0x28
> [<ffffffc0003e5dd8>] bus_add_driver+0x1d0/0x238
> [<ffffffc0003e7218>] driver_register+0x60/0xf8
> [<ffffffc0003e8328>] __platform_driver_register+0x40/0x48
> [<ffffffc00086f720>] ohci_platform_init+0x50/0x60
> [<ffffffc000082938>] do_one_initcall+0x90/0x1a0
> [<ffffffc00084caac>] kernel_init_freeable+0x154/0x1f8
> [<ffffffc0005e9c40>] kernel_init+0x10/0xe0
> [<ffffffc000085c50>] ret_from_fork+0x10/0x40
> ohci-platform: probe of 7ffb0000.ohci failed with error -5
> 
> I need to check whether we break anything by no longer returning
> swiotlb_dma_ops.

Interesting. Note that this driver calls 'dma_coerce_mask_and_coherent()',
which can be regarded a bug by itself, because it overrides the
platform specific DMA mask.

> > This removes the global dma_ops variable and the special-casing
> > for ACPI, and just returns the dma ops that got set for the
> > device, or the dummy_dma_ops if none were present.
> > 
> > The original code has apparently been copied from arm32 where we
> > rely on it for ISA devices things like the floppy controller, but
> > we should have no such devices on ARM64.
> 
> IIRC, this was required for DT before we had the arch_set_dma_ops()
> (though I may be wrong, I haven't checked the logs). The dummy ops were
> introduced with ACPI to avoid any default, I think this was related to
> the _CCA property.

Right.

> > @@ -985,7 +977,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> >  			struct iommu_ops *iommu, bool coherent)
> >  {
> >  	if (!acpi_disabled && !dev->archdata.dma_ops)
> > -		dev->archdata.dma_ops = dma_ops;
> > +		dev->archdata.dma_ops = &swiotlb_dma_ops;
> 
> Why do we still keep the !acpi_disabled check here? If I remove it, the
> WARN_ON() above disappears.

Ah, good. That must be my mistake then. This looks much better. On a
related note, we should also urgently fix the arch_setup_dma_ops() function
to no longer ignore the base and size arguments. For dma_mase, we can simply
WARN_ON(dma_base != 0), so we can implement support for that whenever we need
it, but for the size we need to prevent drivers from calling dma_set_mask()
with an argument larger than the size we pass in here, unless the size is
also larger than max_pfn.

	Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC] ARM64: simplify dma_get_ops
  2015-11-16 19:57   ` Arnd Bergmann
@ 2015-11-17 12:22     ` Catalin Marinas
  2015-11-17 12:50       ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2015-11-17 12:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 16, 2015 at 08:57:41PM +0100, Arnd Bergmann wrote:
> On Monday 16 November 2015 18:39:41 Catalin Marinas wrote:
> > On Mon, Nov 16, 2015 at 05:25:48PM +0100, Arnd Bergmann wrote:
> > > @@ -985,7 +977,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> > >  			struct iommu_ops *iommu, bool coherent)
> > >  {
> > >  	if (!acpi_disabled && !dev->archdata.dma_ops)
> > > -		dev->archdata.dma_ops = dma_ops;
> > > +		dev->archdata.dma_ops = &swiotlb_dma_ops;
> > 
> > Why do we still keep the !acpi_disabled check here? If I remove it, the
> > WARN_ON() above disappears.
> 
> Ah, good. That must be my mistake then. This looks much better.

I merged this patch with the above change. Thanks.

> On a related note, we should also urgently fix the
> arch_setup_dma_ops() function to no longer ignore the base and size
> arguments. For dma_base, we can simply WARN_ON(dma_base != 0), so we
> can implement support for that whenever we need it,

I think we should, at least until we implement support for
dev->dma_pfn_offset. I'm not sure about iommu though, maybe there are
working configurations with dma_base != 0.

> but for the size we need to prevent drivers from calling
> dma_set_mask() with an argument larger than the size we pass in here,
> unless the size is also larger than max_pfn.

We have a default mask set up in of_dma_configure() based on size and
dma_base. Can we check the new mask against the default one?

-- 
Catalin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC] ARM64: simplify dma_get_ops
  2015-11-17 12:22     ` Catalin Marinas
@ 2015-11-17 12:50       ` Arnd Bergmann
  2015-11-27 16:57         ` Catalin Marinas
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2015-11-17 12:50 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday 17 November 2015 12:22:51 Catalin Marinas wrote:
> 
> > On a related note, we should also urgently fix the
> > arch_setup_dma_ops() function to no longer ignore the base and size
> > arguments. For dma_base, we can simply WARN_ON(dma_base != 0), so we
> > can implement support for that whenever we need it,
> 
> I think we should, at least until we implement support for
> dev->dma_pfn_offset. I'm not sure about iommu though, maybe there are
> working configurations with dma_base != 0.

I think we can assume for now that all IOMMUs are similar to the
ARM SMMU and don't need this.

> > but for the size we need to prevent drivers from calling
> > dma_set_mask() with an argument larger than the size we pass in here,
> > unless the size is also larger than max_pfn.
> 
> We have a default mask set up in of_dma_configure() based on size and
> dma_base. Can we check the new mask against the default one?

The size variable here is the mask that of_dma_configure() computes,
though it is not a "default": it is whatever the parent bus can support,
independent of additional restrictions that may be present in the
device and that are set by the driver.

Checking against that is what I meant above, see below for a prototype
that I have not even compile-tested and that might be missing some corner
cases.

We actually have the option of swapping out the dev->dma_ops in set_mask
so we don't have to go through the swiotlb code for devices that don't
need it.

	Arnd

diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef256b8c9..2af91a5bec4e 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 9e351c1f89e2..0433b911b1bd 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -341,6 +341,31 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
 	return ret;
 }
 
+static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
+{
+	/* device is not DMA capable */
+	if (!dev->dma_mask)
+		return -EIO;
+
+	/* mask is below swiotlb bounce buffer, so fail */
+	if (!swiotlb_dma_supported(dev, mask))
+		return -EIO;
+
+	/*
+	 * because of the swiotlb, we can return success for
+	 * larger masks, but need to ensure that bounce buffers
+	 * are used above parent_dma_mask, so set that as
+	 * the effective mask.
+	 */
+	if (mask > dev->dev_archdata.parent_dma_mask)
+		mask = dev->dev_archdata.parent_dma_mask;
+
+
+	*dev->dma_mask = mask;
+
+	return 0;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -356,6 +381,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = swiotlb_dma_supported,
 	.mapping_error = swiotlb_dma_mapping_error,
+	.set_dma_mask = __swiotlb_set_dma_mask,
 };
 
 static int __init atomic_pool_init(void)
@@ -979,6 +1005,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!acpi_disabled && !dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * we don't yet support buses that have a non-zero mapping.
+	 *  Let's hope we won't need it
+	 */
+	WARN_ON(dma_base != 0);
+
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	dev->archdata.parent_dma_mask = size;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC] ARM64: simplify dma_get_ops
  2015-11-17 12:50       ` Arnd Bergmann
@ 2015-11-27 16:57         ` Catalin Marinas
  2015-11-27 20:46           ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2015-11-27 16:57 UTC (permalink / raw)
  To: linux-arm-kernel

(sorry for the delay, I got distracted by other things)

On Tue, Nov 17, 2015 at 01:50:24PM +0100, Arnd Bergmann wrote:
> On Tuesday 17 November 2015 12:22:51 Catalin Marinas wrote:
> > 
> > > On a related note, we should also urgently fix the
> > > arch_setup_dma_ops() function to no longer ignore the base and size
> > > arguments. For dma_base, we can simply WARN_ON(dma_base != 0), so we
> > > can implement support for that whenever we need it,
> > 
> > I think we should, at least until we implement support for
> > dev->dma_pfn_offset. I'm not sure about iommu though, maybe there are
> > working configurations with dma_base != 0.
> 
> I think we can assume for now that all IOMMUs are similar to the
> ARM SMMU and don't need this.
> 
> > > but for the size we need to prevent drivers from calling
> > > dma_set_mask() with an argument larger than the size we pass in here,
> > > unless the size is also larger than max_pfn.
> > 
> > We have a default mask set up in of_dma_configure() based on size and
> > dma_base. Can we check the new mask against the default one?
> 
> The size variable here is the mask that of_dma_configure() computes,
> though it is not a "default": it is whatever the parent bus can support,
> independent of additional restrictions that may be present in the
> device and that are set by the driver.
> 
> Checking against that is what I meant above, see below for a prototype
> that I have not even compile-tested and that might be missing some corner
> cases.
> 
> We actually have the option of swapping out the dev->dma_ops in set_mask
> so we don't have to go through the swiotlb code for devices that don't
> need it.

We could indeed have a lighter implementation that only does cache
maintenance but I guess this assumes that the device supports all the
physical address space. Swiotlb detects the masks and doesn't bounce the
buffer unless necessary, so the overhead shouldn't be that large.

> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -341,6 +341,31 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>  	return ret;
>  }
>  
> +static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
> +{
> +	/* device is not DMA capable */
> +	if (!dev->dma_mask)
> +		return -EIO;
> +
> +	/* mask is below swiotlb bounce buffer, so fail */
> +	if (!swiotlb_dma_supported(dev, mask))
> +		return -EIO;
> +
> +	/*
> +	 * because of the swiotlb, we can return success for
> +	 * larger masks, but need to ensure that bounce buffers
> +	 * are used above parent_dma_mask, so set that as
> +	 * the effective mask.
> +	 */
> +	if (mask > dev->dev_archdata.parent_dma_mask)
> +		mask = dev->dev_archdata.parent_dma_mask;

Is there any check for parent_dma_mask being supported by swiotlb? If
not, should we move the mask setting above?

-- 
Catalin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC] ARM64: simplify dma_get_ops
  2015-11-27 16:57         ` Catalin Marinas
@ 2015-11-27 20:46           ` Arnd Bergmann
  0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2015-11-27 20:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday 27 November 2015 16:57:21 Catalin Marinas wrote:
> On Tue, Nov 17, 2015 at 01:50:24PM +0100, Arnd Bergmann wrote:
> > On Tuesday 17 November 2015 12:22:51 Catalin Marinas wrote:
> > > 
> > The size variable here is the mask that of_dma_configure() computes,
> > though it is not a "default": it is whatever the parent bus can support,
> > independent of additional restrictions that may be present in the
> > device and that are set by the driver.
> > 
> > Checking against that is what I meant above, see below for a prototype
> > that I have not even compile-tested and that might be missing some corner
> > cases.
> > 
> > We actually have the option of swapping out the dev->dma_ops in set_mask
> > so we don't have to go through the swiotlb code for devices that don't
> > need it.
> 
> We could indeed have a lighter implementation that only does cache
> maintenance but I guess this assumes that the device supports all the
> physical address space. Swiotlb detects the masks and doesn't bounce the
> buffer unless necessary, so the overhead shouldn't be that large.

Right, that part is certainly optional.

> > --- a/arch/arm64/mm/dma-mapping.c
> > +++ b/arch/arm64/mm/dma-mapping.c
> > @@ -341,6 +341,31 @@ static int __swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> >  	return ret;
> >  }
> >  
> > +static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
> > +{
> > +	/* device is not DMA capable */
> > +	if (!dev->dma_mask)
> > +		return -EIO;
> > +
> > +	/* mask is below swiotlb bounce buffer, so fail */
> > +	if (!swiotlb_dma_supported(dev, mask))
> > +		return -EIO;
> > +
> > +	/*
> > +	 * because of the swiotlb, we can return success for
> > +	 * larger masks, but need to ensure that bounce buffers
> > +	 * are used above parent_dma_mask, so set that as
> > +	 * the effective mask.
> > +	 */
> > +	if (mask > dev->dev_archdata.parent_dma_mask)
> > +		mask = dev->dev_archdata.parent_dma_mask;
> 
> Is there any check for parent_dma_mask being supported by swiotlb? If
> not, should we move the mask setting above?

I think swiotlb assumes that the bus can always handle all of RAM,
as that is typically the case on x86: you can have PCI devices that
don't support 64-bit DMA, but the PCI host bridge always does, and
there are practically never any DMA masters outside of PCI.

powerpc has a 'max_direct_dma_addr' variable in dev_archdata, and we
should probably use the same name rather than parent_dma_mask (but
note the difference when an offset is involved). On powerpc, we assume
that the device driver knows what memory the bus supports, which works
because there are fewer SoC vendors reusing the same parts in different
ways. The max_direct_dma_addr there appears to only be used for PCI
devices.

	Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-11-27 20:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-16 16:25 [RFC] ARM64: simplify dma_get_ops Arnd Bergmann
2015-11-16 18:39 ` Catalin Marinas
2015-11-16 19:57   ` Arnd Bergmann
2015-11-17 12:22     ` Catalin Marinas
2015-11-17 12:50       ` Arnd Bergmann
2015-11-27 16:57         ` Catalin Marinas
2015-11-27 20:46           ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).