Re: [PATCH v6 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
Hi Andy, Sorry for the delay in response. I will do what you suggest in your email. I do have one response to one of your comments below. On Thu, Jul 2, 2020 at 4:43 AM Andy Shevchenko wrote: > > On Wed, Jul 01, 2020 at 05:21:38PM -0400, Jim Quinlan wrote: > > The new field 'dma_range_map' in struct device is used to facilitate the > > use of single or multiple offsets between mapping regions of cpu addrs and > > dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only > > capable of holding a single uniform offset and had no region bounds > > checking. > > > > The function of_dma_get_range() has been modified so that it takes a single > > argument -- the device node -- and returns a map, NULL, or an error code. > > The map is an array that holds the information regarding the DMA regions. > > Each range entry contains the address offset, the cpu_start address, the > > dma_start address, and the size of the region. > > > > of_dma_configure() is the typical manner to set range offsets but there are > > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel > > driver code. These cases now invoke the function > > dma_attach_offset_range(dev, cpu_addr, dma_addr, size). > > ... > > > + if (dev && dev->dma_range_map) > > + pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev, > > PFN_PHYS(pfn))); > > Instead of casting use PHYS_PFN() and it will be consistent with latter in > the same line. > > > + if (dev && dev->dma_range_map) > > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > > addr)); > > Ditto. > > ... > > > + dev_err(dev, "set dma_offset%08llx%s\n", > > KEYSTONE_HIGH_PHYS_START > > + - KEYSTONE_LOW_PHYS_START, ret ? " failed" : ""); > > Please, avoid such indentation. > Better split it to the three lines, argument per line (expect dev which will > go > on the first one). > > This applies to all similar places. > > ... > > > unsigned long pfn = (dma_handle >> PAGE_SHIFT); > > PHYS_PFN() / PFN_DOWN() ? > > > + if (!WARN_ON(!dev) && dev->dma_range_map) > > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > > dma_handle)); > > PHYS_PFN() ? > > ... > > > + r = kcalloc(num_ranges + 1, sizeof(struct bus_dma_region), > > GFP_KERNEL); > > sizeof(*r) ? > > > + if (!r) > > + return ERR_PTR(-ENOMEM); > > ... > > > + ret = IS_ERR(map) ? PTR_ERR(map) : 0; > > PTR_ERR_OR_ZERO() > > ... > > > + /* We want the offset map to be device-managed, so alloc & > > copy */ > > + dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, > > sizeof(*r), > > + GFP_KERNEL); > > The question is how many times per device lifetime this can be called? Someone else questioned this. There are two cases that come to mind: our overnight test which load/unloads the RC driver over and over, and a customer that does an unbind/bind of the RC driver when their EP is hung and wants to restart. Both cases are atypical and are weak arguments to justify the second copy. I will drop the copy. Thanks, Jim Quinlan Broadcom STB > > ... > > > > + if (!dev->dma_range_map) > > + return -ENOMEM; > > + memcpy((void *)dev->dma_range_map, map, sizeof(*r) * > > num_ranges + 1); > > If it's continuous, perhaps kmemdup() ? > > ... > > > + rc = IS_ERR(map) ? PTR_ERR(map) : 0; > > PTR_ERR_OR_ZERO() > > ... > > > + = dma_offset_from_phys_addr(dev, > > PFN_PHYS(mem->pfn_base)); > > + > > + return (dma_addr_t)PFN_PHYS(mem->pfn_base) - dma_offset; > > Looking at this more, I think you need to introduce in the same header (pfn.h) > something like: > > #define PFN_DMA_ADDR() > #define DMA_ADDR_PFN() > > -- > With Best Regards, > Andy Shevchenko > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Wed, Jul 01, 2020 at 05:21:38PM -0400, Jim Quinlan wrote: > The new field 'dma_range_map' in struct device is used to facilitate the > use of single or multiple offsets between mapping regions of cpu addrs and > dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only > capable of holding a single uniform offset and had no region bounds > checking. > > The function of_dma_get_range() has been modified so that it takes a single > argument -- the device node -- and returns a map, NULL, or an error code. > The map is an array that holds the information regarding the DMA regions. > Each range entry contains the address offset, the cpu_start address, the > dma_start address, and the size of the region. > > of_dma_configure() is the typical manner to set range offsets but there are > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel > driver code. These cases now invoke the function > dma_attach_offset_range(dev, cpu_addr, dma_addr, size). ... > + if (dev && dev->dma_range_map) > + pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev, > PFN_PHYS(pfn))); Instead of casting use PHYS_PFN() and it will be consistent with latter in the same line. > + if (dev && dev->dma_range_map) > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > addr)); Ditto. ... > + dev_err(dev, "set dma_offset%08llx%s\n", > KEYSTONE_HIGH_PHYS_START > + - KEYSTONE_LOW_PHYS_START, ret ? " failed" : ""); Please, avoid such indentation. Better split it to the three lines, argument per line (expect dev which will go on the first one). This applies to all similar places. ... > unsigned long pfn = (dma_handle >> PAGE_SHIFT); PHYS_PFN() / PFN_DOWN() ? > + if (!WARN_ON(!dev) && dev->dma_range_map) > + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, > dma_handle)); PHYS_PFN() ? ... > + r = kcalloc(num_ranges + 1, sizeof(struct bus_dma_region), GFP_KERNEL); sizeof(*r) ? > + if (!r) > + return ERR_PTR(-ENOMEM); ... > + ret = IS_ERR(map) ? PTR_ERR(map) : 0; PTR_ERR_OR_ZERO() ... > + /* We want the offset map to be device-managed, so alloc & copy > */ > + dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, > sizeof(*r), > + GFP_KERNEL); The question is how many times per device lifetime this can be called? ... > + if (!dev->dma_range_map) > + return -ENOMEM; > + memcpy((void *)dev->dma_range_map, map, sizeof(*r) * num_ranges > + 1); If it's continuous, perhaps kmemdup() ? ... > + rc = IS_ERR(map) ? PTR_ERR(map) : 0; PTR_ERR_OR_ZERO() ... > + = dma_offset_from_phys_addr(dev, > PFN_PHYS(mem->pfn_base)); > + > + return (dma_addr_t)PFN_PHYS(mem->pfn_base) - dma_offset; Looking at this more, I think you need to introduce in the same header (pfn.h) something like: #define PFN_DMA_ADDR() #define DMA_ADDR_PFN() -- With Best Regards, Andy Shevchenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v6 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
The new field 'dma_range_map' in struct device is used to facilitate the use of single or multiple offsets between mapping regions of cpu addrs and dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only capable of holding a single uniform offset and had no region bounds checking. The function of_dma_get_range() has been modified so that it takes a single argument -- the device node -- and returns a map, NULL, or an error code. The map is an array that holds the information regarding the DMA regions. Each range entry contains the address offset, the cpu_start address, the dma_start address, and the size of the region. of_dma_configure() is the typical manner to set range offsets but there are a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel driver code. These cases now invoke the function dma_attach_offset_range(dev, cpu_addr, dma_addr, size). Signed-off-by: Jim Quinlan --- arch/arm/include/asm/dma-mapping.h| 9 +- arch/arm/mach-keystone/keystone.c | 17 ++-- arch/sh/drivers/pci/pcie-sh7786.c | 9 +- arch/sh/kernel/dma-coherent.c | 14 +-- arch/x86/pci/sta2x11-fixup.c | 7 +- drivers/acpi/arm64/iort.c | 5 +- drivers/gpu/drm/sun4i/sun4i_backend.c | 7 +- drivers/iommu/io-pgtable-arm.c| 2 +- .../platform/sunxi/sun4i-csi/sun4i_csi.c | 6 +- .../platform/sunxi/sun6i-csi/sun6i_csi.c | 5 +- drivers/of/address.c | 95 ++- drivers/of/device.c | 50 ++ drivers/of/of_private.h | 9 +- drivers/of/unittest.c | 35 +-- drivers/remoteproc/remoteproc_core.c | 2 +- .../staging/media/sunxi/cedrus/cedrus_hw.c| 8 +- drivers/usb/core/message.c| 4 +- drivers/usb/core/usb.c| 2 +- include/linux/device.h| 4 +- include/linux/dma-direct.h| 10 +- include/linux/dma-mapping.h | 37 kernel/dma/coherent.c | 11 ++- kernel/dma/mapping.c | 53 +++ 23 files changed, 279 insertions(+), 122 deletions(-) diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index bdd80ddbca34..b6796383f205 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -35,8 +35,9 @@ static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus) #ifndef __arch_pfn_to_dma static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn) { - if (dev) - pfn -= dev->dma_pfn_offset; + if (dev && dev->dma_range_map) + pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev, PFN_PHYS(pfn))); + return (dma_addr_t)__pfn_to_bus(pfn); } @@ -44,8 +45,8 @@ static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr) { unsigned long pfn = __bus_to_pfn(addr); - if (dev) - pfn += dev->dma_pfn_offset; + if (dev && dev->dma_range_map) + pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, addr)); return pfn; } diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c index 638808c4e122..a499ece6f054 100644 --- a/arch/arm/mach-keystone/keystone.c +++ b/arch/arm/mach-keystone/keystone.c @@ -8,6 +8,7 @@ */ #include #include +#include #include #include #include @@ -24,8 +25,6 @@ #include "keystone.h" -static unsigned long keystone_dma_pfn_offset __read_mostly; - static int keystone_platform_notifier(struct notifier_block *nb, unsigned long event, void *data) { @@ -38,9 +37,12 @@ static int keystone_platform_notifier(struct notifier_block *nb, return NOTIFY_BAD; if (!dev->of_node) { - dev->dma_pfn_offset = keystone_dma_pfn_offset; - dev_err(dev, "set dma_pfn_offset%08lx\n", - dev->dma_pfn_offset); + int ret = dma_attach_offset_range(dev, KEYSTONE_HIGH_PHYS_START, + KEYSTONE_LOW_PHYS_START, + KEYSTONE_HIGH_PHYS_SIZE); + dev_err(dev, "set dma_offset%08llx%s\n", KEYSTONE_HIGH_PHYS_START + - KEYSTONE_LOW_PHYS_START, ret ? " failed" : ""); + } return NOTIFY_OK; } @@ -51,11 +53,8 @@ static struct notifier_block platform_nb = { static void __init keystone_init(void) { - if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) { - keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START - - KEYSTONE_LOW_PHYS_START); + if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) bus_