* [PATCH V4 0/2] DMA: update acpi_dma_get_range to return dma map regions
@ 2022-09-09 9:28 Jianmin Lv
2022-09-09 9:28 ` [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv
2022-09-09 9:28 ` [PATCH V4 2/2] LoongArch: Use acpi_arch_dma_setup() and remove ARCH_HAS_PHYS_TO_DMA Jianmin Lv
0 siblings, 2 replies; 6+ messages in thread
From: Jianmin Lv @ 2022-09-09 9:28 UTC (permalink / raw)
To: lpieralisi, robin.murphy, chenhuacai
Cc: guohanjun, sudeep.holla, rafael, lenb, robert.moore,
linux-kernel, linux-acpi, loongarch
The patch series changed acpi_dma_get_range to return dma regions
as of_dma_get_range, so that dev->dma_range_map can be initialized
conveniently.
And acpi_arch_dma_setup for ARM64 is changed wih removing dma_base
and size from it's parameters.
Remove ARCH_HAS_PHYS_TO_DMA for LoongArch and use generic
phys_to_dma/dma_to_phys in include/linux/dma-direct.h.
V1 -> V2
- Removed dma_base and size from acpi_arch_dma_setup' parameters
- Add patch to remove ARCH_HAS_PHYS_TO_DMA for LoongArch
V2 -> V3
- Add kerneldoc for acpi_dma_get_range changing
- Remove redundant code in acpi_arch_dma_setup, and check map
V3 -> V4
- Change title to "Use acpi_arch_dma_setup() and remove ARCH_HAS_PHYS_TO_DMA"
- Use resource_size() to get size
Jianmin Lv (2):
ACPI / scan: Support multiple dma windows with different offsets
LoongArch: Use acpi_arch_dma_setup() and remove ARCH_HAS_PHYS_TO_DMA
arch/loongarch/Kconfig | 1 -
arch/loongarch/kernel/dma.c | 52 ++++++++++++++--------------------
arch/loongarch/kernel/setup.c | 2 +-
drivers/acpi/arm64/dma.c | 29 +++++++++++--------
drivers/acpi/scan.c | 53 +++++++++++++++--------------------
include/acpi/acpi_bus.h | 3 +-
include/linux/acpi.h | 12 ++++----
7 files changed, 71 insertions(+), 81 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets
2022-09-09 9:28 [PATCH V4 0/2] DMA: update acpi_dma_get_range to return dma map regions Jianmin Lv
@ 2022-09-09 9:28 ` Jianmin Lv
2022-09-09 11:19 ` Lorenzo Pieralisi
2022-09-09 9:28 ` [PATCH V4 2/2] LoongArch: Use acpi_arch_dma_setup() and remove ARCH_HAS_PHYS_TO_DMA Jianmin Lv
1 sibling, 1 reply; 6+ messages in thread
From: Jianmin Lv @ 2022-09-09 9:28 UTC (permalink / raw)
To: lpieralisi, robin.murphy, chenhuacai
Cc: guohanjun, sudeep.holla, rafael, lenb, robert.moore,
linux-kernel, linux-acpi, loongarch
For DT, of_dma_get_range returns bus_dma_region typed dma regions,
which makes multiple dma windows with different offset available
for translation between dma address and cpu address.
But for ACPI, acpi_dma_get_range doesn't return similar dma regions,
causing no path for setting dev->dma_range_map conveniently. So the
patch changes acpi_dma_get_range and returns bus_dma_region typed
dma regions according to of_dma_get_range.
After changing acpi_dma_get_range, acpi_arch_dma_setup is changed for
ARM64, where original dma_addr and size are removed as these
arguments are now redundant, and pass 0 and U64_MAX for dma_base
and size of arch_setup_dma_ops, so this is a simplification consistent
with what other ACPI architectures also pass to iommu_setup_dma_ops().
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
---
drivers/acpi/arm64/dma.c | 29 +++++++++++++---------
drivers/acpi/scan.c | 53 +++++++++++++++++-----------------------
include/acpi/acpi_bus.h | 3 +--
include/linux/acpi.h | 7 +++---
4 files changed, 45 insertions(+), 47 deletions(-)
diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
index f16739ad3cc0..1ef8e7ded4cd 100644
--- a/drivers/acpi/arm64/dma.c
+++ b/drivers/acpi/arm64/dma.c
@@ -4,11 +4,12 @@
#include <linux/device.h>
#include <linux/dma-direct.h>
-void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
+void acpi_arch_dma_setup(struct device *dev)
{
int ret;
u64 end, mask;
- u64 dmaaddr = 0, size = 0, offset = 0;
+ u64 size = 0;
+ const struct bus_dma_region *map = NULL;
/*
* If @dev is expected to be DMA-capable then the bus code that created
@@ -26,25 +27,31 @@ void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
else
size = 1ULL << 32;
- ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size);
+ ret = acpi_dma_get_range(dev, &map);
+ if (!ret && map) {
+ const struct bus_dma_region *r = map;
+
+ for (end = 0; r->size; r++) {
+ if (r->dma_start + r->size - 1 > end)
+ end = r->dma_start + r->size - 1;
+ }
+
+ size = end + 1;
+ dev->dma_range_map = map;
+ }
+
if (ret == -ENODEV)
ret = iort_dma_get_ranges(dev, &size);
+
if (!ret) {
/*
* Limit coherent and dma mask based on size retrieved from
* firmware.
*/
- end = dmaaddr + size - 1;
+ end = size - 1;
mask = DMA_BIT_MASK(ilog2(end) + 1);
dev->bus_dma_limit = end;
dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
*dev->dma_mask = min(*dev->dma_mask, mask);
}
-
- *dma_addr = dmaaddr;
- *dma_size = size;
-
- ret = dma_direct_set_offset(dev, dmaaddr + offset, dmaaddr, size);
-
- dev_dbg(dev, "dma_offset(%#08llx)%s\n", offset, ret ? " failed!" : "");
}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 42cec8120f18..b4505ec467fe 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -20,6 +20,7 @@
#include <linux/platform_data/x86/apple.h>
#include <linux/pgtable.h>
#include <linux/crc32.h>
+#include <linux/dma-direct.h>
#include "internal.h"
@@ -1467,25 +1468,21 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
* acpi_dma_get_range() - Get device DMA parameters.
*
* @dev: device to configure
- * @dma_addr: pointer device DMA address result
- * @offset: pointer to the DMA offset result
- * @size: pointer to DMA range size result
+ * @map: pointer to DMA ranges result
*
- * Evaluate DMA regions and return respectively DMA region start, offset
- * and size in dma_addr, offset and size on parsing success; it does not
- * update the passed in values on failure.
+ * Evaluate DMA regions and return pointer to DMA regions on
+ * parsing success; it does not update the passed in values on failure.
*
* Return 0 on success, < 0 on failure.
*/
-int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
- u64 *size)
+int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
{
struct acpi_device *adev;
LIST_HEAD(list);
struct resource_entry *rentry;
int ret;
struct device *dma_dev = dev;
- u64 len, dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
+ struct bus_dma_region *r;
/*
* Walk the device tree chasing an ACPI companion with a _DMA
@@ -1510,31 +1507,28 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
ret = acpi_dev_get_dma_resources(adev, &list);
if (ret > 0) {
+ r = kcalloc(ret + 1, sizeof(*r), GFP_KERNEL);
+ if (!r) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ *map = r;
+
list_for_each_entry(rentry, &list, node) {
- if (dma_offset && rentry->offset != dma_offset) {
+ if (rentry->res->start >= rentry->res->end) {
ret = -EINVAL;
- dev_warn(dma_dev, "Can't handle multiple windows with different offsets\n");
+ dev_dbg(dma_dev, "Invalid DMA regions configuration\n");
goto out;
}
- dma_offset = rentry->offset;
- /* Take lower and upper limits */
- if (rentry->res->start < dma_start)
- dma_start = rentry->res->start;
- if (rentry->res->end > dma_end)
- dma_end = rentry->res->end;
- }
-
- if (dma_start >= dma_end) {
- ret = -EINVAL;
- dev_dbg(dma_dev, "Invalid DMA regions configuration\n");
- goto out;
+ r->cpu_start = rentry->res->start;
+ r->dma_start = rentry->res->start - rentry->offset;
+ r->size = resource_size(rentry->res);
+ r->offset = rentry->offset;
+ r++;
}
- *dma_addr = dma_start - dma_offset;
- len = dma_end - dma_start;
- *size = max(len, len + 1);
- *offset = dma_offset;
}
out:
acpi_dev_free_resource_list(&list);
@@ -1624,20 +1618,19 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
const u32 *input_id)
{
const struct iommu_ops *iommu;
- u64 dma_addr = 0, size = 0;
if (attr == DEV_DMA_NOT_SUPPORTED) {
set_dma_ops(dev, &dma_dummy_ops);
return 0;
}
- acpi_arch_dma_setup(dev, &dma_addr, &size);
+ acpi_arch_dma_setup(dev);
iommu = acpi_iommu_configure_id(dev, input_id);
if (PTR_ERR(iommu) == -EPROBE_DEFER)
return -EPROBE_DEFER;
- arch_setup_dma_ops(dev, dma_addr, size,
+ arch_setup_dma_ops(dev, 0, U64_MAX,
iommu, attr == DEV_DMA_COHERENT);
return 0;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index e7d27373ff71..73ac4a1d6947 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -613,8 +613,7 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
int acpi_iommu_fwspec_init(struct device *dev, u32 id,
struct fwnode_handle *fwnode,
const struct iommu_ops *ops);
-int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
- u64 *size);
+int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map);
int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
const u32 *input_id);
static inline int acpi_dma_configure(struct device *dev,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6f64b2f3dc54..bb41623dab77 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -281,12 +281,12 @@ void acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa);
#ifdef CONFIG_ARM64
void acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa);
-void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size);
+void acpi_arch_dma_setup(struct device *dev);
#else
static inline void
acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) { }
static inline void
-acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) { }
+acpi_arch_dma_setup(struct device *dev) { }
#endif
int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma);
@@ -977,8 +977,7 @@ static inline enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
return DEV_DMA_NOT_SUPPORTED;
}
-static inline int acpi_dma_get_range(struct device *dev, u64 *dma_addr,
- u64 *offset, u64 *size)
+static inline int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
{
return -ENODEV;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH V4 2/2] LoongArch: Use acpi_arch_dma_setup() and remove ARCH_HAS_PHYS_TO_DMA
2022-09-09 9:28 [PATCH V4 0/2] DMA: update acpi_dma_get_range to return dma map regions Jianmin Lv
2022-09-09 9:28 ` [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv
@ 2022-09-09 9:28 ` Jianmin Lv
1 sibling, 0 replies; 6+ messages in thread
From: Jianmin Lv @ 2022-09-09 9:28 UTC (permalink / raw)
To: lpieralisi, robin.murphy, chenhuacai
Cc: guohanjun, sudeep.holla, rafael, lenb, robert.moore,
linux-kernel, linux-acpi, loongarch
Use _DMA defined in ACPI spec for translation between
DMA address and CPU address, and implement acpi_arch_dma_setup
for initializing dev->dma_range_map, where acpi_dma_get_range
is called for parsing _DMA.
e.g.
If we have two dma ranges:
cpu address dma address size offset
0x200080000000 0x2080000000 0x400000000 0x1fe000000000
0x400080000000 0x4080000000 0x400000000 0x3fc000000000
_DMA for pci devices should be declared in host bridge as
flowing:
Name (_DMA, ResourceTemplate() {
QWordMemory (ResourceProducer,
PosDecode,
MinFixed,
MaxFixed,
NonCacheable,
ReadWrite,
0x0,
0x4080000000,
0x447fffffff,
0x3fc000000000,
0x400000000,
,
,
)
QWordMemory (ResourceProducer,
PosDecode,
MinFixed,
MaxFixed,
NonCacheable,
ReadWrite,
0x0,
0x2080000000,
0x247fffffff,
0x1fe000000000,
0x400000000,
,
,
)
})
Acked-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
---
arch/loongarch/Kconfig | 1 -
arch/loongarch/kernel/dma.c | 52 ++++++++++++++---------------------
arch/loongarch/kernel/setup.c | 2 +-
include/linux/acpi.h | 9 ++++--
4 files changed, 28 insertions(+), 36 deletions(-)
diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index c7dd6ad779af..551dd99e98b8 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -10,7 +10,6 @@ config LOONGARCH
select ARCH_ENABLE_MEMORY_HOTPLUG
select ARCH_ENABLE_MEMORY_HOTREMOVE
select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
- select ARCH_HAS_PHYS_TO_DMA
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_INLINE_READ_LOCK if !PREEMPTION
diff --git a/arch/loongarch/kernel/dma.c b/arch/loongarch/kernel/dma.c
index 8c9b5314a13e..7a9c6a9dd2d0 100644
--- a/arch/loongarch/kernel/dma.c
+++ b/arch/loongarch/kernel/dma.c
@@ -2,39 +2,29 @@
/*
* Copyright (C) 2020-2022 Loongson Technology Corporation Limited
*/
-#include <linux/init.h>
+#include <linux/acpi.h>
#include <linux/dma-direct.h>
-#include <linux/dma-mapping.h>
-#include <linux/dma-map-ops.h>
-#include <linux/swiotlb.h>
-#include <asm/bootinfo.h>
-#include <asm/dma.h>
-#include <asm/loongson.h>
-
-/*
- * We extract 4bit node id (bit 44~47) from Loongson-3's
- * 48bit physical address space and embed it into 40bit.
- */
-
-static int node_id_offset;
-
-dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
-{
- long nid = (paddr >> 44) & 0xf;
-
- return ((nid << 44) ^ paddr) | (nid << node_id_offset);
-}
-
-phys_addr_t dma_to_phys(struct device *dev, dma_addr_t daddr)
+void acpi_arch_dma_setup(struct device *dev)
{
- long nid = (daddr >> node_id_offset) & 0xf;
+ int ret;
+ u64 mask, end = 0;
+ const struct bus_dma_region *map = NULL;
+
+ ret = acpi_dma_get_range(dev, &map);
+ if (!ret && map) {
+ const struct bus_dma_region *r = map;
+
+ for (end = 0; r->size; r++) {
+ if (r->dma_start + r->size - 1 > end)
+ end = r->dma_start + r->size - 1;
+ }
+
+ mask = DMA_BIT_MASK(ilog2(end) + 1);
+ dev->bus_dma_limit = end;
+ dev->dma_range_map = map;
+ dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
+ *dev->dma_mask = min(*dev->dma_mask, mask);
+ }
- return ((nid << node_id_offset) ^ daddr) | (nid << 44);
-}
-
-void __init plat_swiotlb_setup(void)
-{
- swiotlb_init(true, SWIOTLB_VERBOSE);
- node_id_offset = ((readl(LS7A_DMA_CFG) & LS7A_DMA_NODE_MASK) >> LS7A_DMA_NODE_SHF) + 36;
}
diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
index 8f5c2f9a1a83..d97c69dbe553 100644
--- a/arch/loongarch/kernel/setup.c
+++ b/arch/loongarch/kernel/setup.c
@@ -247,7 +247,7 @@ static void __init arch_mem_init(char **cmdline_p)
sparse_init();
memblock_set_bottom_up(true);
- plat_swiotlb_setup();
+ swiotlb_init(true, SWIOTLB_VERBOSE);
dma_contiguous_reserve(PFN_PHYS(max_low_pfn));
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index bb41623dab77..a71d73a0d43e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -279,14 +279,17 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa) { }
void acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa);
+#if defined(CONFIG_ARM64) || defined(CONFIG_LOONGARCH)
+void acpi_arch_dma_setup(struct device *dev);
+#else
+static inline void acpi_arch_dma_setup(struct device *dev) { }
+#endif
+
#ifdef CONFIG_ARM64
void acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa);
-void acpi_arch_dma_setup(struct device *dev);
#else
static inline void
acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) { }
-static inline void
-acpi_arch_dma_setup(struct device *dev) { }
#endif
int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma);
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets
2022-09-09 9:28 ` [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv
@ 2022-09-09 11:19 ` Lorenzo Pieralisi
2022-09-09 11:48 ` Robin Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Pieralisi @ 2022-09-09 11:19 UTC (permalink / raw)
To: Jianmin Lv
Cc: robin.murphy, chenhuacai, guohanjun, sudeep.holla, rafael, lenb,
robert.moore, linux-kernel, linux-acpi, loongarch
On Fri, Sep 09, 2022 at 05:28:10PM +0800, Jianmin Lv wrote:
> For DT, of_dma_get_range returns bus_dma_region typed dma regions,
> which makes multiple dma windows with different offset available
> for translation between dma address and cpu address.
>
> But for ACPI, acpi_dma_get_range doesn't return similar dma regions,
> causing no path for setting dev->dma_range_map conveniently. So the
> patch changes acpi_dma_get_range and returns bus_dma_region typed
> dma regions according to of_dma_get_range.
>
> After changing acpi_dma_get_range, acpi_arch_dma_setup is changed for
> ARM64, where original dma_addr and size are removed as these
> arguments are now redundant, and pass 0 and U64_MAX for dma_base
> and size of arch_setup_dma_ops, so this is a simplification consistent
> with what other ACPI architectures also pass to iommu_setup_dma_ops().
How about this commit log:
"In DT systems configurations, of_dma_get_range() returns struct
bus_dma_region DMA regions; they are used to set-up devices
DMA windows with different offset available for translation between DMA
address and CPU address.
In ACPI systems configuration, acpi_dma_get_range() does not return
DMA regions yet and that precludes setting up the dev->dma_range_map
pointer and therefore DMA regions with multiple offsets.
Update acpi_dma_get_range() to return struct bus_dma_region
DMA regions like of_dma_get_range() does.
After updating acpi_dma_get_range(), acpi_arch_dma_setup() is changed for
ARM64, where the original dma_addr and size are removed as these
arguments are now redundant, and pass 0 and U64_MAX for dma_base
and size of arch_setup_dma_ops; this is a simplification consistent
with what other ACPI architectures also pass to iommu_setup_dma_ops()."
>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> ---
> drivers/acpi/arm64/dma.c | 29 +++++++++++++---------
> drivers/acpi/scan.c | 53 +++++++++++++++++-----------------------
> include/acpi/acpi_bus.h | 3 +--
> include/linux/acpi.h | 7 +++---
> 4 files changed, 45 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
> index f16739ad3cc0..1ef8e7ded4cd 100644
> --- a/drivers/acpi/arm64/dma.c
> +++ b/drivers/acpi/arm64/dma.c
> @@ -4,11 +4,12 @@
> #include <linux/device.h>
> #include <linux/dma-direct.h>
>
> -void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
> +void acpi_arch_dma_setup(struct device *dev)
> {
> int ret;
> u64 end, mask;
> - u64 dmaaddr = 0, size = 0, offset = 0;
> + u64 size = 0;
> + const struct bus_dma_region *map = NULL;
>
> /*
> * If @dev is expected to be DMA-capable then the bus code that created
> @@ -26,25 +27,31 @@ void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
> else
> size = 1ULL << 32;
>
> - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size);
> + ret = acpi_dma_get_range(dev, &map);
> + if (!ret && map) {
> + const struct bus_dma_region *r = map;
> +
> + for (end = 0; r->size; r++) {
> + if (r->dma_start + r->size - 1 > end)
> + end = r->dma_start + r->size - 1;
> + }
> +
> + size = end + 1;
> + dev->dma_range_map = map;
> + }
> +
> if (ret == -ENODEV)
> ret = iort_dma_get_ranges(dev, &size);
> +
Nit: this is a spurious line change.
> if (!ret) {
> /*
> * Limit coherent and dma mask based on size retrieved from
> * firmware.
> */
> - end = dmaaddr + size - 1;
> + end = size - 1;
> mask = DMA_BIT_MASK(ilog2(end) + 1);
> dev->bus_dma_limit = end;
> dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
> *dev->dma_mask = min(*dev->dma_mask, mask);
> }
> -
> - *dma_addr = dmaaddr;
> - *dma_size = size;
> -
> - ret = dma_direct_set_offset(dev, dmaaddr + offset, dmaaddr, size);
> -
> - dev_dbg(dev, "dma_offset(%#08llx)%s\n", offset, ret ? " failed!" : "");
> }
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 42cec8120f18..b4505ec467fe 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -20,6 +20,7 @@
> #include <linux/platform_data/x86/apple.h>
> #include <linux/pgtable.h>
> #include <linux/crc32.h>
> +#include <linux/dma-direct.h>
>
> #include "internal.h"
>
> @@ -1467,25 +1468,21 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
> * acpi_dma_get_range() - Get device DMA parameters.
> *
> * @dev: device to configure
> - * @dma_addr: pointer device DMA address result
> - * @offset: pointer to the DMA offset result
> - * @size: pointer to DMA range size result
> + * @map: pointer to DMA ranges result
> *
> - * Evaluate DMA regions and return respectively DMA region start, offset
> - * and size in dma_addr, offset and size on parsing success; it does not
> - * update the passed in values on failure.
> + * Evaluate DMA regions and return pointer to DMA regions on
> + * parsing success; it does not update the passed in values on failure.
> *
> * Return 0 on success, < 0 on failure.
> */
> -int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
> - u64 *size)
> +int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
> {
> struct acpi_device *adev;
> LIST_HEAD(list);
> struct resource_entry *rentry;
> int ret;
> struct device *dma_dev = dev;
> - u64 len, dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
> + struct bus_dma_region *r;
>
> /*
> * Walk the device tree chasing an ACPI companion with a _DMA
> @@ -1510,31 +1507,28 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
>
> ret = acpi_dev_get_dma_resources(adev, &list);
> if (ret > 0) {
> + r = kcalloc(ret + 1, sizeof(*r), GFP_KERNEL);
Why (ret + 1) ?
> + if (!r) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + *map = r;
I don't think that's right. This function can still fail at this stage,
we should not update the *map value yet.
Lorenzo
> +
> list_for_each_entry(rentry, &list, node) {
> - if (dma_offset && rentry->offset != dma_offset) {
> + if (rentry->res->start >= rentry->res->end) {
> ret = -EINVAL;
> - dev_warn(dma_dev, "Can't handle multiple windows with different offsets\n");
> + dev_dbg(dma_dev, "Invalid DMA regions configuration\n");
> goto out;
> }
> - dma_offset = rentry->offset;
>
> - /* Take lower and upper limits */
> - if (rentry->res->start < dma_start)
> - dma_start = rentry->res->start;
> - if (rentry->res->end > dma_end)
> - dma_end = rentry->res->end;
> - }
> -
> - if (dma_start >= dma_end) {
> - ret = -EINVAL;
> - dev_dbg(dma_dev, "Invalid DMA regions configuration\n");
> - goto out;
> + r->cpu_start = rentry->res->start;
> + r->dma_start = rentry->res->start - rentry->offset;
> + r->size = resource_size(rentry->res);
> + r->offset = rentry->offset;
> + r++;
> }
>
> - *dma_addr = dma_start - dma_offset;
> - len = dma_end - dma_start;
> - *size = max(len, len + 1);
> - *offset = dma_offset;
> }
> out:
> acpi_dev_free_resource_list(&list);
> @@ -1624,20 +1618,19 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
> const u32 *input_id)
> {
> const struct iommu_ops *iommu;
> - u64 dma_addr = 0, size = 0;
>
> if (attr == DEV_DMA_NOT_SUPPORTED) {
> set_dma_ops(dev, &dma_dummy_ops);
> return 0;
> }
>
> - acpi_arch_dma_setup(dev, &dma_addr, &size);
> + acpi_arch_dma_setup(dev);
>
> iommu = acpi_iommu_configure_id(dev, input_id);
> if (PTR_ERR(iommu) == -EPROBE_DEFER)
> return -EPROBE_DEFER;
>
> - arch_setup_dma_ops(dev, dma_addr, size,
> + arch_setup_dma_ops(dev, 0, U64_MAX,
> iommu, attr == DEV_DMA_COHERENT);
>
> return 0;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index e7d27373ff71..73ac4a1d6947 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -613,8 +613,7 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
> int acpi_iommu_fwspec_init(struct device *dev, u32 id,
> struct fwnode_handle *fwnode,
> const struct iommu_ops *ops);
> -int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
> - u64 *size);
> +int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map);
> int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
> const u32 *input_id);
> static inline int acpi_dma_configure(struct device *dev,
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 6f64b2f3dc54..bb41623dab77 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -281,12 +281,12 @@ void acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa);
>
> #ifdef CONFIG_ARM64
> void acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa);
> -void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size);
> +void acpi_arch_dma_setup(struct device *dev);
> #else
> static inline void
> acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) { }
> static inline void
> -acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) { }
> +acpi_arch_dma_setup(struct device *dev) { }
> #endif
>
> int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma);
> @@ -977,8 +977,7 @@ static inline enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
> return DEV_DMA_NOT_SUPPORTED;
> }
>
> -static inline int acpi_dma_get_range(struct device *dev, u64 *dma_addr,
> - u64 *offset, u64 *size)
> +static inline int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
> {
> return -ENODEV;
> }
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets
2022-09-09 11:19 ` Lorenzo Pieralisi
@ 2022-09-09 11:48 ` Robin Murphy
2022-09-09 13:08 ` Jianmin Lv
0 siblings, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2022-09-09 11:48 UTC (permalink / raw)
To: Lorenzo Pieralisi, Jianmin Lv
Cc: chenhuacai, guohanjun, sudeep.holla, rafael, lenb, robert.moore,
linux-kernel, linux-acpi, loongarch
On 2022-09-09 12:19, Lorenzo Pieralisi wrote:
> On Fri, Sep 09, 2022 at 05:28:10PM +0800, Jianmin Lv wrote:
>> For DT, of_dma_get_range returns bus_dma_region typed dma regions,
>> which makes multiple dma windows with different offset available
>> for translation between dma address and cpu address.
>>
>> But for ACPI, acpi_dma_get_range doesn't return similar dma regions,
>> causing no path for setting dev->dma_range_map conveniently. So the
>> patch changes acpi_dma_get_range and returns bus_dma_region typed
>> dma regions according to of_dma_get_range.
>>
>> After changing acpi_dma_get_range, acpi_arch_dma_setup is changed for
>> ARM64, where original dma_addr and size are removed as these
>> arguments are now redundant, and pass 0 and U64_MAX for dma_base
>> and size of arch_setup_dma_ops, so this is a simplification consistent
>> with what other ACPI architectures also pass to iommu_setup_dma_ops().
>
> How about this commit log:
>
> "In DT systems configurations, of_dma_get_range() returns struct
> bus_dma_region DMA regions; they are used to set-up devices
> DMA windows with different offset available for translation between DMA
> address and CPU address.
>
> In ACPI systems configuration, acpi_dma_get_range() does not return
> DMA regions yet and that precludes setting up the dev->dma_range_map
> pointer and therefore DMA regions with multiple offsets.
>
> Update acpi_dma_get_range() to return struct bus_dma_region
> DMA regions like of_dma_get_range() does.
>
> After updating acpi_dma_get_range(), acpi_arch_dma_setup() is changed for
> ARM64, where the original dma_addr and size are removed as these
> arguments are now redundant, and pass 0 and U64_MAX for dma_base
> and size of arch_setup_dma_ops; this is a simplification consistent
> with what other ACPI architectures also pass to iommu_setup_dma_ops()."
>
>>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>> ---
>> drivers/acpi/arm64/dma.c | 29 +++++++++++++---------
>> drivers/acpi/scan.c | 53 +++++++++++++++++-----------------------
>> include/acpi/acpi_bus.h | 3 +--
>> include/linux/acpi.h | 7 +++---
>> 4 files changed, 45 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
>> index f16739ad3cc0..1ef8e7ded4cd 100644
>> --- a/drivers/acpi/arm64/dma.c
>> +++ b/drivers/acpi/arm64/dma.c
>> @@ -4,11 +4,12 @@
>> #include <linux/device.h>
>> #include <linux/dma-direct.h>
>>
>> -void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
>> +void acpi_arch_dma_setup(struct device *dev)
>> {
>> int ret;
>> u64 end, mask;
>> - u64 dmaaddr = 0, size = 0, offset = 0;
>> + u64 size = 0;
>> + const struct bus_dma_region *map = NULL;
>>
>> /*
>> * If @dev is expected to be DMA-capable then the bus code that created
>> @@ -26,25 +27,31 @@ void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
>> else
>> size = 1ULL << 32;
>>
>> - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size);
>> + ret = acpi_dma_get_range(dev, &map);
>> + if (!ret && map) {
>> + const struct bus_dma_region *r = map;
>> +
>> + for (end = 0; r->size; r++) {
>> + if (r->dma_start + r->size - 1 > end)
>> + end = r->dma_start + r->size - 1;
>> + }
>> +
>> + size = end + 1;
>> + dev->dma_range_map = map;
>> + }
>> +
>> if (ret == -ENODEV)
>> ret = iort_dma_get_ranges(dev, &size);
>> +
>
> Nit: this is a spurious line change.
>
>> if (!ret) {
>> /*
>> * Limit coherent and dma mask based on size retrieved from
>> * firmware.
>> */
>> - end = dmaaddr + size - 1;
>> + end = size - 1;
>> mask = DMA_BIT_MASK(ilog2(end) + 1);
>> dev->bus_dma_limit = end;
>> dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
>> *dev->dma_mask = min(*dev->dma_mask, mask);
>> }
>> -
>> - *dma_addr = dmaaddr;
>> - *dma_size = size;
>> -
>> - ret = dma_direct_set_offset(dev, dmaaddr + offset, dmaaddr, size);
>> -
>> - dev_dbg(dev, "dma_offset(%#08llx)%s\n", offset, ret ? " failed!" : "");
>> }
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 42cec8120f18..b4505ec467fe 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -20,6 +20,7 @@
>> #include <linux/platform_data/x86/apple.h>
>> #include <linux/pgtable.h>
>> #include <linux/crc32.h>
>> +#include <linux/dma-direct.h>
>>
>> #include "internal.h"
>>
>> @@ -1467,25 +1468,21 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
>> * acpi_dma_get_range() - Get device DMA parameters.
>> *
>> * @dev: device to configure
>> - * @dma_addr: pointer device DMA address result
>> - * @offset: pointer to the DMA offset result
>> - * @size: pointer to DMA range size result
>> + * @map: pointer to DMA ranges result
>> *
>> - * Evaluate DMA regions and return respectively DMA region start, offset
>> - * and size in dma_addr, offset and size on parsing success; it does not
>> - * update the passed in values on failure.
>> + * Evaluate DMA regions and return pointer to DMA regions on
>> + * parsing success; it does not update the passed in values on failure.
>> *
>> * Return 0 on success, < 0 on failure.
>> */
>> -int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
>> - u64 *size)
>> +int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
>> {
>> struct acpi_device *adev;
>> LIST_HEAD(list);
>> struct resource_entry *rentry;
>> int ret;
>> struct device *dma_dev = dev;
>> - u64 len, dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
>> + struct bus_dma_region *r;
>>
>> /*
>> * Walk the device tree chasing an ACPI companion with a _DMA
>> @@ -1510,31 +1507,28 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
>>
>> ret = acpi_dev_get_dma_resources(adev, &list);
>> if (ret > 0) {
>> + r = kcalloc(ret + 1, sizeof(*r), GFP_KERNEL);
>
> Why (ret + 1) ?
The range map is terminated by an entry with size == 0, although the
equivalent allocations in of_dma_get_range() and dma_direct_set_offset()
are similarly uncommented.
>> + if (!r) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + *map = r;
>
> I don't think that's right. This function can still fail at this stage,
> we should not update the *map value yet.
The caller does at least correctly ignore the map if we return failure,
but I guess it is still at odds with the kerneldoc saying "it does not
update the passed in values on failure", and presumably the allocation
also ends up being leaked in that case. Dang, sorry I missed this in my
last review...
Cheers,
Robin.
>
> Lorenzo
>
>> +
>> list_for_each_entry(rentry, &list, node) {
>> - if (dma_offset && rentry->offset != dma_offset) {
>> + if (rentry->res->start >= rentry->res->end) {
>> ret = -EINVAL;
>> - dev_warn(dma_dev, "Can't handle multiple windows with different offsets\n");
>> + dev_dbg(dma_dev, "Invalid DMA regions configuration\n");
>> goto out;
>> }
>> - dma_offset = rentry->offset;
>>
>> - /* Take lower and upper limits */
>> - if (rentry->res->start < dma_start)
>> - dma_start = rentry->res->start;
>> - if (rentry->res->end > dma_end)
>> - dma_end = rentry->res->end;
>> - }
>> -
>> - if (dma_start >= dma_end) {
>> - ret = -EINVAL;
>> - dev_dbg(dma_dev, "Invalid DMA regions configuration\n");
>> - goto out;
>> + r->cpu_start = rentry->res->start;
>> + r->dma_start = rentry->res->start - rentry->offset;
>> + r->size = resource_size(rentry->res);
>> + r->offset = rentry->offset;
>> + r++;
>> }
>>
>> - *dma_addr = dma_start - dma_offset;
>> - len = dma_end - dma_start;
>> - *size = max(len, len + 1);
>> - *offset = dma_offset;
>> }
>> out:
>> acpi_dev_free_resource_list(&list);
>> @@ -1624,20 +1618,19 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>> const u32 *input_id)
>> {
>> const struct iommu_ops *iommu;
>> - u64 dma_addr = 0, size = 0;
>>
>> if (attr == DEV_DMA_NOT_SUPPORTED) {
>> set_dma_ops(dev, &dma_dummy_ops);
>> return 0;
>> }
>>
>> - acpi_arch_dma_setup(dev, &dma_addr, &size);
>> + acpi_arch_dma_setup(dev);
>>
>> iommu = acpi_iommu_configure_id(dev, input_id);
>> if (PTR_ERR(iommu) == -EPROBE_DEFER)
>> return -EPROBE_DEFER;
>>
>> - arch_setup_dma_ops(dev, dma_addr, size,
>> + arch_setup_dma_ops(dev, 0, U64_MAX,
>> iommu, attr == DEV_DMA_COHERENT);
>>
>> return 0;
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index e7d27373ff71..73ac4a1d6947 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -613,8 +613,7 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
>> int acpi_iommu_fwspec_init(struct device *dev, u32 id,
>> struct fwnode_handle *fwnode,
>> const struct iommu_ops *ops);
>> -int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
>> - u64 *size);
>> +int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map);
>> int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>> const u32 *input_id);
>> static inline int acpi_dma_configure(struct device *dev,
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 6f64b2f3dc54..bb41623dab77 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -281,12 +281,12 @@ void acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa);
>>
>> #ifdef CONFIG_ARM64
>> void acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa);
>> -void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size);
>> +void acpi_arch_dma_setup(struct device *dev);
>> #else
>> static inline void
>> acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) { }
>> static inline void
>> -acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) { }
>> +acpi_arch_dma_setup(struct device *dev) { }
>> #endif
>>
>> int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma);
>> @@ -977,8 +977,7 @@ static inline enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev)
>> return DEV_DMA_NOT_SUPPORTED;
>> }
>>
>> -static inline int acpi_dma_get_range(struct device *dev, u64 *dma_addr,
>> - u64 *offset, u64 *size)
>> +static inline int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
>> {
>> return -ENODEV;
>> }
>> --
>> 2.31.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets
2022-09-09 11:48 ` Robin Murphy
@ 2022-09-09 13:08 ` Jianmin Lv
0 siblings, 0 replies; 6+ messages in thread
From: Jianmin Lv @ 2022-09-09 13:08 UTC (permalink / raw)
To: Robin Murphy, Lorenzo Pieralisi
Cc: chenhuacai, guohanjun, sudeep.holla, rafael, lenb, robert.moore,
linux-kernel, linux-acpi, loongarch
On 2022/9/9 下午7:48, Robin Murphy wrote:
> On 2022-09-09 12:19, Lorenzo Pieralisi wrote:
>> On Fri, Sep 09, 2022 at 05:28:10PM +0800, Jianmin Lv wrote:
>>> For DT, of_dma_get_range returns bus_dma_region typed dma regions,
>>> which makes multiple dma windows with different offset available
>>> for translation between dma address and cpu address.
>>>
>>> But for ACPI, acpi_dma_get_range doesn't return similar dma regions,
>>> causing no path for setting dev->dma_range_map conveniently. So the
>>> patch changes acpi_dma_get_range and returns bus_dma_region typed
>>> dma regions according to of_dma_get_range.
>>>
>>> After changing acpi_dma_get_range, acpi_arch_dma_setup is changed for
>>> ARM64, where original dma_addr and size are removed as these
>>> arguments are now redundant, and pass 0 and U64_MAX for dma_base
>>> and size of arch_setup_dma_ops, so this is a simplification consistent
>>> with what other ACPI architectures also pass to iommu_setup_dma_ops().
>>
>> How about this commit log:
>>
>> "In DT systems configurations, of_dma_get_range() returns struct
>> bus_dma_region DMA regions; they are used to set-up devices
>> DMA windows with different offset available for translation between DMA
>> address and CPU address.
>>
>> In ACPI systems configuration, acpi_dma_get_range() does not return
>> DMA regions yet and that precludes setting up the dev->dma_range_map
>> pointer and therefore DMA regions with multiple offsets.
>>
>> Update acpi_dma_get_range() to return struct bus_dma_region
>> DMA regions like of_dma_get_range() does.
>>
>> After updating acpi_dma_get_range(), acpi_arch_dma_setup() is changed for
>> ARM64, where the original dma_addr and size are removed as these
>> arguments are now redundant, and pass 0 and U64_MAX for dma_base
>> and size of arch_setup_dma_ops; this is a simplification consistent
>> with what other ACPI architectures also pass to iommu_setup_dma_ops()."
>>
Sure, it seem that the log is more better than before, thanks very much！
Jianmin
>>>
>>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>>> ---
>>> drivers/acpi/arm64/dma.c | 29 +++++++++++++---------
>>> drivers/acpi/scan.c | 53 +++++++++++++++++-----------------------
>>> include/acpi/acpi_bus.h | 3 +--
>>> include/linux/acpi.h | 7 +++---
>>> 4 files changed, 45 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
>>> index f16739ad3cc0..1ef8e7ded4cd 100644
>>> --- a/drivers/acpi/arm64/dma.c
>>> +++ b/drivers/acpi/arm64/dma.c
>>> @@ -4,11 +4,12 @@
>>> #include <linux/device.h>
>>> #include <linux/dma-direct.h>
>>> -void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64
>>> *dma_size)
>>> +void acpi_arch_dma_setup(struct device *dev)
>>> {
>>> int ret;
>>> u64 end, mask;
>>> - u64 dmaaddr = 0, size = 0, offset = 0;
>>> + u64 size = 0;
>>> + const struct bus_dma_region *map = NULL;
>>> /*
>>> * If @dev is expected to be DMA-capable then the bus code that
>>> created
>>> @@ -26,25 +27,31 @@ void acpi_arch_dma_setup(struct device *dev, u64
>>> *dma_addr, u64 *dma_size)
>>> else
>>> size = 1ULL << 32;
>>> - ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size);
>>> + ret = acpi_dma_get_range(dev, &map);
>>> + if (!ret && map) {
>>> + const struct bus_dma_region *r = map;
>>> +
>>> + for (end = 0; r->size; r++) {
>>> + if (r->dma_start + r->size - 1 > end)
>>> + end = r->dma_start + r->size - 1;
>>> + }
>>> +
>>> + size = end + 1;
>>> + dev->dma_range_map = map;
>>> + }
>>> +
>>> if (ret == -ENODEV)
>>> ret = iort_dma_get_ranges(dev, &size);
>>> +
>>
>> Nit: this is a spurious line change.
>>
Ok, I'll remove it.
>>> if (!ret) {
>>> /*
>>> * Limit coherent and dma mask based on size retrieved from
>>> * firmware.
>>> */
>>> - end = dmaaddr + size - 1;
>>> + end = size - 1;
>>> mask = DMA_BIT_MASK(ilog2(end) + 1);
>>> dev->bus_dma_limit = end;
>>> dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
>>> *dev->dma_mask = min(*dev->dma_mask, mask);
>>> }
>>> -
>>> - *dma_addr = dmaaddr;
>>> - *dma_size = size;
>>> -
>>> - ret = dma_direct_set_offset(dev, dmaaddr + offset, dmaaddr, size);
>>> -
>>> - dev_dbg(dev, "dma_offset(%#08llx)%s\n", offset, ret ? " failed!"
>>> : "");
>>> }
>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>> index 42cec8120f18..b4505ec467fe 100644
>>> --- a/drivers/acpi/scan.c
>>> +++ b/drivers/acpi/scan.c
>>> @@ -20,6 +20,7 @@
>>> #include <linux/platform_data/x86/apple.h>
>>> #include <linux/pgtable.h>
>>> #include <linux/crc32.h>
>>> +#include <linux/dma-direct.h>
>>> #include "internal.h"
>>> @@ -1467,25 +1468,21 @@ enum dev_dma_attr acpi_get_dma_attr(struct
>>> acpi_device *adev)
>>> * acpi_dma_get_range() - Get device DMA parameters.
>>> *
>>> * @dev: device to configure
>>> - * @dma_addr: pointer device DMA address result
>>> - * @offset: pointer to the DMA offset result
>>> - * @size: pointer to DMA range size result
>>> + * @map: pointer to DMA ranges result
>>> *
>>> - * Evaluate DMA regions and return respectively DMA region start,
>>> offset
>>> - * and size in dma_addr, offset and size on parsing success; it does
>>> not
>>> - * update the passed in values on failure.
>>> + * Evaluate DMA regions and return pointer to DMA regions on
>>> + * parsing success; it does not update the passed in values on failure.
>>> *
>>> * Return 0 on success, < 0 on failure.
>>> */
>>> -int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
>>> - u64 *size)
>>> +int acpi_dma_get_range(struct device *dev, const struct
>>> bus_dma_region **map)
>>> {
>>> struct acpi_device *adev;
>>> LIST_HEAD(list);
>>> struct resource_entry *rentry;
>>> int ret;
>>> struct device *dma_dev = dev;
>>> - u64 len, dma_start = U64_MAX, dma_end = 0, dma_offset = 0;
>>> + struct bus_dma_region *r;
>>> /*
>>> * Walk the device tree chasing an ACPI companion with a _DMA
>>> @@ -1510,31 +1507,28 @@ int acpi_dma_get_range(struct device *dev,
>>> u64 *dma_addr, u64 *offset,
>>> ret = acpi_dev_get_dma_resources(adev, &list);
>>> if (ret > 0) {
>>> + r = kcalloc(ret + 1, sizeof(*r), GFP_KERNEL);
>>
>> Why (ret + 1) ?
>
> The range map is terminated by an entry with size == 0, although the
> equivalent allocations in of_dma_get_range() and dma_direct_set_offset()
> are similarly uncommented.
>
Yes, really.
>>> + if (!r) {
>>> + ret = -ENOMEM;
>>> + goto out;
>>> + }
>>> +
>>> + *map = r;
>>
>> I don't think that's right. This function can still fail at this stage,
>> we should not update the *map value yet.
>
> The caller does at least correctly ignore the map if we return failure,
> but I guess it is still at odds with the kerneldoc saying "it does not
> update the passed in values on failure", and presumably the allocation
> also ends up being leaked in that case. Dang, sorry I missed this in my
> last review...
Sorry, it's really a bug code here, I'll change it as following:
r = kcalloc(ret + 1, sizeof(*r), GFP_KERNEL);
if (!r) {
ret = -ENOMEM;
goto out;
}
list_for_each_entry(rentry, &list, node) {
if (rentry->res->start >= rentry->res->end) {
kfree(r);
ret = -EINVAL;
dev_dbg(dma_dev, "Invalid DMA regions
configuration\n");
goto out;
}
r->cpu_start = rentry->res->start;
r->dma_start = rentry->res->start - rentry->offset;
r->size = resource_size(rentry->res);
r->offset = rentry->offset;
r++;
}
*map = r;
Thanks,
Jianmin
>
> Cheers,
> Robin.
>
>>
>> Lorenzo
>>
>>> +
>>> list_for_each_entry(rentry, &list, node) {
>>> - if (dma_offset && rentry->offset != dma_offset) {
>>> + if (rentry->res->start >= rentry->res->end) {
>>> ret = -EINVAL;
>>> - dev_warn(dma_dev, "Can't handle multiple windows
>>> with different offsets\n");
>>> + dev_dbg(dma_dev, "Invalid DMA regions
>>> configuration\n");
>>> goto out;
>>> }
>>> - dma_offset = rentry->offset;
>>> - /* Take lower and upper limits */
>>> - if (rentry->res->start < dma_start)
>>> - dma_start = rentry->res->start;
>>> - if (rentry->res->end > dma_end)
>>> - dma_end = rentry->res->end;
>>> - }
>>> -
>>> - if (dma_start >= dma_end) {
>>> - ret = -EINVAL;
>>> - dev_dbg(dma_dev, "Invalid DMA regions configuration\n");
>>> - goto out;
>>> + r->cpu_start = rentry->res->start;
>>> + r->dma_start = rentry->res->start - rentry->offset;
>>> + r->size = resource_size(rentry->res);
>>> + r->offset = rentry->offset;
>>> + r++;
>>> }
>>> - *dma_addr = dma_start - dma_offset;
>>> - len = dma_end - dma_start;
>>> - *size = max(len, len + 1);
>>> - *offset = dma_offset;
>>> }
>>> out:
>>> acpi_dev_free_resource_list(&list);
>>> @@ -1624,20 +1618,19 @@ int acpi_dma_configure_id(struct device *dev,
>>> enum dev_dma_attr attr,
>>> const u32 *input_id)
>>> {
>>> const struct iommu_ops *iommu;
>>> - u64 dma_addr = 0, size = 0;
>>> if (attr == DEV_DMA_NOT_SUPPORTED) {
>>> set_dma_ops(dev, &dma_dummy_ops);
>>> return 0;
>>> }
>>> - acpi_arch_dma_setup(dev, &dma_addr, &size);
>>> + acpi_arch_dma_setup(dev);
>>> iommu = acpi_iommu_configure_id(dev, input_id);
>>> if (PTR_ERR(iommu) == -EPROBE_DEFER)
>>> return -EPROBE_DEFER;
>>> - arch_setup_dma_ops(dev, dma_addr, size,
>>> + arch_setup_dma_ops(dev, 0, U64_MAX,
>>> iommu, attr == DEV_DMA_COHERENT);
>>> return 0;
>>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>>> index e7d27373ff71..73ac4a1d6947 100644
>>> --- a/include/acpi/acpi_bus.h
>>> +++ b/include/acpi/acpi_bus.h
>>> @@ -613,8 +613,7 @@ enum dev_dma_attr acpi_get_dma_attr(struct
>>> acpi_device *adev);
>>> int acpi_iommu_fwspec_init(struct device *dev, u32 id,
>>> struct fwnode_handle *fwnode,
>>> const struct iommu_ops *ops);
>>> -int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset,
>>> - u64 *size);
>>> +int acpi_dma_get_range(struct device *dev, const struct
>>> bus_dma_region **map);
>>> int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
>>> const u32 *input_id);
>>> static inline int acpi_dma_configure(struct device *dev,
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>> index 6f64b2f3dc54..bb41623dab77 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -281,12 +281,12 @@ void acpi_numa_x2apic_affinity_init(struct
>>> acpi_srat_x2apic_cpu_affinity *pa);
>>> #ifdef CONFIG_ARM64
>>> void acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa);
>>> -void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64
>>> *dma_size);
>>> +void acpi_arch_dma_setup(struct device *dev);
>>> #else
>>> static inline void
>>> acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) { }
>>> static inline void
>>> -acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64
>>> *dma_size) { }
>>> +acpi_arch_dma_setup(struct device *dev) { }
>>> #endif
>>> int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity
>>> *ma);
>>> @@ -977,8 +977,7 @@ static inline enum dev_dma_attr
>>> acpi_get_dma_attr(struct acpi_device *adev)
>>> return DEV_DMA_NOT_SUPPORTED;
>>> }
>>> -static inline int acpi_dma_get_range(struct device *dev, u64 *dma_addr,
>>> - u64 *offset, u64 *size)
>>> +static inline int acpi_dma_get_range(struct device *dev, const
>>> struct bus_dma_region **map)
>>> {
>>> return -ENODEV;
>>> }
>>> --
>>> 2.31.1
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-09 13:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-09 9:28 [PATCH V4 0/2] DMA: update acpi_dma_get_range to return dma map regions Jianmin Lv
2022-09-09 9:28 ` [PATCH V4 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv
2022-09-09 11:19 ` Lorenzo Pieralisi
2022-09-09 11:48 ` Robin Murphy
2022-09-09 13:08 ` Jianmin Lv
2022-09-09 9:28 ` [PATCH V4 2/2] LoongArch: Use acpi_arch_dma_setup() and remove ARCH_HAS_PHYS_TO_DMA Jianmin Lv
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).