* [PATCH V2 0/2] DMA: update acpi_dma_get_range to return dma map regions @ 2022-08-05 7:31 Jianmin Lv 2022-08-05 7:31 ` [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv 2022-08-05 7:31 ` [PATCH V2 2/2] LoongArch: Remove ARCH_HAS_PHYS_TO_DMA Jianmin Lv 0 siblings, 2 replies; 10+ messages in thread From: Jianmin Lv @ 2022-08-05 7:31 UTC (permalink / raw) To: lpieralisi, robin.murphy, guohanjun, sudeep.holla, rafael, lenb, robert.moore Cc: linux-kernel, linux-acpi 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 Jianmin Lv (2): ACPI / scan: Support multiple dma windows with different offsets LoongArch: Remove ARCH_HAS_PHYS_TO_DMA arch/loongarch/Kconfig | 1 - arch/loongarch/kernel/dma.c | 62 +++++++++++++++++++++---------------------- arch/loongarch/kernel/setup.c | 2 +- drivers/acpi/arm64/dma.c | 44 ++++++++++++++++++++++-------- drivers/acpi/scan.c | 48 ++++++++++++++++----------------- include/acpi/acpi_bus.h | 3 +-- include/linux/acpi.h | 12 +++++---- 7 files changed, 97 insertions(+), 75 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets 2022-08-05 7:31 [PATCH V2 0/2] DMA: update acpi_dma_get_range to return dma map regions Jianmin Lv @ 2022-08-05 7:31 ` Jianmin Lv 2022-08-05 12:46 ` Robin Murphy 2022-08-05 7:31 ` [PATCH V2 2/2] LoongArch: Remove ARCH_HAS_PHYS_TO_DMA Jianmin Lv 1 sibling, 1 reply; 10+ messages in thread From: Jianmin Lv @ 2022-08-05 7:31 UTC (permalink / raw) To: lpieralisi, robin.murphy, guohanjun, sudeep.holla, rafael, lenb, robert.moore Cc: linux-kernel, linux-acpi 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, and pass 0 and U64_MAX for dma_base and size of arch_setup_dma_ops. Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> --- drivers/acpi/arm64/dma.c | 44 +++++++++++++++++++++++++++++++++----------- drivers/acpi/scan.c | 48 ++++++++++++++++++++++++------------------------ include/acpi/acpi_bus.h | 3 +-- include/linux/acpi.h | 7 +++---- 4 files changed, 61 insertions(+), 41 deletions(-) diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c index f16739a..64a41b2 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,7 +27,33 @@ 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) { + const struct bus_dma_region *r = map; + u64 dma_start, dma_end = 0; + + /* determine the overall bounds of all dma regions */ + for (dma_start = U64_MAX; r->size; r++) { + + /* Take lower and upper limits */ + if (r->dma_start < dma_start) + dma_start = r->dma_start; + if (r->dma_start + r->size - 1 > dma_end) + dma_end = r->dma_start + r->size - 1; + } + + if (dma_start >= dma_end) { + dev_dbg(dev, "Invalid DMA regions configuration\n"); + return; + } + + mask = DMA_BIT_MASK(ilog2(dma_end) + 1); + dev->bus_dma_limit = dma_end; + dev->dma_range_map = map; + dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask); + *dev->dma_mask = min(*dev->dma_mask, mask); + } + if (ret == -ENODEV) ret = iort_dma_get_ranges(dev, &size); if (!ret) { @@ -34,17 +61,12 @@ void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) * 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!" : ""); + ret = dma_direct_set_offset(dev, 0, 0, size); + } } diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 762b61f..736517e 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" @@ -1492,15 +1493,15 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev) * * 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; + int num_ranges = 0; + struct bus_dma_region *r; /* * Walk the device tree chasing an ACPI companion with a _DMA @@ -1525,31 +1526,31 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, ret = acpi_dev_get_dma_resources(adev, &list); if (ret > 0) { + list_for_each_entry(rentry, &list, node) + num_ranges++; + + r = kcalloc(num_ranges + 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 = rentry->res->end - rentry->res->start + 1; + 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); @@ -1639,20 +1640,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 0dc1ea0b..e106073 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -611,8 +611,7 @@ struct acpi_pci_root { 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 44975c1..34e0545 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -280,12 +280,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, #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); @@ -974,8 +974,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; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets 2022-08-05 7:31 ` [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv @ 2022-08-05 12:46 ` Robin Murphy 2022-08-06 6:13 ` Jianmin Lv ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Robin Murphy @ 2022-08-05 12:46 UTC (permalink / raw) To: Jianmin Lv, lpieralisi, guohanjun, sudeep.holla, rafael, lenb, robert.moore Cc: linux-kernel, linux-acpi On 2022-08-05 08:31, 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, and pass 0 and > U64_MAX for dma_base and size of arch_setup_dma_ops. Hmm, I realise the reasoning behind this exists largely in my head, so for the benefit of everyone else we should perhaps clarify that these arguments are now redundant, so this is a simplification consistent with what other ACPI architectures also pass to iommu_setup_dma_ops(). > Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> > --- > drivers/acpi/arm64/dma.c | 44 +++++++++++++++++++++++++++++++++----------- > drivers/acpi/scan.c | 48 ++++++++++++++++++++++++------------------------ > include/acpi/acpi_bus.h | 3 +-- > include/linux/acpi.h | 7 +++---- > 4 files changed, 61 insertions(+), 41 deletions(-) > > diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c > index f16739a..64a41b2 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,7 +27,33 @@ 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) { > + const struct bus_dma_region *r = map; > + u64 dma_start, dma_end = 0; > + > + /* determine the overall bounds of all dma regions */ > + for (dma_start = U64_MAX; r->size; r++) { > + > + /* Take lower and upper limits */ > + if (r->dma_start < dma_start) > + dma_start = r->dma_start; dma_start now only serves as an overcomplicated way to tell if the list was empty, so we don't need it... > + if (r->dma_start + r->size - 1 > dma_end) > + dma_end = r->dma_start + r->size - 1; > + } > + > + if (dma_start >= dma_end) { > + dev_dbg(dev, "Invalid DMA regions configuration\n"); > + return; ...but we also don't need this check anway - either there was at least one valid resource, so dma_end is now set to something sensible, or there were none, in which case the implementation of acpi_dma_get_range() here returns success while leaving map == NULL, so we've already crashed dereferencing r->size. > + } > + > + mask = DMA_BIT_MASK(ilog2(dma_end) + 1); > + dev->bus_dma_limit = dma_end; > + dev->dma_range_map = map; > + dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask); > + *dev->dma_mask = min(*dev->dma_mask, mask); Don't duplicate all this, just set "size = dma_end + 1" so the existing code below picks it up. > + } > + > if (ret == -ENODEV) > ret = iort_dma_get_ranges(dev, &size); > if (!ret) { > @@ -34,17 +61,12 @@ void acpi_arch_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > * 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!" : ""); > + ret = dma_direct_set_offset(dev, 0, 0, size); Don't call this - we're setting dev->dma_range_map now, so it will only scream and fail (or do nothing if there's no offset). > + } > } > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 762b61f..736517e 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" > > @@ -1492,15 +1493,15 @@ enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev) > * > * Return 0 on success, < 0 on failure. > */ Don't forget to update the kerneldoc as well. > -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; > + int num_ranges = 0; > + struct bus_dma_region *r; > > /* > * Walk the device tree chasing an ACPI companion with a _DMA > @@ -1525,31 +1526,31 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, > > ret = acpi_dev_get_dma_resources(adev, &list); > if (ret > 0) { > + list_for_each_entry(rentry, &list, node) > + num_ranges++; We already have the number of resources in ret. Looking at this, I also now wonder if we're doing the right thing if the object is present but contains no resources. The spec isn't clear whether that's even really valid, but if it is, is it meaningful? It seems we'd currently consider an empty object equivalent to no object, but if anything it should perhaps be interpreted as the opposite, i.e. that no DMA is possible because the bus does not decode any ranges. Is anyone more familiar with the intent of the spec here? Thanks, Robin. > + > + r = kcalloc(num_ranges + 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 = rentry->res->end - rentry->res->start + 1; > + 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); > @@ -1639,20 +1640,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 0dc1ea0b..e106073 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -611,8 +611,7 @@ struct acpi_pci_root { > 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 44975c1..34e0545 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -280,12 +280,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, > > #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); > @@ -974,8 +974,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; > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets 2022-08-05 12:46 ` Robin Murphy @ 2022-08-06 6:13 ` Jianmin Lv 2022-08-06 7:21 ` Jianmin Lv 2022-08-08 8:24 ` Lorenzo Pieralisi 2022-08-12 7:52 ` Lorenzo Pieralisi 2 siblings, 1 reply; 10+ messages in thread From: Jianmin Lv @ 2022-08-06 6:13 UTC (permalink / raw) To: Robin Murphy, lpieralisi, guohanjun, sudeep.holla, rafael, lenb, robert.moore Cc: linux-kernel, linux-acpi On 2022/8/5 下午8:46, Robin Murphy wrote: > On 2022-08-05 08:31, 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, and pass 0 and >> U64_MAX for dma_base and size of arch_setup_dma_ops. > > Hmm, I realise the reasoning behind this exists largely in my head, so > for the benefit of everyone else we should perhaps clarify that these > arguments are now redundant, so this is a simplification consistent with > what other ACPI architectures also pass to iommu_setup_dma_ops(). > Ok, I'll add reason of changing it into commit log for everyone else. >> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> >> --- >> drivers/acpi/arm64/dma.c | 44 >> +++++++++++++++++++++++++++++++++----------- >> drivers/acpi/scan.c | 48 >> ++++++++++++++++++++++++------------------------ >> include/acpi/acpi_bus.h | 3 +-- >> include/linux/acpi.h | 7 +++---- >> 4 files changed, 61 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c >> index f16739a..64a41b2 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,7 +27,33 @@ 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) { >> + const struct bus_dma_region *r = map; >> + u64 dma_start, dma_end = 0; >> + >> + /* determine the overall bounds of all dma regions */ >> + for (dma_start = U64_MAX; r->size; r++) { >> + >> + /* Take lower and upper limits */ >> + if (r->dma_start < dma_start) >> + dma_start = r->dma_start; > > dma_start now only serves as an overcomplicated way to tell if the list > was empty, so we don't need it... Ok,thanks. > >> + if (r->dma_start + r->size - 1 > dma_end) >> + dma_end = r->dma_start + r->size - 1; >> + } >> + >> + if (dma_start >= dma_end) { >> + dev_dbg(dev, "Invalid DMA regions configuration\n"); >> + return; > > ...but we also don't need this check anway - either there was at least > one valid resource, so dma_end is now set to something sensible, or > there were none, in which case the implementation of > acpi_dma_get_range() here returns success while leaving map == NULL, so > we've already crashed dereferencing r->size. > Ok, thanks. >> + } >> + >> + mask = DMA_BIT_MASK(ilog2(dma_end) + 1); >> + dev->bus_dma_limit = dma_end; >> + dev->dma_range_map = map; >> + dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask); >> + *dev->dma_mask = min(*dev->dma_mask, mask); > > Don't duplicate all this, just set "size = dma_end + 1" so the existing > code below picks it up. > TBH, I'm not familiar with ARM64's DMA stuff, but from the code here, I think there are two ways to get dma ranges: acpi_dma_get_range or iort_dma_get_ranges. Before the patch, after obtaining the dmaaddr, offset and size in any way of the both, the bus_dma_limit, mask and dma_range_map will be initialized, and dma_range_map is created a *new* one for *both* ways. But after changing acpi_dma_get_range, when we are ok to obtain dma_range_map by acpi_dma_get_range, we'll use it instead of creating a new one like before. For iort_dma_get_ranges, we have to still create a new dma_range_map. So, I have to handle separately each way here. It seems that it's enough to handle dma_range_map separately, and others can be still initialized in one place. >> + } >> + >> if (ret == -ENODEV) >> ret = iort_dma_get_ranges(dev, &size); >> if (!ret) { >> @@ -34,17 +61,12 @@ void acpi_arch_dma_setup(struct device *dev, u64 >> *dma_addr, u64 *dma_size) >> * 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!" >> : ""); >> + ret = dma_direct_set_offset(dev, 0, 0, size); > > Don't call this - we're setting dev->dma_range_map now, so it will only > scream and fail (or do nothing if there's no offset). > Same as above. >> + } >> } >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c >> index 762b61f..736517e 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" >> @@ -1492,15 +1493,15 @@ enum dev_dma_attr acpi_get_dma_attr(struct >> acpi_device *adev) >> * >> * Return 0 on success, < 0 on failure. >> */ > > Don't forget to update the kerneldoc as well. > Ok, thanks. >> -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; >> + int num_ranges = 0; >> + struct bus_dma_region *r; >> /* >> * Walk the device tree chasing an ACPI companion with a _DMA >> @@ -1525,31 +1526,31 @@ int acpi_dma_get_range(struct device *dev, u64 >> *dma_addr, u64 *offset, >> ret = acpi_dev_get_dma_resources(adev, &list); >> if (ret > 0) { >> + list_for_each_entry(rentry, &list, node) >> + num_ranges++; > > We already have the number of resources in ret. > Ok, thanks. > Looking at this, I also now wonder if we're doing the right thing if the > object is present but contains no resources. The spec isn't clear > whether that's even really valid, but if it is, is it meaningful? It > seems we'd currently consider an empty object equivalent to no object, > but if anything it should perhaps be interpreted as the opposite, i.e. > that no DMA is possible because the bus does not decode any ranges. Is > anyone more familiar with the intent of the spec here? > > Thanks, > Robin. > According to _DMA of ACPI spec, _DMA is optional, if without it(no _DMA object configured in DSDT), kernel assumes that any address range placed on bus by a child device is valid and can be decoded. So I think it is still legal if _DMA is present and no resources contained in it. I mean an empty _DMA object equals no _DMA object. But obviously it's not meaningful for an empty _DMA object, anybody should not pass an empty _DMA object. >> + >> + r = kcalloc(num_ranges + 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 = rentry->res->end - rentry->res->start + 1; >> + 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); >> @@ -1639,20 +1640,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 0dc1ea0b..e106073 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -611,8 +611,7 @@ struct acpi_pci_root { >> 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 44975c1..34e0545 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -280,12 +280,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, >> #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); >> @@ -974,8 +974,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; >> } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets 2022-08-06 6:13 ` Jianmin Lv @ 2022-08-06 7:21 ` Jianmin Lv 0 siblings, 0 replies; 10+ messages in thread From: Jianmin Lv @ 2022-08-06 7:21 UTC (permalink / raw) To: Robin Murphy, lpieralisi, guohanjun, sudeep.holla, rafael, lenb, robert.moore Cc: linux-kernel, linux-acpi On 2022/8/6 下午2:13, Jianmin Lv wrote: > > > On 2022/8/5 下午8:46, Robin Murphy wrote: >> On 2022-08-05 08:31, 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, and pass 0 and >>> U64_MAX for dma_base and size of arch_setup_dma_ops. >> >> Hmm, I realise the reasoning behind this exists largely in my head, so >> for the benefit of everyone else we should perhaps clarify that these >> arguments are now redundant, so this is a simplification consistent >> with what other ACPI architectures also pass to iommu_setup_dma_ops(). >> > > Ok, I'll add reason of changing it into commit log for everyone else. > > >>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> >>> --- >>> drivers/acpi/arm64/dma.c | 44 >>> +++++++++++++++++++++++++++++++++----------- >>> drivers/acpi/scan.c | 48 >>> ++++++++++++++++++++++++------------------------ >>> include/acpi/acpi_bus.h | 3 +-- >>> include/linux/acpi.h | 7 +++---- >>> 4 files changed, 61 insertions(+), 41 deletions(-) >>> >>> diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c >>> index f16739a..64a41b2 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,7 +27,33 @@ 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) { >>> + const struct bus_dma_region *r = map; >>> + u64 dma_start, dma_end = 0; >>> + >>> + /* determine the overall bounds of all dma regions */ >>> + for (dma_start = U64_MAX; r->size; r++) { >>> + >>> + /* Take lower and upper limits */ >>> + if (r->dma_start < dma_start) >>> + dma_start = r->dma_start; >> >> dma_start now only serves as an overcomplicated way to tell if the >> list was empty, so we don't need it... > > Ok,thanks. > >> >>> + if (r->dma_start + r->size - 1 > dma_end) >>> + dma_end = r->dma_start + r->size - 1; >>> + } >>> + >>> + if (dma_start >= dma_end) { >>> + dev_dbg(dev, "Invalid DMA regions configuration\n"); >>> + return; >> >> ...but we also don't need this check anway - either there was at least >> one valid resource, so dma_end is now set to something sensible, or >> there were none, in which case the implementation of >> acpi_dma_get_range() here returns success while leaving map == NULL, >> so we've already crashed dereferencing r->size. >> > > Ok, thanks. > > >>> + } >>> + >>> + mask = DMA_BIT_MASK(ilog2(dma_end) + 1); >>> + dev->bus_dma_limit = dma_end; >>> + dev->dma_range_map = map; >>> + dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask); >>> + *dev->dma_mask = min(*dev->dma_mask, mask); >> >> Don't duplicate all this, just set "size = dma_end + 1" so the >> existing code below picks it up. >> > > TBH, I'm not familiar with ARM64's DMA stuff, but from the code here, I > think there are two ways to get dma ranges: acpi_dma_get_range or > iort_dma_get_ranges. > > Before the patch, after obtaining the dmaaddr, offset and size in any > way of the both, the bus_dma_limit, mask and dma_range_map will be > initialized, and dma_range_map is created a *new* one for *both* ways. > > But after changing acpi_dma_get_range, when we are ok to obtain > dma_range_map by acpi_dma_get_range, we'll use it instead of creating a > new one like before. For iort_dma_get_ranges, we have to still create a > new dma_range_map. So, I have to handle separately each way here. > > It seems that it's enough to handle dma_range_map separately, and others > can be still initialized in one place. > Maybe I got what you mean, I looked into dma_direct_set_offset, which is only meaningful when acpi_dma_get_range return 0 with correct offset and dmaaddr. iort_dma_get_ranges is only used to retrieve size. Right? If so, code here should be: dev->dma_range_map = map; size = dma_end + 1; >>> + } >>> + >>> if (ret == -ENODEV) >>> ret = iort_dma_get_ranges(dev, &size); >>> if (!ret) { >>> @@ -34,17 +61,12 @@ void acpi_arch_dma_setup(struct device *dev, u64 >>> *dma_addr, u64 *dma_size) >>> * 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!" >>> : ""); >>> + ret = dma_direct_set_offset(dev, 0, 0, size); >> >> Don't call this - we're setting dev->dma_range_map now, so it will >> only scream and fail (or do nothing if there's no offset). >> > > Same as above. > Same, If what I understand is right above, dma_direct_set_offset is should be removed as you suggested. >>> + } >>> } >>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c >>> index 762b61f..736517e 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" >>> @@ -1492,15 +1493,15 @@ enum dev_dma_attr acpi_get_dma_attr(struct >>> acpi_device *adev) >>> * >>> * Return 0 on success, < 0 on failure. >>> */ >> >> Don't forget to update the kerneldoc as well. >> > > Ok, thanks. > > >>> -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; >>> + int num_ranges = 0; >>> + struct bus_dma_region *r; >>> /* >>> * Walk the device tree chasing an ACPI companion with a _DMA >>> @@ -1525,31 +1526,31 @@ int acpi_dma_get_range(struct device *dev, >>> u64 *dma_addr, u64 *offset, >>> ret = acpi_dev_get_dma_resources(adev, &list); >>> if (ret > 0) { >>> + list_for_each_entry(rentry, &list, node) >>> + num_ranges++; >> >> We already have the number of resources in ret. >> > > Ok, thanks. > > >> Looking at this, I also now wonder if we're doing the right thing if >> the object is present but contains no resources. The spec isn't clear >> whether that's even really valid, but if it is, is it meaningful? It >> seems we'd currently consider an empty object equivalent to no object, >> but if anything it should perhaps be interpreted as the opposite, i.e. >> that no DMA is possible because the bus does not decode any ranges. Is >> anyone more familiar with the intent of the spec here? >> >> Thanks, >> Robin. >> > > According to _DMA of ACPI spec, _DMA is optional, if without it(no _DMA > object configured in DSDT), kernel assumes that any address range placed > on bus by a child device is valid and can be decoded. So I think it is > still legal if _DMA is present and no resources contained in it. I mean > an empty _DMA object equals no _DMA object. But obviously it's not > meaningful for an empty _DMA object, anybody should not pass an empty > _DMA object. > >>> + >>> + r = kcalloc(num_ranges + 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 = rentry->res->end - rentry->res->start + 1; >>> + 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); >>> @@ -1639,20 +1640,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 0dc1ea0b..e106073 100644 >>> --- a/include/acpi/acpi_bus.h >>> +++ b/include/acpi/acpi_bus.h >>> @@ -611,8 +611,7 @@ struct acpi_pci_root { >>> 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 44975c1..34e0545 100644 >>> --- a/include/linux/acpi.h >>> +++ b/include/linux/acpi.h >>> @@ -280,12 +280,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, >>> #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); >>> @@ -974,8 +974,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; >>> } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets 2022-08-05 12:46 ` Robin Murphy 2022-08-06 6:13 ` Jianmin Lv @ 2022-08-08 8:24 ` Lorenzo Pieralisi 2022-08-12 7:52 ` Lorenzo Pieralisi 2 siblings, 0 replies; 10+ messages in thread From: Lorenzo Pieralisi @ 2022-08-08 8:24 UTC (permalink / raw) To: Robin Murphy Cc: Jianmin Lv, guohanjun, sudeep.holla, rafael, lenb, robert.moore, linux-kernel, linux-acpi On Fri, Aug 05, 2022 at 01:46:07PM +0100, Robin Murphy wrote: [...] > > -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; > > + int num_ranges = 0; > > + struct bus_dma_region *r; > > /* > > * Walk the device tree chasing an ACPI companion with a _DMA > > @@ -1525,31 +1526,31 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, > > ret = acpi_dev_get_dma_resources(adev, &list); > > if (ret > 0) { > > + list_for_each_entry(rentry, &list, node) > > + num_ranges++; > > We already have the number of resources in ret. > > Looking at this, I also now wonder if we're doing the right thing if the > object is present but contains no resources. The spec isn't clear whether > that's even really valid, but if it is, is it meaningful? It seems we'd > currently consider an empty object equivalent to no object, but if anything > it should perhaps be interpreted as the opposite, i.e. that no DMA is > possible because the bus does not decode any ranges. Is anyone more familiar > with the intent of the spec here? It is unclear. I agree with you (ie that an empty object implies that no DMA is possible) but I believe this requires a spec update to avoid any forthcoming issue; it should not be a real problem (other than a FW bug) but it is better to be on the safe side. Thanks, Lorenzo > Thanks, > Robin. > > > + > > + r = kcalloc(num_ranges + 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 = rentry->res->end - rentry->res->start + 1; > > + 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); > > @@ -1639,20 +1640,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 0dc1ea0b..e106073 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -611,8 +611,7 @@ struct acpi_pci_root { > > 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 44975c1..34e0545 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -280,12 +280,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, > > #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); > > @@ -974,8 +974,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; > > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets 2022-08-05 12:46 ` Robin Murphy 2022-08-06 6:13 ` Jianmin Lv 2022-08-08 8:24 ` Lorenzo Pieralisi @ 2022-08-12 7:52 ` Lorenzo Pieralisi 2022-08-29 13:00 ` Jianmin Lv 2 siblings, 1 reply; 10+ messages in thread From: Lorenzo Pieralisi @ 2022-08-12 7:52 UTC (permalink / raw) To: Robin Murphy Cc: Jianmin Lv, guohanjun, sudeep.holla, rafael, lenb, robert.moore, linux-kernel, linux-acpi On Fri, Aug 05, 2022 at 01:46:07PM +0100, Robin Murphy wrote: [...] > > -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; > > + int num_ranges = 0; > > + struct bus_dma_region *r; > > /* > > * Walk the device tree chasing an ACPI companion with a _DMA > > @@ -1525,31 +1526,31 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, > > ret = acpi_dev_get_dma_resources(adev, &list); > > if (ret > 0) { > > + list_for_each_entry(rentry, &list, node) > > + num_ranges++; > > We already have the number of resources in ret. > > Looking at this, I also now wonder if we're doing the right thing if the > object is present but contains no resources. The spec isn't clear whether > that's even really valid, but if it is, is it meaningful? It seems we'd > currently consider an empty object equivalent to no object, but if anything > it should perhaps be interpreted as the opposite, i.e. that no DMA is > possible because the bus does not decode any ranges. Is anyone more familiar > with the intent of the spec here? I think we are currently considering no object differently from an empty object, since for no object we would return -ENODEV in acpi_dma_get_range(), we would not even get to parsing the resources (and return 0) and we would fall back to checking IORT to gather the DMA address space size. I think you are right, we should change the behaviour if an object is present but it has no resources though, by reading the specs an empty _DMA object implies no DMA is possible and that's not what we are doing at the moment (hopefully there is no firmware out there with such a set-up but there is only one way to discover it). This behavioural change should be a separate patch obviously for bisectability (and a possible revert). Lorenzo > Thanks, > Robin. > > > + > > + r = kcalloc(num_ranges + 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 = rentry->res->end - rentry->res->start + 1; > > + 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); > > @@ -1639,20 +1640,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 0dc1ea0b..e106073 100644 > > --- a/include/acpi/acpi_bus.h > > +++ b/include/acpi/acpi_bus.h > > @@ -611,8 +611,7 @@ struct acpi_pci_root { > > 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 44975c1..34e0545 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -280,12 +280,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, > > #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); > > @@ -974,8 +974,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; > > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets 2022-08-12 7:52 ` Lorenzo Pieralisi @ 2022-08-29 13:00 ` Jianmin Lv 0 siblings, 0 replies; 10+ messages in thread From: Jianmin Lv @ 2022-08-29 13:00 UTC (permalink / raw) To: Lorenzo Pieralisi, Robin Murphy Cc: guohanjun, sudeep.holla, rafael, lenb, robert.moore, linux-kernel, linux-acpi On 2022/8/12 下午3:52, Lorenzo Pieralisi wrote: > On Fri, Aug 05, 2022 at 01:46:07PM +0100, Robin Murphy wrote: > > [...] > >>> -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; >>> + int num_ranges = 0; >>> + struct bus_dma_region *r; >>> /* >>> * Walk the device tree chasing an ACPI companion with a _DMA >>> @@ -1525,31 +1526,31 @@ int acpi_dma_get_range(struct device *dev, u64 *dma_addr, u64 *offset, >>> ret = acpi_dev_get_dma_resources(adev, &list); >>> if (ret > 0) { >>> + list_for_each_entry(rentry, &list, node) >>> + num_ranges++; >> >> We already have the number of resources in ret. >> >> Looking at this, I also now wonder if we're doing the right thing if the >> object is present but contains no resources. The spec isn't clear whether >> that's even really valid, but if it is, is it meaningful? It seems we'd >> currently consider an empty object equivalent to no object, but if anything >> it should perhaps be interpreted as the opposite, i.e. that no DMA is >> possible because the bus does not decode any ranges. Is anyone more familiar >> with the intent of the spec here? > > I think we are currently considering no object differently from an > empty object, since for no object we would return -ENODEV in > acpi_dma_get_range(), we would not even get to parsing the resources > (and return 0) and we would fall back to checking IORT to gather the > DMA address space size. > > I think you are right, we should change the behaviour if an object > is present but it has no resources though, by reading the specs an > empty _DMA object implies no DMA is possible and that's not what > we are doing at the moment (hopefully there is no firmware out > there with such a set-up but there is only one way to discover it). > > This behavioural change should be a separate patch obviously for > bisectability (and a possible revert). > > Lorenzo > Hi, Robin and Lorenzo For the issue of empty _DMA, I consulted ASWG(ACPI Spec Work Group), and received an reply from some people that an _DMA object is allowed to contain *no* resources, and indicates disabled DMA(some arm systems are being used empty _DMA as an indicator to the OS that DMA is disabled for specified device). ASWG has isssued an request to clarify it in spec after approved. I think we can submit a seperate patch for that after it is confirmed. Thanks. >> Thanks, >> Robin. >> >>> + >>> + r = kcalloc(num_ranges + 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 = rentry->res->end - rentry->res->start + 1; >>> + 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); >>> @@ -1639,20 +1640,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 0dc1ea0b..e106073 100644 >>> --- a/include/acpi/acpi_bus.h >>> +++ b/include/acpi/acpi_bus.h >>> @@ -611,8 +611,7 @@ struct acpi_pci_root { >>> 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 44975c1..34e0545 100644 >>> --- a/include/linux/acpi.h >>> +++ b/include/linux/acpi.h >>> @@ -280,12 +280,12 @@ int acpi_table_parse_madt(enum acpi_madt_type id, >>> #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); >>> @@ -974,8 +974,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; >>> } ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V2 2/2] LoongArch: Remove ARCH_HAS_PHYS_TO_DMA 2022-08-05 7:31 [PATCH V2 0/2] DMA: update acpi_dma_get_range to return dma map regions Jianmin Lv 2022-08-05 7:31 ` [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv @ 2022-08-05 7:31 ` Jianmin Lv 2022-08-05 16:36 ` kernel test robot 1 sibling, 1 reply; 10+ messages in thread From: Jianmin Lv @ 2022-08-05 7:31 UTC (permalink / raw) To: lpieralisi, robin.murphy, guohanjun, sudeep.holla, rafael, lenb, robert.moore Cc: linux-kernel, linux-acpi Use _DMA defined in ACPI spec for translation betweeen 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, , , ) }) Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> --- arch/loongarch/Kconfig | 1 - arch/loongarch/kernel/dma.c | 62 +++++++++++++++++++++---------------------- arch/loongarch/kernel/setup.c | 2 +- include/linux/acpi.h | 9 ++++--- 4 files changed, 38 insertions(+), 36 deletions(-) diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig index b57daee..9dedcf9 100644 --- a/arch/loongarch/Kconfig +++ b/arch/loongarch/Kconfig @@ -7,7 +7,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 8c9b531..7850e6b 100644 --- a/arch/loongarch/kernel/dma.c +++ b/arch/loongarch/kernel/dma.c @@ -2,39 +2,39 @@ /* * 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; + const struct bus_dma_region *map = NULL; + + ret = acpi_dma_get_range(dev, &map); + if (!ret) { + const struct bus_dma_region *r = map; + u64 mask, dma_start, dma_end = 0; + + /* determine the overall bounds of all dma regions */ + for (dma_start = U64_MAX; r->size; r++) { + + /* Take lower and upper limits */ + if (r->dma_start < dma_start) + dma_start = r->dma_start; + if (r->dma_start + r->size - 1 > dma_end) + dma_end = r->dma_start + r->size - 1; + } + + if (dma_start >= dma_end) { + dev_dbg(dev, "Invalid DMA regions configuration\n"); + return; + } + + mask = DMA_BIT_MASK(ilog2(dma_end) + 1); + dev->bus_dma_limit = dma_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 c74860b..974f085 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 34e0545..33977b87 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -278,14 +278,17 @@ int acpi_table_parse_madt(enum acpi_madt_type id, 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); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2 2/2] LoongArch: Remove ARCH_HAS_PHYS_TO_DMA 2022-08-05 7:31 ` [PATCH V2 2/2] LoongArch: Remove ARCH_HAS_PHYS_TO_DMA Jianmin Lv @ 2022-08-05 16:36 ` kernel test robot 0 siblings, 0 replies; 10+ messages in thread From: kernel test robot @ 2022-08-05 16:36 UTC (permalink / raw) To: Jianmin Lv, lpieralisi, robin.murphy, guohanjun, sudeep.holla, rafael, lenb, robert.moore Cc: kbuild-all, linux-kernel, linux-acpi Hi Jianmin, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on rafael-pm/linux-next] [also build test WARNING on linus/master v5.19 next-20220804] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Jianmin-Lv/DMA-update-acpi_dma_get_range-to-return-dma-map-regions/20220805-153307 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: loongarch-randconfig-r011-20220805 (https://download.01.org/0day-ci/archive/20220806/202208060021.6kjospL6-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/30f8fbc6b485f30e8533a7848a4ad9a2e1351be8 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Jianmin-Lv/DMA-update-acpi_dma_get_range-to-return-dma-map-regions/20220805-153307 git checkout 30f8fbc6b485f30e8533a7848a4ad9a2e1351be8 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch SHELL=/bin/bash arch/loongarch/kernel/ If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> arch/loongarch/kernel/dma.c:8:6: warning: no previous prototype for 'acpi_arch_dma_setup' [-Wmissing-prototypes] 8 | void acpi_arch_dma_setup(struct device *dev) | ^~~~~~~~~~~~~~~~~~~ vim +/acpi_arch_dma_setup +8 arch/loongarch/kernel/dma.c 7 > 8 void acpi_arch_dma_setup(struct device *dev) 9 { 10 int ret; 11 const struct bus_dma_region *map = NULL; 12 13 ret = acpi_dma_get_range(dev, &map); 14 if (!ret) { 15 const struct bus_dma_region *r = map; 16 u64 mask, dma_start, dma_end = 0; 17 18 /* determine the overall bounds of all dma regions */ 19 for (dma_start = U64_MAX; r->size; r++) { 20 21 /* Take lower and upper limits */ 22 if (r->dma_start < dma_start) 23 dma_start = r->dma_start; 24 if (r->dma_start + r->size - 1 > dma_end) 25 dma_end = r->dma_start + r->size - 1; 26 } 27 28 if (dma_start >= dma_end) { 29 dev_dbg(dev, "Invalid DMA regions configuration\n"); 30 return; 31 } 32 33 mask = DMA_BIT_MASK(ilog2(dma_end) + 1); 34 dev->bus_dma_limit = dma_end; 35 dev->dma_range_map = map; 36 dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask); 37 *dev->dma_mask = min(*dev->dma_mask, mask); 38 } 39 -- 0-DAY CI Kernel Test Service https://01.org/lkp ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-08-29 13:07 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-05 7:31 [PATCH V2 0/2] DMA: update acpi_dma_get_range to return dma map regions Jianmin Lv 2022-08-05 7:31 ` [PATCH V2 1/2] ACPI / scan: Support multiple dma windows with different offsets Jianmin Lv 2022-08-05 12:46 ` Robin Murphy 2022-08-06 6:13 ` Jianmin Lv 2022-08-06 7:21 ` Jianmin Lv 2022-08-08 8:24 ` Lorenzo Pieralisi 2022-08-12 7:52 ` Lorenzo Pieralisi 2022-08-29 13:00 ` Jianmin Lv 2022-08-05 7:31 ` [PATCH V2 2/2] LoongArch: Remove ARCH_HAS_PHYS_TO_DMA Jianmin Lv 2022-08-05 16:36 ` kernel test robot
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).