[RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

[RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

[Robin Murphy]

Arch-specific implementions for dma_set_{coherent_,}mask() currently
rely on an inconsistent mix of arch-defined Kconfig symbols and macro
overrides. Now that we have a nice centralised home for DMA API gubbins,
let's consolidate these loose ends under consistent config options.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

Here's hoping the buildbot comes by to point out what I've inevitably
missed, although I did check a cursory cross-compile of ppc64_defconfig
to iron out the obvious howlers.

The motivation here is that I'm looking at adding set_mask overrides
for arm64, and having discovered a bit of a mess it seemed prudent to
clean up before ingraining it any more.

Robin.


 arch/arm/Kconfig                       | 3 ---
 arch/powerpc/Kconfig                   | 4 +---
 arch/powerpc/include/asm/dma-mapping.h | 3 ---
 include/linux/dma-mapping.h            | 4 +++-
 kernel/dma/Kconfig                     | 6 ++++++
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 843edfd000be..ab0c081b6ec2 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -227,9 +227,6 @@ config ZONE_DMA
 config ARCH_SUPPORTS_UPROBES
 	def_bool y
 
-config ARCH_HAS_DMA_SET_COHERENT_MASK
-	bool
-
 config GENERIC_ISA_DMA
 	bool
 
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 9f2b75fe2c2d..08d85412d783 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -119,9 +119,6 @@ config GENERIC_HWEIGHT
 	bool
 	default y
 
-config ARCH_HAS_DMA_SET_COHERENT_MASK
-        bool
-
 config PPC
 	bool
 	default y
@@ -129,6 +126,7 @@ config PPC
 	# Please keep this list sorted alphabetically.
 	#
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
+	select ARCH_HAS_DMA_SET_MASK
 	select ARCH_HAS_DMA_SET_COHERENT_MASK
 	select ARCH_HAS_ELF_RANDOMIZE
 	select ARCH_HAS_FORTIFY_SOURCE
diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
index 8fa394520af6..fe912c4367f2 100644
--- a/arch/powerpc/include/asm/dma-mapping.h
+++ b/arch/powerpc/include/asm/dma-mapping.h
@@ -107,9 +107,6 @@ static inline void set_dma_offset(struct device *dev, dma_addr_t off)
 		dev->archdata.dma_offset = off;
 }
 
-#define HAVE_ARCH_DMA_SET_MASK 1
-extern int dma_set_mask(struct device *dev, u64 dma_mask);
-
 extern u64 __dma_get_required_mask(struct device *dev);
 
 #define ARCH_HAS_DMA_MMAP_COHERENT
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index ffeca3ab59c0..30fe0c900420 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -596,7 +596,9 @@ static inline int dma_supported(struct device *dev, u64 mask)
 	return ops->dma_supported(dev, mask);
 }
 
-#ifndef HAVE_ARCH_DMA_SET_MASK
+#ifdef CONFIG_ARCH_HAS_DMA_SET_MASK
+int dma_set_mask(struct device *dev, u64 mask);
+#else
 static inline int dma_set_mask(struct device *dev, u64 mask)
 {
 	if (!dev->dma_mask || !dma_supported(dev, mask))
diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index 9bd54304446f..01001371d892 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -16,6 +16,12 @@ config ARCH_DMA_ADDR_T_64BIT
 config HAVE_GENERIC_DMA_COHERENT
 	bool
 
+config ARCH_HAS_DMA_SET_MASK
+        bool
+
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+        bool
+
 config ARCH_HAS_SYNC_DMA_FOR_DEVICE
 	bool
 
-- 
2.17.1.dirty

[RFC PATCH 2/2] dma-mapping: Clean up dma_get_required_mask() hooks

[Robin Murphy]

As for the other mask-related hooks, standardise the arch override into
a Kconfig option, and also pull the generic implementation into the DMA
mapping code rather than having it hide away in the platform bus code.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/ia64/Kconfig                   |  1 +
 arch/ia64/include/asm/dma-mapping.h |  2 --
 arch/powerpc/Kconfig                |  1 +
 arch/powerpc/include/asm/device.h   |  2 --
 drivers/base/platform.c             | 23 -----------------------
 drivers/pci/controller/vmd.c        |  4 ++--
 include/linux/dma-mapping.h         |  2 +-
 kernel/dma/Kconfig                  |  3 +++
 kernel/dma/mapping.c                | 23 +++++++++++++++++++++++
 9 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index ff861420b8f5..a6274e79b155 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -12,6 +12,7 @@ menu "Processor type and features"
 
 config IA64
 	bool
+	select ARCH_HAS_DMA_GET_REQUIRED_MASK
 	select ARCH_MIGHT_HAVE_PC_PARPORT
 	select ARCH_MIGHT_HAVE_PC_SERIO
 	select PCI if (!IA64_HP_SIM)
diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
index 76e4d6632d68..522745ae67bb 100644
--- a/arch/ia64/include/asm/dma-mapping.h
+++ b/arch/ia64/include/asm/dma-mapping.h
@@ -10,8 +10,6 @@
 #include <linux/scatterlist.h>
 #include <linux/dma-debug.h>
 
-#define ARCH_HAS_DMA_GET_REQUIRED_MASK
-
 extern const struct dma_map_ops *dma_ops;
 extern struct ia64_machine_vector ia64_mv;
 extern void set_iommu_machvec(void);
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 08d85412d783..3581c576c762 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -126,6 +126,7 @@ config PPC
 	# Please keep this list sorted alphabetically.
 	#
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
+	select ARCH_HAS_DMA_GET_REQUIRED_MASK
 	select ARCH_HAS_DMA_SET_MASK
 	select ARCH_HAS_DMA_SET_COHERENT_MASK
 	select ARCH_HAS_ELF_RANDOMIZE
diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/include/asm/device.h
index 0245bfcaac32..17cceab5ccf9 100644
--- a/arch/powerpc/include/asm/device.h
+++ b/arch/powerpc/include/asm/device.h
@@ -54,6 +54,4 @@ struct pdev_archdata {
 	u64 dma_mask;
 };
 
-#define ARCH_HAS_DMA_GET_REQUIRED_MASK
-
 #endif /* _ASM_POWERPC_DEVICE_H */
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dff82a3c2caa..dae427a77b0a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -16,7 +16,6 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/dma-mapping.h>
-#include <linux/bootmem.h>
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
@@ -1179,28 +1178,6 @@ int __init platform_bus_init(void)
 	return error;
 }
 
-#ifndef ARCH_HAS_DMA_GET_REQUIRED_MASK
-u64 dma_get_required_mask(struct device *dev)
-{
-	u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);
-	u32 high_totalram = ((max_pfn - 1) >> (32 - PAGE_SHIFT));
-	u64 mask;
-
-	if (!high_totalram) {
-		/* convert to mask just covering totalram */
-		low_totalram = (1 << (fls(low_totalram) - 1));
-		low_totalram += low_totalram - 1;
-		mask = low_totalram;
-	} else {
-		high_totalram = (1 << (fls(high_totalram) - 1));
-		high_totalram += high_totalram - 1;
-		mask = (((u64)high_totalram) << 32) + 0xffffffff;
-	}
-	return mask;
-}
-EXPORT_SYMBOL_GPL(dma_get_required_mask);
-#endif
-
 static __initdata LIST_HEAD(early_platform_driver_list);
 static __initdata LIST_HEAD(early_platform_device_list);
 
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 942b64fc7f1f..9dd721d36783 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -393,7 +393,7 @@ static int vmd_dma_supported(struct device *dev, u64 mask)
 	return vmd_dma_ops(dev)->dma_supported(to_vmd_dev(dev), mask);
 }
 
-#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
+#ifdef CONFIG_ARCH_HAS_DMA_GET_REQUIRED_MASK
 static u64 vmd_get_required_mask(struct device *dev)
 {
 	return vmd_dma_ops(dev)->get_required_mask(to_vmd_dev(dev));
@@ -439,7 +439,7 @@ static void vmd_setup_dma_ops(struct vmd_dev *vmd)
 	ASSIGN_VMD_DMA_OPS(source, dest, sync_sg_for_device);
 	ASSIGN_VMD_DMA_OPS(source, dest, mapping_error);
 	ASSIGN_VMD_DMA_OPS(source, dest, dma_supported);
-#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
+#ifdef CONFIG_ARCH_HAS_DMA_GET_REQUIRED_MASK
 	ASSIGN_VMD_DMA_OPS(source, dest, get_required_mask);
 #endif
 	add_dma_domain(domain);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 30fe0c900420..788d7a609dd8 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -130,7 +130,7 @@ struct dma_map_ops {
 			enum dma_data_direction direction);
 	int (*mapping_error)(struct device *dev, dma_addr_t dma_addr);
 	int (*dma_supported)(struct device *dev, u64 mask);
-#ifdef ARCH_HAS_DMA_GET_REQUIRED_MASK
+#ifdef CONFIG_ARCH_HAS_DMA_GET_REQUIRED_MASK
 	u64 (*get_required_mask)(struct device *dev);
 #endif
 };
diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index 01001371d892..cb12bb2d270a 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -16,6 +16,9 @@ config ARCH_DMA_ADDR_T_64BIT
 config HAVE_GENERIC_DMA_COHERENT
 	bool
 
+config ARCH_HAS_DMA_GET_REQUIRED_MASK
+        bool
+
 config ARCH_HAS_DMA_SET_MASK
         bool
 
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index d2a92ddaac4d..fdadc9524cb2 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bootmem.h>
 #include <linux/dma-mapping.h>
 #include <linux/export.h>
 #include <linux/gfp.h>
@@ -198,6 +199,28 @@ EXPORT_SYMBOL(dmam_release_declared_memory);
 
 #endif
 
+#ifndef CONFIG_ARCH_HAS_DMA_GET_REQUIRED_MASK
+u64 dma_get_required_mask(struct device *dev)
+{
+	u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);
+	u32 high_totalram = ((max_pfn - 1) >> (32 - PAGE_SHIFT));
+	u64 mask;
+
+	if (!high_totalram) {
+		/* convert to mask just covering totalram */
+		low_totalram = (1 << (fls(low_totalram) - 1));
+		low_totalram += low_totalram - 1;
+		mask = low_totalram;
+	} else {
+		high_totalram = (1 << (fls(high_totalram) - 1));
+		high_totalram += high_totalram - 1;
+		mask = (((u64)high_totalram) << 32) + 0xffffffff;
+	}
+	return mask;
+}
+EXPORT_SYMBOL_GPL(dma_get_required_mask);
+#endif
+
 /*
  * Create scatter-list for the already allocated DMA buffer.
  */
-- 
2.17.1.dirty

Re: [RFC PATCH 2/2] dma-mapping: Clean up dma_get_required_mask() hooks

[Christoph Hellwig]

On Wed, Jul 04, 2018 at 06:50:12PM +0100, Robin Murphy wrote:
> As for the other mask-related hooks, standardise the arch override into
> a Kconfig option, and also pull the generic implementation into the DMA
> mapping code rather than having it hide away in the platform bus code.

Heh, I have a somewhat similar patch in my queue.  I didn't want it out
because dma_get_required_mask is rather ill defined at the moment and
I wanted to clean that up first.  But I guess I could apply this first
and clean up later.

I just fear you might be wanting to add an arm64 user, so I'd really like
to understand why and how.

Re: [RFC PATCH 2/2] dma-mapping: Clean up dma_get_required_mask() hooks

[Robin Murphy]

On 05/07/18 20:38, Christoph Hellwig wrote:
> On Wed, Jul 04, 2018 at 06:50:12PM +0100, Robin Murphy wrote:
>> As for the other mask-related hooks, standardise the arch override into
>> a Kconfig option, and also pull the generic implementation into the DMA
>> mapping code rather than having it hide away in the platform bus code.
> 
> Heh, I have a somewhat similar patch in my queue.  I didn't want it out
> because dma_get_required_mask is rather ill defined at the moment and
> I wanted to clean that up first.  But I guess I could apply this first
> and clean up later.
> 
> I just fear you might be wanting to add an arm64 user, so I'd really like
> to understand why and how.

Nah, this guy is just pure cleanup since it also fell out of my 'git 
grep ARCH_HAS_DMA'

Robin.

Re: [RFC PATCH 2/2] dma-mapping: Clean up dma_get_required_mask() hooks

[Christoph Hellwig]

On Wed, Jul 04, 2018 at 06:50:12PM +0100, Robin Murphy wrote:
> As for the other mask-related hooks, standardise the arch override into
> a Kconfig option, and also pull the generic implementation into the DMA
> mapping code rather than having it hide away in the platform bus code.

I compared this a bit to what I had around against an older kernel,
and I think we should probably go with something more like the
version I had, which I can dust off again.

What I've done is to:

 1) provide the get_required_mask unconditionally in struct dma_map_ops
 2) default to what is the current dma_get_required_mask implementation
    if nothing else is specified.

What I still had on my todo list but not done yet:

 3) go through all instances and check if the current default
    makes sense, at it based on direct addressability.  For most
    iommu instances it seems like we should just return a 64-bit mask.
 4) figure out how to take the dma offsets into account for it
 5) move the function to the dma-direct code, as that is where it
    belongs
 5) figure out if there is a better name for the method, as with
    swiotlb & co it isn't really the required mask, but more something
    like the optimal mask
 6) document the whole thing..
 7) sort out the powerpc indirection mess.

Do you agree with that general plan?  If so I can dust off my old
patch.

Re: [RFC PATCH 2/2] dma-mapping: Clean up dma_get_required_mask() hooks

[Robin Murphy]

On 10/07/18 12:39, Christoph Hellwig wrote:
> On Wed, Jul 04, 2018 at 06:50:12PM +0100, Robin Murphy wrote:
>> As for the other mask-related hooks, standardise the arch override into
>> a Kconfig option, and also pull the generic implementation into the DMA
>> mapping code rather than having it hide away in the platform bus code.
> 
> I compared this a bit to what I had around against an older kernel,
> and I think we should probably go with something more like the
> version I had, which I can dust off again.
> 
> What I've done is to:
> 
>   1) provide the get_required_mask unconditionally in struct dma_map_ops
>   2) default to what is the current dma_get_required_mask implementation
>      if nothing else is specified.

Yeah, there's already 17 pointers in dma_map_ops of which about half are 
optional, so these awkward #ifdefs to save one more probably aren't 
worth the inconsistency they bring. It feels like this guy mostly goes 
hand-in-hand with dma_supported, so ack to giving it the same look and feel.

> What I still had on my todo list but not done yet:
> 
>   3) go through all instances and check if the current default
>      makes sense, at it based on direct addressability.  For most
>      iommu instances it seems like we should just return a 64-bit mask.

That's reasonable, although in many cases we should know the effective 
IOMMU input address size which would be even neater.

>   4) figure out how to take the dma offsets into account for it

AFAICS it might boil down to simply:

	mask = roundup_pow_of_two(phys_to_dma(dev, PFN_PHYS(max_pfn))) - 1;

>   5) move the function to the dma-direct code, as that is where it
>      belongs
>   5) figure out if there is a better name for the method, as with
>      swiotlb & co it isn't really the required mask, but more something
>      like the optimal mask
>   6) document the whole thing..
>   7) sort out the powerpc indirection mess.
> 
> Do you agree with that general plan?  If so I can dust off my old
> patch.

Sounds good; in the meantime I'll happily drop these two.

Robin.

Re: [RFC PATCH 2/2] dma-mapping: Clean up dma_get_required_mask() hooks

[Christoph Hellwig]

On Tue, Jul 10, 2018 at 01:29:20PM +0100, Robin Murphy wrote:
>> What I've done is to:
>>
>>   1) provide the get_required_mask unconditionally in struct dma_map_ops
>>   2) default to what is the current dma_get_required_mask implementation
>>      if nothing else is specified.
>
> Yeah, there's already 17 pointers in dma_map_ops of which about half are 
> optional, so these awkward #ifdefs to save one more probably aren't worth 
> the inconsistency they bring. It feels like this guy mostly goes 
> hand-in-hand with dma_supported, so ack to giving it the same look and 
> feel.

This whole area needs a major refactoring - we currentl have three
different APIs to deal with addressability: dma_get_required_mask,
dma_capable/dma_set_mask and dma_capable from dma-direct.h, and there
is plenty of unexplainable mismatches between them.

Sorting this out has been on my TODO list, but I think it can only
effectively be done once the direct mapping implementations are
reasonably consolidated.

>> What I still had on my todo list but not done yet:
>>
>>   3) go through all instances and check if the current default
>>      makes sense, at it based on direct addressability.  For most
>>      iommu instances it seems like we should just return a 64-bit mask.
>
> That's reasonable, although in many cases we should know the effective 
> IOMMU input address size which would be even neater.

Sure.  Maybe I just need to steps 1 and 2 and let maintainers fill
in.

>>   4) figure out how to take the dma offsets into account for it
>
> AFAICS it might boil down to simply:
>
> 	mask = roundup_pow_of_two(phys_to_dma(dev, PFN_PHYS(max_pfn))) - 1;

That looks way to sensible.  Which reminds me that I need to research
the history behind the low_totalram/high_totalram magic in
dma_get_required_mask.

Re: [RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

[Christoph Hellwig]

On Wed, Jul 04, 2018 at 06:50:11PM +0100, Robin Murphy wrote:
> Arch-specific implementions for dma_set_{coherent_,}mask() currently
> rely on an inconsistent mix of arch-defined Kconfig symbols and macro
> overrides. Now that we have a nice centralised home for DMA API gubbins,
> let's consolidate these loose ends under consistent config options.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> Here's hoping the buildbot comes by to point out what I've inevitably
> missed, although I did check a cursory cross-compile of ppc64_defconfig
> to iron out the obvious howlers.

The patch looks sensible to me, although I was hoping to get rid of these
hooks in this or the next merge window as they are a horrible bad idea.

> The motivation here is that I'm looking at adding set_mask overrides
> for arm64, and having discovered a bit of a mess it seemed prudent to
> clean up before ingraining it any more.

What are you trying to do?  I really don't want to see more users of
the hooks as they are are a horribly bad idea.

Re: [RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

[Robin Murphy]

On 05/07/18 20:37, Christoph Hellwig wrote:
> On Wed, Jul 04, 2018 at 06:50:11PM +0100, Robin Murphy wrote:
>> Arch-specific implementions for dma_set_{coherent_,}mask() currently
>> rely on an inconsistent mix of arch-defined Kconfig symbols and macro
>> overrides. Now that we have a nice centralised home for DMA API gubbins,
>> let's consolidate these loose ends under consistent config options.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> Here's hoping the buildbot comes by to point out what I've inevitably
>> missed, although I did check a cursory cross-compile of ppc64_defconfig
>> to iron out the obvious howlers.
> 
> The patch looks sensible to me, although I was hoping to get rid of these
> hooks in this or the next merge window as they are a horrible bad idea.
> 
>> The motivation here is that I'm looking at adding set_mask overrides
>> for arm64, and having discovered a bit of a mess it seemed prudent to
>> clean up before ingraining it any more.
> 
> What are you trying to do?  I really don't want to see more users of
> the hooks as they are are a horribly bad idea.

I really need to fix the ongoing problem we have where, due to funky 
integrations, devices suffer some downstream addressing limit (described 
by DT dma-ranges or ACPI IORT/_DMA) which we carefully set up in 
dma_configure(), but then just gets lost when the driver probes and 
innocently calls dma_set_mask() with something wider. I think it's 
effectively the generalised case of the VIA 32-bit quirk, if I 
understand that one correctly.

The approach that seemed to me to be safest is largely based on the one 
proposed in a thread from ages ago[1]; namely to make dma_configure() 
better at distinguishing firmware-specified masks from bus defaults, 
capture the firmware mask in dev->archdata during arch_setup_dma_ops(), 
then use the custom set_mask routines to ensure any subsequent updates 
never exceed that. It doesn't seem possible to make this work robustly 
without storing *some* additional per-device data, and for that archdata 
is a lesser evil than struct device itself. Plus even though it's not 
actually an arch-specific issue it feels like there's such a risk of 
breaking other platforms that I'm reticent to even try handling it 
entirely in generic code.

Robin.

[1] 
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1306507.html

Re: [RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

[Christoph Hellwig]

On Fri, Jul 06, 2018 at 03:20:34PM +0100, Robin Murphy wrote:
>> What are you trying to do?  I really don't want to see more users of
>> the hooks as they are are a horribly bad idea.
>
> I really need to fix the ongoing problem we have where, due to funky 
> integrations, devices suffer some downstream addressing limit (described by 
> DT dma-ranges or ACPI IORT/_DMA) which we carefully set up in 
> dma_configure(), but then just gets lost when the driver probes and 
> innocently calls dma_set_mask() with something wider. I think it's 
> effectively the generalised case of the VIA 32-bit quirk, if I understand 
> that one correctly.

I'd much rather fix this in generic code.  How funky are your limitations?
In fact when I did the 32-bit quirk (which will also be used by a Xiling
PCIe root port usable on a lot of architectures) I did initially consider
adding a bus_dma_mask or similar to struct device, but opted for the
most simple implementation for now.  I'd be happy to chanfe this.

Especially these days where busses and IP blocks are generally not tied
to a specific cpu instruction set I really believe that having any
more architecture code than absolutely required is a bad idea.

> The approach that seemed to me to be safest is largely based on the one 
> proposed in a thread from ages ago[1]; namely to make dma_configure() 
> better at distinguishing firmware-specified masks from bus defaults, 
> capture the firmware mask in dev->archdata during arch_setup_dma_ops(), 
> then use the custom set_mask routines to ensure any subsequent updates 
> never exceed that. It doesn't seem possible to make this work robustly 
> without storing *some* additional per-device data, and for that archdata is 
> a lesser evil than struct device itself. Plus even though it's not actually 
> an arch-specific issue it feels like there's such a risk of breaking other 
> platforms that I'm reticent to even try handling it entirely in generic 
> code.

My plan for a few merge windows from now is that dma_mask and
coherent_mask are 100% in device control and dma_set_mask will never
fail.  It will be up to the dma ops to make sure things are addressible.

Re: [RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

[Robin Murphy]

On 08/07/18 16:07, Christoph Hellwig wrote:
> On Fri, Jul 06, 2018 at 03:20:34PM +0100, Robin Murphy wrote:
>>> What are you trying to do?  I really don't want to see more users of
>>> the hooks as they are are a horribly bad idea.
>>
>> I really need to fix the ongoing problem we have where, due to funky
>> integrations, devices suffer some downstream addressing limit (described by
>> DT dma-ranges or ACPI IORT/_DMA) which we carefully set up in
>> dma_configure(), but then just gets lost when the driver probes and
>> innocently calls dma_set_mask() with something wider. I think it's
>> effectively the generalised case of the VIA 32-bit quirk, if I understand
>> that one correctly.
> 
> I'd much rather fix this in generic code.  How funky are your limitations?
> In fact when I did the 32-bit quirk (which will also be used by a Xiling
> PCIe root port usable on a lot of architectures) I did initially consider
> adding a bus_dma_mask or similar to struct device, but opted for the
> most simple implementation for now.  I'd be happy to chanfe this.
> 
> Especially these days where busses and IP blocks are generally not tied
> to a specific cpu instruction set I really believe that having any
> more architecture code than absolutely required is a bad idea.

Oh, for sure, the generic fix would be the longer-term goal, this was 
just an expedient compromise because I want to get *something* landed 
for 4.19. Since in practice this is predominantly affecting arm64, doing 
the arch-specific fix to appease affected customers then working to 
generalise it afterwards seemed to carry the lowest risk.

That said, I think I can see a relatively safe and clean alternative 
approach based on converting dma_32bit_limit to a mask, so I'll spin 
some patches around that idea ASAP to continue the discussion.

>> The approach that seemed to me to be safest is largely based on the one
>> proposed in a thread from ages ago[1]; namely to make dma_configure()
>> better at distinguishing firmware-specified masks from bus defaults,
>> capture the firmware mask in dev->archdata during arch_setup_dma_ops(),
>> then use the custom set_mask routines to ensure any subsequent updates
>> never exceed that. It doesn't seem possible to make this work robustly
>> without storing *some* additional per-device data, and for that archdata is
>> a lesser evil than struct device itself. Plus even though it's not actually
>> an arch-specific issue it feels like there's such a risk of breaking other
>> platforms that I'm reticent to even try handling it entirely in generic
>> code.
> 
> My plan for a few merge windows from now is that dma_mask and
> coherent_mask are 100% in device control and dma_set_mask will never
> fail.  It will be up to the dma ops to make sure things are addressible.

It's entirely possible to plug an old PCI soundcard via a bridge adapter 
into a modern board where the card's 24-bit DMA mask reaches nothing but 
the SoC's boot flash, and no IOMMU is available (e.g. some of the 
smaller NXP Layercape stuff); I still think there should be an error in 
such rare cases when DMA is utterly impossible, but otherwise I agree it 
would be much nicer for drivers to just provide their preferred mask and 
let the ops massage it as necessary.

Robin.

Re: [RFC PATCH 1/2] dma-mapping: Clean up dma_set_*mask() hooks

[Christoph Hellwig]

On Mon, Jul 09, 2018 at 03:53:50PM +0100, Robin Murphy wrote:
> Oh, for sure, the generic fix would be the longer-term goal, this was just 
> an expedient compromise because I want to get *something* landed for 4.19. 
> Since in practice this is predominantly affecting arm64, doing the 
> arch-specific fix to appease affected customers then working to generalise 
> it afterwards seemed to carry the lowest risk.
>
> That said, I think I can see a relatively safe and clean alternative 
> approach based on converting dma_32bit_limit to a mask, so I'll spin some 
> patches around that idea ASAP to continue the discussion.

Great!  I really want to sort out this area as soon as possible, and
introducing more arch specific code isn't really helping with that.  In
fact my whole drive to consolidate code is based on the fact that
I want to fix issues like this in one code base instead of 20 slightly
different ones.

FYI, this is the Xilinx/RISC-V use case for the 32-bit limit,
which I'll need to respin a bit based on linux-pci feedback:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/xilinx-dma-32

> It's entirely possible to plug an old PCI soundcard via a bridge adapter 
> into a modern board where the card's 24-bit DMA mask reaches nothing but 
> the SoC's boot flash, and no IOMMU is available (e.g. some of the smaller 
> NXP Layercape stuff); I still think there should be an error in such rare 
> cases when DMA is utterly impossible, but otherwise I agree it would be 
> much nicer for drivers to just provide their preferred mask and let the ops 
> massage it as necessary.

Ok, I guess we need still need to keep the return value for that.