| Message ID | 08256121f325ceed7f6b88c1a5d3cf949698787d.1530726467.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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.
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.
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. */
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(-)