Re: [PATCH v2 09/14] device core: Add ability to handle multiple dma offsets

From: Nicolas Saenz Julienne <hidden>
Date: 2020-05-27 15:00:15
Also in: linux-iommu, linux-pci, linux-usb, lkml

Hi Jim,
one thing comes to mind, there is a small test suite in drivers/of/unittest.c
(specifically of_unittest_pci_dma_ranges()) you could extend it to include your
use cases.

On Tue, 2020-05-26 at 15:12 -0400, Jim Quinlan wrote:

The new field in struct device 'dma_pfn_offset_map' is used to facilitate
the use of multiple pfn offsets between cpu addrs and dma addrs.  It is
similar to 'dma_pfn_offset' except that the offset chosen depends on the
cpu or dma address involved.

Signed-off-by: Jim Quinlan <redacted>
---
 drivers/of/address.c        | 65 +++++++++++++++++++++++++++++++++++--
 drivers/usb/core/message.c  |  3 ++
 drivers/usb/core/usb.c      |  3 ++
 include/linux/device.h      | 10 +++++-
 include/linux/dma-direct.h  | 10 ++++--
 include/linux/dma-mapping.h | 46 ++++++++++++++++++++++++++
 kernel/dma/Kconfig          | 13 ++++++++
 7 files changed, 144 insertions(+), 6 deletions(-)

[...]

quoted hunk ↗ jump to hunk

@@ -977,10 +1020,19 @@ int of_dma_get_range(struct device *dev, struct

device_node *np, u64 *dma_addr,
 		pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
 			 range.bus_addr, range.cpu_addr, range.size);
 
+		num_ranges++;
 		if (dma_offset && range.cpu_addr - range.bus_addr != dma_offset)
{
-			pr_warn("Can't handle multiple dma-ranges with different
offsets on node(%pOF)\n", node);
-			/* Don't error out as we'd break some existing DTs */
-			continue;
+			if (!IS_ENABLED(CONFIG_DMA_PFN_OFFSET_MAP)) {
+				pr_warn("Can't handle multiple dma-ranges with
different offsets on node(%pOF)\n", node);
+				pr_warn("Perhaps set DMA_PFN_OFFSET_MAP=y?\n");
+				/*
+				 * Don't error out as we'd break some existing
+				 * DTs that are using configs w/o
+				 * CONFIG_DMA_PFN_OFFSET_MAP set.
+				 */
+				continue;

dev->bus_dma_limit is set in of_dma_configure(), this function's caller, based
on dma_start's value (set after this continue). So you'd be effectively setting
the dev->bus_dma_limit to whatever we get from the first dma-range.

This can be troublesome depending on how the dma-ranges are setup, for example
if the first dma-range doesn't include the CMA area, in arm64 generally set as
high as possible in ZONE_DMA32, that would render it useless for
dma/{direct/swiotlb}. Again depending on the bus_dma_limit value, if smaller
than ZONE_DMA you'd be unable to allocate any DMA memory.

IMO, a solution to this calls for a revamp of dma-direct's dma_capable(): match
the target DMA memory area with each dma-range we have to see if it fits.

quoted hunk ↗ jump to hunk

+			}
+			dma_multi_pfn_offset = true;
 		}
 		dma_offset = range.cpu_addr - range.bus_addr;

@@ -991,6 +1043,13 @@ int of_dma_get_range(struct device *dev, struct

device_node *np, u64 *dma_addr,
 			dma_end = range.bus_addr + range.size;
 	}
 
+	if (dma_multi_pfn_offset) {
+		dma_offset = 0;
+		ret = attach_dma_pfn_offset_map(dev, node, num_ranges);
+		if (ret)
+			return ret;
+	}
+
 	if (dma_start >= dma_end) {
 		ret = -EINVAL;
 		pr_debug("Invalid DMA ranges configuration on node(%pOF)\n",

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 6197938dcc2d..aaa3e58f5eb4 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c

@@ -1960,6 +1960,9 @@ int usb_set_configuration(struct usb_device *dev, int

configuration)
 		 */
 		intf->dev.dma_mask = dev->dev.dma_mask;
 		intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+		intf->dev.dma_pfn_offset_map = dev->dev.dma_pfn_offset_map;
+#endif

Thanks for looking at this, that said, I see more instances of drivers changing
dma_pfn_offset outside of the core code. Why not doing this there too?

Also, are we 100% sure that dev->dev.dma_pfn_offset isn't going to be freed
before we're done using intf->dev? Maybe it's safer to copy the ranges?

quoted hunk ↗ jump to hunk

 		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
 		intf->minor = -1;
 		device_initialize(&intf->dev);

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index f16c26dc079d..d2ed4d90e56e 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c

@@ -612,6 +612,9 @@ struct usb_device *usb_alloc_dev(struct usb_device

*parent,
 	 */
 	dev->dev.dma_mask = bus->sysdev->dma_mask;
 	dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+	dev->dev.dma_pfn_offset_map = bus->sysdev->dma_pfn_offset_map;
+#endif
 	set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
 	dev->state = USB_STATE_ATTACHED;
 	dev->lpm_disable_count = 1;

diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..67a240ad4fc5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -493,6 +493,8 @@ struct dev_links_info {
  * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
  *		DMA limit than the device itself supports.
  * @dma_pfn_offset: offset of DMA memory range relatively of RAM
+ * @dma_pfn_offset_map:	Like dma_pfn_offset but used when there are

multiple
+ *		pfn offsets for multiple dma-ranges.
  * @dma_parms:	A low level driver may set these to teach IOMMU code
about
  * 		segment limitations.
  * @dma_pools:	Dma pools (if dma'ble device).

@@ -578,7 +580,13 @@ struct device {
 					     allocations such descriptors. */
 	u64		bus_dma_limit;	/* upstream dma constraint */
 	unsigned long	dma_pfn_offset;
-
+#ifdef CONFIG_DMA_PFN_OFFSET_MAP
+	const struct dma_pfn_offset_region *dma_pfn_offset_map;
+					/* Like dma_pfn_offset, but for
+					 * the unlikely case of multiple
+					 * offsets. If non-null, dma_pfn_offset
+					 * will be set to 0. */
+#endif

I'm still sad this doesn't fully replace dma_pfn_offset & bus_dma_limit. I feel
the extra logic involved in incorporating this as default isn't going to be
noticeable as far as performance is concerned to single dma-range users, and
it'd make for a nicer DMA code. Also you'd force everyone to test their changes
on the multi dma-ranges code path, as opposed to having this disabled 99.9% of
the time (hence broken every so often).

Note that I sympathize with the amount of work involved on improving that, so
better wait to hear what more knowledgeable people have to say about this :)

Regards,
Nicolas

Attachments

signature.asc [application/pgp-signature] 488 bytes

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help