Re: [PATCH RESEND v10 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset
On Thu, Aug 20, 2020 at 09:37:12AM -0400, Jim Quinlan wrote:
> On Tue, Aug 18, 2020 at 4:14 AM Andy Shevchenko
> wrote:
> > On Mon, Aug 17, 2020 at 05:53:09PM -0400, Jim Quinlan wrote:
...
> > > +static inline u64 dma_offset_from_dma_addr(struct device *dev,
> > > dma_addr_t dma_addr)
> > > +{
> > > + const struct bus_dma_region *m = dev->dma_range_map;
> > > +
> > > + if (!m)
> > > + return 0;
> > > + for (; m->size; m++)
> > > + if (dma_addr >= m->dma_start && dma_addr - m->dma_start <
> > > m->size)
> > > + return m->offset;
> > > + return 0;
> > > +}
> > > +
> > > +static inline u64 dma_offset_from_phys_addr(struct device *dev,
> > > phys_addr_t paddr)
> > > +{
> > > + const struct bus_dma_region *m = dev->dma_range_map;
> > > +
> > > + if (!m)
> > > + return 0;
> > > + for (; m->size; m++)
> > > + if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
> > > + return m->offset;
> > > + return 0;
> > > +}
> >
> > Perhaps for these the form with one return 0 is easier to read
> >
> > if (m) {
> > for (; m->size; m++)
> > if (paddr >= m->cpu_start && paddr - m->cpu_start <
> > m->size)
> > return m->offset;
> > }
> > return 0;
> >
> > ?
> I see what you are saying but I don't think there is enough difference
> between the two to justify changing it.
The difference is that you have return 0 / non-0 cases one each. I think it's
slightly easier to read and understand, but it's up to you.
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND v10 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset
Hi Anday,
On Tue, Aug 18, 2020 at 4:14 AM Andy Shevchenko
wrote:
>
> On Mon, Aug 17, 2020 at 05:53:09PM -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) {
> > + phys_addr_t paddr = PFN_PHYS(pfn);
> > +
>
> > + pfn -= (dma_offset_from_phys_addr(dev, paddr) >> PAGE_SHIFT);
>
> PFN_DOWN() ?
Yep.
>
> > + }
>
> ...
>
> > + pfn += (dma_offset_from_dma_addr(dev, addr) >> PAGE_SHIFT);
>
> Ditto.
Yep.
>
>
> ...
>
> > +static inline u64 dma_offset_from_dma_addr(struct device *dev, dma_addr_t
> > dma_addr)
> > +{
> > + const struct bus_dma_region *m = dev->dma_range_map;
> > +
> > + if (!m)
> > + return 0;
> > + for (; m->size; m++)
> > + if (dma_addr >= m->dma_start && dma_addr - m->dma_start <
> > m->size)
> > + return m->offset;
> > + return 0;
> > +}
> > +
> > +static inline u64 dma_offset_from_phys_addr(struct device *dev,
> > phys_addr_t paddr)
> > +{
> > + const struct bus_dma_region *m = dev->dma_range_map;
> > +
> > + if (!m)
> > + return 0;
> > + for (; m->size; m++)
> > + if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
> > + return m->offset;
> > + return 0;
> > +}
>
> Perhaps for these the form with one return 0 is easier to read
>
> if (m) {
> for (; m->size; m++)
> if (paddr >= m->cpu_start && paddr - m->cpu_start <
> m->size)
> return m->offset;
> }
> return 0;
>
> ?
I see what you are saying but I don't think there is enough difference
between the two to justify changing it.
>
> ...
>
> > + if (mem->use_dev_dma_pfn_offset) {
> > + u64 base_addr = (u64)mem->pfn_base << PAGE_SHIFT;
>
> PFN_PHYS() ?
Yep.
>
> > +
> > + return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> > + }
>
> ...
>
> > + * It returns -ENOMEM if out of memory, 0 otherwise.
>
> This doesn't describe cases dev->dma_range_map != NULL and offset == 0.
Okay, I'll fix this.
>
> > +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> > + dma_addr_t dma_start, u64 size)
> > +{
> > + struct bus_dma_region *map;
> > + u64 offset = (u64)cpu_start - (u64)dma_start;
> > +
> > + if (!offset)
> > + return 0;
> > +
> > + if (dev->dma_range_map) {
> > + dev_err(dev, "attempt to add DMA range to existing map\n");
> > + return -EINVAL;
> > + }
> > +
> > + map = kcalloc(2, sizeof(*map), GFP_KERNEL);
> > + if (!map)
> > + return -ENOMEM;
> > + map[0].cpu_start = cpu_start;
> > + map[0].dma_start = dma_start;
> > + map[0].offset = offset;
> > + map[0].size = size;
> > + dev->dma_range_map = map;
> > +
> > + return 0;
> > +}
>
> ...
>
> > +void *dma_copy_dma_range_map(const struct bus_dma_region *map)
> > +{
> > + int num_ranges;
> > + struct bus_dma_region *new_map;
> > + const struct bus_dma_region *r = map;
> > +
> > + for (num_ranges = 0; r->size; num_ranges++)
> > + r++;
>
> > + new_map = kcalloc(num_ranges + 1, sizeof(*map), GFP_KERNEL);
> > + if (new_map)
> > + memcpy(new_map, map, sizeof(*map) * num_ranges);
>
> Looks like krealloc() on the first glance...
It's not. We are making a distinct copy of the original, not resizing it.
>
> > +
> > + return new_map;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks again,
Jim
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND v10 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset
On Mon, Aug 17, 2020 at 05:53:09PM -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) {
> + phys_addr_t paddr = PFN_PHYS(pfn);
> +
> + pfn -= (dma_offset_from_phys_addr(dev, paddr) >> PAGE_SHIFT);
PFN_DOWN() ?
> + }
...
> + pfn += (dma_offset_from_dma_addr(dev, addr) >> PAGE_SHIFT);
Ditto.
...
> +static inline u64 dma_offset_from_dma_addr(struct device *dev, dma_addr_t
> dma_addr)
> +{
> + const struct bus_dma_region *m = dev->dma_range_map;
> +
> + if (!m)
> + return 0;
> + for (; m->size; m++)
> + if (dma_addr >= m->dma_start && dma_addr - m->dma_start <
> m->size)
> + return m->offset;
> + return 0;
> +}
> +
> +static inline u64 dma_offset_from_phys_addr(struct device *dev, phys_addr_t
> paddr)
> +{
> + const struct bus_dma_region *m = dev->dma_range_map;
> +
> + if (!m)
> + return 0;
> + for (; m->size; m++)
> + if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
> + return m->offset;
> + return 0;
> +}
Perhaps for these the form with one return 0 is easier to read
if (m) {
for (; m->size; m++)
if (paddr >= m->cpu_start && paddr - m->cpu_start <
m->size)
return m->offset;
}
return 0;
?
...
> + if (mem->use_dev_dma_pfn_offset) {
> + u64 base_addr = (u64)mem->pfn_base << PAGE_SHIFT;
PFN_PHYS() ?
> +
> + return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> + }
...
> + * It returns -ENOMEM if out of memory, 0 otherwise.
This doesn't describe cases dev->dma_range_map != NULL and offset == 0.
> +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> + dma_addr_t dma_start, u64 size)
> +{
> + struct bus_dma_region *map;
> + u64 offset = (u64)cpu_start - (u64)dma_start;
> +
> + if (!offset)
> + return 0;
> +
> + if (dev->dma_range_map) {
> + dev_err(dev, "attempt to add DMA range to existing map\n");
> + return -EINVAL;
> + }
> +
> + map = kcalloc(2, sizeof(*map), GFP_KERNEL);
> + if (!map)
> + return -ENOMEM;
> + map[0].cpu_start = cpu_start;
> + map[0].dma_start = dma_start;
> + map[0].offset = offset;
> + map[0].size = size;
> + dev->dma_range_map = map;
> +
> + return 0;
> +}
...
> +void *dma_copy_dma_range_map(const struct bus_dma_region *map)
> +{
> + int num_ranges;
> + struct bus_dma_region *new_map;
> + const struct bus_dma_region *r = map;
> +
> + for (num_ranges = 0; r->size; num_ranges++)
> + r++;
> + new_map = kcalloc(num_ranges + 1, sizeof(*map), GFP_KERNEL);
> + if (new_map)
> + memcpy(new_map, map, sizeof(*map) * num_ranges);
Looks like krealloc() on the first glance...
> +
> + return new_map;
> +}
--
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH RESEND v10 07/11] device-mapping: 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| 10 +--
arch/arm/mach-keystone/keystone.c | 17 +++--
arch/sh/drivers/pci/pcie-sh7786.c | 9 +--
arch/sh/kernel/dma-coherent.c | 15 ++--
arch/x86/pci/sta2x11-fixup.c | 7 +-
drivers/acpi/arm64/iort.c | 5 +-
drivers/base/core.c | 2 +
drivers/gpu/drm/sun4i/sun4i_backend.c | 5 +-
drivers/iommu/io-pgtable-arm.c| 2 +-
.../platform/sunxi/sun4i-csi/sun4i_csi.c | 5 +-
.../platform/sunxi/sun6i-csi/sun6i_csi.c | 4 +-
drivers/of/address.c | 72 +--
drivers/of/device.c | 43 ++-
drivers/of/of_private.h | 10 +--
drivers/of/unittest.c | 31 +---
drivers/remoteproc/remoteproc_core.c | 8 ++-
.../staging/media/sunxi/cedrus/cedrus_hw.c| 7 +-
drivers/usb/core/message.c| 9 ++-
drivers/usb/core/usb.c| 7 +-
include/linux/device.h| 4 +-
include/linux/dma-direct.h| 8 +--
include/linux/dma-mapping.h | 36 ++
kernel/dma/coherent.c | 10 +--
kernel/dma/mapping.c | 65 +
24 files changed, 269 insertions(+), 122 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h
b/arch/arm/include/asm/dma-mapping.h
index bdd80ddbca34..2405afeb7957 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -35,8 +35,11 @@ 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) {
+ phys_addr_t paddr = PFN_PHYS(pfn);
+
+ pfn -= (dma_offset_from_phys_addr(dev, paddr) >> PAGE_SHIFT);
+ }
return (dma_addr_t)__pfn_to_bus(pfn);
}
@@ -45,8 +48,7 @@ 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;
-
+ pfn += (dma_offset_from_dma_addr(dev, addr) >> PAGE_SHIFT);
return pfn;
}
diff --git a/arch/arm/mach-keystone/keystone.c
b/arch/arm/mach-keystone/keystone.c
index 638808c4e122..78808942ad1c 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_set_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
